linux-kernel - Re: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabledg

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: <yc43jizchnn4johokgc2xyr2g4yyxf4strt6vvtzye6zzor46r@dvgqo4k6q437>
Date: Thu, 5 Sep 2024 21:49:42 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: kimriver liu <kimriver.liu@...ngine.com>
Cc: jarkko.nikula@...ux.intel.com, andriy.shevchenko@...ux.intel.com, 
	mika.westerberg@...ux.intel.com, jsd@...ihalf.com, linux-i2c@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c: designware: fix master is holding SCL low while
 ENABLE bit is disabledg

Hi Kimriver,

On Thu, Sep 05, 2024 at 03:42:11PM GMT, kimriver liu wrote:
> From: "kimriver.liu" <kimriver.liu@...ngine.com>

Is there any reason to have "kimriver.liu" instead of "Kimriver
Liu"?

> Failure in normal Stop operational path
> 
> This failure happens rarely and is hard to reproduce. Debug trace
> showed that IC_STATUS had value of 0x23 when STOP_DET occurred,
> immediately disable ENABLE bit that can result in
> IC_RAW_INTR_STAT.MASTER_ON_HOLD holding SCL low.
> 
> Failure in ENABLE bit is disabled path
> 
> It was observed that master is holding SCL low and the IC_ENABLE is
> already disabled, Enable ABORT bit and ENABLE bit simultaneously
> cannot take effect.
> 
> Check if the master is holding SCL low after ENABLE bit is already
> disabled. If SCL is held low, The software can set this ABORT bit only
> when ENABLE is already set，otherwise,
> the controller ignores any write to ABORT bit. When the abort is done,
> then proceed with disabling the controller.
> 
> These kernel logs show up whenever an I2C transaction is attempted
> after this failure.
> i2c_designware e95e0000.i2c: timeout in disabling adapter
> i2c_designware e95e0000.i2c: timeout waiting for bus ready
> 
> The patch can be fix the controller cannot be disabled while SCL is
> held low in ENABLE bit is already disabled.

I'm sorry, but this commit log is difficult to understand. Could
you please polish it, fix the grammar and the punctuation and
make it more understandable?

> Signed-off-by: kimriver.liu <kimriver.liu@...ngine.com>

As I said above, would be nicer to have "Kimriver Liu" rather
than "kimriver.liu".

I'm wondering if we need the Fixes: tag here. If this is not a
frequent issue then we can probably omit it.

> ---

Andy already pointed out that you are missing versioning (is this
v2? v3?) and changelog.

>  drivers/i2c/busses/i2c-designware-common.c | 12 +++++++++++
>  drivers/i2c/busses/i2c-designware-master.c | 23 +++++++++++++++++++++-
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index e8a688d04aee..54acf8554582 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -453,6 +453,18 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
>  
>  	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>  	if (abort_needed) {
> +		if (!(enable & DW_IC_ENABLE_ENABLE)) {
> +			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
> +			enable |= DW_IC_ENABLE_ENABLE;
> +
> +			/*
> +			 * Wait two ic_clk delay when enabling the i2c to ensure ENABLE bit
> +			 * is already set by the driver (for 400KHz this is 25us)
> +			 * as described in the DesignWare I2C databook.
> +			 */
> +			fsleep(25);
> +		}
> +
>  		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
>  		ret = regmap_read_poll_timeout(dev->map, DW_IC_ENABLE, enable,
>  					       !(enable & DW_IC_ENABLE_ABORT), 10,
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index c7e56002809a..aba0b8fdfe9a 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -253,6 +253,26 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  	__i2c_dw_write_intr_mask(dev, DW_IC_INTR_MASTER_MASK);
>  }
>  
> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)
> +{
> +	u32 status;
> +	int ret;
> +
> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> +	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
> +		return true;
> +
> +	ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +			1100, 20000);
> +	if (ret) {
> +		dev_err(dev->dev, "i2c master controller not idle %d\n", ret);

Please be a bit more descriptive. It's not an error for the i2c
master not to be idle, it is if you try to disable the adapter.

Besides, it makes sense to me that this print is done in the
i2c_dw_xfer function rather than here. The task of this function
is only to check whether the controller is idling or active.

> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev)
>  {
>  	u32 val;
> @@ -796,7 +816,8 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	 * additional interrupts are a hardware bug or this driver doesn't
>  	 * handle them correctly yet.
>  	 */

Please update the comment according to your change.

> -	__i2c_dw_disable_nowait(dev);
> +	if (i2c_dw_is_master_idling(dev))

Please print the error message here.

Thanks for your patch,
Andi

> +		__i2c_dw_disable_nowait(dev);
>  
>  	if (dev->msg_err) {
>  		ret = dev->msg_err;
> -- 
> 2.17.1
>