[RFC PATCH] i2c: algo-bit: return -EBUSY if the bus is not idle after STOP

[RFC PATCH] i2c: algo-bit: return -EBUSY if the bus is not idle after STOP

[Stanley Chong]

When a device on the bus holds SDA low, bit_xfer() can still report
success. i2c_stop() releases SDA, but it never checks that SDA and SCL
really went high. The caller gets a positive message count and a
buffer full of zeros.

I found this on a Raspberry Pi 3B with i2c-gpio. An Arduino Uno pulls
SDA low on command. With SDA held low, all 12 test transfers returned
success. One of them is a write to address 0x70, where no device
exists.

This patch reads SDA and SCL after the STOP condition. If either line
is still low, the bus is not free, and bit_xfer() returns -EBUSY.
If the adapter has no getsda() or getscl(), that line is not checked.

I chose -EBUSY because the I2C core already uses it for a line that is
stuck low. i2c_generic_bus_free() returns -EBUSY when SDA is low, and
i2c_generic_scl_recovery() returns -EBUSY when SCL is stuck low.
i2c-aspeed also returns -EBUSY for its SDA and SCL low timeouts.

The check runs in the bailout path, before post_xfer(). At this point
the lines are still in the mode that pre_xfer() set up. So the check
reads the lines the same way the ACK reads do during the transfer.

With the patch, all 12 transfers return -EBUSY in 0.2 to 0.4 ms.
Normal transfers do not change. When SCL is held low, the error code
changes from -ENXIO to -EBUSY. See the questions below.

Signed-off-by: Stanley Chong <ale.sarick@gmail.com>
---
This is an RFC because the patch changes the error code in some cases.
I would like to know if this direction is acceptable before I go
further.

Test setup
  Raspberry Pi 3B, kernel rpi-6.18.y (6.18.34+rpt-rpi-v7)
  i2c-gpio via dtoverlay, bus 5 on GPIO2/3, udelay 5 us, timeout 100 ms
  Arduino Uno as a slave at 0x48 (LM75 format, 45.0 C = 0x2d00).
  The same Uno is the fault injector: it pulls SDA or SCL low and
  releases it on command.
  Each case: read 2 bytes from 0x48, write 1 byte to 0x48,
  write to 0x70 (no device), read from 0x70 (no device).
  Note: the Pi reported undervoltage during the tests.

Results (stock = rpi-6.18.y module, patched = this patch)

  case          stock                          patched
  ------------  -----------------------------  ----------------------------
  normal        0x48 OK (0x2d00), 0x70 ENXIO   same
  SDA held low  12/12 success (0x0000,         12/12 -EBUSY, 0.20-0.34 ms
                write to 0x70 "OK")
  SDA released  normal immediately             normal immediately
  SCL held low  4/4 -ENXIO after 0.87-0.88 s   4/4 -EBUSY after 0.88 s
  SCL released  normal immediately             normal immediately

Questions
1. SCL stuck low: -ENXIO becomes -EBUSY.
   drm_do_probe_ddc_edid() stops retrying when it gets -ENXIO. With
   this patch, a DDC bus with SCL stuck low is tried 5 times instead
   of once. radeon and amdgpu use a timeout of usecs_to_jiffies(2200).
   That is at least one jiffy: 4 ms at HZ=250, 10 ms at HZ=100. I have
   not measured this on a real DDC bus.

2. Some DDC buses read low when they are idle.
   In bugzilla 200045 (the 2018 revert of "i2c: algo-bit: init the
   bus to a known state"), radeon buses 0x92, 0x93 and 0x14 read SCL=0
   even after pre_xfer() was called (comment 14). Today such a bus
   fails at the address phase with -ENXIO. With this patch it returns
   -EBUSY, so DRM retries it as in question 1. Is that acceptable, or
   should the check be opt-in per adapter?

3. Rise time.
   The check reads the lines adap->udelay after releasing SDA (5 us for
   i2c-gpio at 100 kHz). The I2C spec allows at most 1 us rise time in
   Standard-mode, so a compliant bus is high by then. A bus with weak
   pull-ups is out of spec, but it may work today. With this patch it
   could start getting -EBUSY. Is that acceptable, or should the check
   wait longer?

4. Recovery.
   Neither algo-bit nor i2c-gpio sets up bus recovery, and the core
   only retries on -EAGAIN. So -EBUSY goes straight back to the
   caller. Should bit_xfer() try i2c_recover_bus() first when
   bus_recovery_info is set?

Build: tested on rpi-6.18.y. The same change applies to mainline.
checkpatch.pl --strict reports 0 errors, 0 warnings, 0 checks.

 drivers/i2c/algos/i2c-algo-bit.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index 7ca565c..c36787e 100644
--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -132,13 +132,17 @@ static void i2c_repstart(struct i2c_algo_bit_data *adap)
 }


-static void i2c_stop(struct i2c_algo_bit_data *adap)
+static int i2c_stop(struct i2c_algo_bit_data *adap)
 {
 	/* assert: scl is low */
 	sdalo(adap);
 	sclhi(adap);
 	setsda(adap, 1);
 	udelay(adap->udelay);
+	if ((adap->getsda && !getsda(adap)) || (adap->getscl && !getscl(adap)))
+		return -EBUSY;
+
+	return 0;
 }


@@ -586,7 +590,11 @@ static int bit_xfer(struct i2c_adapter *i2c_adap,

 bailout:
 	bit_dbg(3, &i2c_adap->dev, "emitting stop condition\n");
-	i2c_stop(adap);
+	if (i2c_stop(adap) == -EBUSY) {
+		bit_dbg(1, &i2c_adap->dev,
+			"bus busy, giving up\n");
+		ret = -EBUSY;
+	}

 	if (adap->post_xfer)
 		adap->post_xfer(i2c_adap);