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

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

Hi Lukas,

First of all thank you for the review and for all the remarks,
I agree with all of them, so I'll incorporate all of them in v2
of this patch-set including squashing the 2 patches together.

On 11/29/21 19:59, Lukas Wunner wrote:
> On Mon, Nov 29, 2021 at 04:39:43PM +0100, Lukas Wunner wrote:
>> 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.
> 
> 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.
> 
> It may be necessary to call lockdep_register_key() for each level or
> for each hotplug port and assign the lock with lockdep_set_class()
> (or ..._and_name() and use the dev_name()).
> 
> It's these complications that made me put aside the problem back in the day.
> My apologies for not remembering them earlier.

No worries.

I've been looking at how to rework things and this should be pretty doable.

My plan is to have a global static array of lock_class_key-s in
drivers/pci/hotplug/pciehp_hpc.c, one per depth level (say 32 of them ?)
and then use lockdep_set_class() and that looks like it should be pretty
doable.

When using a static global array of lock_class_key-s there is no need to call
lockdep_register_key().

Also see:

drivers/usb/storage/usb.c

Which does something similar to avoid issues when a single usb-device has
multiple USB-storage interfaces.

Regards,

Hans