Re: [PATCH 2/2] [i2c-bcm2835] ALWAYS enable INTD

[Stefan Wahren]

[Forware to Dave and Phil]

Am 30.10.23 um 17:21 schrieb mike.isely@xxxxxxxxxxxxxxxxx:
> From: Mike Isely <mike.isely@xxxxxxxxxxxxxxxxx>
>
> There is a race in the bcm2835 i2c hardware: When one starts a write
> transaction, two things apparently take place at the same time: (1) an
> interrupt is posted to cause the FIFO to be filled with TX data,
> and (2) an I2C transaction is started on the wire with the slave
> select byte.  The race happens if there's no slave, as this causes a
> slave selection timeout, raising the ERR flag in the hardware and
> setting DONE.  The setting of that DONE flag races against TXW.  If
> TXW gets set first, then an interrupt is raised if INTT was set.  If
> ERR gets set first, then an interrupt is raised if INTD was set.  It's
> one or the other, not both - probably because DONE being set disables
> the hardware INTT interrupt path.
>
> MOST of the time, TXW gets set first, the ISR runs, sees ERR is set
> and cleanly fails the transaction.  However some of the time DONE gets
> set first - but since the driver doesn't enable INTD until it's on the
> last message - there's no interrupt at all.  Thus the ISR never fires
> and the driver detects a timeout instead.  At best, the "wrong" error
> code is delivered to the owner of the transaction.  At worst, if the
> timeout doesn't propertly clean up the hardware (see prior commit
> fixing that), the next - likely unrelated - transaction will get
> fouled, leading to bizarre behavior in logic otherwise unrelated to
> the source of the original error.
>
> The fix here is to set INTD on for all messages not just the last one.
> In that way, unexpected failures which might set DONE earlier than
> expected will always trigger an interrupt and be handled correctly.
>
> The datasheet for this hardware doesn't describe any scenario where
> the hardware can realistically hang - even a stretched clock will be
> noticed if it takes too long.  So in theory a timeout should really
> NEVER happen, and with this fix I was completely unable to trigger any
> further timeouts at all.
>
> Signed-off-by: Mike Isely <isely@xxxxxxxxx>
> ---
>   drivers/i2c/busses/i2c-bcm2835.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> index 96de875394e1..70005c037ff9 100644
> --- a/drivers/i2c/busses/i2c-bcm2835.c
> +++ b/drivers/i2c/busses/i2c-bcm2835.c
> @@ -235,26 +235,22 @@ static void bcm2835_drain_rxfifo(struct bcm2835_i2c_dev *i2c_dev)
>
>   static void bcm2835_i2c_start_transfer(struct bcm2835_i2c_dev *i2c_dev)
>   {
> -	u32 c = BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN;
> +	u32 c = BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN | BCM2835_I2C_C_INTD;
>   	struct i2c_msg *msg = i2c_dev->curr_msg;
> -	bool last_msg = (i2c_dev->num_msgs == 1);
>
>   	if (!i2c_dev->num_msgs)
>   		return;
>
>   	i2c_dev->num_msgs--;
>   	i2c_dev->msg_buf = msg->buf;
>   	i2c_dev->msg_buf_remaining = msg->len;
>
>   	if (msg->flags & I2C_M_RD)
>   		c |= BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
>   	else
>   		c |= BCM2835_I2C_C_INTT;
>
> -	if (last_msg)
> -		c |= BCM2835_I2C_C_INTD;
> -
>   	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_A, msg->addr);
>   	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DLEN, msg->len);
>   	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
>   }