On 12/20/2023 7:02 PM, Linus Walleij wrote:
Hi Maria,
On Tue, Dec 12, 2023 at 10:06 AM Maria Yu <quic_aiquny@xxxxxxxxxxx> wrote:
Currently pinctrl_select_state is an export symbol and don't have
effective re-entrance protect design. During async probing of devices
it's possible to end up in pinctrl_select_state() from multiple
contexts simultaneously, so make it thread safe.
More over, when the real racy happened, the system frequently have
printk message like:
"not freeing pin xx (xxx) as part of deactivating group xxx - it is
already used for some other setting".
Finally the system crashed after the flood log.
Add per pinctrl lock to ensure the old state and new state transition
atomization.
Also move dev error print message outside the region with interrupts
disabled.
Fixes: 4198a9b57106 ("pinctrl: avoid reload of p state in list iteration")
Signed-off-by: Maria Yu <quic_aiquny@xxxxxxxxxxx>
Overall this looks good!
@@ -1262,9 +1263,12 @@ static void pinctrl_link_add(struct pinctrl_dev *pctldev,
static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
{
struct pinctrl_setting *setting, *setting2;
- struct pinctrl_state *old_state = READ_ONCE(p->state);
+ struct pinctrl_state *old_state;
int ret;
+ unsigned long flags;
+ spin_lock_irqsave(&p->lock, flags);
(...)
+ spin_unlock_irqrestore(&p->lock, flags);
(...)
+ spin_unlock_irqrestore(&p->lock, flags);
Is it possible to use a scoped guard for pinctrl_commit_state()?
Good idea.
I will address this in next patchset.
#include <linux/cleanup.h>
guard(spinlock_irqsave)(&p->lock);
It saves some code (and no need for flags) and avoid possible
bugs when people add new errorpaths to the code.
Yours,
Linus Walleij
--
Thx and BRs,
Aiqun(Maria) Yu
