On Sun, Feb 12, 2023 at 03:51:05PM -0500, Kent Overstreet wrote: > On Sun, Feb 12, 2023 at 03:19:16PM -0500, Alan Stern wrote: > > I'll revise the patch to use the device's name for the class. While it > > may not be globally unique, it should be sufficiently specific. > > > > (Device names are often set after the device is initialized. Does > > lockdep mind if a lock_class_key's name is changed after it has been > > registered?) > > The device name should _not_ be something dynamic, it should be > something easily tied back to a source code location - i.e. related to > the driver name, not the device name. > > That's how people read and use lockdep reports! > > Do it exactly the same way mutex_init() does it, just lift it up a level > to a wrapper around device_initialize() - stringify the pointer to the > mutex (embedded in struct device, embedded in what-have-you driver code) > and use that. I really don't think that's a good idea here. When you've got a bus containing multiple devices, typically all those device structures are created by the same line of code. So knowing the source code location won't tell you _which_ device structure is involved in the locking cycle or what driver it's using. By contrast, knowing the device name would. Furthermore, to the extent that the device's name identifies what kind of device it is, the name would tell you what where the structure was created and which driver it is using. For example, knowing that a struct device was initialized in line 2104 of drivers/usb/core/message.c tells you only that the device is a USB interface. It doesn't tell you which USB interface. But knowing that the device's name is 1-7:1.1 not only tells me that the device is a USB interface, it also allows me to do: $ ls -l /sys/bus/usb/devices/1-7:1.1/driver lrwxrwxrwx. 1 root root 0 Feb 12 19:56 /sys/bus/usb/devices/1-7:1.1/driver -> ../../../../../../bus/usb/drivers/usbhid/ telling me that the device is some sort of HID device. Probably my laptop's touchpad (which could easily be verified). Even without direct interaction with the system, searching through the kernel log would give me this information. > > At this stage, converting would be most impractical. And I don't think > > it's really needed. > > They do make you deal with lock restarts; unwinding typical stateful > kernel code is not necessarily super practical :) > > Anyways, it sounds like the lockdep-class-per-driver approach will get > you more information, that's certainly a reasonable approach for now. Thanks for the review and suggestions. Alan Stern
