On 2023-08-18 20:19, Jason Gunthorpe wrote:
On Fri, Aug 18, 2023 at 07:50:18PM +0100, Robin Murphy wrote:
There is a difference between knowingly introducing new lockdep warnings
into upstream and letting warnings discovered upstream rot for a while.
Furthermore I'd say it's one thing to have deadlocks or warnings slip in as
part of some functional change, but quite another when the change is solely
reworking the locking in an attempt to make it better. This is clearly not
better.
Exactly what isn't better? We now print a warning when pre-existing
locking rules are violated. That is the only issue here.
What locking rules? 9 years ago when of_iommu_configure() was introduced
(97890ba9289c), of_dma_configure() ran at device creation time, and that
remains the case in some places, so cannot necessarily be called wrong.
Subsequently making it run off driver probe in the general case was a
bit of a hack, but at the time probe deferral was the only tool we had
available to easily enforce the ordering of IOMMU drivers relative to
client drivers. In hindsight there are subtleties which weren't really
apparent at the time, but what *was* clear was that the benefit of
letting IOMMU drivers be normal drivers without any special
OF_IOMMU_DECLARE£() weirdness was significant, and thus today's mess was
unwittingly born.
The existing locking rules are that there aren't any locking rules
(other than the probe_device_lock bodge), which obviously could be
better, but does rather disprove the assertion that they could be
violated. A robust locking scheme would be great, but designing one
based on something which already violates *other* IOMMU API assumptions,
such that a) it can't ever really work, and b) it makes those underlying
issues even harder to fix, is not productive.
Thanks,
Robin.
