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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <651bfdea-19ce-1b4b-3ebd-bbd558d8403b@arm.com>
Date:   Thu, 19 Jan 2023 19:36:19 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     joro@...tes.org, will@...nel.org, hch@....de,
        iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/8] iommu: Factor out a "first device in group" helper

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