usb: roles: Fix a false positive recursive locking complaint

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(-)

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