Re: [PATCH v2 02/10] iommu: Add a lockdep assertion for remaining dev->iommu_group reads

From: Jason Gunthorpe <jgg@nvidia.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>,
	David Woodhouse <dwmw2@infradead.org>,
	Heiko Stuebner <heiko@sntech.de>,
	iommu@lists.linux.dev, Jernej Skrabec <jernej.skrabec@gmail.com>,
	Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-sunxi@lists.linux.dev,
	Orson Zhai <orsonzhai@gmail.com>,
	Samuel Holland <samuel@sholland.org>,
	Chen-Yu Tsai <wens@csie.org>, Will Deacon <will@kernel.org>,
	Chunyan Zhang <zhang.lyra@gmail.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Lu Baolu <baolu.lu@linux.intel.com>
Subject: Re: [PATCH v2 02/10] iommu: Add a lockdep assertion for remaining dev->iommu_group reads
Date: Tue, 8 Aug 2023 13:54:36 -0300	[thread overview]
Message-ID: <ZNJzTIgaqimQ08tA@nvidia.com> (raw)
In-Reply-To: <a94abd4d-fb0a-8f72-7dee-82144c90187b@arm.com>

On Tue, Aug 08, 2023 at 05:22:55PM +0100, Robin Murphy wrote:
> Oh, the things that happen if I take holiday... :)
> 
> On 31/07/2023 6:50 pm, Jason Gunthorpe wrote:
> > The remaining reads are all in functions called under ops->device_group.
> > 
> > Broadly these functions are walking around the device tree (eg going up
> > the PCI bus tree) and are trying to de-duplicate group allocations
> > according to their logic.
> > 
> > Since these functions don't hold any particular per-device locks their
> > reads to dev->iommu_group are being locked by the caller's
> > iommu_probe_device_lock, and this explains why iommu_probe_device_lock
> > needs to be a global lock.
> 
> This confuzzles me. iommu_probe_device_lock is a global (but tightly-scoped)
> lock because its sole purpose is as a point hack to serialise calls to
> iommu_probe_device(), 

Well, that may have been the intention, but as a side effect it turns
out that it is the only thing that locks the access to the
dev->iommu_group as well. This is some other bug that I suppose nobody
noticed.

> concurrently for the same device, but due to the long-standing "replay"
> hacks, currently can. It is not meant to have anything to do with groups,
> and expanding its scope is a really really terrible idea.

Regardless, it does serialize the group stuff so what I did here is
recognize that as its main purpose and made the probe serialization a
secondary thing, which is eventually entirely removed.

I could have constructed this the other way and said that the group
locking is missing and added another global lock, but that seems
equally confusing since it isn't missing, it is just mis-named :)

> I finally now have some time to work on IOMMU gubbins again, so I'll be
> updating the bus ops removal series ASAP, then the next step after that is
> some bus_type callback surgery to pull the {of,acpi}_iommu_configure()
> parsing and ops->of_xlate calls to the proper point in the core
> iommu_probe_device() path, and all this mess finally goes away for good.

That is great, but it won't address the dev->group locking.

I'm not sure there is further value in trying to remove the
device_lock() around probe, but cleaning up the iommu_configure stuff
would be nice.

Jason

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip