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

[Lukas Wunner <lukas@wunner.de>]

On Tue, Nov 30, 2021 at 11:15:40AM +0100, Hans de Goede wrote:
> On 11/29/21 19:59, Lukas Wunner wrote:
> > Hm, I found the notes that I took when I investigated Theodore's report:
> > Using a subclass may be problematic because it's limited to a value < 8
> > (MAX_LOCKDEP_SUBCLASSES).  If there's a hotplug port at a deeper level
> > than 8 in the PCI hierarchy (can easily happen, Thunderbolt daisy chain
> > allows up to 6 devices, each device contains a PCIe switch, so 2 levels per
> > device), look_up_lock_class() in kernel/locking/lockdep.c will emit a BUG
> > error message.

Actually, after thinking about this some more it has occurred to me
that you should be able to make do with 8 subclasses if you change
the algorithm in patch [1/2] to something like this:

	while (bus->parent) {
-		depth++;
		bus = bus->parent;
+		if (bus->self->is_hotplug_bridge)
+			depth++;
	}

(It may be necessary to add a NULL pointer check for bus->self,
off the top of my hat I'm not sure if that's set for the root bus.)

Because with the patches as they are, the array of 8 subclasses is
sparsely populated, i.e. if a parent port is not a hotplug port,
a subclass is wasted.  You only care about hotplug ports (more
specifically those handled by pciehp) because those are the only
ones with a reset_lock.  The above change should result in a
densely populated array of subclasses.

Naturally, because this makes the function pciehp-specific,
it should be moved from include/linux/pci.h to pciehp_hpc.c.

With Thunderbolt you can have 6 devices plus the hotplug port in
the host controller.  And there may be an 8th hotplug port at the
Root Port if the host controller can be "unplugged" when it goes
to D3cold.  (That's handled by acpiphp, not pciehp, and I'm not
even sure if is_hotplug_bridge is set in that case.)
So 8 subclasses should be sufficient.

Aside from Thunderbolt, nested hotplug ports exist in data centers
with hot-removable NVMe slots in hot-removable chassis, but I highly
doubt those use cases have more than 8 levels of hotplug ports.

So that would probably be a pragmatic and minimalistic approach to this
problem.

Thanks,

Lukas