On 11/29/2023 11:08 PM, Linus Walleij wrote:
On Wed, Nov 29, 2023 at 3:56 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
On Wed, 29 Nov 2023 at 04:09, Linus Walleij <linus.walleij@xxxxxxxxxx> 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
