[PATCH v2 0/3] Fix a lockdep complaint related to USB role switching

[PATCH v2 0/3] Fix a lockdep complaint related to USB role switching

[Bart Van Assche]

Hi Greg,

This patch series suppresses a lockdep complaint about recursive locking
that is triggered by switching USB roles. Please consider this patch series
for the next merge window.

Thanks,

Bart.

Changes compared to v1:
 - Added two patches. One that combines the two mutex_init() definitions and
   another patch that introduces mutex_init_with_key().
 - Changed patch 3/3 such that it uses mutex_init_with_key(). Added Amit's
   Signed-off-by.

Bart Van Assche (3):
  locking/mutex: Define mutex_init() once
  locking/mutex: Introduce mutex_init_with_key()
  usb: roles: Fix a false positive recursive locking complaint

 drivers/usb/roles/class.c |  6 +++++-
 include/linux/mutex.h     | 19 ++++++++++++-------
 2 files changed, 17 insertions(+), 8 deletions(-)

[PATCH v2 1/3] locking/mutex: Define mutex_init() once

[Bart Van Assche]

With CONFIG_PREEMPT_RT disabled __mutex_init() is a function. With
CONFIG_PREEMPT_RT enabled, __mutex_init() is a macro. I assume this is why
mutex_init() is defined twice as exactly the same macro.

