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

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

Jason,


On 10/15/2024 10:31 PM, Jason Gunthorpe wrote:
> On Tue, Oct 15, 2024 at 10:15:30PM +0530, Vasant Hegde wrote:
>>>> 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.
>>>
>>> deferred attach is broken to not have the core code take the lock, all
>>> most all drivers depend on group_mutex for correctness.
>>>
>>> I think we should fix that, not retain unnecessary locks in drivers :\
>>
>> If you recall, I had a patch to fix core code [1]. Robin had a concern and hence
>> that patch was dropped.
> 
> I do, but I think the concern was addressed.
> 
> There might have been a small argument if AMD had a non-sleeping
> attach path (at least in some case), but here you are switching to a
> mutex so that is out the window.
> 
> It is one thing to say we don't want the group mutex in core because
> it is just documentation, it is quite another to say you will keep a
> fully redundant mutex in the driver!!
> 
> Drivers are relying on the group mutex, we have a lockdep assertion
> helper now, we need to hold it consistently when invoking the
> attach_dev and related ops.

Yeah. Its redundant mutex lock. I am happy to get rid of it.
I will look into it as separate fix.

-Vasant