linux-kernel - 回复: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

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] [day] [month] [year] [list]

Message-ID: <0498cff909fe4794b7827ad399cb4e43@siengine.com>
Date: Thu, 5 Sep 2024 07:32:06 +0000
From: Liu Kimriver/刘金河 <kimriver.liu@...ngine.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC: "jarkko.nikula@...ux.intel.com" <jarkko.nikula@...ux.intel.com>,
        "mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
        "jsd@...ihalf.com" <jsd@...ihalf.com>,
        "andi.shyti@...nel.org"
	<andi.shyti@...nel.org>,
        "linux-i2c@...r.kernel.org"
	<linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: 回复: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

Andy Shevchenko: 
  Today, I followed your suggestion to resend the patch, but I found that 
there is an inaccurate issue with determining whether i2c is enabled,
I will update the patch again this afternoon and send it to you. Thank you.

>  	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>  	if (abort_needed) {
-		if (!enable) {
+		if (!(enable & DW_IC_ENABLE_ENABLE)) {
 			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
 			enable |= DW_IC_ENABLE_ENABLE;

Best Regards
Kimriver.liu

-----邮件原件-----
发件人: Andy Shevchenko <andriy.shevchenko@...ux.intel.com> 
发送时间: 2024年9月4日 20:55
收件人: Liu Kimriver/刘金河 <kimriver.liu@...ngine.com>
抄送: jarkko.nikula@...ux.intel.com; mika.westerberg@...ux.intel.com; jsd@...ihalf.com; andi.shyti@...nel.org; linux-i2c@...r.kernel.org; linux-kernel@...r.kernel.org
主题: Re: [PATCH] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

On Wed, Sep 04, 2024 at 02:42:24PM +0800, kimriver liu wrote:
> From: "kimriver.liu" <kimriver.liu@...ngine.com>
> 
> 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.

...

>  	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>  	if (abort_needed) {
> +		if (!enable) {
> +			regmap_write(dev->map, DW_IC_ENABLE, DW_IC_ENABLE_ENABLE);
> +			enable |= DW_IC_ENABLE_ENABLE;

> +			usleep_range(25, 100);

fsleep()

And add a short comment to explain the chosen value.

> +		}

...

> +static int i2c_dw_check_mst_activity(struct dw_i2c_dev *dev) {
> +	u32 status = 0;
> +	int ret = 0;
> +
> +	regmap_read(dev->map, DW_IC_STATUS, &status);
> +	if (status & DW_IC_STATUS_MASTER_ACTIVITY) {
> +		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 mst activity not idle %d\n", ret);
> +	}
> +
> +	return ret;

This can be rewritten as

	u32 status = 0;
	int ret;

	regmap_read(dev->map, DW_IC_STATUS, &status);
	if (!status & DW_IC_STATUS_MASTER_ACTIVITY))
		return 0;

	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 mst activity not idle %d\n", ret);

	return ret;

> +}

...

> +	ret = i2c_dw_check_mst_activity(dev);
> +	if (!ret)
> +		__i2c_dw_disable_nowait(dev);

...but looking at the usage, I think the proper is to have the above to return boolean. And also update the name to follow the usual pattern for boolean helpers.

static bool i2c_dw_is_mst_idling(struct dw_i2c_dev *dev) ...
	if (i2c_dw_is_mst_idling(dev))
		__i2c_dw_disable_nowait(dev);

...

Also what does the heck "mst" stand for? Please, use decrypted words in function names and error messages..

--
With Best Regards,
Andy Shevchenko