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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 4668 bytes --]

Hi Stanley,

thanks for the patch, the thorough explanations, and your care to
prevent regressions!

> 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.

Basically correct, but the check should not be applied after a STOP but
before a START. Remember that I2C is (theoretically) a multi-master bus.
If not both lines are high, it could be that another controller is
transmitting. This is why it should be checked before starting a
message. We retry in the hope that the other controller is done by then,
so the bus can be used by ourself again.

> 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.

We have defined error value for most cases. Not all driver adhere it to
it 100%, but we are striving for that because it is so much easier for
the upper layers. Check:

	Documentation/i2c/fault-codes.rst

So, in your case, if the check is moved to the beginning, -EAGAIN would
be the proper error code. (And -EBUSY probably needs an update saying
that it is also the response of a failed recovery)

> 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.
> 
> 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.

-ENXIO means no device is responding (so no retries). This is plain
wrong. -EAGAIN will be much better because the bus might be
(theoretically) free somewhen. This is actually an improvement.

> 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?

This is actually improved to the expected behaviour. If SCL is stuck
low, it will fail as well, just a tad later.

> 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?

I think the problem is gone when you move the check before START because
we did not modify a signal yet.

> 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?

When to do recovery is a question of its own. I would leave it out for
now and tackle your issue seperately first.

Does this all make sense to you?

Thanks again and happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]