linux-kernel - Re: [PATCH v4 2/6] i2c: npcm: use a software flag to indicate a BER condition

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 for Android: free password hash cracker in your pocket

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

Message-ID: <5mxsciw6443k5rlpymrs7addvme53f5v3zuj5u7tvlggfeirik@dy2bvyz2lyue>
Date: Wed, 25 Sep 2024 22:22:19 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: warp5tw@...il.com
Cc: avifishman70@...il.com, tmaimon77@...il.com, tali.perry1@...il.com, 
	venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com, 
	andriy.shevchenko@...ux.intel.com, wsa@...nel.org, rand.sec96@...il.com, 
	wsa+renesas@...g-engineering.com, tali.perry@...oton.com, Avi.Fishman@...oton.com, 
	tomer.maimon@...oton.com, KWLIU@...oton.com, JJLIU0@...oton.com, kfting@...oton.com, 
	openbmc@...ts.ozlabs.org, linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/6] i2c: npcm: use a software flag to indicate a BER
 condition

Hi Tyrone,

patch looks good, but...

On Fri, Sep 20, 2024 at 06:18:16PM GMT, warp5tw@...il.com wrote:
> From: Tyrone Ting <kfting@...oton.com>
> 
> From: Tyrone Ting <kfting@...oton.com>
> 
> If not clearing the BB (bus busy) condition in the BER (bus error)
> interrupt, the driver causes a timeout and hence the i2c core
> doesn't do the i2c transfer retry but returns the driver's return
> value to the upper layer instead.
> 
> Clear the BB condition in the BER interrupt and a software flag is
> used. The driver does an i2c recovery without causing the timeout
> if the flag is set.
> 
> Signed-off-by: Tyrone Ting <kfting@...oton.com>
> Reviewed-by: Tali Perry <tali.perry1@...il.com>
> ---
>  drivers/i2c/busses/i2c-npcm7xx.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index 2b76dbfba438..2d034503d8bc 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -334,6 +334,7 @@ struct npcm_i2c {
>  	u64 nack_cnt;
>  	u64 timeout_cnt;
>  	u64 tx_complete_cnt;
> +	bool ber_state;

I need some comment here, what is this ber state? You described
it in the commit log, but people won't be browsing much of the
commit log rather than the code itself.

You could perhaps mention the meaning as:

	bool ber_state; /* brief description */

and...

>  };
>  
>  static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,
> @@ -1521,6 +1522,7 @@ static void npcm_i2c_irq_handle_ber(struct npcm_i2c *bus)
>  	if (npcm_i2c_is_master(bus)) {
>  		npcm_i2c_master_abort(bus);
>  	} else {
> +		bus->ber_state = true;
>  		npcm_i2c_clear_master_status(bus);
>  
>  		/* Clear BB (BUS BUSY) bit */
> @@ -1699,6 +1701,7 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
>  		dev_dbg(bus->dev, "bus%d-0x%x recovery skipped, bus not stuck",
>  			bus->num, bus->dest_addr);
>  		npcm_i2c_reset(bus);
> +		bus->ber_state = false;
>  		return 0;
>  	}
>  
> @@ -1763,6 +1766,7 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
>  		if (bus->rec_succ_cnt < ULLONG_MAX)
>  			bus->rec_succ_cnt++;
>  	}
> +	bus->ber_state = false;
>  	return status;
>  }
>  
> @@ -2158,7 +2162,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  
>  	} while (time_is_after_jiffies(time_left) && bus_busy);
>  
> -	if (bus_busy) {
> +	if (bus_busy || bus->ber_state) {

/*
 * Check the BER (bus error) state, if it's true means that blah
 * blah blah, while when it's false blah blah blah and we should
 * or should not do blah blah blah
 */
if (bus_busy || bus->ber_state) {
	...
}

Thanks,
Andi

>  		iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
>  		npcm_i2c_reset(bus);
>  		i2c_recover_bus(adap);
> -- 
> 2.34.1
>