From d68a2ced8a0555c5f6b29a1f38f91cf82cb0224d Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 18 May 2026 13:06:32 +0100 Subject: [PATCH] soundwire: Use get_device()/put_device() to protect callbacks Use get_device()/put_device() around calling driver callbacks instead of holding a mutex. This avoids mutex nesting and possible mutex inversion. Rename sdw_dev_lock to probe_remove_lock to make its purpose explicit. When the core SoundWire code calls a driver callback it must be sure that the target driver is not unloaded while the callback is running. The code was using the sdw_dev_lock mutex to block driver remove() while the callback is executing. This meant the callbacks were always called while holding that mutex, which could lead to mutex inversion. For example during enumeration: 1) Take sdw_dev_lock 2) Call update_status() 3) Take regmap lock But during BRA activity we could have: 1) Take regmap lock 2) Start BRA transaction 3) Take sdw_dev_lock 4) Call port_prep() and/or bus_config() callbacks A better way to prevent the target driver being unloaded is to increment its reference count by calling get_device(). This pattern is seen in other drivers that need to call callbacks in another device. The mutex is only needed around the call to get_device() to prevent the driver being removed while its reference count is being incremented. After that, the mutex can be released becase the driver cannot be removed until its reference count is decremented. Change-Id: I1f5cfd81d6204cf3aa92ddc2896af17307edd9c1 Signed-off-by: Richard Fitzgerald --- drivers/soundwire/bus.c | 24 +++++++++--------------- drivers/soundwire/bus.h | 3 +++ drivers/soundwire/bus_type.c | 8 ++++---- drivers/soundwire/slave.c | 21 +++++++++++++++++++-- drivers/soundwire/stream.c | 18 +++++++----------- include/linux/soundwire/sdw.h | 4 ++-- 6 files changed, 44 insertions(+), 34 deletions(-) diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 1ce708657d8c60..317e6c34fc819b 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -962,17 +962,15 @@ static int sdw_slave_clk_stop_callback(struct sdw_slave *slave, { int ret = 0; - mutex_lock(&slave->sdw_dev_lock); - - if (slave->probed) { + if (sdw_slave_device_get(slave)) { struct device *dev = &slave->dev; struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); if (drv->ops && drv->ops->clk_stop) ret = drv->ops->clk_stop(slave, mode, type); - } - mutex_unlock(&slave->sdw_dev_lock); + sdw_slave_device_put(slave); + } return ret; } @@ -1799,9 +1797,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) if (slave->prop.use_domain_irq && slave->irq) handle_nested_irq(slave->irq); - mutex_lock(&slave->sdw_dev_lock); - - if (slave->probed) { + if (sdw_slave_device_get(slave)) { struct device *dev = &slave->dev; struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); @@ -1813,9 +1809,9 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) drv->ops->interrupt_callback(slave, &slave_intr); } - } - mutex_unlock(&slave->sdw_dev_lock); + sdw_slave_device_put(slave); + } } /* Ack interrupt */ @@ -1887,17 +1883,15 @@ static int sdw_update_slave_status(struct sdw_slave *slave, { int ret = 0; - mutex_lock(&slave->sdw_dev_lock); - - if (slave->probed) { + if (sdw_slave_device_get(slave)) { struct device *dev = &slave->dev; struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); if (drv->ops && drv->ops->update_status) ret = drv->ops->update_status(slave, status); - } - mutex_unlock(&slave->sdw_dev_lock); + sdw_slave_device_put(slave); + } return ret; } diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index 8115c64dd48e2f..6b2982877b7de4 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -18,6 +18,9 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus) } #endif +struct device *sdw_slave_device_get(struct sdw_slave *slave); +void sdw_slave_device_put(struct sdw_slave *slave); + int sdw_of_find_slaves(struct sdw_bus *bus); void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id); diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index a05aa36828cbd6..6225cbe97aa849 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -112,7 +112,7 @@ static int sdw_bus_probe(struct device *dev) return ret; } - mutex_lock(&slave->sdw_dev_lock); + mutex_lock(&slave->probe_remove_lock); /* device is probed so let's read the properties now */ if (drv->ops && drv->ops->read_prop) @@ -151,7 +151,7 @@ static int sdw_bus_probe(struct device *dev) dev_warn(dev, "failed to update status at probe: %d\n", ret); } - mutex_unlock(&slave->sdw_dev_lock); + mutex_unlock(&slave->probe_remove_lock); dev_dbg(dev, "probe complete\n"); @@ -163,11 +163,11 @@ static void sdw_bus_remove(struct device *dev) struct sdw_slave *slave = dev_to_sdw_dev(dev); struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); - mutex_lock(&slave->sdw_dev_lock); + mutex_lock(&slave->probe_remove_lock); slave->probed = false; - mutex_unlock(&slave->sdw_dev_lock); + mutex_unlock(&slave->probe_remove_lock); if (drv->remove) drv->remove(slave); diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index e0c49cbcb1bab4..22d89315244b73 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -2,6 +2,8 @@ // Copyright(c) 2015-17 Intel Corporation. #include +#include +#include #include #include #include @@ -9,12 +11,27 @@ #include "bus.h" #include "sysfs_local.h" +struct device *sdw_slave_device_get(struct sdw_slave *slave) +{ + guard(mutex)(&slave->probe_remove_lock); + + if (!slave->probed) + return NULL; + + return get_device(&slave->dev); +} + +void sdw_slave_device_put(struct sdw_slave *slave) +{ + put_device(&slave->dev); +} + static void sdw_slave_release(struct device *dev) { struct sdw_slave *slave = dev_to_sdw_dev(dev); of_node_put(slave->dev.of_node); - mutex_destroy(&slave->sdw_dev_lock); + mutex_destroy(&slave->probe_remove_lock); kfree(slave); } @@ -64,7 +81,7 @@ int sdw_slave_add(struct sdw_bus *bus, slave->dev_num = 0; slave->probed = false; slave->first_interrupt_done = false; - mutex_init(&slave->sdw_dev_lock); + mutex_init(&slave->probe_remove_lock); for (i = 0; i < SDW_MAX_PORTS; i++) init_completion(&slave->port_ready[i]); diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index cbf7bd3d4e7bac..b71bfc599f6660 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -430,9 +430,7 @@ static int sdw_do_port_prep(struct sdw_slave_runtime *s_rt, int ret = 0; struct sdw_slave *slave = s_rt->slave; - mutex_lock(&slave->sdw_dev_lock); - - if (slave->probed) { + if (sdw_slave_device_get(slave)) { struct device *dev = &slave->dev; struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); @@ -442,9 +440,9 @@ static int sdw_do_port_prep(struct sdw_slave_runtime *s_rt, dev_err(dev, "Slave Port Prep cmd %d failed: %d\n", cmd, ret); } - } - mutex_unlock(&slave->sdw_dev_lock); + sdw_slave_device_put(slave); + } return ret; } @@ -642,9 +640,7 @@ static int sdw_notify_config(struct sdw_master_runtime *m_rt) list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) { slave = s_rt->slave; - mutex_lock(&slave->sdw_dev_lock); - - if (slave->probed) { + if (sdw_slave_device_get(slave)) { struct device *dev = &slave->dev; struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); @@ -653,13 +649,13 @@ static int sdw_notify_config(struct sdw_master_runtime *m_rt) if (ret < 0) { dev_err(dev, "Notify Slave: %d failed\n", slave->dev_num); - mutex_unlock(&slave->sdw_dev_lock); + sdw_slave_device_put(slave); return ret; } } - } - mutex_unlock(&slave->sdw_dev_lock); + sdw_slave_device_put(slave); + } } return 0; diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index a46cbaec594912..8026b9e6232495 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -659,7 +659,7 @@ struct sdw_slave_ops { * for a Slave happens for the first time after enumeration * @is_mockup_device: status flag used to squelch errors in the command/control * protocol for SoundWire mockup devices - * @sdw_dev_lock: mutex used to protect callbacks/remove races + * @probe_remove_lock: mutex used to protect callbacks/remove races * @sdca_data: structure containing all device data for SDCA helpers */ struct sdw_slave { @@ -684,7 +684,7 @@ struct sdw_slave { u32 unattach_request; bool first_interrupt_done; bool is_mockup_device; - struct mutex sdw_dev_lock; /* protect callbacks/remove races */ + struct mutex probe_remove_lock; struct sdca_device_data sdca_data; };