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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

>> > -    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 (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).

Jan


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