[PATCH] spi: bcm2835: Do not call gpiod_put() on invalid descriptor

[PATCH] spi: bcm2835: Do not call gpiod_put() on invalid descriptor

[Florian Fainelli]

If we are unable to lookup the chip-select GPIO, the error path will
call bcm2835_spi_cleanup() which unconditionally calls gpiod_put() on
the cs->gpio variable which we just determined was invalid.

Fixes: 21f252cd29f0 ("spi: bcm2835: reduce the abuse of the GPIO API")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/spi/spi-bcm2835.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index e1b9b1235787..a5d621b94d5e 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -1162,7 +1162,8 @@ static void bcm2835_spi_cleanup(struct spi_device *spi)
 				 sizeof(u32),
 				 DMA_TO_DEVICE);
 
-	gpiod_put(bs->cs_gpio);
+	if (!IS_ERR(bs->cs_gpio))
+		gpiod_put(bs->cs_gpio);
 	spi_set_csgpiod(spi, 0, NULL);
 
 	kfree(target);
-- 
2.34.1

Re: [PATCH] spi: bcm2835: Do not call gpiod_put() on invalid descriptor

[Bartosz Golaszewski]

On Wed, Apr 2, 2025 at 12:43 AM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> If we are unable to lookup the chip-select GPIO, the error path will
> call bcm2835_spi_cleanup() which unconditionally calls gpiod_put() on
> the cs->gpio variable which we just determined was invalid.
>
> Fixes: 21f252cd29f0 ("spi: bcm2835: reduce the abuse of the GPIO API")
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/spi/spi-bcm2835.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index e1b9b1235787..a5d621b94d5e 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -1162,7 +1162,8 @@ static void bcm2835_spi_cleanup(struct spi_device *spi)
>                                  sizeof(u32),
>                                  DMA_TO_DEVICE);
>
> -       gpiod_put(bs->cs_gpio);
> +       if (!IS_ERR(bs->cs_gpio))
> +               gpiod_put(bs->cs_gpio);
>         spi_set_csgpiod(spi, 0, NULL);
>
>         kfree(target);
> --
> 2.34.1
>
>

We could also just set it to NULL on error in bcm2835_spi_setup() but
I'm fine either way.

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH] spi: bcm2835: Do not call gpiod_put() on invalid descriptor

[Mark Brown]

On Tue, 01 Apr 2025 15:42:38 -0700, Florian Fainelli wrote:
> If we are unable to lookup the chip-select GPIO, the error path will
> call bcm2835_spi_cleanup() which unconditionally calls gpiod_put() on
> the cs->gpio variable which we just determined was invalid.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: bcm2835: Do not call gpiod_put() on invalid descriptor
      commit: d6691010523fe1016f482a1e1defcc6289eeea48

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH] spi: bcm2835: Do not call gpiod_put() on invalid descriptor

[Andy Shevchenko]

On Wed, Apr 02, 2025 at 01:36:28PM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 2, 2025 at 12:43 AM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
> >
> > If we are unable to lookup the chip-select GPIO, the error path will
> > call bcm2835_spi_cleanup() which unconditionally calls gpiod_put() on
> > the cs->gpio variable which we just determined was invalid.

...

> > -       gpiod_put(bs->cs_gpio);
> > +       if (!IS_ERR(bs->cs_gpio))
> > +               gpiod_put(bs->cs_gpio);

> We could also just set it to NULL on error in bcm2835_spi_setup() but
> I'm fine either way.

I think this patch papers over the real issue:
1) the cleanup call does everything and not split to have the exact reversed order of the setup;
2) the GPIO here as far as I understand is not optional and on errors may contain an error pointer
but gpiod_put() ignores that.

TL;DR: I think the proper fix is to make gpio_put() to accept an error pointer as NULL. I.o.w.
if (desc) --> if (!IS_ERR_OR_NULL(desc)) in all conditionals related to gpiod*put*() calls.

-- 
With Best Regards,
Andy Shevchenko