linux-kernel - Re: [PATCH v2 2/3] i2c: pxa: prevent calling of the generic recovery init code

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: <0bfcb570-dab3-4038-a1aa-8bc7fe2feee8@gmail.com>
Date: Wed, 13 Aug 2025 17:17:28 +0200
From: Gabor Juhos <j4g8y7@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Wolfram Sang <wsa@...nel.org>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Andi Shyti <andi.shyti@...nel.org>, Russell King
 <rmk+kernel@...linux.org.uk>, Andrew Lunn <andrew@...n.ch>,
 Hanna Hawa <hhhawa@...zon.com>, Robert Marko <robert.marko@...tura.hr>,
 Linus Walleij <linus.walleij@...aro.org>, linux-i2c@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Imre Kaloz <kaloz@...nwrt.org>, stable@...r.kernel.org
Subject: Re: [PATCH v2 2/3] i2c: pxa: prevent calling of the generic recovery
 init code

2025. 08. 13. 15:10 keltezéssel, Andy Shevchenko írta:
> On Wed, Aug 13, 2025 at 12:36:45PM +0200, Gabor Juhos wrote:
>> 2025. 08. 11. 22:26 keltezéssel, Andy Shevchenko írta:
>>> On Mon, Aug 11, 2025 at 09:49:56PM +0200, Gabor Juhos wrote:
> 
> ...
> 
>>> TBH this sounds to me like trying to hack the solution and as you pointed out
>>> the problem is in pinctrl state changes. I think it may affect not only I2C case.
>>
>> It is not an error in the pinctrl code. I have checked and even fixed a few bugs
>> in that.
>>
>>> And I didn't get how recovery code affects the initialisation (enumeration).
>>
>> Without the fix, it is not possible to initiate a transaction on the bus, which
>> in turn prevents enumeration.
> 
> But why? As you said below the first pin control state is changed during the
> probe, which is fine, and the culprit one happens on the recovery.

Erm, no. Both happens during probe, before the I2C core tries to enumerate the
devices on the bus.

> Why is recovery involved in probe? This is quite confusing...
Let me try to explain it differently. Here is the simplified call chain:

  i2c_pxa_probe()
     ...
     i2c_pxa_init_recovery()
        pinctrl_select_state()                  <- selects GPIO state
        pinctrl_select_state()                  <- selects default (I2C) state
     ...
     i2c_add_numbered_adapter()
         i2c_register_adapter()
             ...
             i2c_init_recovery()
                 i2c_gpio_init_recovery()
                     i2c_gpio_init_generic_recovery()
                         pinctrl_select_state() <- selects GPIO state***
                         ...
                         pinctrl_select_state() <- selects default (I2C) state
             ...
             bus_for_each_drv()
                 __process_new_adapter()
                     i2c_do_add_adapter()
                         i2c_detect()           <- enumerates the devices

The culprit is the first pinctrl_select_state() call in
i2c_gpio_init_generic_recovery() marked with '***'.

That call causes the controller to go stuck, which makes it impossible to
transfer anything on the bus.

> 
>>> Do we set pin control state back and forth during probe? May be this is a root cause?
>>
>> Yes, basically. The state gets changed back and forth twice. Once in driver
>> probe before the controller gets initialized, then once again in
>> i2c_gpio_init_generic_recovery(). The problem is caused by the second state
>> change as it runs after the controller gets enabled which confuses the hardware.
> 
> ...
> 
>>>>  static int i2c_pxa_init_recovery(struct pxa_i2c *i2c)
>>>>  {
>>>>  	struct i2c_bus_recovery_info *bri = &i2c->recovery;
>>>
>>>>  		return 0;
>>>>  	}
>>>>  
>>>> +	bri->init_recovery = i2c_pxa_init_recovery_cb;
>>>
>>> This is unfortunate. I would keep the naming schema consistent, i.e. rename
>>> existing function and use its original name for the new callback.
>>
>> I agree, but since the change is targeted also to stable kernels, I wanted to
>> keep the change as minimal as possible.
> 
> Renaming is not a big deal AFAICS, but leaving this _cb will be confusing in a
> long term. I prefer name to be changed.

Ok, will change the name.

Regards,
Gabor