pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction()

[RFC/RFT PATCH] pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction()

Posted by Bartosz Golaszewski 10 months ago

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Since commit 9d846b1aebbe ("gpiolib: check the return value of
gpio_chip::get_direction()") we check the return value of the
get_direction() callback as per its API contract. This driver returns
-EINVAL if the pin in question is set to one of the alternative
(non-GPIO) functions. This isn't really an error that should be
communicated to GPIOLIB so default to returning the "safe" value of
INPUT in this case. The GPIO subsystem does not have the notion of
"unknown" direction.

Fixes: 9d846b1aebbe ("gpiolib: check the return value of gpio_chip::get_direction()")
Reported-by: Mark Brown <broonie@kernel.org>
Closes: https://lore.kernel.org/all/Z7VFB1nST6lbmBIo@finisterre.sirena.org.uk/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index cc1fe0555e19..eaeec096bc9a 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -346,14 +346,14 @@ static int bcm2835_gpio_get_direction(struct gpio_chip *chip, unsigned int offse
 	struct bcm2835_pinctrl *pc = gpiochip_get_data(chip);
 	enum bcm2835_fsel fsel = bcm2835_pinctrl_fsel_get(pc, offset);
 
-	/* Alternative function doesn't clearly provide a direction */
-	if (fsel > BCM2835_FSEL_GPIO_OUT)
-		return -EINVAL;
+	if (fsel == BCM2835_FSEL_GPIO_OUT)
+		return GPIO_LINE_DIRECTION_OUT;
 
-	if (fsel == BCM2835_FSEL_GPIO_IN)
-		return GPIO_LINE_DIRECTION_IN;
-
-	return GPIO_LINE_DIRECTION_OUT;
+	/*
+	 * Alternative function doesn't clearly provide a direction. Default
+	 * to INPUT.
+	 */
+	return GPIO_LINE_DIRECTION_IN;
 }
 
 static void bcm2835_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
-- 
2.45.2

Re: [RFC/RFT PATCH] pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction()

Posted by Linus Walleij 9 months, 2 weeks ago

