Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [alsa-devel] [PATCH] soundwire: bus: fix device number leak on errors
@ 2020-01-13 22:56 Pierre-Louis Bossart
  2020-01-14  6:37 ` Vinod Koul
  0 siblings, 1 reply; 3+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-13 22:56 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

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;
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [alsa-devel] [PATCH] soundwire: bus: fix device number leak on errors
  2020-01-13 22:56 [alsa-devel] [PATCH] soundwire: bus: fix device number leak on errors Pierre-Louis Bossart
@ 2020-01-14  6:37 ` Vinod Koul
  2020-01-14 16:10   ` Pierre-Louis Bossart
  0 siblings, 1 reply; 3+ messages in thread
From: Vinod Koul @ 2020-01-14  6:37 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang

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

-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [alsa-devel] [PATCH] soundwire: bus: fix device number leak on errors
  2020-01-14  6:37 ` Vinod Koul
@ 2020-01-14 16:10   ` Pierre-Louis Bossart
  0 siblings, 0 replies; 3+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-14 16:10 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak,
	Sanyog Kale, Bard liao, Rander Wang



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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13 22:56 [alsa-devel] [PATCH] soundwire: bus: fix device number leak on errors Pierre-Louis Bossart
2020-01-14  6:37 ` Vinod Koul
2020-01-14 16:10   ` Pierre-Louis Bossart

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git