soundwire: bus: Don't exit early if no device IDs were programmed

Only exit sdw_handle_slave_status() right after calling
sdw_program_device_num() if it actually programmed an ID into at
least one device.

sdw_handle_slave_status() should protect itself against phantom
device #0 ATTACHED indications. In that case there is no actual
device still on #0. The early exit relies on there being a status
change to ATTACHED on the reprogrammed device to trigger another
call to sdw_handle_slave_status() which will then handle the status
of all peripherals. If no device was actually programmed with an
ID there won't be a new ATTACHED indication. This can lead to the
status of other peripherals not being handled.

The status passed to sdw_handle_slave_status() is obviously always
from a point of time in the past, and may indicate accumulated
unhandled events (depending how the bus manager operates). It's
possible that a device ID is reprogrammed but the last PING status
captured state just before that, when it was still reporting on
ID #0. Then sdw_handle_slave_status() is called with this PING info,
just before a new PING status is available showing it now on its new
ID. So sdw_handle_slave_status() will receive a phantom report of a
device on #0, but it will not find one.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Link: https://lore.kernel.org/r/20220914160248.1047627-6-rf@opensource.cirrus.com
Signed-off-by: Vinod Koul <vkoul@kernel.org>

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 6e569a8..8eded1a 100644 (file)
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -729,7 +729,7 @@ 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, bool *programmed)
 {
        u8 buf[SDW_NUM_DEV_ID_REGISTERS] = {0};
        struct sdw_slave *slave, *_s;
@@ -739,6 +739,8 @@ static int sdw_program_device_num(struct sdw_bus *bus)
        int count = 0, ret;
        u64 addr;
 
+       *programmed = false;
+
        /* No Slave, so use raw xfer api */
        ret = sdw_fill_msg(&msg, NULL, SDW_SCP_DEVID_0,
                           SDW_NUM_DEV_ID_REGISTERS, 0, SDW_MSG_FLAG_READ, buf);
@@ -797,6 +799,8 @@ static int sdw_program_device_num(struct sdw_bus *bus)
                                        return ret;
                                }
 
+                               *programmed = true;
+
                                break;
                        }
                }
@@ -1756,7 +1760,7 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 {
        enum sdw_slave_status prev_status;
        struct sdw_slave *slave;
-       bool attached_initializing;
+       bool attached_initializing, id_programmed;
        int i, ret = 0;
 
        /* first check if any Slaves fell off the bus */
@@ -1787,14 +1791,23 @@ 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
+                * Programming a device number will have side effects,
+                * so we deal with other devices at a later time.
+                * This relies on those devices reporting ATTACHED, which will
+                * trigger another call to this function. This will only
+                * happen if at least one device ID was programmed.
+                * Error returns from sdw_program_device_num() are currently
+                * ignored because there's no useful recovery that can be done.
+                * Returning the error here could result in the current status
+                * of other devices not being handled, because if no device IDs
+                * were programmed there's nothing to guarantee a status change
+                * to trigger another call to this function.
                 */
-               return ret;
+               sdw_program_device_num(bus, &id_programmed);
+               if (id_programmed)
+                       return 0;
        }
 
        /* Continue to check other slave statuses */