Hi Linus,
this is a first belated round of pin control fixes for the v6.7 series..
The most interesting patch is the list iterator fix in the core by Maria
Yu, it took a while for me to realize what was going on there.
Some details on the fixes are in the tag.
Please pull them in!
Yours,
Linus Walleij
The following changes since commit b85ea95d086471afb4ad062012a4d73cd328fa86:
Linux 6.7-rc1 (2023-11-12 16:19:07 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git
tags/pinctrl-v6.7-2
for you to fetch changes up to 90785ea8158b6923c5d6a024f2b1c076110577b5:
dt-bindings: pinctrl: s32g2: change a maintainer email address
(2023-11-24 11:21:55 +0100)
----------------------------------------------------------------
Some pin control fixes for the v6.7 cycle:
- Fix a really interesting potential core bug in the list iterator
requireing the use of READ_ONCE() discovered when testing kernel
compiles with clang.
- Check devm_kcalloc() return value and an array bounds in the STM32
driver.
- Fix an exotic string truncation issue in the s32cc driver, found
by the kernel test robot (impressive!)
- Fix an undocumented struct member in the cy8c95x0 driver.
- Fix a symbol overlap with MIPS in the Lochnagar driver, MIPS
defines a global symbol "RST" which is a bit too generic and
collide with stuff. OK this one should be renamed too, we will
fix that as well.
- Fix erroneous branch taking in the Realtek driver.
- Fix the mail address in MAINTAINERS for the s32g2 driver.
----------------------------------------------------------------
Antonio Borneo (1):
pinctrl: stm32: fix array read out of bound
Charles Keepax (1):
pinctrl: lochnagar: Don't build on MIPS
Chen Ni (1):
pinctrl: stm32: Add check for devm_kcalloc
Chester Lin (2):
pinctrl: s32cc: Avoid possible string truncation
dt-bindings: pinctrl: s32g2: change a maintainer email address
Linus Walleij (1):
pinctrl: cy8c95x0: Fix doc warning
Maria Yu (1):
pinctrl: avoid reload of p state in list iteration
Tzuyi Chang (1):
pinctrl: realtek: Fix logical error when finding descriptor
.../bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml | 2 +-
drivers/pinctrl/cirrus/Kconfig | 3 ++-
drivers/pinctrl/core.c | 6 +++---
drivers/pinctrl/nxp/pinctrl-s32cc.c | 4 ++--
drivers/pinctrl/pinctrl-cy8c95x0.c | 1 +
drivers/pinctrl/realtek/pinctrl-rtd.c | 4 ++--
drivers/pinctrl/stm32/pinctrl-stm32.c | 13 ++++++++++---
7 files changed, 21 insertions(+), 12 deletions(-)
On Wed, 29 Nov 2023 at 04:09, Linus Walleij <[email protected]> wrote:
>
> The most interesting patch is the list iterator fix in the core by Maria
> Yu, it took a while for me to realize what was going on there.
That commit message still doesn't explain what the problem was.
Why is p->state volatile there? It seems to be a serious locking bug
if p->state can randomly change there, and the READ_ONCE() looks like
a "this hides the problem" rather than an actual real fix.
Linus
On Wed, Nov 29, 2023 at 3:56 PM Linus Torvalds
<[email protected]> wrote:
> On Wed, 29 Nov 2023 at 04:09, Linus Walleij <[email protected]> wrote:
> >
> > The most interesting patch is the list iterator fix in the core by Maria
> > Yu, it took a while for me to realize what was going on there.
>
> That commit message still doesn't explain what the problem was.
>
> Why is p->state volatile there? It seems to be a serious locking bug
> if p->state can randomly change there, and the READ_ONCE() looks like
> a "this hides the problem" rather than an actual real fix.
Thanks for looking into it Linus, Maria can you look closer at this and
try to pinpoint exactly what happens?
Is the bug never manifesting with GCC for example?
In the meantime I'll cook a fixes branch without this one commit.
Yours,
Linus Walleij
The pull request you sent on Wed, 29 Nov 2023 13:09:03 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git tags/pinctrl-v6.7-2
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/3b47bc037bd44f142ac09848e8d3ecccc726be99
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On 11/29/2023 11:08 PM, Linus Walleij wrote:
> On Wed, Nov 29, 2023 at 3:56 PM Linus Torvalds
> <[email protected]> wrote:
>> On Wed, 29 Nov 2023 at 04:09, Linus Walleij <[email protected]> wrote:
>>>
>>> The most interesting patch is the list iterator fix in the core by Maria
>>> Yu, it took a while for me to realize what was going on there.
>>
>> That commit message still doesn't explain what the problem was.
>>
>> Why is p->state volatile there? It seems to be a serious locking bug
>> if p->state can randomly change there, and the READ_ONCE() looks like
>> a "this hides the problem" rather than an actual real fix.
This is indeed an interesting issue. Thx for the comment, Linus.
**Let me explain how: "p->state becomes volatile in the list iterator",
and "why READ_ONCE is suggested".
The current critical code is:
list_for_each_entry(setting, &p->state->settings, node)
after elaborating the define list_for_each_entry, so above critical code
will be:
for (setting = list_head(&p->state->settings, typeof(*setting), node); \
&setting->node != (&p->state->settings); \
setting = list_next(setting , node))
The asm code(refer result from Clang version 10.0) can cleared explain
the step of p->state reload actions:
loop:
ldr x22,[x22] ; x22=list_next(setting , node))
add x9,x8,#0x18 ; x9=&p->state->setting
cmp x22,x9 ; setting,x9
b.eq 0xFFFFFF9B24483530
ldr w9,[x22,#0x10] ; w9,[setting,#16]
cmp w9,#0x2 ; w9,#2
b.ne 0xFFFFFF9B24483504
mov x0,x22 ; x0,setting
bl 0xFFFFFF9B24486048 ; pinmux_disable_setting
ldr x8,[x19,#0x28] ; x19=p, x8=[p->state], *reload p->state*
b loop
The *reload p->state* inside the loop for checking is not needed and can
cause possible infinite loop. So READ_ONCE is highly suggested even if
p->state is not randomly changed. And then unnecessary "ldr
x8,[x19,#0x28]" can be removed from above loop code.
**Comments about the locking bug:
currently pinctrl_select_state is an export symbol and don't have
effective reentrance protect design. That's why current infinite loop
issue was observed with customer's multi thread call with
pinctrl_select_state without lock protection. pinctrl_select_state
totally rely on driver module user side to ensure the reentrant state.
Usually the suggested usage from driver side who are using pinctrl would be:
LOCK;
pinctrl_select_state();
gpio pulling;
udelay();
check state;
other hardware behaviors;
UNLOCK;
So the locking bug fix I have told customer side to fix from their own
driver part. Since usually not only a simple pinctrl_select_state call
can finish the hardware state transaction.
I myself also is fine to have a small per pinctrl lock to only protect
the current pinctrl_select_state->pinctrl_commit_state reentrance
issues. Pls any pinctrl maintainer help to comment to suggest or not and
I can prepare a change as well.
>
> Thanks for looking into it Linus, Maria can you look closer at this and
> try to pinpoint exactly what happens?
Sure, I am trying to explain from my end. If there is other thoughts pls
feel free to chime in.
>
> Is the bug never manifesting with GCC for example?
>
> In the meantime I'll cook a fixes branch without this one commit.
>
> Yours,
> Linus Walleij
--
Thx and BRs,
Aiqun(Maria) Yu
Hi Nathan, Nick,
(just picking some LLVM compiler people I know of... and trust)
Context is this patch:
https://lore.kernel.org/linux-gpio/[email protected]/
On Thu, Nov 30, 2023 at 6:37 AM Aiqun(Maria) Yu <[email protected]> wrote:
> On 11/29/2023 11:08 PM, Linus Walleij wrote:
> > On Wed, Nov 29, 2023 at 3:56 PM Linus Torvalds
> > <[email protected]> wrote:
> >> On Wed, 29 Nov 2023 at 04:09, Linus Walleij <[email protected]> wrote:
> >>>
> >>> The most interesting patch is the list iterator fix in the core by Maria
> >>> Yu, it took a while for me to realize what was going on there.
> >>
> >> That commit message still doesn't explain what the problem was.
> >>
> >> Why is p->state volatile there? It seems to be a serious locking bug
> >> if p->state can randomly change there, and the READ_ONCE() looks like
> >> a "this hides the problem" rather than an actual real fix.
>
> This is indeed an interesting issue. Thx for the comment, Linus.
> **Let me explain how: "p->state becomes volatile in the list iterator",
> and "why READ_ONCE is suggested".
>
> The current critical code is:
> list_for_each_entry(setting, &p->state->settings, node)
>
> after elaborating the define list_for_each_entry, so above critical code
> will be:
> for (setting = list_head(&p->state->settings, typeof(*setting), node); \
> &setting->node != (&p->state->settings); \
> setting = list_next(setting , node))
>
> The asm code(refer result from Clang version 10.0) can cleared explain
> the step of p->state reload actions:
> loop:
> ldr x22,[x22] ; x22=list_next(setting , node))
> add x9,x8,#0x18 ; x9=&p->state->setting
> cmp x22,x9 ; setting,x9
> b.eq 0xFFFFFF9B24483530
>
> ldr w9,[x22,#0x10] ; w9,[setting,#16]
> cmp w9,#0x2 ; w9,#2
> b.ne 0xFFFFFF9B24483504
>
> mov x0,x22 ; x0,setting
> bl 0xFFFFFF9B24486048 ; pinmux_disable_setting
>
> ldr x8,[x19,#0x28] ; x19=p, x8=[p->state], *reload p->state*
> b loop
>
> The *reload p->state* inside the loop for checking is not needed and can
> cause possible infinite loop. So READ_ONCE is highly suggested even if
> p->state is not randomly changed. And then unnecessary "ldr
> x8,[x19,#0x28]" can be removed from above loop code.
>
> **Comments about the locking bug:
> currently pinctrl_select_state is an export symbol and don't have
> effective reentrance protect design. That's why current infinite loop
> issue was observed with customer's multi thread call with
> pinctrl_select_state without lock protection. pinctrl_select_state
> totally rely on driver module user side to ensure the reentrant state.
>
> Usually the suggested usage from driver side who are using pinctrl would be:
> LOCK;
> pinctrl_select_state();
> gpio pulling;
> udelay();
> check state;
> other hardware behaviors;
> UNLOCK;
>
> So the locking bug fix I have told customer side to fix from their own
> driver part. Since usually not only a simple pinctrl_select_state call
> can finish the hardware state transaction.
>
> I myself also is fine to have a small per pinctrl lock to only protect
> the current pinctrl_select_state->pinctrl_commit_state reentrance
> issues. Pls any pinctrl maintainer help to comment to suggest or not and
> I can prepare a change as well.
Luckily I am the pin control maintainer :D
And I also ha my morning coffee and looked over the patch again.
So tilting the compiler to generate code that is less prone to race
conditions with READ_ONCE() isn't really the solution is it? We need
to introduce a proper lock that stops this from happening if it is
a problem people are facing.
Can you try to make a patch that removes READ_ONCE()
and introduce a lock instead?
Racing is rarely an issue in pin control for reasons explained
in another context here:
https://lore.kernel.org/linux-gpio/CACRpkdZ0cnJpYuzU=47-oW-7N_YGMo2vXpKOeXeNi5PhPY7QMA@mail.gmail.com/
...but if people still manage to run into it, we better have a lock
there. Just make sure it is not just an issue with outoftree code,
but a real problem?
The change that changes the code to use the old_state variable
should stay in, it makes the code more readable, it's just the
READ_ONCE() macro which is dubious.
Yours,
Linus Walleij
On Fri, Dec 1, 2023 at 9:10 AM Linus Walleij <[email protected]> wrote:
> Hi Nathan, Nick,
>
> (just picking some LLVM compiler people I know of... and trust)
Forget that part of the mail, written out-of-context when I was
running different
disassembly of GCC vs LLVM code. I figured it out and concluded that
the compilers
are fine and doing the right thing, it's a none-issue in that regard.
Yours,
Linus Walleij
On 12/1/2023 4:10 PM, Linus Walleij wrote:
> Hi Nathan, Nick,
>
> (just picking some LLVM compiler people I know of... and trust)
>
> Context is this patch:
> https://lore.kernel.org/linux-gpio/[email protected]/
>
> On Thu, Nov 30, 2023 at 6:37 AM Aiqun(Maria) Yu <[email protected]> wrote:
>> On 11/29/2023 11:08 PM, Linus Walleij wrote:
>>> On Wed, Nov 29, 2023 at 3:56 PM Linus Torvalds
>>> <[email protected]> wrote:
>>>> On Wed, 29 Nov 2023 at 04:09, Linus Walleij <[email protected]> wrote:
>>>>>
>>>>> The most interesting patch is the list iterator fix in the core by Maria
>>>>> Yu, it took a while for me to realize what was going on there.
>>>>
>>>> That commit message still doesn't explain what the problem was.
>>>>
>>>> Why is p->state volatile there? It seems to be a serious locking bug
>>>> if p->state can randomly change there, and the READ_ONCE() looks like
>>>> a "this hides the problem" rather than an actual real fix.
>>
>> This is indeed an interesting issue. Thx for the comment, Linus.
>> **Let me explain how: "p->state becomes volatile in the list iterator",
>> and "why READ_ONCE is suggested".
>>
>> The current critical code is:
>> list_for_each_entry(setting, &p->state->settings, node)
>>
>> after elaborating the define list_for_each_entry, so above critical code
>> will be:
>> for (setting = list_head(&p->state->settings, typeof(*setting), node); \
>> &setting->node != (&p->state->settings); \
>> setting = list_next(setting , node))
>>
>> The asm code(refer result from Clang version 10.0) can cleared explain
>> the step of p->state reload actions:
>> loop:
>> ldr x22,[x22] ; x22=list_next(setting , node))
>> add x9,x8,#0x18 ; x9=&p->state->setting
>> cmp x22,x9 ; setting,x9
>> b.eq 0xFFFFFF9B24483530
>>
>> ldr w9,[x22,#0x10] ; w9,[setting,#16]
>> cmp w9,#0x2 ; w9,#2
>> b.ne 0xFFFFFF9B24483504
>>
>> mov x0,x22 ; x0,setting
>> bl 0xFFFFFF9B24486048 ; pinmux_disable_setting
>>
>> ldr x8,[x19,#0x28] ; x19=p, x8=[p->state], *reload p->state*
>> b loop
>>
>> The *reload p->state* inside the loop for checking is not needed and can
>> cause possible infinite loop. So READ_ONCE is highly suggested even if
>> p->state is not randomly changed. And then unnecessary "ldr
>> x8,[x19,#0x28]" can be removed from above loop code.
>>
>> **Comments about the locking bug:
>> currently pinctrl_select_state is an export symbol and don't have
>> effective reentrance protect design. That's why current infinite loop
>> issue was observed with customer's multi thread call with
>> pinctrl_select_state without lock protection. pinctrl_select_state
>> totally rely on driver module user side to ensure the reentrant state.
>>
>> Usually the suggested usage from driver side who are using pinctrl would be:
>> LOCK;
>> pinctrl_select_state();
>> gpio pulling;
>> udelay();
>> check state;
>> other hardware behaviors;
>> UNLOCK;
>>
>> So the locking bug fix I have told customer side to fix from their own
>> driver part. Since usually not only a simple pinctrl_select_state call
>> can finish the hardware state transaction.
>>
>> I myself also is fine to have a small per pinctrl lock to only protect
>> the current pinctrl_select_state->pinctrl_commit_state reentrance
>> issues. Pls any pinctrl maintainer help to comment to suggest or not and
>> I can prepare a change as well.
>
> Luckily I am the pin control maintainer :D
> And I also ha my morning coffee and looked over the patch again.
>
> So tilting the compiler to generate code that is less prone to race
> conditions with READ_ONCE() isn't really the solution is it? We need
> to introduce a proper lock that stops this from happening if it is
> a problem people are facing.
>
> Can you try to make a patch that removes READ_ONCE()
> and introduce a lock instead?
>
> Racing is rarely an issue in pin control for reasons explained
> in another context here:
> https://lore.kernel.org/linux-gpio/CACRpkdZ0cnJpYuzU=47-oW-7N_YGMo2vXpKOeXeNi5PhPY7QMA@mail.gmail.com/
>
> ...but if people still manage to run into it, we better have a lock
> there. Just make sure it is not just an issue with outoftree code,
> but a real problem?
>
> The change that changes the code to use the old_state variable
> should stay in, it makes the code more readable, it's just the
> READ_ONCE() macro which is dubious.
Thx for confirm. I am preparing the new change now. :)
READ_ONCE can only avoid the possible infinite loop and not crash the
whole kernel, while the lock is needed to protect the multi parallel
call of pinctrl_commit_state api have a consistent atomic hardware
result as well.
>
> Yours,
> Linus Walleij
--
Thx and BRs,
Aiqun(Maria) Yu
On 12/1/2023 6:06 PM, Aiqun(Maria) Yu wrote:
> On 12/1/2023 4:10 PM, Linus Walleij wrote:
>> Hi Nathan, Nick,
>>
>> (just picking some LLVM compiler people I know of... and trust)
>>
>> Context is this patch:
>> https://lore.kernel.org/linux-gpio/[email protected]/
>>
>> On Thu, Nov 30, 2023 at 6:37 AM Aiqun(Maria) Yu
>> <[email protected]> wrote:
>>> On 11/29/2023 11:08 PM, Linus Walleij wrote:
>>>> On Wed, Nov 29, 2023 at 3:56 PM Linus Torvalds
>>>> <[email protected]> wrote:
>>>>> On Wed, 29 Nov 2023 at 04:09, Linus Walleij
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> The most interesting patch is the list iterator fix in the core by
>>>>>> Maria
>>>>>> Yu, it took a while for me to realize what was going on there.
>>>>>
>>>>> That commit message still doesn't explain what the problem was.
>>>>>
>>>>> Why is p->state volatile there? It seems to be a serious locking bug
>>>>> if p->state can randomly change there, and the READ_ONCE() looks like
>>>>> a "this hides the problem" rather than an actual real fix.
>>>
>>> This is indeed an interesting issue. Thx for the comment, Linus.
>>> **Let me explain how: "p->state becomes volatile in the list iterator",
>>> and "why READ_ONCE is suggested".
>>>
>>> The current critical code is:
>>> list_for_each_entry(setting, &p->state->settings, node)
>>>
>>> after elaborating the define list_for_each_entry, so above critical code
>>> will be:
>>> for (setting = list_head(&p->state->settings, typeof(*setting),
>>> node); \
>>> &setting->node != (&p->state->settings); \
>>> setting = list_next(setting , node))
>>>
>>> The asm code(refer result from Clang version 10.0) can cleared explain
>>> the step of p->state reload actions:
>>> loop:
>>> ldr x22,[x22] ; x22=list_next(setting , node))
>>> add x9,x8,#0x18 ; x9=&p->state->setting
>>> cmp x22,x9 ; setting,x9
>>> b.eq 0xFFFFFF9B24483530
>>>
>>> ldr w9,[x22,#0x10] ; w9,[setting,#16]
>>> cmp w9,#0x2 ; w9,#2
>>> b.ne 0xFFFFFF9B24483504
>>>
>>> mov x0,x22 ; x0,setting
>>> bl 0xFFFFFF9B24486048 ; pinmux_disable_setting
>>>
>>> ldr x8,[x19,#0x28] ; x19=p, x8=[p->state], *reload p->state*
>>> b loop
>>>
>>> The *reload p->state* inside the loop for checking is not needed and can
>>> cause possible infinite loop. So READ_ONCE is highly suggested even if
>>> p->state is not randomly changed. And then unnecessary "ldr
>>> x8,[x19,#0x28]" can be removed from above loop code.
>>>
>>> **Comments about the locking bug:
>>> currently pinctrl_select_state is an export symbol and don't have
>>> effective reentrance protect design. That's why current infinite loop
>>> issue was observed with customer's multi thread call with
>>> pinctrl_select_state without lock protection. pinctrl_select_state
>>> totally rely on driver module user side to ensure the reentrant state.
>>>
>>> Usually the suggested usage from driver side who are using pinctrl
>>> would be:
>>> LOCK;
>>> pinctrl_select_state();
>>> gpio pulling;
>>> udelay();
>>> check state;
>>> other hardware behaviors;
>>> UNLOCK;
>>>
>>> So the locking bug fix I have told customer side to fix from their own
>>> driver part. Since usually not only a simple pinctrl_select_state call
>>> can finish the hardware state transaction.
>>>
>>> I myself also is fine to have a small per pinctrl lock to only protect
>>> the current pinctrl_select_state->pinctrl_commit_state reentrance
>>> issues. Pls any pinctrl maintainer help to comment to suggest or not and
>>> I can prepare a change as well.
>>
>> Luckily I am the pin control maintainer :D
>> And I also ha my morning coffee and looked over the patch again.
>>
>> So tilting the compiler to generate code that is less prone to race
>> conditions with READ_ONCE() isn't really the solution is it? We need
>> to introduce a proper lock that stops this from happening if it is
>> a problem people are facing.
>>
>> Can you try to make a patch that removes READ_ONCE()
>> and introduce a lock instead?
>>
>> Racing is rarely an issue in pin control for reasons explained
>> in another context here:
>> https://lore.kernel.org/linux-gpio/CACRpkdZ0cnJpYuzU=47-oW-7N_YGMo2vXpKOeXeNi5PhPY7QMA@mail.gmail.com/
>>
>> ...but if people still manage to run into it, we better have a lock
>> there. Just make sure it is not just an issue with outoftree code,
>> but a real problem?
>>
>> The change that changes the code to use the old_state variable
>> should stay in, it makes the code more readable, it's just the
>> READ_ONCE() macro which is dubious.
> Thx for confirm. I am preparing the new change now. :)
change uploaded link here:
https://lore.kernel.org/linux-gpio/[email protected]/
>
> READ_ONCE can only avoid the possible infinite loop and not crash the
> whole kernel, while the lock is needed to protect the multi parallel
> call of pinctrl_commit_state api have a consistent atomic hardware
> result as well.
>>
>> Yours,
>> Linus Walleij
>
--
Thx and BRs,
Aiqun(Maria) Yu