Re: [Xen-devel] [PATCH v5 01/10] vt-d: fix the IOMMU flush issue

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.05.16 at 10:04, <quan.xu@xxxxxxxxx> wrote:
> On May 23, 2016 11:43 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >>> On 23.05.16 at 17:22, <quan.xu@xxxxxxxxx> wrote:
>> > On May 23, 2016 9:31 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >> >>> On 18.05.16 at 10:08, <quan.xu@xxxxxxxxx> wrote:
>> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >> > @@ -557,14 +557,16 @@ static void iommu_flush_all(void)
>> >> >      }
>> >> >  }
>> >> >
>> >> > -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long
>> gfn,
>> >> > -        int dma_old_pte_present, unsigned int page_count)
>> >> > +static int __intel_iommu_iotlb_flush(struct domain *d, unsigned long
>> gfn,
>> >> > +                                     bool_t dma_old_pte_present,
>> >> > +                                     unsigned int page_count)
>> >>
>> >> I realize you say so in the overview mail, but the continuing lack of
>> >> __must_check here causes review trouble again. And I have a hard time
>> >> seeing how adding these annotations right away would "disrupt the
>> >> order", as long as the series is properly ordered / broken up.
>> >>
>> >
>> > If I add __must_check annotations here right now, e.g.
>> >
>> > -static void intel_iommu_iotlb_flush()
>> > +static int __must_check iommu_flush_iotlb_pages()
>> >
>> > ...
>> >
>> > @@ -179,8 +179,9 @@ struct iommu_ops {
>> > -    void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int
>> > page_count);
>> > +    int __must_check (*iotlb_flush)(struct domain *d, unsigned long
>> > + gfn,
>> > unsigned int page_count);
>> > ...
>> > }
>> >
>> > Should belong  to here too.
>> 
>> Correct. And where's the problem?
>>
> 
> IMO it is not a big deal..
> 
> I think this makes this patch 1 fat.. why not focus on the positive 
> propagation value from IOMMU flush interfaces in this patch.
> If we are clear I will add annotation and rename them in another patches, it 
> is acceptable to me.

The patch getting too large is easy to deal with: Split it at a reasonable
boundary. It's one thing that I want to be clear: Any conversion of a
void return type of some function to non-void should be accompanied
by it at the same time becoming __must_check. I dislike having to
repeat yet again what I have been saying a number of times: Without
doing so, it is harder for you as the person writing the patch to verify
all callers deal with errors, and it's perhaps even harder for reviewers
to verify you did.

Jan


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