| Message ID | 20250401224238.2854256-1-florian.fainelli@broadcom.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | spi: bcm2835: Do not call gpiod_put() on invalid descriptor | expand |
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>
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
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.
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);
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(-)