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

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

On May 25, 2016 4:07 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 25.05.16 at 08:41, <quan.xu@xxxxxxxxx> wrote:
> > On May 24, 2016 4:22 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> >>> On 18.05.16 at 10:08, <quan.xu@xxxxxxxxx> wrote:
> >> >  static int device_power_down(void)  {
> >> > -    console_suspend();
> >> > +    if ( console_suspend() )
> >> > +        return SAVED_CONSOLE;
> >>
> >> I said so on the previous round, and I need to repeat it now: If
> >> console_suspend() fails, you saved _nothing_.
> >>
> >
> > Ah, we may have some different views for SAVED_*, which I mean has
> > been saved and we are no need to resume.
> >
> > e.g. if console_suspend() fails, I did return SAVED_CONSOLE, reading
> > my patch again, and I really resume nothing at all.
> >
> > device_power_up()
> > {
> >      ...
> >     +    case SAVED_CONSOLE:
> >     +        break;
> >     ...
> > }
> >
> >
> > I know we can also propagate SAVED_NONE for console_suspend() failure,
> > then we need adjust device_power_up() relevantly.
> 
> My main point is that the names of these enumerators should reflect their
> purpose. If one reads "SAVED_CONSOLE", then (s)he should be allowed this
> to mean that console state was saved (and hence needs to be restored upon
> error / resume).
> 


I'll follow this.. it is much better than what I understand.


> >> > -    time_suspend();
> >> > +    if ( time_suspend() )
> >> > +        return SAVED_TIME;
> >> >
> >> > -    i8259A_suspend();
> >> > +    if ( i8259A_suspend() )
> >> > +        return SAVED_I8259A;
> >> >
> >> > +    /* ioapic_suspend cannot fail */
> >> >      ioapic_suspend();
> >> >
> >> > -    iommu_suspend();
> >> > +    if ( iommu_suspend() )
> >> > +        return SAVED_IOMMU;
> >> >
> >> > -    lapic_suspend();
> >> > +    if ( lapic_suspend() )
> >> > +        return SAVED_LAPIC;
> >> >
> >> >      return 0;
> >>
> >> And this silently means SAVED_NONE, whereas here you saved everything.
> >> Yielding clearly bogus code ...
> >>
> >
> >
> >  '0' is just on success here.  Look at the condition where we call
> > device_power_up():
> >
> > +        if ( error > 0 )
> > +            device_power_up(error);
> >
> > Then, it is not bogus code.
> 
> See above: Zero should not mean both "nothing saved" and "saved
> everything".
> 
> >> Also, having come here - did I miss iommu_flush_iotlb_global()
> >> gaining a __must_check annotation somewhere?
> >
> > I will add __must_check annotation to iommu_flush_iotlb_global().
> >
> >> And the struct iommu_flush pointers
> >> and handlers? And, by analogy, iommu_flush_context_*()?
> >
> > I am better only add __must_check annotation to flush->iotlb and
> > handlers, but leaving flush->context/handers and
> > iommu_flush_context_*() as are in current patch set..
> > the coming patch set will fix them.
> 
> I don't follow the logic behind this: The purpose of this series is to make 
> sure
> flushing errors get properly bubbled up, which includes adding __must_check
> annotations. I'm not saying this needs to happen in this patch, but it should
> happen in this series

I will add a patch..

> (and please following the same basic model: A caller or a
> __must_check function should either already be __must_check, or should
> become so at the same time).
> 

Agreed.

Quan


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