Re: [PATCH 1/2] soundwire: bus: Prevent lockdep asserts when stream has multiple buses

[Pierre-Louis Bossart]

On 6/15/23 16:12, Richard Fitzgerald wrote:
> Give the bus_lock and msg_lock of each bus a different unique key
> so that it is possible to acquire the locks of multiple buses
> without lockdep asserting a possible deadlock.
> 
> Using mutex_init() to initialize a mutex gives all those mutexes
> the same lock class. Lockdep checking treats it as an error to
> attempt to take a mutex while already holding a mutex of the same
> class. This causes a lockdep assert when sdw_acquire_bus_lock()
> attempts to lock multiple buses, and when do_bank_switch() takes
> multiple msg_lock.
> 
> [  138.697350] WARNING: possible recursive locking detected
> [  138.697366] 6.3.0-test #1 Tainted: G            E
> [  138.697380] --------------------------------------------
> [  138.697394] play/903 is trying to acquire lock:
> [  138.697409] ffff99b8c41aa8c8 (&bus->bus_lock){+.+.}-{3:3}, at:
> sdw_prepare_stream+0x52/0x2e0
> [  138.697443]
>                but task is already holding lock:
> [  138.697468] ffff99b8c41af8c8 (&bus->bus_lock){+.+.}-{3:3}, at:
> sdw_prepare_stream+0x52/0x2e0
> [  138.697493]
>                other info that might help us debug this:
> [  138.697521]  Possible unsafe locking scenario:
> 
> [  138.697540]        CPU0
> [  138.697550]        ----
> [  138.697559]   lock(&bus->bus_lock);
> [  138.697570]   lock(&bus->bus_lock);
> [  138.697581]
>                 *** DEADLOCK ***
> 
> Giving each mutex a unique key allows multiple to be held
> without triggering a lockdep assert. But note that it does not
> allow them to be taken in one order then a different order.
> If two mutexes are taken in the order A, B then they must
> always be taken in that order otherwise they could deadlock.
> 
> Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>

> ---
>  drivers/soundwire/bus.c       | 15 +++++++++++++--
>  include/linux/soundwire/sdw.h |  3 +++
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index b44f8d0affa6..dba920ec88f6 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -69,8 +69,17 @@ int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
>  		return -EINVAL;
>  	}
>  
> -	mutex_init(&bus->msg_lock);
> -	mutex_init(&bus->bus_lock);
> +	/*
> +	 * Give each bus_lock and msg_lock a unique key so that lockdep won't
> +	 * trigger a deadlock warning when the locks of several buses are
> +	 * grabbed during configuration of a multi-bus stream.
> +	 */
> +	lockdep_register_key(&bus->msg_lock_key);
> +	__mutex_init(&bus->msg_lock, "msg_lock", &bus->msg_lock_key);
> +
> +	lockdep_register_key(&bus->bus_lock_key);
> +	__mutex_init(&bus->bus_lock, "bus_lock", &bus->bus_lock_key);
> +
>  	INIT_LIST_HEAD(&bus->slaves);
>  	INIT_LIST_HEAD(&bus->m_rt_list);
>  
> @@ -181,6 +190,8 @@ void sdw_bus_master_delete(struct sdw_bus *bus)
>  	sdw_master_device_del(bus);
>  
>  	sdw_bus_debugfs_exit(bus);
> +	lockdep_unregister_key(&bus->bus_lock_key);
> +	lockdep_unregister_key(&bus->msg_lock_key);
>  	ida_free(&sdw_bus_ida, bus->id);
>  }
>  EXPORT_SYMBOL(sdw_bus_master_delete);
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index c076a3f879b3..f523ceabd059 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -5,6 +5,7 @@
>  #define __SOUNDWIRE_H
>  
>  #include <linux/bug.h>
> +#include <linux/lockdep_types.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/bitfield.h>
>  
> @@ -907,7 +908,9 @@ struct sdw_bus {
>  	struct list_head slaves;
>  	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
>  	struct mutex bus_lock;
> +	struct lock_class_key bus_lock_key;
>  	struct mutex msg_lock;
> +	struct lock_class_key msg_lock_key;
>  	int (*compute_params)(struct sdw_bus *bus);
>  	const struct sdw_master_ops *ops;
>  	const struct sdw_master_port_ops *port_ops;