Re: [PATCH v4 4/6] mm: Add structure to keep sva information

[Tina Zhang]

On 9/22/23 03:04, Jason Gunthorpe wrote:
> On Tue, Sep 12, 2023 at 08:59:34PM +0800, Tina Zhang wrote:
> > Introduce iommu_mm_data structure to keep sva information (pasid and the
> > related sva domains). Add iommu_mm pointer, pointing to an instance of
> > iommu_mm_data structure, to mm.
> >
> > Reviewed-by: Vasant Hegde <vasant.hegde@xxxxxxx>
> > Signed-off-by: Tina Zhang <tina.zhang@xxxxxxxxx>
> > ---
> >   include/linux/iommu.h    | 5 +++++
> >   include/linux/mm_types.h | 2 ++
> >   2 files changed, 7 insertions(+)
>
> This is not a great way to structure the patches
>
> This patch should move the pasid into the struct and do all the
> infrastructure to allocate/free the struct.
>
> The next patch should just add the list head to the now existing struct:
Agree. It would be great if we can put a new filed adding and its 
related handing logic into one patch. It will be convenient not only for 
the author but also for the reviewers, as it can convey the intention of 
the code change more easily.

In this case, the reason we put new members' introduction into one patch 
and their handling logic into another patch, is that it seems an easy 
way to replace the old pasid field w/o involving extra temporary logic 
to keep everything work between patches. Fortunately, we only have two 
new fields, which might make this option not so ugly :)

Regards,
-Tina

> > +struct iommu_mm_data {
> > +	u32			pasid;
> > +	struct list_head	sva_domains;
> > +};
>
> The code looks fine though
>
> Jason