Re: [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is stuck

[Jonas Gorski]

Hi,

Am Mi., 6. Sept. 2023 um 00:53 Uhr schrieb Andi Shyti <andi.shyti@xxxxxxxxxx>:
>
> Hi Jonas,
>
> On Mon, Sep 04, 2023 at 11:00:04AM +0200, Jonas Gorski wrote:
> > If a transaction times out, the START_BUSY signal may have gotten stuck,
> > and subsequent transactaction attempts will fail as the bus is still
> > considered busy.
> >
> > To work around this, check if the START_BUSY bit is still asserted, and
> > reset the controller in case it is.
> >
> > This is also done by the alternative, non-upstream iproc-smbus driver
> > implementation [1].
> >
> > Works around situations like:
> >
> >     bcm-iproc-2c 1803b000.i2c: transaction timed out
> >     bcm-iproc-2c 1803b000.i2c: bus is busy
> >     bcm-iproc-2c 1803b000.i2c: bus is busy
> >     bcm-iproc-2c 1803b000.i2c: bus is busy
> >     bcm-iproc-2c 1803b000.i2c: bus is busy
> >     bcm-iproc-2c 1803b000.i2c: bus is busy
> >     ...
> >
> > where the bus never recovers after a timeout.
> >
> > [1] https://github.com/opencomputeproject/onie/blob/master/patches/kernel/3.2.69/driver-iproc-smbus.patch
> >
> > Fixes: e6e5dd3566e0 ("i2c: iproc: Add Broadcom iProc I2C Driver")
> > Signed-off-by: Jonas Gorski <jonas.gorski@xxxxxxxx>
>
> I think the right Fixes tag should be:
>
> Fixes: 3f98ad45e585 ("i2c: iproc: add polling support")

That was the last change that part of the code, but the "issue" was
not introduced there. The code before that already did a timeout check
and flush in that case, without the reset.

Obviously the fix wouldn't apply without changes to a version without
that commit, but the version would be nevertheless affected by the
issue. That's why I chose the commit introducing the timeout handling.

> Cc: Rayagonda Kokatanur <rayagonda.kokatanur@xxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v5.2+
>
> > ---
> > The iproc-smbus driver does some additional checks/mitigations, but
> > since my I2C understanding is only very rudimentary, I didn't add them,
> > also the reset was enough to fix the issue I was seeing.
> >
> > I was a bit conflicted about the Fixes tag, but since it fixes/work
> > around misbehaviour seen I decided to add one.
> >
> > The issue was happening only in production, and only once per boot (so
> > far), but with 100% probability within a few hours.
> >
> >  drivers/i2c/busses/i2c-bcm-iproc.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> > index 05c80680dff4..69f9c199fa3b 100644
> > --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> > +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> > @@ -796,6 +796,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
> >       if (!time_left && !iproc_i2c->xfer_is_done) {
> >               dev_err(iproc_i2c->device, "transaction timed out\n");
> >
> > +             /* check if START_BUSY did not clear */
>
> as Ray asked, can you please expand this comment?

Will do, thanks for the reviews!

Best Regards,
Jonas

-- 
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany


Phone: 
+49-30-6108-1-6100


Managing Directors: 
Dr.-Ing. Hagen Woesner, Andreas 
Köpsel


Commercial register: 
Amtsgericht Berlin-Charlottenburg HRB 141569 
B
VAT ID No: DE283257294