Re: [PATCH 3/8] iommu: Factor out a "first device in group" helper

[Robin Murphy]

On 19/01/2023 7:23 pm, Jason Gunthorpe wrote:
> On Thu, Jan 19, 2023 at 07:18:21PM +0000, Robin Murphy wrote:
> > This pattern for picking the first device out of the group list is
> > repeated a few times now, so it's clearly worth factoring out.
> >
> > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> > ---
> >   drivers/iommu/iommu.c | 22 ++++++++++------------
> >   1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index bc53ffbba4de..5b37766a09e2 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1084,6 +1084,11 @@ void iommu_group_remove_device(struct device *dev)
> >   }
> >   EXPORT_SYMBOL_GPL(iommu_group_remove_device);
> >   
> > +static struct device *iommu_group_first_dev(struct iommu_group *group)
> > +{
>
> Add a
>   lockdep_assert_held(&group->lock);
>
> ?

Sure, could do. I guess I didn't consider it since 
iommu_group_device_count() and __iommu_group_for_each_dev() don't assert 
it either, but that's not to say they couldn't change too.

> > -	group->blocking_domain =
> > -		__iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
> > +	group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_BLOCKED);
> >   	if (!group->blocking_domain) {
> >   		/*
> >   		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
> >   		 * create an empty domain instead.
> >   		 */
> > -		group->blocking_domain = __iommu_domain_alloc(
> > -			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
> > +		group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_UNMANAGED);
> >   		if (!group->blocking_domain)
> >   			return -EINVAL;
>
> These are extra hunks?

The annoyingly-subtle difference between "dev->dev->bus" and "dev->bus" 
is precisely one of the reasons I think hiding the group_device behind a 
helper is worthwhile ;)

Thanks,
Robin.