| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20230911161553.24313-1-brgl@bgdev.pl>
Date: Mon, 11 Sep 2023 18:15:53 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Mark Brown <broonie@...nel.org>,
Florian Fainelli <florian.fainelli@...adcom.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Linus Walleij <linus.walleij@...aro.org>,
Ray Jui <rjui@...adcom.com>,
Scott Branden <sbranden@...adcom.com>
Cc: Broadcom internal kernel review list
<bcm-kernel-feedback-list@...adcom.com>, linux-spi@...r.kernel.org,
linux-rpi-kernel@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: [PATCH v3] spi: bcm2835: reduce the abuse of the GPIO API
From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Currently the bcm2835 SPI driver uses functions that are available
exclusively to GPIO providers as a way to handle a platform quirk. Let's
use a slightly better alternative that avoids poking around in GPIOLIB's
internals and use GPIO lookup tables.
Link: https://www.spinics.net/lists/linux-gpio/msg36218.html
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
---
This is only build-tested. It should work, but it would be great if
someone from broadcom could test this.
Andy pointed out elsewhere that we can use GPIO_LOOKUP() even in
dynamically allocated tables, hence v3.
v1 -> v2:
- don't use devres for managing the GPIO but put it manually in .cleanup()
- add a mailing list link explaining the background of the bug
- fix kerneldoc
v2 -> v3:
- use GPIO_LOOKUP() macro for creating the lookup entry as it looks
better and results in less LOC
drivers/spi/spi-bcm2835.c | 60 +++++++++++++++++++++++----------------
1 file changed, 35 insertions(+), 25 deletions(-)
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index e7bb2714678a..dfd9c4997052 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -11,6 +11,7 @@
* spi-atmel.c, Copyright (C) 2006 Atmel Corporation
*/
+#include <linux/cleanup.h>
#include <linux/clk.h>
#include <linux/completion.h>
#include <linux/debugfs.h>
@@ -26,9 +27,10 @@
#include <linux/of_address.h>
#include <linux/platform_device.h>
#include <linux/gpio/consumer.h>
-#include <linux/gpio/machine.h> /* FIXME: using chip internals */
-#include <linux/gpio/driver.h> /* FIXME: using chip internals */
+#include <linux/gpio/machine.h> /* FIXME: using GPIO lookup tables */
#include <linux/of_irq.h>
+#include <linux/overflow.h>
+#include <linux/slab.h>
#include <linux/spi/spi.h>
/* SPI register offsets */
@@ -83,6 +85,7 @@ MODULE_PARM_DESC(polling_limit_us,
* struct bcm2835_spi - BCM2835 SPI controller
* @regs: base address of register map
* @clk: core clock, divided to calculate serial clock
+ * @cs_gpio: chip-select GPIO descriptor
* @clk_hz: core clock cached speed
* @irq: interrupt, signals TX FIFO empty or RX FIFO ¾ full
* @tfr: SPI transfer currently processed
@@ -117,6 +120,7 @@ MODULE_PARM_DESC(polling_limit_us,
struct bcm2835_spi {
void __iomem *regs;
struct clk *clk;
+ struct gpio_desc *cs_gpio;
unsigned long clk_hz;
int irq;
struct spi_transfer *tfr;
@@ -1156,15 +1160,11 @@ static void bcm2835_spi_handle_err(struct spi_controller *ctlr,
bcm2835_spi_reset_hw(bs);
}
-static int chip_match_name(struct gpio_chip *chip, void *data)
-{
- return !strcmp(chip->label, data);
-}
-
static void bcm2835_spi_cleanup(struct spi_device *spi)
{
struct bcm2835_spidev *target = spi_get_ctldata(spi);
struct spi_controller *ctlr = spi->controller;
+ struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
if (target->clear_rx_desc)
dmaengine_desc_free(target->clear_rx_desc);
@@ -1175,6 +1175,9 @@ static void bcm2835_spi_cleanup(struct spi_device *spi)
sizeof(u32),
DMA_TO_DEVICE);
+ gpiod_put(bs->cs_gpio);
+ spi_set_csgpiod(spi, 0, NULL);
+
kfree(target);
}
@@ -1221,7 +1224,7 @@ static int bcm2835_spi_setup(struct spi_device *spi)
struct spi_controller *ctlr = spi->controller;
struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
struct bcm2835_spidev *target = spi_get_ctldata(spi);
- struct gpio_chip *chip;
+ struct gpiod_lookup_table *lookup __free(kfree) = NULL;
int ret;
u32 cs;
@@ -1288,29 +1291,36 @@ static int bcm2835_spi_setup(struct spi_device *spi)
}
/*
- * Translate native CS to GPIO
+ * TODO: The code below is a slightly better alternative to the utter
+ * abuse of the GPIO API that I found here before. It creates a
+ * temporary lookup table, assigns it to the SPI device, gets the GPIO
+ * descriptor and then releases the lookup table.
*
- * FIXME: poking around in the gpiolib internals like this is
- * not very good practice. Find a way to locate the real problem
- * and fix it. Why is the GPIO descriptor in spi->cs_gpiod
- * sometimes not assigned correctly? Erroneous device trees?
+ * More on the problem that it addresses:
+ * https://www.spinics.net/lists/linux-gpio/msg36218.html
*/
+ lookup = kzalloc(struct_size(lookup, table, 1), GFP_KERNEL);
+ if (!lookup) {
+ ret = -ENOMEM;
+ goto err_cleanup;
+ }
+
+ lookup->dev_id = dev_name(&spi->dev);
+ lookup->table[0] = GPIO_LOOKUP("pinctrl-bcm2835",
+ 8 - (spi_get_chipselect(spi, 0)),
+ "cs", GPIO_LOOKUP_FLAGS_DEFAULT);
- /* get the gpio chip for the base */
- chip = gpiochip_find("pinctrl-bcm2835", chip_match_name);
- if (!chip)
- return 0;
-
- spi_set_csgpiod(spi, 0, gpiochip_request_own_desc(chip,
- 8 - (spi_get_chipselect(spi, 0)),
- DRV_NAME,
- GPIO_LOOKUP_FLAGS_DEFAULT,
- GPIOD_OUT_LOW));
- if (IS_ERR(spi_get_csgpiod(spi, 0))) {
- ret = PTR_ERR(spi_get_csgpiod(spi, 0));
+ gpiod_add_lookup_table(lookup);
+
+ bs->cs_gpio = gpiod_get(&spi->dev, "cs", GPIOD_OUT_LOW);
+ gpiod_remove_lookup_table(lookup);
+ if (IS_ERR(bs->cs_gpio)) {
+ ret = PTR_ERR(bs->cs_gpio);
goto err_cleanup;
}
+ spi_set_csgpiod(spi, 0, bs->cs_gpio);
+
/* and set up the "mode" and level */
dev_info(&spi->dev, "setting up native-CS%i to use GPIO\n",
spi_get_chipselect(spi, 0));
--
2.39.2
Powered by blists - more mailing lists