linux-kernel - Re: [PATCH] i2c: iproc: reset bus after timeout if START

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ecaf734a-5d95-d68c-c6ee-0047eebc7883@broadcom.com>
Date:   Tue, 5 Sep 2023 09:26:14 -0700
From:   Ray Jui <ray.jui@...adcom.com>
To:     Jonas Gorski <jonas.gorski@...dn.de>,
        Andi Shyti <andi.shyti@...nel.org>,
        Ray Jui <rjui@...adcom.com>,
        Scott Branden <sbranden@...adcom.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Wolfram Sang <wsa@...nel.org>,
        Kevin Cernekee <cernekee@...omium.org>
Cc:     linux-i2c@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c: iproc: reset bus after timeout if START_BUSY is
 stuck

Hi Jonas,

On 9/4/2023 2:00 AM, 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@...dn.de>
> ---
> 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 */

Can you please rephrase the comment to make it more clear?

For example, something like this:

/*
 * If START_BUSY is still not clear, it means the controller may have
 * been stuck. In this case, reset the controller to recover.
 */

> +		if (!!(iproc_i2c_rd_reg(iproc_i2c, M_CMD_OFFSET) &
> +		       BIT(M_CMD_START_BUSY_SHIFT))) {
> +			/* re-initialize i2c for recovery */
> +			bcm_iproc_i2c_enable_disable(iproc_i2c, false);
> +			bcm_iproc_i2c_init(iproc_i2c);
> +			bcm_iproc_i2c_enable_disable(iproc_i2c, true);
> +		}
> +
>  		/* flush both TX/RX FIFOs */
>  		val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
>  		iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4194 bytes)