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]

Message-ID: <c4558640-58b7-7c56-b164-403333405b09@sionneau.net>
Date:   Thu, 17 Aug 2023 16:39:57 +0200
From:   Yann Sionneau <yann@...nneau.net>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
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

Hi,

Le 11/08/2023 à 15:59, Andy Shevchenko a écrit :
> 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
Ack!
>
>> parameter.
>> In which case, if the TX FIFO gets empty and the last command didn't have
> "In this case when the..."
Ack!
>
>> the STOP bit (IC_DATA_CMD[9]), the dw_apb_i2c will hold SCL low until
> "the controller will..."
Ack!
>
>> 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..."
Ack!
>
>> 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.
Ok I didn't know, ack!
>
> __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.
Sorry I was not very clear. In fact I meant a SoC reset, not a Linux 
reboot. It's just that on our SoC with boot-from-flash a reset will also 
reboot the Linux. But indeed what fixes the issue is the reset of the SoC.
>
> ...
>
>>   	int timeout = 100;
>>   	u32 status;
>> +	u32 raw_intr_stats;
>> +	u32 enable;
>> +	bool abort_needed;
>> +	bool abort_done = false;
> Perhaps reversed xmas tree order?
Oh, I didn't know about this, thanks, ack!
>
> 	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?
I was also wondering about this... I can propose to extract this in 2 
loops. First loop to wait for the abort to finish, with its own timeout. 
Then the untouched second loop that waits for the disabling to finish.
>
> Also it's a tight busyloop, are you sure it's what you need?

I don't know, I would expect that this would not take much time, I 
already have a V2 for this patch with all your remarks taken into 
account, including the splitting into 2 loops (previous comment).

I am waiting before sending it to have the opportunity to test it on the 
real device, it will be done on the August 21st since I am in holidays 
for now.

I will print the number of iterations it takes for the abort to finish. 
If the abort is quick, maybe there is no need for sleeping. I didn't see 
any info about the time it takes inside the IP documentation.

Thanks for the review!

I'll get back to you when I have done the testing of the V2 patch :) (or 
maybe you want it on the mailing list now as an RFC ?)

-- 

Yann