Re: [Xen-devel] [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

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

On May 10, 2016 5:25 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/acpi/power.c
> > +++ b/xen/arch/x86/acpi/power.c
> >  static int device_power_down(void)
> >  {
> > -    console_suspend();
> > +    if ( console_suspend() )
> > +        return TYPE_CONSOLE;
> 
> This (together with the resume side) makes me guess that the use of TYPE_ as
> a prefix confused not just me, but also you:

Yes,  this is really not a good prefix, and probably pretty bad to use 'ERROR_'.
What about 'PRIOR_'?  then I also need to adjust  device_power_up() as ...


> Here you want to tell the caller
> that everything _prior_ to console suspend (which happens to be nothing in
> this specific case) needs to be undone. Yet below you use TYPE_CONSOLE as
> an indication that
> console_resume() needs to be called.

... this,

+    switch ( prior )
+    {
       ...
+        time_resume();
+    case PRIOR_TIME:
+        console_resume();
+    case PRIOR_CONSOLE:
        ...
+    }

> 
> > -    time_suspend();
> > +    if ( time_suspend() )
> > +        return TYPE_TIME;
> >
> > -    i8259A_suspend();
> > +    if ( i8259A_suspend() )
> > +        return TYPE_I8259A;
> >
> > +    /* ioapic_suspend should never fail */
> >      ioapic_suspend();
> 
> The comment is bogus: "should" means it can in theory. Yet the function
> having void return type means it just cannot fail.
> 

I'll use 'cannot', instead of 'should'.
Another question, I check the code again, and the rest of the functions 
(console_suspend/ time_suspend/ i8259A_suspend / ioapic_suspend / lapic_suspend 
), in device_power_down(), always returned '0'.
Maybe I need to fix these functions  annotation from 'int' to 'void', and then 
I can add a comment on the device_power_down().  

More that, if the   ' iommu_suspend()'  is the only fail, could we re-consider 
the previous v2/v3 solution with 'goto'?

> > @@ -169,6 +204,7 @@ static int enter_state(u32 state)
> >      {
> >          printk(XENLOG_ERR "Some devices failed to power down.");
> >          system_state = SYS_STATE_resume;
> > +        device_power_up(error);
> 
> Either error's and device_power_down()'s return type get changed to enum
> dev_power_type, or this needs a "error > 0" conditional.
> 
> > @@ -1267,7 +1273,9 @@ static void __hwdom_init
> intel_iommu_hwdom_init(struct domain *d)
> >      setup_hwdom_pci_devices(d, setup_hwdom_device);
> >      setup_hwdom_rmrr(d);
> >
> > -    iommu_flush_all();
> > +    if ( iommu_flush_all() )
> > +        printk(XENLOG_WARNING VTDPREFIX
> > +               " intel_iommu_hwdom_init: IOMMU flush all failed.\n");
> 
> As said (and I think a number of times) before: At least when you already hold
> the error value in your hands, please also log it.

Agreed, and I will apply for other patches.

> Also personally I'm opposed to
> including function names in non-debug log messages, but I'll leave that
> decision to the VT-d maintainers here.
> 

Albeit Reluctantly, I will fix it.

Quan


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