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

[Jason Gunthorpe <jgg@ziepe.ca>]

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.

But I agree we should deal with this in a seperate series

Jason