Suppress the following lockdep complaint:
INFO: trying to register non-static key. The code is fine but needs lockdep annotation, or maybe you didn't initialize this object before use? turning off the locking correctness validator.
Cc: Hans de Goede hdegoede@redhat.com Cc: Andy Shevchenko andy.shevchenko@gmail.com Cc: Heikki Krogerus heikki.krogerus@linux.intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org Fixes: fde0aa6c175a ("usb: common: Small class for USB role switches") Signed-off-by: Bart Van Assche bvanassche@acm.org --- drivers/usb/roles/class.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c index d7aa913ceb8a..f648ce3dd9b5 100644 --- a/drivers/usb/roles/class.c +++ b/drivers/usb/roles/class.c @@ -21,6 +21,7 @@ static const struct class role_class = {
struct usb_role_switch { struct device dev; + struct lock_class_key key; struct mutex lock; /* device lock*/ struct module *module; /* the module this device depends on */ enum usb_role role; @@ -326,6 +327,8 @@ static void usb_role_switch_release(struct device *dev) { struct usb_role_switch *sw = to_role_switch(dev);
+ mutex_destroy(&sw->lock); + lockdep_unregister_key(&sw->key); kfree(sw); }
@@ -364,7 +367,8 @@ usb_role_switch_register(struct device *parent, if (!sw) return ERR_PTR(-ENOMEM);
- mutex_init(&sw->lock); + lockdep_register_key(&sw->key); + __mutex_init(&sw->lock, "usb_role_switch_desc::lock", &sw->key);
sw->allow_userspace_control = desc->allow_userspace_control; sw->usb2_port = desc->usb2_port;
HI Bart,
On Wed, Sep 4, 2024 at 1:19 PM Bart Van Assche bvanassche@acm.org wrote:
Suppress the following lockdep complaint:
INFO: trying to register non-static key. The code is fine but needs lockdep annotation, or maybe you didn't initialize this object before use? turning off the locking correctness validator.
Cc: Hans de Goede hdegoede@redhat.com Cc: Andy Shevchenko andy.shevchenko@gmail.com Cc: Heikki Krogerus heikki.krogerus@linux.intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: stable@vger.kernel.org Fixes: fde0aa6c175a ("usb: common: Small class for USB role switches") Signed-off-by: Bart Van Assche bvanassche@acm.org
drivers/usb/roles/class.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c index d7aa913ceb8a..f648ce3dd9b5 100644 --- a/drivers/usb/roles/class.c +++ b/drivers/usb/roles/class.c @@ -21,6 +21,7 @@ static const struct class role_class = {
struct usb_role_switch { struct device dev;
struct lock_class_key key; struct mutex lock; /* device lock*/ struct module *module; /* the module this device depends on */ enum usb_role role;@@ -326,6 +327,8 @@ static void usb_role_switch_release(struct device *dev) { struct usb_role_switch *sw = to_role_switch(dev);
mutex_destroy(&sw->lock);lockdep_unregister_key(&sw->key); kfree(sw);}
@@ -364,7 +367,8 @@ usb_role_switch_register(struct device *parent, if (!sw) return ERR_PTR(-ENOMEM);
mutex_init(&sw->lock);
lockdep_register_key(&sw->key);__mutex_init(&sw->lock, "usb_role_switch_desc::lock", &sw->key);
https://lore.kernel.org/all/ZsiYRAJST%2F2hAju1@kuha.fi.intel.com/ was already accepted and is perhaps better than what you are suggesting as it does not use the internal methods of mutex_init(). CCing Amit as well so that his patch can be submitted to stable trees as well.
sw->allow_userspace_control = desc->allow_userspace_control; sw->usb2_port = desc->usb2_port;
Thanks, Badhri
On 9/4/24 2:00 PM, Badhri Jagan Sridharan wrote:
https://lore.kernel.org/all/ZsiYRAJST%2F2hAju1@kuha.fi.intel.com/ was already accepted
Thanks, I hadn't noticed this yet.
and is perhaps better than what you are suggesting as it does not use the internal methods of mutex_init().
Although I do not have a strong opinion about which patch is sent to Linus, I think my patch has multiple advantages compared to the patch mentioned above: - Cleaner. lockdep_set_class() is not used. Hence, it is not possible that the wrong lockdep key is used (the one assigned by mutex_init()). - The lock_class_key declaration occurs close to the sw->lock declaration. - The lockdep_register_key() call occurs close to __mutex_init() call that uses the registered key. - Needs less memory in debug kernels. The advantage of __mutex_init() compared to mutex_init() is that it does not allocate (static) memory for a lockdep key.
Thanks,
Bart.
Hi Bart,
On 9/4/24 2:15 PM, Bart Van Assche wrote:
On 9/4/24 2:00 PM, Badhri Jagan Sridharan wrote:
https://lore.kernel.org/all/ZsiYRAJST%2F2hAju1@kuha.fi.intel.com/ was already accepted
Thanks, I hadn't noticed this yet.
and is perhaps better than what you are suggesting as it does not use the internal methods of mutex_init().
Although I do not have a strong opinion about which patch is sent to Linus, I think my patch has multiple advantages compared to the patch mentioned above:
- Cleaner. lockdep_set_class() is not used. Hence, it is not possible
that the wrong lockdep key is used (the one assigned by mutex_init()).
- The lock_class_key declaration occurs close to the sw->lock
declaration.
- The lockdep_register_key() call occurs close to __mutex_init() call
that uses the registered key.
- Needs less memory in debug kernels. The advantage of __mutex_init()
compared to mutex_init() is that it does not allocate (static) memory for a lockdep key.
Thanks for the patch.
While I agree on (1) & (4), *may* be a good reason to reconsider. However, I have seen almost 30+ instances of the prior method (https://lore.kernel.org/all/20240822223717.253433-1-amitsd@google.com/) of registering lockdep key, which is what I followed. However, if that's is not the right way, it brings into question the purpose of lockdep_set_class() considering I would always and unconditionally use __mutex_init() if I want to manage the lockdep class keys myself or mutex_init() if I didn't.
Thanks,
Amit
Thanks,
Bart.
On 9/4/24 3:34 PM, Amit Sunil Dhamne wrote:
However, I have seen almost 30+ instances of the prior method (https://lore.kernel.org/all/20240822223717.253433-1-amitsd@google.com/) of registering lockdep key, which is what I followed.
Many of these examples are for spinlocks. It would be good to have a variant of spin_lock_init() that does not instantiate a struct lock_class_key and instead accepts a lock_class_key pointer as argument.
However, if that's is not the right way, it brings into question the purpose of lockdep_set_class() considering I would always and unconditionally use __mutex_init() if I want to manage the lockdep class keys myself or mutex_init() if I didn't.
What I'm proposing is not a new pattern. There are multiple examples in the kernel tree of lockdep_register_key() calls followed by a __mutex_init() call:
$ git grep -wB3 __mutex_init | grep lockdep_register_key | wc -l 5
Thanks,
Bart.
On Thu, Sep 5, 2024 at 6:01 PM Bart Van Assche bvanassche@acm.org wrote:
On 9/4/24 3:34 PM, Amit Sunil Dhamne wrote:
However, I have seen almost 30+ instances of the prior method (https://lore.kernel.org/all/20240822223717.253433-1-amitsd@google.com/) of registering lockdep key, which is what I followed.
Many of these examples are for spinlocks. It would be good to have a variant of spin_lock_init() that does not instantiate a struct lock_class_key and instead accepts a lock_class_key pointer as argument.
However, if that's is not the right way, it brings into question the purpose of lockdep_set_class() considering I would always and unconditionally use __mutex_init() if I want to manage the lockdep class keys myself or mutex_init() if I didn't.
What I'm proposing is not a new pattern. There are multiple examples in the kernel tree of lockdep_register_key() calls followed by a __mutex_init() call:
$ git grep -wB3 __mutex_init | grep lockdep_register_key | wc -l 5
I see pros and cons for both approaches, but I take Bart's as the simpler one. However, since it might be confusing, what I would suggest is to add a respective wrapper to mutex.h and have a non-__ named macro for this case.
On Thu, Sep 5, 2024 at 9:13 PM Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Thu, Sep 5, 2024 at 6:01 PM Bart Van Assche bvanassche@acm.org wrote:
On 9/4/24 3:34 PM, Amit Sunil Dhamne wrote:
However, I have seen almost 30+ instances of the prior method (https://lore.kernel.org/all/20240822223717.253433-1-amitsd@google.com/) of registering lockdep key, which is what I followed.
Many of these examples are for spinlocks. It would be good to have a variant of spin_lock_init() that does not instantiate a struct lock_class_key and instead accepts a lock_class_key pointer as argument.
However, if that's is not the right way, it brings into question the purpose of lockdep_set_class() considering I would always and unconditionally use __mutex_init() if I want to manage the lockdep class keys myself or mutex_init() if I didn't.
What I'm proposing is not a new pattern. There are multiple examples in the kernel tree of lockdep_register_key() calls followed by a __mutex_init() call:
$ git grep -wB3 __mutex_init | grep lockdep_register_key | wc -l 5
I see pros and cons for both approaches, but I take Bart's as the simpler one. However, since it might be confusing, what I would suggest is to add a respective wrapper to mutex.h and have a non-__ named macro for this case.
To be clear, something like
#define mutex_init_with_lockdep(...) do { ... __mutex_init(); } while (0)
in the mutex.h.
On 9/5/24 11:14 AM, Andy Shevchenko wrote:
To be clear, something like
#define mutex_init_with_lockdep(...) do { ... __mutex_init(); } while (0)
in the mutex.h.
How about using the name "mutex_init_with_key()" since the name "lockdep" refers to the lock dependency infrastructure and the additional argument will have type struct lock_class_key *?
Amit, do you want me to add your Signed-off-by to my patch since your patch was posted first on the linux-usb mailing list?
Thanks,
Bart.
Hi,
On 9/5/24 11:22 AM, Bart Van Assche wrote:
On 9/5/24 11:14 AM, Andy Shevchenko wrote:
To be clear, something like
#define mutex_init_with_lockdep(...) do { ... __mutex_init(); } while (0)
in the mutex.h.
How about using the name "mutex_init_with_key()" since the name "lockdep" refers to the lock dependency infrastructure and the additional argument will have type struct lock_class_key *?
Amit, do you want me to add your Signed-off-by to my patch since your patch was posted first on the linux-usb mailing list?
Yes, thank you :) .
Also, thanks for the clarification on my previous questions Bart and for your suggestions Andy!
-
Amit
Thanks,
Bart.
On Thu, Sep 5, 2024 at 9:23 PM Bart Van Assche bvanassche@acm.org wrote:
On 9/5/24 11:14 AM, Andy Shevchenko wrote:
To be clear, something like
#define mutex_init_with_lockdep(...) do { ... __mutex_init(); } while (0)
in the mutex.h.
How about using the name "mutex_init_with_key()" since the name "lockdep" refers to the lock dependency infrastructure and the additional argument will have type struct lock_class_key *?
LGTM, go for it!
linux-stable-mirror@lists.linaro.org
-
Amit Sunil Dhamne -
Andy Shevchenko -
Badhri Jagan Sridharan -
Bart Van Assche