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@xxxxxxxxxx> >> >> 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
