Re: [RFC PATCH] pinmux: Use sequential access to access desc->pinmux data

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Thu, Sep 12, 2024 at 4:25 PM Mukesh Ojha <quic_mojha@xxxxxxxxxxx> wrote:

> When two client of the same gpio call pinctrl_select_state() for the
> same functionality, we are seeing NULL pointer issue while accessing
> desc->mux_owner.

Uh-oh it looks like a very real issue, weird that we didn't run into
it earlier.

I guess we were not parallelizing probe so much in the past so it
didn't happen for that reason.

>         /* Set owner */
>         pindesc->pctldev = pctldev;
> +#ifdef CONFIG_PINMUX
> +       spin_lock_init(&pindesc->lock);
> +#endif

Can we rename it "mux_lock" so it is clear what it is locking?

> @@ -115,6 +115,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
>         struct pin_desc *desc;
>         const struct pinmux_ops *ops = pctldev->desc->pmxops;
>         int status = -EINVAL;
> +       unsigned long flags;
>
>         desc = pin_desc_get(pctldev, pin);
>         if (desc == NULL) {
> @@ -127,6 +128,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
>         dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n",
>                 pin, desc->name, owner);
>
> +       spin_lock_irqsave(&desc->lock, flags);

Could you please rewrite all of these using scoped guards as that
avoids a lot of possible bugs?

#include <linux/cleanup.h>

guard(spinlock_irqsave)(&desc->mux_lock);

This means the lock will be released when you exit the
function .

tighter locks around a block of code are possible with:
scoped_guard(spinlock_irqsave, &desc->mux_lock) { ... }

It also removes the need to define a flags variable.

Yours,
Linus Walleij