| Message ID | 20180101133749.29567-5-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Rewrite GPIO SPI to use descriptors | expand |
Hi Linus, On Mon, Jan 1, 2018 at 2:37 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > I don't like the use of a NULL GPIO descriptor to handle the > "no chipselect connected" case, as it makes the code hard to read. > Use a clear bool "has_cs" that we use to explicitly handle the > case when no chip select is connected to it is clear to readers > how this is achieved. Hmm, having two indicators for the same feature (spi_gpio->has_cs == true and spi_gpio->cs_gpios[0] = NULL), and the need to keep them in sync in the future, also complicates things. > When there is no chip select connected, we don't even allocate a > placeholder for the GPIO descriptor. That's good, although it may have no effect on actual memory consumption due to slab granularity. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 3, 2018 at 9:21 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hmm, having two indicators for the same feature (spi_gpio->has_cs == true and > spi_gpio->cs_gpios[0] = NULL), and the need to keep them in sync in the > future, also complicates things. The patch does not do this. It removes the latter and adds the former. That is why it removes the assignment of NULL to cs_gpios[0]. >> When there is no chip select connected, we don't even allocate a >> placeholder for the GPIO descriptor. > > That's good, although it may have no effect on actual memory consumption > due to slab granularity. No idea, I'm not doing it to save memory anyways. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Wed, Jan 3, 2018 at 9:58 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Jan 3, 2018 at 9:21 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > >> Hmm, having two indicators for the same feature (spi_gpio->has_cs == true and >> spi_gpio->cs_gpios[0] = NULL), and the need to keep them in sync in the >> future, also complicates things. > > The patch does not do this. > > It removes the latter and adds the former. That is why it removes the > assignment of NULL to cs_gpios[0]. So what is cs_gpios[0] if has_cs == true? Oh, it will point to unallocated memory right after the structure... See, that's the ambiguity of having two variables... We do have NULL pointers and ERR_PTRs, so this ambiguity can (and IMHO should) be avoided. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 3, 2018 at 10:17 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Jan 3, 2018 at 9:58 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Wed, Jan 3, 2018 at 9:21 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> >>> Hmm, having two indicators for the same feature (spi_gpio->has_cs == true and >>> spi_gpio->cs_gpios[0] = NULL), and the need to keep them in sync in the >>> future, also complicates things. >> >> The patch does not do this. >> >> It removes the latter and adds the former. That is why it removes the >> assignment of NULL to cs_gpios[0]. > > So what is cs_gpios[0] if has_cs == true? Oh, it will point to unallocated > memory right after the structure... I think it is a problem with all dynamic arrays: they are prone to out-of-bounds accesses. That in turn comes from the following design pattern i the spi alloc_master() helper: status = spi_gpio_request(pdata, dev_name(&pdev->dev), &master_flags); if (status < 0) return status; master = spi_alloc_master(&pdev->dev, sizeof(*spi_gpio) + (sizeof(unsigned long) * num_devices)); if (!master) { status = -ENOMEM; goto gpio_free; } spi_gpio = spi_master_get_devdata(master); So someone wanted to save a slab by using this instead of allocating the array dynamically. It's not a very admirable coding style to begin with. I doubt the system gains much from this. If you agree I can rewrite it to have struct gpio_desc ** and dynamically allocate that to the number of descriptors with devm_kzalloc() instead, that is more in line with kernel patterns and this memory optimization anyways seems a bit overdone anyways. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 03, 2018 at 10:40:52AM +0100, Linus Walleij wrote: > So someone wanted to save a slab by using this instead of allocating > the array dynamically. It's not a very admirable coding style to begin > with. I doubt the system gains much from this. Right, David was very focused on saving memory. > If you agree I can rewrite it to have struct gpio_desc ** and > dynamically allocate that to the number of descriptors with devm_kzalloc() > instead, that is more in line with kernel patterns and this memory > optimization anyways seems a bit overdone anyways. Yes, that would be clearer - it's something that's good to fix anyway rather than trying to contort around it.
diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c index ab2cb9427481..c12e588e54e7 100644 --- a/drivers/spi/spi-gpio.c +++ b/drivers/spi/spi-gpio.c @@ -47,6 +47,7 @@ struct spi_gpio { struct gpio_desc *sck; struct gpio_desc *miso; struct gpio_desc *mosi; + bool has_cs; /* Will be allocated times number of devices beyond end of struct */ struct gpio_desc *cs_gpios[0]; }; @@ -215,15 +216,18 @@ static u32 spi_gpio_spec_txrx_word_mode3(struct spi_device *spi, static void spi_gpio_chipselect(struct spi_device *spi, int is_active) { struct spi_gpio *spi_gpio = spi_to_spi_gpio(spi); - struct gpio_desc *cs = spi_gpio->cs_gpios[spi->chip_select]; /* set initial clock line level */ if (is_active) gpiod_set_value_cansleep(spi_gpio->sck, spi->mode & SPI_CPOL); - if (cs) + /* Drive chip select line, if we have one */ + if (spi_gpio->has_cs) { + struct gpio_desc *cs = spi_gpio->cs_gpios[spi->chip_select]; + /* SPI chip selects are normally active-low */ gpiod_set_value_cansleep(cs, (spi->mode & SPI_CS_HIGH) ? is_active : !is_active); + } } static int spi_gpio_setup(struct spi_device *spi) @@ -293,9 +297,6 @@ static int spi_gpio_request(struct device *dev, if (IS_ERR(spi_gpio->cs_gpios[i])) return PTR_ERR(spi_gpio->cs_gpios[i]); } - /* Dummy chipselect line if the single device is not using chipselect */ - if (!num_chipselects) - spi_gpio->cs_gpios[0] = NULL; return 0; } @@ -354,7 +355,6 @@ static int spi_gpio_probe(struct platform_device *pdev) struct spi_gpio_platform_data *pdata; u16 master_flags = 0; bool use_of = 0; - int num_devices; status = spi_gpio_probe_dt(pdev); if (status < 0) @@ -368,19 +368,17 @@ static int spi_gpio_probe(struct platform_device *pdev) return -ENODEV; #endif - if (use_of && !pdata->num_chipselect) - num_devices = 1; - else - num_devices = pdata->num_chipselect; - master = spi_alloc_master(&pdev->dev, sizeof(*spi_gpio) + - (sizeof(struct gpio_desc *) * num_devices)); + (sizeof(struct gpio_desc *) * pdata->num_chipselect)); if (!master) return -ENOMEM; spi_gpio = spi_master_get_devdata(master); platform_set_drvdata(pdev, spi_gpio); + /* Determine if we have chip selects connected */ + spi_gpio->has_cs = !!pdata->num_chipselect; + spi_gpio->pdev = pdev; if (pdata) spi_gpio->pdata = *pdata; @@ -393,7 +391,8 @@ static int spi_gpio_probe(struct platform_device *pdev) master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); master->flags = master_flags; master->bus_num = pdev->id; - master->num_chipselect = num_devices; + /* The master needs to think there is a chipselect even if not connected */ + master->num_chipselect = spi_gpio->has_cs ? pdata->num_chipselect : 1; master->setup = spi_gpio_setup; master->cleanup = spi_gpio_cleanup; #ifdef CONFIG_OF
I don't like the use of a NULL GPIO descriptor to handle the "no chipselect connected" case, as it makes the code hard to read. Use a clear bool "has_cs" that we use to explicitly handle the case when no chip select is connected to it is clear to readers how this is achieved. When there is no chip select connected, we don't even allocate a placeholder for the GPIO descriptor. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/spi/spi-gpio.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) -- 2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html