On Wed, Feb 19, 2025 at 11:27 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Since commit 9d846b1aebbe ("gpiolib: check the return value of
> gpio_chip::get_direction()") we check the return value of the
> get_direction() callback as per its API contract. This driver returns
> -EINVAL if the pin in question is set to one of the alternative
> (non-GPIO) functions. This isn't really an error that should be
> communicated to GPIOLIB so default to returning the "safe" value of
> INPUT in this case. The GPIO subsystem does not have the notion of
> "unknown" direction.
>
> Fixes: 9d846b1aebbe ("gpiolib: check the return value of gpio_chip::get_direction()")
> Reported-by: Mark Brown <broonie@kernel.org>
> Closes: https://lore.kernel.org/all/Z7VFB1nST6lbmBIo@finisterre.sirena.org.uk/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Patch applied!

Yours,
Linus Walleij

Re: [RFC/RFT PATCH] pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction()

Posted by Stefan Wahren 10 months ago

Am 19.02.25 um 11:27 schrieb Bartosz Golaszewski:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Since commit 9d846b1aebbe ("gpiolib: check the return value of
> gpio_chip::get_direction()") we check the return value of the
> get_direction() callback as per its API contract. This driver returns
> -EINVAL if the pin in question is set to one of the alternative
> (non-GPIO) functions. This isn't really an error that should be
> communicated to GPIOLIB so default to returning the "safe" value of
> INPUT in this case. The GPIO subsystem does not have the notion of
> "unknown" direction.
>
> Fixes: 9d846b1aebbe ("gpiolib: check the return value of gpio_chip::get_direction()")
> Reported-by: Mark Brown <broonie@kernel.org>
> Closes: https://lore.kernel.org/all/Z7VFB1nST6lbmBIo@finisterre.sirena.org.uk/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

Thanks

Re: [RFC/RFT PATCH] pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction()

Posted by Mark Brown 10 months ago

On Wed, Feb 19, 2025 at 11:27:50AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Since commit 9d846b1aebbe ("gpiolib: check the return value of
> gpio_chip::get_direction()") we check the return value of the
> get_direction() callback as per its API contract. This driver returns
> -EINVAL if the pin in question is set to one of the alternative
> (non-GPIO) functions. This isn't really an error that should be
> communicated to GPIOLIB so default to returning the "safe" value of
> INPUT in this case. The GPIO subsystem does not have the notion of
> "unknown" direction.

I see this was already tested for these specific boards.  I've also
found that Avenger96 is failing with bisect pointing to the same commit
this is fixing:

    https://lava.sirena.org.uk/scheduler/job/1126314

as is the Libretech Potato:

    https://lava.sirena.org.uk/scheduler/job/1126285

neither of which produce any output before dying, they'll not be fixed
by this change.  Seems like an audit of the drivers might be in order?

Re: [RFC/RFT PATCH] pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction()

Posted by Bartosz Golaszewski 10 months ago

On Wed, Feb 19, 2025 at 3:27 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Feb 19, 2025 at 11:27:50AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Since commit 9d846b1aebbe ("gpiolib: check the return value of
> > gpio_chip::get_direction()") we check the return value of the
> > get_direction() callback as per its API contract. This driver returns
> > -EINVAL if the pin in question is set to one of the alternative
> > (non-GPIO) functions. This isn't really an error that should be
> > communicated to GPIOLIB so default to returning the "safe" value of
> > INPUT in this case. The GPIO subsystem does not have the notion of
> > "unknown" direction.
>
> I see this was already tested for these specific boards.  I've also
> found that Avenger96 is failing with bisect pointing to the same commit
> this is fixing:
>
>     https://lava.sirena.org.uk/scheduler/job/1126314
>
> as is the Libretech Potato:
>
>     https://lava.sirena.org.uk/scheduler/job/1126285
>
> neither of which produce any output before dying, they'll not be fixed
> by this change.  Seems like an audit of the drivers might be in order?

Right. I don't know if they return EINVAL or some other error so let
me prepare a change that will not bail-out but simply warn on
get_direction() errors in gpiochip_add_data() instead.

This patch can still go upstream IMO.

Bart

Re: [RFC/RFT PATCH] pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction()

Posted by Linus Walleij 9 months, 3 weeks ago

On Wed, Feb 19, 2025 at 3:30 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Wed, Feb 19, 2025 at 3:27 PM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Wed, Feb 19, 2025 at 11:27:50AM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Since commit 9d846b1aebbe ("gpiolib: check the return value of
> > > gpio_chip::get_direction()") we check the return value of the
> > > get_direction() callback as per its API contract. This driver returns
> > > -EINVAL if the pin in question is set to one of the alternative
> > > (non-GPIO) functions. This isn't really an error that should be
> > > communicated to GPIOLIB so default to returning the "safe" value of
> > > INPUT in this case. The GPIO subsystem does not have the notion of
> > > "unknown" direction.
> >
> > I see this was already tested for these specific boards.  I've also
> > found that Avenger96 is failing with bisect pointing to the same commit
> > this is fixing:
> >
> >     https://lava.sirena.org.uk/scheduler/job/1126314
> >
> > as is the Libretech Potato:
> >
> >     https://lava.sirena.org.uk/scheduler/job/1126285
> >
> > neither of which produce any output before dying, they'll not be fixed
> > by this change.  Seems like an audit of the drivers might be in order?
>
> Right. I don't know if they return EINVAL or some other error so let
> me prepare a change that will not bail-out but simply warn on
> get_direction() errors in gpiochip_add_data() instead.
>
> This patch can still go upstream IMO.

I'm fine to apply it, maybe as non-urgent fix at this point? (for -next)

Do you want to send a non-RFC/RFT version or should I just apply it?

Yours,
Linus Walleij

Re: [RFC/RFT PATCH] pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction()

Posted by Bartosz Golaszewski 9 months, 3 weeks ago

On Thu, Feb 27, 2025 at 11:53 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Feb 19, 2025 at 3:30 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Wed, Feb 19, 2025 at 3:27 PM Mark Brown <broonie@kernel.org> wrote:
> > >
> > > On Wed, Feb 19, 2025 at 11:27:50AM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > Since commit 9d846b1aebbe ("gpiolib: check the return value of
> > > > gpio_chip::get_direction()") we check the return value of the
> > > > get_direction() callback as per its API contract. This driver returns
> > > > -EINVAL if the pin in question is set to one of the alternative
> > > > (non-GPIO) functions. This isn't really an error that should be
> > > > communicated to GPIOLIB so default to returning the "safe" value of
> > > > INPUT in this case. The GPIO subsystem does not have the notion of
> > > > "unknown" direction.
> > >
> > > I see this was already tested for these specific boards.  I've also
> > > found that Avenger96 is failing with bisect pointing to the same commit
> > > this is fixing:
> > >
> > >     https://lava.sirena.org.uk/scheduler/job/1126314
> > >
> > > as is the Libretech Potato:
> > >
> > >     https://lava.sirena.org.uk/scheduler/job/1126285
> > >
> > > neither of which produce any output before dying, they'll not be fixed
> > > by this change.  Seems like an audit of the drivers might be in order?
> >
> > Right. I don't know if they return EINVAL or some other error so let
> > me prepare a change that will not bail-out but simply warn on
> > get_direction() errors in gpiochip_add_data() instead.
> >
> > This patch can still go upstream IMO.
>
> I'm fine to apply it, maybe as non-urgent fix at this point? (for -next)
>

Yes, the offending changes in gpiolib.c were dropped so this can go in
the non-urgent way.

> Do you want to send a non-RFC/RFT version or should I just apply it?
>

You can take it as is. It got tested and reviewed, so the tags served
their purpose. :)

Bart

Re: [RFC/RFT PATCH] pinctrl: bcm2835: don't -EINVAL on alternate funcs from get_direction()

Posted by Marek Szyprowski 10 months ago

On 19.02.2025 11:27, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Since commit 9d846b1aebbe ("gpiolib: check the return value of
> gpio_chip::get_direction()") we check the return value of the
> get_direction() callback as per its API contract. This driver returns
> -EINVAL if the pin in question is set to one of the alternative
> (non-GPIO) functions. This isn't really an error that should be
> communicated to GPIOLIB so default to returning the "safe" value of
> INPUT in this case. The GPIO subsystem does not have the notion of
> "unknown" direction.
>
> Fixes: 9d846b1aebbe ("gpiolib: check the return value of gpio_chip::get_direction()")
> Reported-by: Mark Brown <broonie@kernel.org>
> Closes: https://lore.kernel.org/all/Z7VFB1nST6lbmBIo@finisterre.sirena.org.uk/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Closes: 
https://lore.kernel.org/all/dfe03f88-407e-4ef1-ad30-42db53bbd4e4@samsung.com/
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/pinctrl/bcm/pinctrl-bcm2835.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> index cc1fe0555e19..eaeec096bc9a 100644
> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> @@ -346,14 +346,14 @@ static int bcm2835_gpio_get_direction(struct gpio_chip *chip, unsigned int offse
>   	struct bcm2835_pinctrl *pc = gpiochip_get_data(chip);
>   	enum bcm2835_fsel fsel = bcm2835_pinctrl_fsel_get(pc, offset);
>   
> -	/* Alternative function doesn't clearly provide a direction */
> -	if (fsel > BCM2835_FSEL_GPIO_OUT)
> -		return -EINVAL;
> +	if (fsel == BCM2835_FSEL_GPIO_OUT)
> +		return GPIO_LINE_DIRECTION_OUT;
>   
> -	if (fsel == BCM2835_FSEL_GPIO_IN)
> -		return GPIO_LINE_DIRECTION_IN;
> -
> -	return GPIO_LINE_DIRECTION_OUT;
> +	/*
> +	 * Alternative function doesn't clearly provide a direction. Default
> +	 * to INPUT.
> +	 */
> +	return GPIO_LINE_DIRECTION_IN;
>   }
>   
>   static void bcm2835_gpio_set(struct gpio_chip *chip, unsigned offset, int value)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland