[4/5] spi: spi-gpio: Make optional chipselect handling more explicit

Message ID 20180101133749.29567-5-linus.walleij@linaro.org
State New
Headers show
Series
  • Rewrite GPIO SPI to use descriptors
Related show

Commit Message

Linus Walleij Jan. 1, 2018, 1:37 p.m.
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

Comments

Geert Uytterhoeven Jan. 3, 2018, 8:21 a.m. | #1
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
Linus Walleij Jan. 3, 2018, 8:58 a.m. | #2
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
Geert Uytterhoeven Jan. 3, 2018, 9:17 a.m. | #3
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
Linus Walleij Jan. 3, 2018, 9:40 a.m. | #4
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
Mark Brown Jan. 3, 2018, 4:05 p.m. | #5
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.

Patch

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