linux-kernel - [PATCH 3/3] soundwire: bus: Fix lost UNATTACH when re-enumerating

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Aug 2022 13:22:41 +0100
From:   Richard Fitzgerald <rf@...nsource.cirrus.com>
To:     <vkoul@...nel.org>, <yung-chuan.liao@...ux.intel.com>,
        <pierre-louis.bossart@...ux.intel.com>, <sanyog.r.kale@...el.com>
CC:     <alsa-devel@...a-project.org>, <linux-kernel@...r.kernel.org>,
        <patches@...nsource.cirrus.com>,
        Richard Fitzgerald <rf@...nsource.cirrus.com>
Subject: [PATCH 3/3] soundwire: bus: Fix lost UNATTACH when re-enumerating

Rearrange sdw_handle_slave_status() so that any peripherals
on device #0 that are given a device ID are reported as
unattached. The ensures that UNATTACH status is not lost.

Handle unenumerated devices first and update the
sdw_slave_status array to indicate IDs that must have become
UNATTACHED.

Look for UNATTACHED devices after this so we can pick up
peripherals that were UNATTACHED in the original PING status
and those that were still ATTACHED at the time of the PING but
then reverted to unenumerated and were found by
sdw_program_device_num().

As sdw_update_slave_status() is always processing a snapshot of
a PING from some time in the past, it is possible that the status
is changing while sdw_update_slave_status() is running.

A peripheral could report attached in the PING, but detach and
revert to device #0 and then be found in the loop in
sdw_program_device_num(). Previously the code would not have
updated slave->status to UNATTACHED because there was never a
PING with that status. If the slave->status is not updated to
UNATTACHED the next PING will report it as ATTACHED, but its
slave->status is already ATTACHED so the re-attach will not be
properly handled.

This situations happens fairly frequently with multiple
peripherals on a bus that are intentionally reset (for example
after downloading firmware).

Signed-off-by: Richard Fitzgerald <rf@...nsource.cirrus.com>
---
 drivers/soundwire/bus.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index bb8ce26c68b3..1212148ac251 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -718,7 +718,8 @@ void sdw_extract_slave_id(struct sdw_bus *bus,
 }
 EXPORT_SYMBOL(sdw_extract_slave_id);

-static int sdw_program_device_num(struct sdw_bus *bus)
+static int sdw_program_device_num(struct sdw_bus *bus,
+				  enum sdw_slave_status status[])
 {
 	u8 buf[SDW_NUM_DEV_ID_REGISTERS] = {0};
 	struct sdw_slave *slave, *_s;
@@ -776,6 +777,12 @@ static int sdw_program_device_num(struct sdw_bus *bus)
 					return ret;
 				}

+				/*
+				 * It could have dropped off the bus since the
+				 * PING response so update the status array.
+				 */
+				status[slave->dev_num] = SDW_SLAVE_UNATTACHED;
+
 				break;
 			}
 		}
@@ -1735,10 +1742,21 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 {
 	enum sdw_slave_status prev_status;
 	struct sdw_slave *slave;
+	bool programmed_dev_num = false;
 	bool attached_initializing;
 	int i, ret = 0;

-	/* first check if any Slaves fell off the bus */
+	/* Handle any unenumerated peripherals */
+	if (status[0] == SDW_SLAVE_ATTACHED) {
+		dev_dbg(bus->dev, "Slave attached, programming device number\n");
+		ret = sdw_program_device_num(bus, status);
+		if (ret < 0)
+			dev_warn(bus->dev, "Slave attach failed: %d\n", ret);
+
+		programmed_dev_num = true;
+	}
+
+	/* Check if any fell off the bus */
 	for (i = 1; i <= SDW_MAX_DEVICES; i++) {
 		mutex_lock(&bus->bus_lock);
 		if (test_bit(i, bus->assigned) == false) {
@@ -1764,17 +1782,12 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 		}
 	}

-	if (status[0] == SDW_SLAVE_ATTACHED) {
-		dev_dbg(bus->dev, "Slave attached, programming device number\n");
-		ret = sdw_program_device_num(bus);
-		if (ret < 0)
-			dev_err(bus->dev, "Slave attach failed: %d\n", ret);
-		/*
-		 * programming a device number will have side effects,
-		 * so we deal with other devices at a later time
-		 */
-		return ret;
-	}
+	/*
+	 * programming a device number will have side effects,
+	 * so we deal with other devices at a later time
+	 */
+	if (programmed_dev_num)
+		return 0;

 	/* Continue to check other slave statuses */
 	for (i = 1; i <= SDW_MAX_DEVICES; i++) {
-- 
2.30.2