Prepare for introducing a new macro for mutex initialization by combining
the two identical mutex_init() definitions into a single definition. This
patch does not change any functionality because the C preprocessor expands
macros when it encounters the macro name and not when a macro definition
is encountered. See also commit bb630f9f7a7d ("locking/rtmutex: Add mutex
variant for RT").

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/mutex.h | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index a561c629d89f..ef617089db19 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -49,7 +49,6 @@ static inline void mutex_destroy(struct mutex *lock) {}
 
 #endif
 
-#ifndef CONFIG_PREEMPT_RT
 /**
  * mutex_init - initialize the mutex
  * @mutex: the mutex to be initialized
@@ -65,6 +64,7 @@ do {									\
 	__mutex_init((mutex), #mutex, &__key);				\
 } while (0)
 
+#ifndef CONFIG_PREEMPT_RT
 #define __MUTEX_INITIALIZER(lockname) \
 		{ .owner = ATOMIC_LONG_INIT(0) \
 		, .wait_lock = __RAW_SPIN_LOCK_UNLOCKED(lockname.wait_lock) \
@@ -111,12 +111,6 @@ do {							\
 	__mutex_rt_init((mutex), name, key);		\
 } while (0)
 
-#define mutex_init(mutex)				\
-do {							\
-	static struct lock_class_key __key;		\
-							\
-	__mutex_init((mutex), #mutex, &__key);		\
-} while (0)
 #endif /* CONFIG_PREEMPT_RT */
 
 #ifdef CONFIG_DEBUG_MUTEXES

Re: [PATCH v2 1/3] locking/mutex: Define mutex_init() once

[Andy Shevchenko]

+ George Stark (IIRC you also tried to achieve something like this in the past)

On Thu, Sep 5, 2024 at 11:47 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> With CONFIG_PREEMPT_RT disabled __mutex_init() is a function. With
> CONFIG_PREEMPT_RT enabled, __mutex_init() is a macro. I assume this is why
> mutex_init() is defined twice as exactly the same macro.
>
> Prepare for introducing a new macro for mutex initialization by combining
> the two identical mutex_init() definitions into a single definition. This
> patch does not change any functionality because the C preprocessor expands
> macros when it encounters the macro name and not when a macro definition
> is encountered. See also commit bb630f9f7a7d ("locking/rtmutex: Add mutex
> variant for RT").
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  include/linux/mutex.h | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index a561c629d89f..ef617089db19 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -49,7 +49,6 @@ static inline void mutex_destroy(struct mutex *lock) {}
>
>  #endif
>
> -#ifndef CONFIG_PREEMPT_RT
>  /**
>   * mutex_init - initialize the mutex
>   * @mutex: the mutex to be initialized
> @@ -65,6 +64,7 @@ do {                                                                  \
>         __mutex_init((mutex), #mutex, &__key);                          \
>  } while (0)
>
> +#ifndef CONFIG_PREEMPT_RT
>  #define __MUTEX_INITIALIZER(lockname) \
>                 { .owner = ATOMIC_LONG_INIT(0) \
>                 , .wait_lock = __RAW_SPIN_LOCK_UNLOCKED(lockname.wait_lock) \
> @@ -111,12 +111,6 @@ do {                                                       \
>         __mutex_rt_init((mutex), name, key);            \
>  } while (0)
>
> -#define mutex_init(mutex)                              \
> -do {                                                   \
> -       static struct lock_class_key __key;             \
> -                                                       \
> -       __mutex_init((mutex), #mutex, &__key);          \
> -} while (0)
>  #endif /* CONFIG_PREEMPT_RT */
>
>  #ifdef CONFIG_DEBUG_MUTEXES



-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 2/3] locking/mutex: Introduce mutex_init_with_key()

[Bart Van Assche]

The following pattern occurs 5 times in kernel drivers:

	lockdep_register_key(key);
	__mutex_init(mutex, name, key);

In several cases the 'name' argument matches #mutex. Hence, introduce
the mutex_init_with_key() macro. This macro derives the 'name' argument
from the 'mutex' argument.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/mutex.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index ef617089db19..2bf91b57591b 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -64,6 +64,17 @@ do {									\
 	__mutex_init((mutex), #mutex, &__key);				\
 } while (0)
 
+/**
+ * mutex_init_with_key - initialize a mutex with a given lockdep key
+ * @mutex: the mutex to be initialized
+ * @key: the lockdep key to be associated with the mutex
+ *
+ * Initialize the mutex to the unlocked state.
+ *
+ * It is not allowed to initialize an already locked mutex.
+ */
+#define mutex_init_with_key(mutex, key) __mutex_init((mutex), #mutex, (key))
+
 #ifndef CONFIG_PREEMPT_RT
 #define __MUTEX_INITIALIZER(lockname) \
 		{ .owner = ATOMIC_LONG_INIT(0) \

Re: [PATCH v2 2/3] locking/mutex: Introduce mutex_init_with_key()

[Andy Shevchenko]

On Thu, Sep 5, 2024 at 11:47 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> The following pattern occurs 5 times in kernel drivers:
>
>         lockdep_register_key(key);
>         __mutex_init(mutex, name, key);
>
> In several cases the 'name' argument matches #mutex. Hence, introduce
> the mutex_init_with_key() macro. This macro derives the 'name' argument
> from the 'mutex' argument.

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Bart Van Assche]

Suppress the following lockdep complaint by giving each sw->lock
a unique lockdep key instead of using the same lockdep key for all
sw->lock instances:

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: Badhri Jagan Sridharan <badhri@google.com>
Cc: stable@vger.kernel.org
Fixes: fde0aa6c175a ("usb: common: Small class for USB role switches")
Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
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 7aca1ef7f44c..37556aa0eeee 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -22,6 +22,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;
@@ -329,6 +330,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);
 }
 
@@ -367,7 +370,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_with_key(&sw->lock, &sw->key);
 
 	sw->allow_userspace_control = desc->allow_userspace_control;
 	sw->usb2_port = desc->usb2_port;

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

[Andy Shevchenko]

On Thu, Sep 5, 2024 at 11:47 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> Suppress the following lockdep complaint by giving each sw->lock
> a unique lockdep key instead of using the same lockdep key for all
> sw->lock instances:
>
> 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: Badhri Jagan Sridharan <badhri@google.com>
> Cc: stable@vger.kernel.org

If you put these Cc:s after --- line it will reduce the commit message
while having the same effect (assuming use of `git send-email`).
lore.kernel.org archive will keep it.

