[PATCH] pinctrl-bcm2835.c: fix race condition when setting gpio dir

[Stefan Wahren]

Hi Hans,

Am 20.04.23 um 08:47 schrieb Hans Verkuil:
> In the past setting the pin direction called pinctrl_gpio_direction()
> which uses a mutex to serialize this. That was changed to set the
> direction directly in the pin controller driver, but that lost the
> serialization mechanism. Since the direction of multiple pins are in
> the same register you can have a race condition, something that was
> in fact observed with the cec-gpio driver.
> 
> Add a new spinlock to serialize writing to the FSEL registers.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco at xs4all.nl>
> Fixes: 1a4541b68e25 ("pinctrl-bcm2835: don't call pinctrl_gpio_direction()")
> ---
>   drivers/pinctrl/bcm/pinctrl-bcm2835.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> index 8e2551a08c37..345b9dea5ff6 100644
> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> @@ -90,6 +90,8 @@ struct bcm2835_pinctrl {
>   	struct pinctrl_gpio_range gpio_range;
> 
>   	raw_spinlock_t irq_lock[BCM2835_NUM_BANKS];
> +	/* Protect FSEL registers */
> +	spinlock_t fsel_lock;
>   };
> 
>   /* pins are just named GPIO0..GPIO53 */
> @@ -284,14 +286,21 @@ static inline void bcm2835_pinctrl_fsel_set(
>   		struct bcm2835_pinctrl *pc, unsigned pin,
>   		enum bcm2835_fsel fsel)
>   {
> -	u32 val = bcm2835_gpio_rd(pc, FSEL_REG(pin));
> -	enum bcm2835_fsel cur = (val >> FSEL_SHIFT(pin)) & BCM2835_FSEL_MASK;
> +	u32 val;
> +	enum bcm2835_fsel cur;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&pc->fsel_lock, flags);
> +	val = bcm2835_gpio_rd(pc, FSEL_REG(pin));
> +	cur = (val >> FSEL_SHIFT(pin)) & BCM2835_FSEL_MASK;
> 
>   	dev_dbg(pc->dev, "read %08x (%u => %s)\n", val, pin,
> -			bcm2835_functions[cur]);
> +		bcm2835_functions[cur]);
> 
> -	if (cur == fsel)
> +	if (cur == fsel) {
> +		spin_unlock_irqrestore(&pc->fsel_lock, flags);
>   		return;

How about adding a jump label to the latter spin_unlock_irqrestore and 
using a goto here?

> +	}
> 
>   	if (cur != BCM2835_FSEL_GPIO_IN && fsel != BCM2835_FSEL_GPIO_IN) {
>   		/* always transition through GPIO_IN */
> @@ -309,6 +318,7 @@ static inline void bcm2835_pinctrl_fsel_set(
>   	dev_dbg(pc->dev, "write %08x (%u <= %s)\n", val, pin,
>   			bcm2835_functions[fsel]);
>   	bcm2835_gpio_wr(pc, FSEL_REG(pin), val);
> +	spin_unlock_irqrestore(&pc->fsel_lock, flags);
>   }
> 
>   static int bcm2835_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> @@ -1248,6 +1258,7 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev)
>   	pc->gpio_chip = *pdata->gpio_chip;
>   	pc->gpio_chip.parent = dev;
> 
> +	spin_lock_init(&pc->fsel_lock);
>   	for (i = 0; i < BCM2835_NUM_BANKS; i++) {
>   		unsigned long events;
>   		unsigned offset;