[PATCH 0/3] soundwire: Fixes for spurious and missing UNATTACH

[PATCH 0/3] soundwire: Fixes for spurious and missing UNATTACH

[Richard Fitzgerald]

The bus and cadence code has several bugs that cause UNATTACH notifications
to either be sent spuriously or to be missed.

These can be seen occasionally with a single peripheral on the bus, but are
much more frequent with multiple peripherals, where several peripherals
could change state and report in consecutive PINGs.

The root of all of these bugs seems to be a code design flaw that assumed
every PING status change would be handled separately. However, PINGs are
handled by a workqueue function and there is no guarantee when that function
will be scheduled to run or how much CPU time it will receive. PINGs will
continue while the work function is handling a snapshot of a previous PING
so the code must take account that (a) status could change during the
work function and (b) there can be a backlog of changes before the IRQ work
function runs again.

Richard Fitzgerald (2):
  soundwire: bus: Don't lose unattach notifications
  soundwire: bus: Fix lost UNATTACH when re-enumerating

Simon Trimmer (1):
  soundwire: cadence: fix updating slave status when a bus has multiple
    peripherals

 drivers/soundwire/bus.c            | 44 +++++++++++++++------
 drivers/soundwire/cadence_master.c | 63 +++++++++++++-----------------
 2 files changed, 59 insertions(+), 48 deletions(-)

-- 
2.30.2

[PATCH 1/3] soundwire: cadence: fix updating slave status when a bus has multiple peripherals

[Richard Fitzgerald]

From: Simon Trimmer <simont@opensource.cirrus.com>

The cadence IP explicitly reports slave status changes with bits for
each possible change. The function cdns_update_slave_status() attempts
to translate this into the current status of each of the slaves.

However when there are multiple peripherals on a bus any slave that did
not have a status change when the work function ran would not have it's
status updated - the array is initialised to a value that equates to
UNATTACHED and this can cause spurious reports that slaves had dropped
off the bus.

In the case where a slave has no status change or has multiple status
changes the value from the last PING command is used.

Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 drivers/soundwire/cadence_master.c | 63 +++++++++++++-----------------
 1 file changed, 28 insertions(+), 35 deletions(-)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 4fbb19557f5e..245191d22ccd 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -782,6 +782,7 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns,
 	enum sdw_slave_status status[SDW_MAX_DEVICES + 1];
 	bool is_slave = false;
 	u32 mask;
+	u32 val;
 	int i, set_status;
 
 	memset(status, 0, sizeof(status));
@@ -789,41 +790,38 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns,
 	for (i = 0; i <= SDW_MAX_DEVICES; i++) {
 		mask = (slave_intstat >> (i * CDNS_MCP_SLAVE_STATUS_NUM)) &
 			CDNS_MCP_SLAVE_STATUS_BITS;
-		if (!mask)
-			continue;
 
-		is_slave = true;
 		set_status = 0;
 
-		if (mask & CDNS_MCP_SLAVE_INTSTAT_RESERVED) {
-			status[i] = SDW_SLAVE_RESERVED;
-			set_status++;
+		if (mask) {
+			is_slave = true;
+
+			if (mask & CDNS_MCP_SLAVE_INTSTAT_RESERVED) {
+				status[i] = SDW_SLAVE_RESERVED;
+				set_status++;
+			}
+
+			if (mask & CDNS_MCP_SLAVE_INTSTAT_ATTACHED) {
+				status[i] = SDW_SLAVE_ATTACHED;
+				set_status++;
+			}
+
+			if (mask & CDNS_MCP_SLAVE_INTSTAT_ALERT) {
+				status[i] = SDW_SLAVE_ALERT;
+				set_status++;
+			}
+
+			if (mask & CDNS_MCP_SLAVE_INTSTAT_NPRESENT) {
+				status[i] = SDW_SLAVE_UNATTACHED;
+				set_status++;
+			}
 		}
 
-		if (mask & CDNS_MCP_SLAVE_INTSTAT_ATTACHED) {
-			status[i] = SDW_SLAVE_ATTACHED;
-			set_status++;
-		}
-
-		if (mask & CDNS_MCP_SLAVE_INTSTAT_ALERT) {
-			status[i] = SDW_SLAVE_ALERT;
-			set_status++;
-		}
-
-		if (mask & CDNS_MCP_SLAVE_INTSTAT_NPRESENT) {
-			status[i] = SDW_SLAVE_UNATTACHED;
-			set_status++;
-		}
-
-		/* first check if Slave reported multiple status */
-		if (set_status > 1) {
-			u32 val;
-
-			dev_warn_ratelimited(cdns->dev,
-					     "Slave %d reported multiple Status: %d\n",
-					     i, mask);
-
-			/* check latest status extracted from PING commands */
+		/*
+		 * check that there was a single reported Slave status and when
+		 * there is not use the latest status extracted from PING commands
+		 */
+		if (set_status != 1) {
 			val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT);
 			val >>= (i * 2);
 
@@ -842,11 +840,6 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns,
 				status[i] = SDW_SLAVE_RESERVED;
 				break;
 			}
-
-			dev_warn_ratelimited(cdns->dev,
-					     "Slave %d status updated to %d\n",
-					     i, status[i]);
-
 		}
 	}
 