> Fixes: fde0aa6c175a ("usb: common: Small class for USB role switches")
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>

Co-developed-by ?

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>


-- 
With Best Regards,
Andy Shevchenko

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

[Bart Van Assche]

On 9/5/24 1:54 PM, Andy Shevchenko wrote:
> On Thu, Sep 5, 2024 at 11:47 PM Bart Van Assche <bvanassche@acm.org> wrote:
>> 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: Badhri Jagan Sridharan <badhri@google.com>
>> Cc: stable@vger.kernel.org
> 
> If you put these Cc:s after --- line it will reduce the commit message
> while having the same effect (assuming use of `git send-email`).
> lore.kernel.org archive will keep it.
> 
>> Fixes: fde0aa6c175a ("usb: common: Small class for USB role switches")
>> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> 
> Co-developed-by ?

Thanks for the quick follow-up. I will make these changes if I have to
repost this patch series.

Thanks,

Bart.

Re: [PATCH v2 0/3] Fix a lockdep complaint related to USB role switching

[Peter Zijlstra]

On Thu, Sep 05, 2024 at 01:47:06PM -0700, Bart Van Assche wrote:
> Hi Greg,
> 
> This patch series suppresses a lockdep complaint about recursive locking
> that is triggered by switching USB roles. Please consider this patch series
> for the next merge window.
> 
> Thanks,
> 
> Bart.
> 
> Changes compared to v1:
>  - Added two patches. One that combines the two mutex_init() definitions and
>    another patch that introduces mutex_init_with_key().
>  - Changed patch 3/3 such that it uses mutex_init_with_key(). Added Amit's
>    Signed-off-by.
> 
> Bart Van Assche (3):
>   locking/mutex: Define mutex_init() once
>   locking/mutex: Introduce mutex_init_with_key()
>   usb: roles: Fix a false positive recursive locking complaint
> 
>  drivers/usb/roles/class.c |  6 +++++-
>  include/linux/mutex.h     | 19 ++++++++++++-------
>  2 files changed, 17 insertions(+), 8 deletions(-)

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH v2 0/3] Fix a lockdep complaint related to USB role switching

[Greg Kroah-Hartman]

On Thu, Sep 05, 2024 at 01:47:06PM -0700, Bart Van Assche wrote:
> Hi Greg,
> 
> This patch series suppresses a lockdep complaint about recursive locking
> that is triggered by switching USB roles. Please consider this patch series
> for the next merge window.

I already took this commit into my tree for -rc1:
	https://lore.kernel.org/r/20240822223717.253433-1-amitsd@google.com

It's almost identical to yours, but you are messing with mutex states
here, why?  I'll be glad to take a series on top of that one if you can
describe why this one is "more correct" that that one.

thanks,

greg k-h

Re: [PATCH v2 0/3] Fix a lockdep complaint related to USB role switching

[Bart Van Assche]

On 9/11/24 6:41 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 05, 2024 at 01:47:06PM -0700, Bart Van Assche wrote:
>> This patch series suppresses a lockdep complaint about recursive locking
>> that is triggered by switching USB roles. Please consider this patch series
>> for the next merge window.
> 
> I already took this commit into my tree for -rc1:
> 	https://lore.kernel.org/r/20240822223717.253433-1-amitsd@google.com
> 
> It's almost identical to yours, but you are messing with mutex states
> here, why?

What does "messing with mutex states" mean in this context? My patch 3/3
does not modify the mutex lock class key at runtime. Instead, it
sets the mutex lock class key when the mutex is initialized. I think
this is a cleaner approach than modifying the lock class key at runtime.

> I'll be glad to take a series on top of that one if you can
> describe why this one is "more correct" that that one.

I will rebase my patch series on top of your for-next branch. I think
there is agreement that the approach of patch 3/3 in this series is
slightly better than the approach of the patch that has already been
queued.

Thanks,

Bart.