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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/30/21 20:39, Lukas Wunner wrote:
> 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.

Ack. I've prepared and tested a v2 patch using this approach, this
v2 also addresses all your and Bjorn's other remarks.

I will send out the v2 patch right after this email.

Regards,

Hans