-- 
2.30.2

[PATCH 2/3] soundwire: bus: Don't lose unattach notifications

[Richard Fitzgerald]

Ensure that if sdw_handle_slave_status() sees a peripheral
has dropped off the bus it reports it to the client driver.

If there are any devices reporting on address 0 it bails out
after programming the device IDs. So it never reaches the second
loop that calls sdw_update_slave_status().

If the missing device is one that is now showing as unenumerated
it has been given a device ID so will report as attached next
time sdw_handle_slave_status() runs.

With the previous code the client driver would only see another
ATTACHED notification because the UNATTACHED state was lost when
sdw_handle_slave_status() bailed out after programming the
device ID.

This shows up most when the peripheral has to be reset after
downloading updated firmware and there are multiple of these
peripherals on the bus. They will all return to unenumerated state
after the reset, and then there is a mix of unattached, attached
and unenumerated PING states from the peripherals, as each is reset
and they reboot.

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

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 704f75c0bae2..bb8ce26c68b3 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1756,6 +1756,11 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 			dev_warn(&slave->dev, "Slave %d state check1: UNATTACHED, status was %d\n",
 				 i, slave->status);
 			sdw_modify_slave_status(slave, SDW_SLAVE_UNATTACHED);
+
+			/* Ensure driver knows that peripheral unattached */
+			ret = sdw_update_slave_status(slave, status[i]);
+			if (ret < 0)
+				dev_warn(&slave->dev, "Update Slave status failed:%d\n", ret);
 		}
 	}
 
-- 
2.30.2

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

[Richard Fitzgerald]

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@opensource.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

Re: [PATCH 2/3] soundwire: bus: Don't lose unattach notifications

[Pierre-Louis Bossart]

On 8/25/22 14:22, Richard Fitzgerald wrote:
> Ensure that if sdw_handle_slave_status() sees a peripheral
> has dropped off the bus it reports it to the client driver.
> 
> If there are any devices reporting on address 0 it bails out
> after programming the device IDs. So it never reaches the second
> loop that calls sdw_update_slave_status().
> 
> If the missing device is one that is now showing as unenumerated
> it has been given a device ID so will report as attached next
> time sdw_handle_slave_status() runs.
> 
> With the previous code the client driver would only see another
> ATTACHED notification because the UNATTACHED state was lost when
> sdw_handle_slave_status() bailed out after programming the
> device ID.
> 
> This shows up most when the peripheral has to be reset after
> downloading updated firmware and there are multiple of these
> peripherals on the bus. They will all return to unenumerated state
> after the reset, and then there is a mix of unattached, attached
> and unenumerated PING states from the peripherals, as each is reset
> and they reboot.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  drivers/soundwire/bus.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 704f75c0bae2..bb8ce26c68b3 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -1756,6 +1756,11 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
>  			dev_warn(&slave->dev, "Slave %d state check1: UNATTACHED, status was %d\n",
>  				 i, slave->status);
>  			sdw_modify_slave_status(slave, SDW_SLAVE_UNATTACHED);
> +
> +			/* Ensure driver knows that peripheral unattached */
> +			ret = sdw_update_slave_status(slave, status[i]);
> +			if (ret < 0)
> +				dev_warn(&slave->dev, "Update Slave status failed:%d\n", ret);

This is indeed a good fix, this will make sure the driver will
re-initialize the device when it reports as ATTACHED again.

The codec driver needs to keep track on the UNATTACHED change though.

This is the case in all codec drivers except rt715 and rt715-sdca.
Something to change in a follow-up patch (cc: Jack Yu).

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

