linux-kernel - Re: [PATCH] i2c: core: dont change pinmux state to GPIO during recovery setup

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: <CACRpkdYUW-mO6vhh-zkZAuqQOHpwMeJsNw=jSLzbgoEtoCTtNQ@mail.gmail.com>
Date:   Thu, 9 Nov 2023 21:04:29 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Robert Marko <robert.marko@...tura.hr>
Cc:     wsa@...nel.org, codrin.ciubotariu@...rochip.com,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux@...linux.org.uk, linux-gpio@...r.kernel.org
Subject: Re: [PATCH] i2c: core: dont change pinmux state to GPIO during
 recovery setup

Hi Robert!

Thanks for getting back on this issue.

On Thu, Nov 9, 2023 at 8:10 PM Robert Marko <robert.marko@...tura.hr> wrote:

> Yes, I2C recovery is required on this board as otherwise the I2C bus will
> get stuck after a certain number of SFP module plug/unplug events or
> sometimes even just randomly, I2C recovery allows the bus to recover
> and continue working.

OK makes sense.

> Maybe my commit message was confusing, so I will try and explain further.
> I2C recovery did work on Armada 3720 just fine until the driver was converted
> to use the generic I2C recovery which is now part of the I2C core.
>
> After it was converted to it, the I2C bus completely stopped working
> on Armada 3720
> if I2C recovery is enabled by making the recovery pinctrl available in DTS.

Shouldn't we just revert that patch until we can figure this out then?

> I then spent quite a while trying to bisect the exact change that
> causes this issue
> in the conversion as code is almost identical to what the driver was
> doing previously,
> and have bisected it down to pinctrl_select_state(bri->pinctrl,
> bri->pins_gpio) being
> called before SDA and SCL pins are obtained via devm_gpiod_get().
>
> Then I thought that maybe there was a HW bug in the Armada 3720 as the
> pin function
> was being set to GPIO twice via register write so I made sure the
> register was being
> written to only if the desired value is different than the current one
> but that did not help
> at all.
> For whatever reason calling pinctrl_select_state(bri->pinctrl,
> bri->pins_gpio) causes
> the pins to basically lock up, but the weird thing is that calling
> devm_gpiod_get() will
> also end up with the kernel changing the pin function to GPIO and this
> works, hence
> this patch.

That sounds like a race condition... such as if the pin control block is
not clocked when the first pinctrl_select_state() comes in.

Or maybe the i2c hardware needs to be initialized/clocked before
touching it's mux? (Weird but could happen.)

One thing that makes me a bit suspicious is this:
i2c/busses/i2c-pxa.c:
subsys_initcall(i2c_adap_pxa_init);

While:
pinctrl/mvebu/pinctrl-armada-37xx.c
builtin_platform_driver_probe(armada_37xx_pinctrl_driver,
                              armada_37xx_pinctrl_probe);

If there is no probe reordering then the i2c driver will probe before
pin control.

What happens if you move the i2c_adap_pxa_init() call to
module_init() instead of subsys_initcall()?

Yours,
Linus Walleij