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