>  		}
>  	}
>  

Re: [PATCH 1/3] soundwire: cadence: fix updating slave status when a bus has multiple peripherals

[Pierre-Louis Bossart]

On 8/25/22 14:22, Richard Fitzgerald wrote:
> From: Simon Trimmer <simont@opensource.cirrus.com>
> 
> The cadence IP explicitly reports slave status changes with bits for
> each possible change. The function cdns_update_slave_status() attempts
> to translate this into the current status of each of the slaves.
> 
> However when there are multiple peripherals on a bus any slave that did
> not have a status change when the work function ran would not have it's
> status updated - the array is initialised to a value that equates to
> UNATTACHED and this can cause spurious reports that slaves had dropped
> off the bus.
> 
> In the case where a slave has no status change or has multiple status
> changes the value from the last PING command is used.
> 
> Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>

Goodness, this is one bug fix.

We've been chasing such issues for a while with 2 amps on the same link

https://github.com/thesofproject/linux/issues/3638
https://github.com/thesofproject/linux/issues/3325

and also came up with the same conclusion that there were false reports
of UNATTACHED, and the conclusion was also to use the PING status directly.

see https://github.com/thesofproject/linux/pull/3786

I think this patch is much better than what I suggested, in that it
fixes the root cause for the false report instead of double-checking if
a device is truly UNATTACHED.

Nice work!

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>


> ---
>  drivers/soundwire/cadence_master.c | 63 +++++++++++++-----------------
>  1 file changed, 28 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 4fbb19557f5e..245191d22ccd 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -782,6 +782,7 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns,
>  	enum sdw_slave_status status[SDW_MAX_DEVICES + 1];
>  	bool is_slave = false;
>  	u32 mask;
> +	u32 val;
>  	int i, set_status;
>  
>  	memset(status, 0, sizeof(status));
> @@ -789,41 +790,38 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns,
>  	for (i = 0; i <= SDW_MAX_DEVICES; i++) {
>  		mask = (slave_intstat >> (i * CDNS_MCP_SLAVE_STATUS_NUM)) &
>  			CDNS_MCP_SLAVE_STATUS_BITS;
> -		if (!mask)
> -			continue;
>  
> -		is_slave = true;
>  		set_status = 0;
>  
> -		if (mask & CDNS_MCP_SLAVE_INTSTAT_RESERVED) {
> -			status[i] = SDW_SLAVE_RESERVED;
> -			set_status++;
> +		if (mask) {
> +			is_slave = true;
> +
> +			if (mask & CDNS_MCP_SLAVE_INTSTAT_RESERVED) {
> +				status[i] = SDW_SLAVE_RESERVED;
> +				set_status++;
> +			}
> +
> +			if (mask & CDNS_MCP_SLAVE_INTSTAT_ATTACHED) {
> +				status[i] = SDW_SLAVE_ATTACHED;
> +				set_status++;
> +			}
> +
> +			if (mask & CDNS_MCP_SLAVE_INTSTAT_ALERT) {
> +				status[i] = SDW_SLAVE_ALERT;
> +				set_status++;
> +			}
> +
> +			if (mask & CDNS_MCP_SLAVE_INTSTAT_NPRESENT) {
> +				status[i] = SDW_SLAVE_UNATTACHED;
> +				set_status++;
> +			}
>  		}
>  
> -		if (mask & CDNS_MCP_SLAVE_INTSTAT_ATTACHED) {
> -			status[i] = SDW_SLAVE_ATTACHED;
> -			set_status++;
> -		}
> -
> -		if (mask & CDNS_MCP_SLAVE_INTSTAT_ALERT) {
> -			status[i] = SDW_SLAVE_ALERT;
> -			set_status++;
> -		}
> -
> -		if (mask & CDNS_MCP_SLAVE_INTSTAT_NPRESENT) {
> -			status[i] = SDW_SLAVE_UNATTACHED;
> -			set_status++;
> -		}
> -
> -		/* first check if Slave reported multiple status */
> -		if (set_status > 1) {
> -			u32 val;
> -
> -			dev_warn_ratelimited(cdns->dev,
> -					     "Slave %d reported multiple Status: %d\n",
> -					     i, mask);
> -
> -			/* check latest status extracted from PING commands */
> +		/*
> +		 * check that there was a single reported Slave status and when
> +		 * there is not use the latest status extracted from PING commands
> +		 */
> +		if (set_status != 1) {
>  			val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT);
>  			val >>= (i * 2);
>  
> @@ -842,11 +840,6 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns,
>  				status[i] = SDW_SLAVE_RESERVED;
>  				break;
>  			}
> -
> -			dev_warn_ratelimited(cdns->dev,
> -					     "Slave %d status updated to %d\n",
> -					     i, status[i]);
> -
>  		}
>  	}
>  

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

[Pierre-Louis Bossart]

Humm, I am struggling a bit more on this patch.

On 8/25/22 14:22, Richard Fitzgerald wrote:
> 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().

Are those two cases really lost completely? It's a bit surprising, I do
recall that we added a recheck on the status, see the 'update_status'
label in cdns_update_slave_status_work

> 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.
The idea of detecting first devices that become unattached - and later
deal with device0 when they re-attach - was based on the fact that
synchronization takes time. The absolute minimum is 16 frames per the
SoundWire spec.

I don't see how testing for the status[0] first in
sdw_handle_slave_status() helps, the value is taken at the same time as
status[1..11]. If you really want to take the last information, we
should re-read the status from a new PING frame.


> 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@opensource.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++) {

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

[Richard Fitzgerald]

On 25/08/2022 15:24, Pierre-Louis Bossart wrote:
> Humm, I am struggling a bit more on this patch.
> 
> On 8/25/22 14:22, Richard Fitzgerald wrote:
>> 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().
> 
> Are those two cases really lost completely? It's a bit surprising, I do
> recall that we added a recheck on the status, see the 'update_status'
> label in cdns_update_slave_status_work
> 

Yes they are. We see this happen extremely frequently (like, almost
every time) when we reset out peripherals after a firmware change.

I saw that "try again" stuff in cdns_update_slave_status_work() but
it's not fixing the problem. Maybe because it's looking for devices
still on #0 but that isn't the problem.

The cdns_update_slave_status_work() is running in one workqueue thread,
child drivers in other threads. So for example:

1. Child driver #1 resets #1
2. PING: #1 has reverted to #0, #2 still ATTACHED
3. cdns_update_slave_status() snapshots the status. #2 is ATTACHED
4. #1 has gone so mark it UNATTACHED
5. Child driver #2 gets some CPU time and reset #2
5. PING: #2 has reset, both now on #0 but we are handling the previous
PING
6. sdw_handle_slave_status() - snapshot PING (from step 3) says #2 is
attached
7. Device on #0 so call sdw_program_device_num()
8. sdw_program_device_num() loops until no devices on #0, #1 and #2
are both reprogrammed, return from sdw_handle_slave_status()
10. PING: #1 and #2 both attached
11. cdns_update_slave_status() -> sdw_handle_slave_status()
12. #1 has changed UNATTACHED->ATTACHED, but we never got a PING with
     #2 unattached so its slave->status==ATTACHED, "it hasn't changed"
     (wrong!)

Now, at step 10 the Cadence IP may have accumlated both UNATTACH and
ATTACH flags, and perhaps it should be smarter about deciding what
to report if there are multiple states. HOWEVER.... that's the behaviour
of Cadence IP, other IP may be different so it's probably unwise to
assume that the IP has "remembered" the UNATTACH state before it was 
reprogrammed.

If we reprogrammed it, it was definitely UNATTACHED so let's say that.

>> 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.
> The idea of detecting first devices that become unattached - and later
> deal with device0 when they re-attach - was based on the fact that
> synchronization takes time. The absolute minimum is 16 frames per the
> SoundWire spec.
> 
> I don't see how testing for the status[0] first in
> sdw_handle_slave_status() helps, the value is taken at the same time as
> status[1..11]. If you really want to take the last information, we
> should re-read the status from a new PING frame.
> 
> 

The point is to deal with unattached devices second, not first.
If we do it first we might find some more that are unattached since
the ping. Moving the unattach check second means we don't have to
do it twice.

>> 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@opensource.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++) {

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

[Pierre-Louis Bossart]

>> On 8/25/22 14:22, Richard Fitzgerald wrote:
>>> 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().
>>
>> Are those two cases really lost completely? It's a bit surprising, I do
>> recall that we added a recheck on the status, see the 'update_status'
>> label in cdns_update_slave_status_work
>>
> 
> Yes they are. We see this happen extremely frequently (like, almost
> every time) when we reset out peripherals after a firmware change.
> 
> I saw that "try again" stuff in cdns_update_slave_status_work() but
> it's not fixing the problem. Maybe because it's looking for devices
> still on #0 but that isn't the problem.
> 
> The cdns_update_slave_status_work() is running in one workqueue thread,
> child drivers in other threads. So for example:
> 
> 1. Child driver #1 resets #1
> 2. PING: #1 has reverted to #0, #2 still ATTACHED
> 3. cdns_update_slave_status() snapshots the status. #2 is ATTACHED
> 4. #1 has gone so mark it UNATTACHED
> 5. Child driver #2 gets some CPU time and reset #2
> 5. PING: #2 has reset, both now on #0 but we are handling the previous
> PING
> 6. sdw_handle_slave_status() - snapshot PING (from step 3) says #2 is
> attached
> 7. Device on #0 so call sdw_program_device_num()
> 8. sdw_program_device_num() loops until no devices on #0, #1 and #2
> are both reprogrammed, return from sdw_handle_slave_status()
> 10. PING: #1 and #2 both attached
> 11. cdns_update_slave_status() -> sdw_handle_slave_status()
> 12. #1 has changed UNATTACHED->ATTACHED, but we never got a PING with
>     #2 unattached so its slave->status==ATTACHED, "it hasn't changed"
>     (wrong!)
> 
> Now, at step 10 the Cadence IP may have accumlated both UNATTACH and
> ATTACH flags, and perhaps it should be smarter about deciding what
> to report if there are multiple states. HOWEVER.... that's the behaviour
> of Cadence IP, other IP may be different so it's probably unwise to
> assume that the IP has "remembered" the UNATTACH state before it was
> reprogrammed.
> 
> If we reprogrammed it, it was definitely UNATTACHED so let's say that.

Thanks for the detailed answer, this sequence of events will certainly
defeat the Cadence IP and the way sticky bits were handled.

The UNATTACHED case was assumed to be a really rare case of losing sync,
i.e. a SOFT_RESET in SoundWire parlance.

If you explicitly do a device reset, that would be a new scenario that
was not considered before on any of the existing SoundWire commercial
devices. It's however something we need to support, and your work here
is much appreciated.

I still think we should re-check the actual status from a PING frame, in
order to work with more current data than the sticky bits taken at an
earlier time, but that would only be a minor improvement.

I also have a vague feeling that additional work is needed to make sure
the DAIs are not used before that second enumeration and all firmware
download complete. I did a couple of tests last year where I used the
debugfs interface to issue a device reset command while streaming audio,
and the detach/reattach was not handled at the ASoC level.

I really don't see any logical flaws in your patch as is, so

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

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

[Richard Fitzgerald]

On 25/08/2022 16:25, Richard Fitzgerald wrote:
> On 25/08/2022 15:24, Pierre-Louis Bossart wrote:
>> Humm, I am struggling a bit more on this patch.
>>
>> On 8/25/22 14:22, Richard Fitzgerald wrote:
>>> 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().
>>
>> Are those two cases really lost completely? It's a bit surprising, I do
>> recall that we added a recheck on the status, see the 'update_status'
>> label in cdns_update_slave_status_work
>>
> 
> Yes they are. We see this happen extremely frequently (like, almost
> every time) when we reset out peripherals after a firmware change.
> 
> I saw that "try again" stuff in cdns_update_slave_status_work() but
> it's not fixing the problem. Maybe because it's looking for devices
> still on #0 but that isn't the problem.
> 
> The cdns_update_slave_status_work() is running in one workqueue thread,
> child drivers in other threads. So for example:
> 
> 1. Child driver #1 resets #1
> 2. PING: #1 has reverted to #0, #2 still ATTACHED
> 3. cdns_update_slave_status() snapshots the status. #2 is ATTACHED
> 4. #1 has gone so mark it UNATTACHED
> 5. Child driver #2 gets some CPU time and reset #2
> 5. PING: #2 has reset, both now on #0 but we are handling the previous
> PING
> 6. sdw_handle_slave_status() - snapshot PING (from step 3) says #2 is
> attached
> 7. Device on #0 so call sdw_program_device_num()
> 8. sdw_program_device_num() loops until no devices on #0, #1 and #2
> are both reprogrammed, return from sdw_handle_slave_status()
> 10. PING: #1 and #2 both attached
> 11. cdns_update_slave_status() -> sdw_handle_slave_status()
> 12. #1 has changed UNATTACHED->ATTACHED, but we never got a PING with
>      #2 unattached so its slave->status==ATTACHED, "it hasn't changed"
>      (wrong!)
> 
> Now, at step 10 the Cadence IP may have accumlated both UNATTACH and
> ATTACH flags, and perhaps it should be smarter about deciding what
> to report if there are multiple states. HOWEVER.... that's the behaviour
> of Cadence IP, other IP may be different so it's probably unwise to
> assume that the IP has "remembered" the UNATTACH state before it was 
> reprogrammed.
> 

After I wrote that I remembered why I rejected that solution. We don't
know what order multiple events happened, so it's not valid to report
a backlogged UNATTACH just becuse it's more "important". It's not
necessarily accurate.

I would worry about this:

Real-world order:

PING: UNATTACH
See device on #0 and program new device ID
PING: ATTACHED

But because of the delay in handling PINGs the software sees:

See device on #0 and program new device ID
PING: UNATTACH
PING: ATTACHED

Giving a false UANATTACH. We know it's unattached if we found it on #0
so setting its state to UNATTACHED ensures our state is accurate.

>> The idea of detecting first devices that become unattached - and later
>> deal with device0 when they re-attach - was based on the fact that
>> synchronization takes time. The absolute minimum is 16 frames per the
>> SoundWire spec.
>>

My expectation was it was to ensure that the slave->dev was marked
UNATTACHED before trying to re-enumerate it. Either way I think it's not
taking into account that we don't know when the workqueue function will
run or how long it will take. There's two chained workqueue functions to
get to the point of handling a PING. So we can't be sure we'll handle a
PING with the device unattaching before we see it on #0.

>> I don't see how testing for the status[0] first in
>> sdw_handle_slave_status() helps, the value is taken at the same time as
>> status[1..11]. If you really want to take the last information, we
>> should re-read the status from a new PING frame.
>>
>>
> 
> The point is to deal with unattached devices second, not first.
> If we do it first we might find some more that are unattached since
> the ping. Moving the unattach check second means we don't have to
> do it twice.
> 

To clarify: the point was that if we check for unattaches first, when
sdw_program_device_num() updates other slaves to UNATTACHED, we would
then have to run the UNATTACHED loop again to deal with those. If we
check for UNATTACHED second, it can pick up all new UNATTACHED in the
one loop. There's no point checking for UNATTACH first since we can't
rely on the old PING showing the unattach before we see that device on
#0.

There is another possible implementation that we only reprogram a device
on #0 if the slave->state == UNATTACHED. I didn't really like that
partly because we're leaving devices on #0 instead of enumerating them,
but also because I worried that it might carry a risk of race
conditions. But if you prefer that option I can try it.

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

[Richard Fitzgerald]

On 26/08/2022 09:06, Pierre-Louis Bossart wrote:
> 
<SNIP>
> 
> Thanks for the detailed answer, this sequence of events will certainly
> defeat the Cadence IP and the way sticky bits were handled.
> 
> The UNATTACHED case was assumed to be a really rare case of losing sync,
> i.e. a SOFT_RESET in SoundWire parlance.
> 
> If you explicitly do a device reset, that would be a new scenario that
> was not considered before on any of the existing SoundWire commercial
> devices. It's however something we need to support, and your work here
> is much appreciated.
> 
> I still think we should re-check the actual status from a PING frame, in
> order to work with more current data than the sticky bits taken at an
> earlier time, but that would only be a minor improvement.
> 
> I also have a vague feeling that additional work is needed to make sure
> the DAIs are not used before that second enumeration and all firmware
> download complete. I did a couple of tests last year where I used the
> debugfs interface to issue a device reset command while streaming audio,
> and the detach/reattach was not handled at the ASoC level.
> 
> I really don't see any logical flaws in your patch as is, so
> 
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 

I have pushed an alternative fix that waits until it sees an UNATTACHED
status before reprogramming the device ID.
https://lore.kernel.org/lkml/20220829094458.1169504-1-rf@opensource.cirrus.com/T/#t

I've tested it with 4 amps on the same bus, all being reset after their
firmware has been downloaded.

I leave it to you to choose which fix you prefer. The second fix is
simpler and I didn't see any problems in testing.

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

[Richard Fitzgerald]

On 25/08/2022 13:22, Richard Fitzgerald wrote:
> 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.
> 

Don't use this patch!
I found there's a race condition with the Cadence interrupts.
Use my alternative fix.