Re: [Xen-devel] [Patch v6 07/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending (top level ones)

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On June 01, 2016 6:39 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 31.05.16 at 15:57, <quan.xu@xxxxxxxxx> wrote:
> >  static int device_power_down(void)
> >  {
> > -    console_suspend();
> > +    if ( console_suspend() )
> > +        return SAVED_NONE;
> >
> > -    time_suspend();
> > +    if ( time_suspend() )
> > +        return SAVED_CONSOLE;
> >
> > -    i8259A_suspend();
> > +    if ( i8259A_suspend() )
> > +        return SAVED_TIME;
> >
> > +    /* ioapic_suspend cannot fail */
> >      ioapic_suspend();
> >
> > -    iommu_suspend();
> > +    if ( iommu_suspend() )
> > +        return SAVED_IOAPIC;
> >
> > -    lapic_suspend();
> > +    if ( lapic_suspend() )
> > +        return SAVED_IOMMU;
> >
> > -    return 0;
> > +    return SAVED_NONE;
> 
> SAVED_ALL

Agreed. 
I was disturbed by the below 'if ( error > 0 )'.

> 
> > @@ -169,6 +203,10 @@ static int enter_state(u32 state)
> >      {
> >          printk(XENLOG_ERR "Some devices failed to power down.");
> >          system_state = SYS_STATE_resume;
> > +
> > +        if ( error > 0 )
> > +            device_power_up(error);
> 
> if ( error != SAVED_NONE )
> 
> (Or really you could just call this without any if().)

I prefer to drop this if().

> 
> > @@ -2389,16 +2393,25 @@ static int intel_iommu_group_id(u16 seg, u8
> > bus, u8 devfn)  }
> >
> >  static u32 iommu_state[MAX_IOMMUS][MAX_IOMMU_REGS];
> > -static void vtd_suspend(void)
> > +
> > +static int __must_check vtd_suspend(void)
> >  {
> >      struct acpi_drhd_unit *drhd;
> >      struct iommu *iommu;
> >      u32    i;
> > +    int rc = 0;
> 
> Pointless initializer.
> 

Indeed, if "return 0" to make obvious that no error path comes at the end of 
this function.

> >      if ( !iommu_enabled )
> > -        return;
> > +        return 0;
> >
> > -    iommu_flush_all();
> > +    rc = iommu_flush_all();
> > +    if ( unlikely(rc) )
> > +    {
> > +        printk(XENLOG_WARNING VTDPREFIX
> > +               " suspend: IOMMU flush all failed: %d\n", rc);
> > +
> > +        return rc;
> > +    }
> >
> >      for_each_drhd_unit ( drhd )
> >      {
> > @@ -2427,6 +2440,8 @@ static void vtd_suspend(void)
> >          if ( !iommu_intremap && iommu_qinval )
> >              disable_qinval(iommu);
> >      }
> > +
> > +    return rc;
> >  }
> 
> Perhaps better "return 0" to make obvious that no error path comes here.
> 

Agreed.


Quan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel