[PATCH 0/3] i2c: mux: pca954x: Track unknown mux state and report deselect failures

[PATCH 0/3] i2c: mux: pca954x: Track unknown mux state and report deselect failures

[Bart Van Assche]

Hi Peter,

If a control register write to a pca954x mux fails (e.g. if only the ACK
was lost), the write may still have reached the hardware, leaving the
mux in an unknown state. Currently the driver records last_chan as 0
(disconnected) after any failed write, which causes idle_state_store()
to skip the disconnect write if the idle state is updated to
MUX_IDLE_DISCONNECT. In addition, failures in pca954x_deselect_mux() go
unnoticed because the i2c-mux core ignores the deselect return value.
Track an unknown mux state by setting last_chan to -1 after any failed
control register write, split out pca954x_set_idle_state() from
pca954x_deselect_mux(), and log a rate-limited warning when deselecting
a channel fails.

Please consider this series for the next merge window.

Thanks,

Bart.

Bart Van Assche (3):
  i2c: mux: pca954x: Track unknown channel state
  i2c: mux: pca954x: Introduce pca954x_set_idle_state()
  i2c: mux: pca954x: Report deselect failures

 drivers/i2c/muxes/i2c-mux-pca954x.c | 60 +++++++++++++++++++----------
 1 file changed, 40 insertions(+), 20 deletions(-)

[PATCH 1/3] i2c: mux: pca954x: Track unknown channel state

[Bart Van Assche]

pca954x sets last_chan to 0 (disconnected) if initializing, selecting
or disconnecting the mux fails. A failed write may have reached the
mux, e.g. if only the ACK was lost, so the mux state is unknown after
such a failure. Since idle_state_store() skips the disconnect write if
last_chan is 0 and the new idle state is MUX_IDLE_DISCONNECT, a channel
may remain connected after the idle state has been set to disconnect.

Set last_chan to -1 (unknown) after any failed control register write
so that idle_state_store() no longer mistakes an unknown mux state for
disconnected. Make the last_chan test in idle_state_store() explicit
since last_chan is no longer a plain register value. Make
pca954x_init() use a local variable for the register value so that
last_chan is only updated once the outcome of the register write is
known.

Fixes: f1fb64b04bf4 ("i2c: mux: pca954x: allow management of device idle state via sysfs")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 8fca709ed279..e34b4668a1fa 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -107,7 +107,7 @@ struct chip_desc {
 struct pca954x {
 	const struct chip_desc *chip;
 
-	u8 last_chan;		/* last register value */
+	s16 last_chan;		/* last register value, or -1 if unknown */
 	/* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
 	s32 idle_state;
 
@@ -327,7 +327,7 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
 	/* Only select the channel if its different from the last channel */
 	if (data->last_chan != regval) {
 		ret = pca954x_reg_write(muxc->parent, client, regval);
-		data->last_chan = ret < 0 ? 0 : regval;
+		data->last_chan = ret < 0 ? -1 : regval;
 	}
 
 	return ret;
@@ -338,6 +338,7 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 	struct pca954x *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
 	s32 idle_state;
+	int ret;
 
 	idle_state = READ_ONCE(data->idle_state);
 	if (idle_state >= 0)
@@ -346,9 +347,9 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 
 	if (idle_state == MUX_IDLE_DISCONNECT) {
 		/* Deselect active channel */
-		data->last_chan = 0;
-		return pca954x_reg_write(muxc->parent, client,
-					 data->last_chan);
+		ret = pca954x_reg_write(muxc->parent, client, 0);
+		data->last_chan = ret < 0 ? -1 : 0;
+		return ret;
 	}
 
 	/* otherwise leave as-is */
@@ -392,7 +393,7 @@ static ssize_t idle_state_store(struct device *dev,
 	 * Set the mux into a state consistent with the new
 	 * idle_state.
 	 */
-	if (data->last_chan || val != MUX_IDLE_DISCONNECT)
+	if (data->last_chan != 0 || val != MUX_IDLE_DISCONNECT)
 		ret = pca954x_deselect_mux(muxc, 0);
 
 	i2c_unlock_bus(muxc->parent, I2C_LOCK_SEGMENT);
@@ -480,12 +481,13 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
 
 static int pca954x_init(struct i2c_client *client, struct pca954x *data)
 {
+	u8 regval;
 	int ret;
 
 	if (data->idle_state >= 0)
-		data->last_chan = pca954x_regval(data, data->idle_state);
+		regval = pca954x_regval(data, data->idle_state);
 	else
-		data->last_chan = 0; /* Disconnect multiplexer */
+		regval = 0; /* Disconnect multiplexer */
 
 	if (device_is_compatible(&client->dev, "maxim,max7357")) {
 		if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
@@ -505,18 +507,17 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data)
 						      "maxim,preconnection-wiggle-test-enable"))
 				conf |= MAX7357_CONF_PRECONNECT_TEST;
 
-			ret = i2c_smbus_write_byte_data(client, data->last_chan, conf);
+			ret = i2c_smbus_write_byte_data(client, regval, conf);
 		} else {
 			dev_warn(&client->dev, "Write byte data not supported."
 				 "Cannot enable enhanced mode features\n");
-			ret = i2c_smbus_write_byte(client, data->last_chan);
+			ret = i2c_smbus_write_byte(client, regval);
 		}
 	} else {
-		ret = i2c_smbus_write_byte(client, data->last_chan);
+		ret = i2c_smbus_write_byte(client, regval);
 	}
 
-	if (ret < 0)
-		data->last_chan = 0;
+	data->last_chan = ret < 0 ? -1 : regval;
 
 	return ret;
 }

[PATCH 2/3] i2c: mux: pca954x: Introduce pca954x_set_idle_state()

[Bart Van Assche]

idle_state_store() calls pca954x_deselect_mux() with a dummy channel
number of 0 since it only needs to program the mux into the state that
corresponds to idle_state. Move the body of pca954x_deselect_mux() into
a new function pca954x_set_idle_state() that does not have a channel
argument, and call that function from idle_state_store(). This prepares
for making pca954x_deselect_mux() report failures that include the
channel number. Also convert the if-statement for MUX_IDLE_DISCONNECT
into an early return.

No functional change intended.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index e34b4668a1fa..91df2d2fa2b7 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -333,7 +333,8 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
 	return ret;
 }
 
-static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
+/* Program the mux into the state that corresponds to idle_state. */
+static int pca954x_set_idle_state(struct i2c_mux_core *muxc)
 {
 	struct pca954x *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
@@ -345,16 +346,20 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 		/* Set the mux back to a predetermined channel */
 		return pca954x_select_chan(muxc, idle_state);
 
-	if (idle_state == MUX_IDLE_DISCONNECT) {
-		/* Deselect active channel */
-		ret = pca954x_reg_write(muxc->parent, client, 0);
-		data->last_chan = ret < 0 ? -1 : 0;
-		return ret;
-	}
+	/* Leave the mux as-is unless idle_state is MUX_IDLE_DISCONNECT */
+	if (idle_state != MUX_IDLE_DISCONNECT)
+		return 0;
 
-	/* otherwise leave as-is */
+	/* Deselect active channel */
+	ret = pca954x_reg_write(muxc->parent, client, 0);
+	data->last_chan = ret < 0 ? -1 : 0;
 
-	return 0;
+	return ret;
+}
+
+static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
+{
+	return pca954x_set_idle_state(muxc);
 }
 
 static ssize_t idle_state_show(struct device *dev,
@@ -394,7 +399,7 @@ static ssize_t idle_state_store(struct device *dev,
 	 * idle_state.
 	 */
 	if (data->last_chan != 0 || val != MUX_IDLE_DISCONNECT)
-		ret = pca954x_deselect_mux(muxc, 0);
+		ret = pca954x_set_idle_state(muxc);
 
 	i2c_unlock_bus(muxc->parent, I2C_LOCK_SEGMENT);
 

[PATCH 3/3] i2c: mux: pca954x: Report deselect failures

[Bart Van Assche]

The i2c-mux core ignores the return value of the deselect callback.
Hence, if programming the mux into its idle state fails, the failure
goes unnoticed and a channel may remain connected until the next
transfer reprograms the mux. Log a rate-limited warning if
pca954x_deselect_mux() fails.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 91df2d2fa2b7..45a7d841e7cf 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -359,7 +359,21 @@ static int pca954x_set_idle_state(struct i2c_mux_core *muxc)
 
 static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 {
-	return pca954x_set_idle_state(muxc);
+	struct pca954x *data = i2c_mux_priv(muxc);
+	int ret;
+
+	ret = pca954x_set_idle_state(muxc);
+	/*
+	 * The i2c-mux core ignores the deselect return value. Report failures
+	 * since these may leave a channel connected until the next transfer
+	 * reprograms the mux.
+	 */
+	if (ret < 0)
+		dev_warn_ratelimited(&data->client->dev,
+				     "failed to deselect channel %u: %d\n",
+				     chan, ret);
+
+	return ret;
 }
 
 static ssize_t idle_state_show(struct device *dev,