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

[Robin Murphy <robin.murphy@arm.com>]

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@arm.com>
>> ---
>>   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.