[PATCH] usb: roles: Fix a false positive recursive locking complaint

[PATCH] usb: roles: Fix a false positive recursive locking complaint

[Bart Van Assche]

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;

Re: [PATCH] usb: roles: Fix a false positive recursive locking complaint

[Badhri Jagan Sridharan]

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

Re: [PATCH] usb: roles: Fix a false positive recursive locking complaint

[Bart Van Assche]

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.

Re: [PATCH] usb: roles: Fix a false positive recursive locking complaint

[Amit Sunil Dhamne]

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.
>

Re: [PATCH] usb: roles: Fix a false positive recursive locking complaint

[Bart Van Assche]

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.

Re: [PATCH] usb: roles: Fix a false positive recursive locking complaint

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] usb: roles: Fix a false positive recursive locking complaint

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] usb: roles: Fix a false positive recursive locking complaint

[Bart Van Assche]

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.

Re: [PATCH] usb: roles: Fix a false positive recursive locking complaint

[Amit Sunil Dhamne]

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.
>

Re: [PATCH] usb: roles: Fix a false positive recursive locking complaint

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko