linux-kernel - Re: [PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low

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]

Date:   Fri, 11 Aug 2023 16:59:27 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Yann Sionneau <yann@...nneau.net>
Cc:     Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        Yann Sionneau <ysionneau@...ray.eu>,
        Jonathan Borne <jborne@...ray.eu>
Subject: Re: [PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master
 is holding SCL low

On Fri, Aug 11, 2023 at 02:46:23PM +0200, Yann Sionneau wrote:
> From: Yann Sionneau <ysionneau@...ray.eu>
> 
> The designware IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN

DesignWare

> parameter.
> In which case, if the TX FIFO gets empty and the last command didn't have

"In this case when the..."

> the STOP bit (IC_DATA_CMD[9]), the dw_apb_i2c will hold SCL low until

"the controller will..."

> a new command is pushed into the TX FIFO or the transfer is aborted.
> 
> When the dw_apb_i2c is holding SCL low, it cannot be disabled.

"When the controller..."

> The transfer must first be aborted.
> Also, the bus recover won't work because SCL is held low by the master.
> 
> This patch checks if the master is holding SCL low in __i2c_dw_disable

Grep for "This patch" in the Submitting Patches document and fix this
accordingly.

__i2c_dw_disable()

> before trying to disable the IP.
> If SCL is held low, an abort is initiated.
> When the abort is done, the disabling can then proceed.
> 
> This whole situation can happen for instance during SMBUS read data block
> if the slave just responds with "byte count == 0".
> This puts the master in an unrecoverable state, holding SCL low and the
> current __i2c_dw_disable procedure is not working. In this situation

__i2c_dw_disable()

> only a Linux reboot can fix the i2c bus.

If reboot helps, what magic does it do that Linux OS can't repeat in software?
Please, elaborate more.

...

>  	int timeout = 100;
>  	u32 status;
> +	u32 raw_intr_stats;
> +	u32 enable;
> +	bool abort_needed;
> +	bool abort_done = false;

Perhaps reversed xmas tree order?

	bool abort_done = false;
	bool abort_needed;
	u32 raw_intr_stats;
	int timeout = 100;
	u32 status;
	u32 enable;

...

> +	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
> +	if (abort_needed)
> +		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
>  
>  	do {
> +		if (abort_needed && !abort_done) {
> +			regmap_read(dev->map, DW_IC_ENABLE, &enable);
> +			abort_done = !(enable & DW_IC_ENABLE_ABORT);
> +			continue;

This will exhaust the timeout and below can be run at most once,
is it a problem?

Also it's a tight busyloop, are you sure it's what you need?

> +		}

-- 
With Best Regards,
Andy Shevchenko