linux-kernel - Re: [PATCH 1/3] i2c: at91: move i2c_recover_bus() outside of at91_do_twi

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: <YaSt/CXWmZGmt+te@kunai>
Date:   Mon, 29 Nov 2021 11:39:56 +0100
From:   Wolfram Sang <wsa@...nel.org>
To:     Codrin Ciubotariu <codrin.ciubotariu@...rochip.com>
Cc:     linux-i2c@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, nicolas.ferre@...rochip.com,
        alexandre.belloni@...tlin.com, ludovic.desroches@...rochip.com,
        andrew@...people.com, mhoffman@...htlink.com, khali@...ux-fr.org
Subject: Re: [PATCH 1/3] i2c: at91: move i2c_recover_bus() outside of
 at91_do_twi_transfer()

Hi Codrin,

sorry for the super-long delay. There is an issue here with regard to
bus recovery which affetcs more drivers and I can't make up my mind how
to handle it...

> Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery")

Sidenote: I don't think this is a fix.

> +	if (ret < 0) {
> +		/*
> +		 * some faulty I2C slave devices might hold SDA down;
> +		 * we can send a bus clear command, hoping that the pins will be
> +		 * released
> +		 */
> +		i2c_recover_bus(&dev->adapter);
> +	} else {
> +		ret = num;
> +	}

So, one issue is more straightforward. Bus recovery is applied on all
errors. It should only be called when SDA is stuck.

The other issue is that bus recovery is applied after a transfer. The
I2C specs mention bus recovery only at the beginning of a transfer when
SDA is detected low. I think it also makes more sense because the bus
may also be stuck because of a misbehaving bootloader etc. This will be
caught when the check is done at the beginning.

However, moving the detection to the beginning leaves room for a
regression, because your driver already does it at the end of a
transfer. However, I'd think all regressions coming up need seperate
fixing anyhow. Unless I overlooked something, of course.

So, I think it should be moved to the beginning of a transfer, but I am
open for discussion, so we get the best possible bus recovery in Linux.

Happy hacking,

   Wolfram

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)