Re: [PATCH v2 06/10] iommu/amd: Reduce domain lock scope in attach device path

[Vasant Hegde <vasant.hegde@amd.com>]

Hi Joerg,


On 10/15/2024 2:08 PM, Joerg Roedel wrote:
> Hi Vasant,
> 
> On Tue, Sep 10, 2024 at 06:58:08AM +0000, Vasant Hegde wrote:
>> @@ -2119,11 +2135,8 @@ static int attach_device(struct device *dev,
>>  			 struct protection_domain *domain)
>>  {
>>  	struct iommu_dev_data *dev_data;
>> -	unsigned long flags;
>>  	int ret = 0;
>>  
>> -	spin_lock_irqsave(&domain->lock, flags);
>> -
>>  	dev_data = dev_iommu_priv_get(dev);
>>  
>>  	spin_lock(&dev_data->lock);
> 
> This patch inverses the locking order of domain->lock and
> dev_data->lock, have you checked all places to make sure it is safe to
> do so? Has this change been tested with lockdep enabled?

Sorry. I missed to explain this part in commit message.

With all changes locking sequence will be :
  group->mutex -> dev_data->mutex -> domain->lock

In defer attach path (iommu_deferred_attach()), core call attach_device() w/o
group->mutex lock. So I retained dev_data lock.

I have manually audited this change and also tested with LOCKDEP. Its all fine.


> 
> If so, please document that aspect in the changelog and why it is safe.


Sure. Will update in next version.

-Vasant