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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZem9Gtd==gQM4EQ9R8MN2ZQ0JCyMCoTjg0kqCNDjuFMA@mail.gmail.com>
Date:   Thu, 28 Sep 2023 23:11:16 +0200
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

On Tue, Sep 26, 2023 at 6:03 PM Robert Marko <robert.marko@...tura.hr> wrote:

> @@ -359,13 +359,6 @@ static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap)
>         if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery)
>                 return 0;
>
> -       /*
> -        * pins might be taken as GPIO, so we should inform pinctrl about
> -        * this and move the state to GPIO
> -        */
> -       if (bri->pinctrl)
> -               pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
> -

But this might be absolutely necessary for other i2c drivers and this is
set in generic code.

My first question is: why is this platform even defining the "gpio" pin
control state for this i2c device if it is so dangerous to use?
If it can't be used, you just give it too much rope, delete the "gpio"
state for this group from the device tree: problem solved.

(This can be done with the specific /delete-node/ directive if
necessary, e.g. if you want to use the "gpio" state on other boards.)

Second: do you even want to do recovery on this platform then?
Is it necessary? What happens electronically in this case, if we don't
switch the pins to GPIO mode? Is it something akin to the "strict"
property in struct pinmux: that the GPIO block and the device can
affect the same pins at the same time? That warrants an explanation
and a comment.

If you can't delete the "gpio" pin control state, I would add a
bool pinctrl_stay_in_device_mode;
to
struct i2c_bus_recovery_info
in include/linux/i2c.h

And just:

if (bri->pinctrl && !bri->pinctrl_stay_in_device_mode)
    pinctrl_select_state(bri->pinctrl, bri->pins_gpio);

(Also the switch to the "default" state further down could be
contitional !bri->pinctrl_stay_in_device_mode)

But mostly I wonder why the "gpio" pin control state is defined, if it's
not to be used.

Yours,
Linus Walleij