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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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++) {