Re: [PATCH 2/2] PCI: pciehp: Use down_read/write_nested(reset_lock) to fix lockdep errors

[Lukas Wunner <lukas@wunner.de>]

On Mon, Nov 29, 2021 at 01:19:34PM +0100, Hans de Goede wrote:
> Use down_read_nested() and down_write_nested() when taking the
> ctrl->reset_lock rw-sem, passing the PCI-device depth in the hierarchy
> as lock subclass parameter. This fixes the following false-positive lockdep
> report when unplugging a Lenovo X1C8 from a Lenovo 2nd gen TB3 dock:
[...]
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

That's exactly what I had in mind, thanks.

Reported-by: "Theodore Ts'o" <tytso@mit.edu>
Link: https://lore.kernel.org/linux-pci/20190402021933.GA2966@mit.edu/
Link: https://lore.kernel.org/linux-pci/de684a28-9038-8fc6-27ca-3f6f2f6400d7@redhat.com/
Reviewed-by: Lukas Wunner <lukas@wunner.de>


> Note the 2nd patch can probably use a Fixes: tag but I had no
> idea which commit to put there. Or maybe add a Cc: stable to
> both patches?

I'd just add a stable designation and let the stable maintainers decide
which versions to backport to.  The problem I see is the dependency on
the first patch in the series.  In theory there's a syntax to specify
such prerequisites (see "Option 3" in
Documentation/process/stable-kernel-rules.rst), but in practice,
my experience is that stable maintainers may ignore such prerequisite tags.
It might be simplest to just squash the two patches together.

If you do respin, it would be good to explain in the commit message why
one lockdep class is used per hierarchy level:  This is done to conserve
class keys because their number is limited and the complexity grows
quadratically with number of keys according to
Documentation/locking/lockdep-design.rst.

It would also be good to explain why the lockdep splat occurs and why
it's a false positive:  With Thunderbolt, hotplug ports are nested.  When
removing multiple devices in a daisy-chain, each hotplug port's reset_lock
may be acquired recursively.  It's never the same lock, so the lockdep
splat is a false positive.  Because locks at the same hierarchy level
are never acquired recursively, a per-level lockdep class is sufficient
to fix the lockdep splat.

Thanks,

Lukas