linux-kernel - Re: [PATCH] i2c: imx: increase retries on arbitration loss

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: <5c2e0531-e7c3-1b37-35ed-c8e9795a0d18@norik.com>
Date:   Fri, 16 Dec 2022 13:23:29 +0100
From:   Primoz Fiser <primoz.fiser@...ik.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Oleksij Rempel <o.rempel@...gutronix.de>,
        linux-kernel@...r.kernel.org, Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        upstream@...ts.phytec.de, Marco Felsch <m.felsch@...gutronix.de>,
        Oleksij Rempel <linux@...pel-privat.de>,
        NXP Linux Team <linux-imx@....com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        linux-arm-kernel@...ts.infradead.org, linux-i2c@...r.kernel.org,
        francesco.dolcini@...adex.com
Subject: Re: [PATCH] i2c: imx: increase retries on arbitration loss

Hi all,

On 16. 12. 22 12:13, Uwe Kleine-König wrote:
> On Fri, Dec 16, 2022 at 12:02:27PM +0100, Oleksij Rempel wrote:
>> On Fri, Dec 16, 2022 at 11:41:08AM +0100, Primoz Fiser wrote:
>>> Hi Marco,
>>>
>>> On 16. 12. 22 10:45, Marco Felsch wrote:
>>>> Hi Primoz,
>>>>
>>>> On 22-12-16, Primoz Fiser wrote:
>>>>> By default, retries value is set to 0 (no retries). Set retries to more
>>>>> sensible value of 3 to allow i2c core to re-attempt transfer in case of
>>>>> i2c arbitration loss (i2c-imx returns -EAGAIN errno is such case).
>>>>
>>>> apart the fact that the number of retries vary a lot and so the client
>>>> driver behaviour can vary a lot which is not good IMHO, why do you think
>>>> that 3 is a sufficient number?
>>>
>>> IMHO it is better than leaving it at 0 (no retries)?
>>>
>>> Setting it to sensible value like 3 will at least attempt to make transfer
>>> in case arbitration-loss occurs.
>>>
>>>>
>>>> If an arbitration loss happen, why do you think that retrying it 3 times
>>>> changes that?
>>>
>>> I our case, setting retries to non-zero value solves issues with PMIC
>>> shutdown on phyboard-mira which in some rare cases fails with "Failed to
>>> shutdown (err =  -11)" (-EAGAIN).
>>>
>>> To me it makes common sense retries is set to non-zero value especially for
>>> such rare conditions/situations.
>>
>> https://lore.kernel.org/all/Ys1bw9zuIwWS+bqw@shikoro/

Ohh I see.

Reading through the thread I guess we aren't getting this mainlined at 
all :)

But let me switch side and ask why do you think leaving retries = 0 is a 
good idea?

The only solid point in the thread seems to be that in that case we are 
not covering up the potential i2c hardware issues?

Yeah fair point but on the other hand, goal of this patch would be to 
improve robustness in case of otherwise good performing hardware. From 
user perspective I just want it to work despite it retrying under the 
hood from time to time. I think Francesco had the same idea.

> 
> Also in the same thread there is the question about better setting it in
> the i2c core if 3 instead of 0 is a good idea for the imx driver.

Using I2C_RETRIES ioctl for this seems a bit of an overkill considering 
other i2c bus drivers also set retries to non-zero value. But anyways, 
thank you for the idea.


> 
> Best regards
> Uwe
> 

BR,
Primoz