| Message ID | 20240904201839.2901330-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: roles: Fix a false positive recursive locking complaint | expand |
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!
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;
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(-)