[alsa-devel] [PATCH] soundwire: bus: fix device number leak on errors
If the programming of the dev_number fails due to an IO error, a new device_number will be assigned, resulting in a leak.
Make sure we only assign a device_number once per Slave device.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/bus.c | 37 ++++++++++++++++++++++------------- include/linux/soundwire/sdw.h | 4 +++- 2 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index be5d437058ed..8f973e70e6f7 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -456,26 +456,35 @@ static int sdw_get_device_num(struct sdw_slave *slave) static int sdw_assign_device_num(struct sdw_slave *slave) { int ret, dev_num; + bool new_device = false;
/* check first if device number is assigned, if so reuse that */ if (!slave->dev_num) { - mutex_lock(&slave->bus->bus_lock); - dev_num = sdw_get_device_num(slave); - mutex_unlock(&slave->bus->bus_lock); - if (dev_num < 0) { - dev_err(slave->bus->dev, "Get dev_num failed: %d\n", - dev_num); - return dev_num; + if (!slave->dev_num_sticky) { + mutex_lock(&slave->bus->bus_lock); + dev_num = sdw_get_device_num(slave); + mutex_unlock(&slave->bus->bus_lock); + if (dev_num < 0) { + dev_err(slave->bus->dev, "Get dev_num failed: %d\n", + dev_num); + return dev_num; + } + slave->dev_num = dev_num; + slave->dev_num_sticky = dev_num; + new_device = true; + } else { + slave->dev_num = slave->dev_num_sticky; } - } else { + } + + if (!new_device) dev_info(slave->bus->dev, - "Slave already registered dev_num:%d\n", + "Slave already registered, reusing dev_num:%d\n", slave->dev_num);
- /* Clear the slave->dev_num to transfer message on device 0 */ - dev_num = slave->dev_num; - slave->dev_num = 0; - } + /* Clear the slave->dev_num to transfer message on device 0 */ + dev_num = slave->dev_num; + slave->dev_num = 0;
ret = sdw_write(slave, SDW_SCP_DEVNUMBER, dev_num); if (ret < 0) { @@ -485,7 +494,7 @@ static int sdw_assign_device_num(struct sdw_slave *slave) }
/* After xfer of msg, restore dev_num */ - slave->dev_num = dev_num; + slave->dev_num = slave->dev_num_sticky;
return 0; } diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index b7c9eca4332a..b451bb622335 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -546,7 +546,8 @@ struct sdw_slave_ops { * @debugfs: Slave debugfs * @node: node for bus list * @port_ready: Port ready completion flag for each Slave port - * @dev_num: Device Number assigned by Bus + * @dev_num: Current Device Number, values can be 0 or dev_num_sticky + * @dev_num_sticky: one-time static Device Number assigned by Bus * @probed: boolean tracking driver state * @probe_complete: completion utility to control potential races * on startup between driver probe/initialization and SoundWire @@ -575,6 +576,7 @@ struct sdw_slave { struct list_head node; struct completion *port_ready; u16 dev_num; + u16 dev_num_sticky; bool probed; struct completion probe_complete; struct completion enumeration_complete;
On 13-01-20, 16:56, Pierre-Louis Bossart wrote:
If the programming of the dev_number fails due to an IO error, a new device_number will be assigned, resulting in a leak.
Make sure we only assign a device_number once per Slave device.
Although I am not sure if this would be a leak, we assign a new num and old number should have gotten recycled as they would be unattached status.
Anyway this is good improvement as it helps to debug having same dev_num, so Applied, thanks
On 1/14/20 12:37 AM, Vinod Koul wrote:
On 13-01-20, 16:56, Pierre-Louis Bossart wrote:
If the programming of the dev_number fails due to an IO error, a new device_number will be assigned, resulting in a leak.
Make sure we only assign a device_number once per Slave device.
Although I am not sure if this would be a leak, we assign a new num and old number should have gotten recycled as they would be unattached status.
When you program the device number and it fails, there is still a Device0 reporting as attached, so you will loop and try to assign a new device number. In this case there is never a transition to UNATTACHED, the Slave remains ATTACHED as Device0 until the enumeration succeed with a successful non-zero device number.
This only happened to us w/ early prototypes where the PCB routing was questionable and the speed too high, but still it's useful to keep this device number constant
Anyway this is good improvement as it helps to debug having same dev_num, so Applied, thanks
Thanks.
participants (2)
-
Pierre-Louis Bossart -
Vinod Koul