Multi driver SPI chip select count unenforced.

Multi driver SPI chip select count unenforced.

[Joe Burmeister]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

Hi Mark,

I think I've stubbled into an issue with some SPI drivers.


At first I thought it was just the BCM driver which is where I noticed it.
Patch attached.

However, after I found the same broken pattern in a few places.
You can find suspects by grep'ing in the SPI folder for "->chip_select]".


The problem pattern is basically that the driver allocates an array for the 
number of chip selects it expects to support. Then uses the chip_select to index
that array. However the number of devices on the SPI isn't enforced, so in
device tree you can go straight past it.


Even if the driver did enforce its limit, the returned status could be ignored.
Patch for that also attached.


So question is, what is the fix. Currently num_chipselect is not only not enforced
but changed to what is in the device tree by_spi_get_gpio_numbers and
spi_get_gpio_descs. Some drivers may not really have a limit anyway.


I only found this because we have 6 devices hanging off the a Pi3's SPI bus and 
it wasn't shutting down properly. Found this was because of a panic in 
debugfs_remove and that turned out to be because debugfs_dir wasn't a valid
pointer. Which was because it was after the prepare_cs array in the structure
and was just being overwritten with prepare_cs used beyond it's length.

Regards,

Joe


[-- Attachment #2: Handle-SPI-device-setup-callback-failure.patch --]
[-- Type: text/x-patch, Size: 1151 bytes --]

From 9b17e09bcec47eaa58e733336124ce4a53337572 Mon Sep 17 00:00:00 2001
From: Joe Burmeister <joe.burmeister@devtank.co.uk>
Date: Fri, 16 Apr 2021 11:08:07 +0100
Subject: [PATCH] Handle SPI device setup callback failure.

If the setup callback failed, but the controller has auto_runtime_pm
and set_cs, the setup failure could be missed.

Signed-off-by: Joe Burmeister <joe.burmeister@devtank.co.uk>
---
 drivers/spi/spi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8b283b2c1668..0c39178f4401 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3388,8 +3388,15 @@ int spi_setup(struct spi_device *spi)
 
 	mutex_lock(&spi->controller->io_mutex);
 
-	if (spi->controller->setup)
+	if (spi->controller->setup) {
 		status = spi->controller->setup(spi);
+		if (status) {
+			mutex_unlock(&spi->controller->io_mutex);
+			dev_err(&spi->controller->dev, "Failed to setup device: %d\n",
+				status);
+			return status;
+		}
+	}
 
 	if (spi->controller->auto_runtime_pm && spi->controller->set_cs) {
 		status = pm_runtime_get_sync(spi->controller->dev.parent);
-- 
2.30.2


[-- Attachment #3: Remove-BCM2835-SPI-chipselect-limit.patch --]
[-- Type: text/x-patch, Size: 8052 bytes --]

From c02e84b7dc8a89dbe6749779bff315ca572e4c2d Mon Sep 17 00:00:00 2001
From: Joe Burmeister <joe.burmeister@devtank.co.uk>
Date: Wed, 14 Apr 2021 12:11:10 +0100
Subject: [PATCH] Remove BCM2835 SPI chipselect limit.

The limit of 4 chipselects for the BCM2835 was not required and also was
not inforced. Without inforcement it was possible to make a device tree
over this limit which would trample memory.

The chipselect count is now obtained from the device tree and expanded
if more devices are added.

Signed-off-by: Joe Burmeister <joe.burmeister@devtank.co.uk>
---
 drivers/spi/spi-bcm2835.c | 114 +++++++++++++++++++++++++++++++++-----
 1 file changed, 101 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index aab6c7e5c114..4f215ec3bd1b 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -28,6 +28,7 @@
 #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/of_gpio.h>
 #include <linux/of_irq.h>
 #include <linux/spi/spi.h>
 
@@ -134,7 +135,8 @@ struct bcm2835_spi {
 	int tx_prologue;
 	int rx_prologue;
 	unsigned int tx_spillover;
-	u32 prepare_cs[BCM2835_SPI_NUM_CS];
+	unsigned int allocated_cs_num;
+	u32 *prepare_cs;
 
 	struct dentry *debugfs_dir;
 	u64 count_transfer_polling;
@@ -147,9 +149,9 @@ struct bcm2835_spi {
 	unsigned int rx_dma_active;
 	struct dma_async_tx_descriptor *fill_tx_desc;
 	dma_addr_t fill_tx_addr;
-	struct dma_async_tx_descriptor *clear_rx_desc[BCM2835_SPI_NUM_CS];
+	struct dma_async_tx_descriptor **clear_rx_desc;
 	dma_addr_t clear_rx_addr;
-	u32 clear_rx_cs[BCM2835_SPI_NUM_CS] ____cacheline_aligned;
+	u32 *clear_rx_cs;
 };
 
 #if defined(CONFIG_DEBUG_FS)
@@ -859,14 +861,18 @@ static void bcm2835_dma_release(struct spi_controller *ctlr,
 	if (ctlr->dma_tx) {
 		dmaengine_terminate_sync(ctlr->dma_tx);
 
-		if (bs->fill_tx_desc)
+		if (bs->fill_tx_desc) {
 			dmaengine_desc_free(bs->fill_tx_desc);
+			bs->fill_tx_desc = NULL;
+		}
 
-		if (bs->fill_tx_addr)
+		if (bs->fill_tx_addr) {
 			dma_unmap_page_attrs(ctlr->dma_tx->device->dev,
 					     bs->fill_tx_addr, sizeof(u32),
 					     DMA_TO_DEVICE,
 					     DMA_ATTR_SKIP_CPU_SYNC);
+			bs->fill_tx_addr = 0;
+		}
 
 		dma_release_channel(ctlr->dma_tx);
 		ctlr->dma_tx = NULL;
@@ -875,15 +881,19 @@ static void bcm2835_dma_release(struct spi_controller *ctlr,
 	if (ctlr->dma_rx) {
 		dmaengine_terminate_sync(ctlr->dma_rx);
 
-		for (i = 0; i < BCM2835_SPI_NUM_CS; i++)
-			if (bs->clear_rx_desc[i])
+		for (i = 0; i < bs->allocated_cs_num; i++)
+			if (bs->clear_rx_desc[i]) {
 				dmaengine_desc_free(bs->clear_rx_desc[i]);
+				bs->clear_rx_desc[i] = NULL;
+			}
 
-		if (bs->clear_rx_addr)
+		if (bs->clear_rx_addr) {
 			dma_unmap_single(ctlr->dma_rx->device->dev,
 					 bs->clear_rx_addr,
-					 sizeof(bs->clear_rx_cs),
+					 sizeof(u32) * bs->allocated_cs_num,
 					 DMA_TO_DEVICE);
+			bs->clear_rx_addr = 0;
+		}
 
 		dma_release_channel(ctlr->dma_rx);
 		ctlr->dma_rx = NULL;
@@ -978,7 +988,7 @@ static int bcm2835_dma_init(struct spi_controller *ctlr, struct device *dev,
 
 	bs->clear_rx_addr = dma_map_single(ctlr->dma_rx->device->dev,
 					   bs->clear_rx_cs,
-					   sizeof(bs->clear_rx_cs),
+					   sizeof(u32) * bs->allocated_cs_num,
 					   DMA_TO_DEVICE);
 	if (dma_mapping_error(ctlr->dma_rx->device->dev, bs->clear_rx_addr)) {
 		dev_err(dev, "cannot map clear_rx_cs - not using DMA mode\n");
@@ -987,7 +997,7 @@ static int bcm2835_dma_init(struct spi_controller *ctlr, struct device *dev,
 		goto err_release;
 	}
 
-	for (i = 0; i < BCM2835_SPI_NUM_CS; i++) {
+	for (i = 0; i < bs->allocated_cs_num; i++) {
 		bs->clear_rx_desc[i] = dmaengine_prep_dma_cyclic(ctlr->dma_rx,
 					   bs->clear_rx_addr + i * sizeof(u32),
 					   sizeof(u32), 0,
@@ -1209,6 +1219,48 @@ static int bcm2835_spi_setup(struct spi_device *spi)
 	struct gpio_chip *chip;
 	u32 cs;
 
+	if (spi->chip_select >= bs->allocated_cs_num) {
+		unsigned int new_allocated_cs_num = spi->chip_select + 1;
+		void *new_prepare_cs, *new_clear_rx_desc, *new_clear_rx_cs;
+		int err;
+
+		dev_info(&spi->dev, "Increasing CS count to %u\n",
+			new_allocated_cs_num);
+
+		bcm2835_dma_release(ctlr, bs);
+
+		new_prepare_cs  = kmalloc_array(new_allocated_cs_num,
+			sizeof(u32), GFP_KERNEL);
+		new_clear_rx_desc = kmalloc_array(new_allocated_cs_num,
+			sizeof(struct dma_async_tx_descriptor *), GFP_KERNEL);
+		new_clear_rx_cs = kmalloc_array(new_allocated_cs_num,
+			sizeof(u32), GFP_DMA);
+
+		if (!new_prepare_cs || !new_clear_rx_desc || !new_clear_rx_cs) {
+			dev_err(&spi->dev, "Failed to allocate new CS arrays.\n");
+			return -ENOMEM;
+		}
+
+		memcpy(new_prepare_cs, bs->prepare_cs,
+			bs->allocated_cs_num * sizeof(u32));
+
+		kfree(bs->prepare_cs);
+		kfree(bs->clear_rx_desc);
+		kfree(bs->clear_rx_cs);
+
+		bs->prepare_cs  = new_prepare_cs;
+		bs->clear_rx_desc = new_clear_rx_desc;
+		bs->clear_rx_cs = new_clear_rx_cs;
+
+		bs->allocated_cs_num = new_allocated_cs_num;
+
+		err = bcm2835_dma_init(ctlr, &spi->dev, bs);
+		if (err) {
+			dev_err(&spi->dev, "Failed to reinit DMA after CS count change.");
+			return err;
+		}
+	}
+
 	/*
 	 * Precalculate SPI slave's CS register value for ->prepare_message():
 	 * The driver always uses software-controlled GPIO chip select, hence
@@ -1233,7 +1285,7 @@ static int bcm2835_spi_setup(struct spi_device *spi)
 						    BCM2835_SPI_CS_CLEAR_RX;
 		dma_sync_single_for_device(ctlr->dma_rx->device->dev,
 					   bs->clear_rx_addr,
-					   sizeof(bs->clear_rx_cs),
+					   sizeof(u32) * bs->allocated_cs_num,
 					   DMA_TO_DEVICE);
 	}
 
@@ -1248,6 +1300,7 @@ static int bcm2835_spi_setup(struct spi_device *spi)
 	 */
 	if (spi->cs_gpiod)
 		return 0;
+
 	if (spi->chip_select > 1) {
 		/* error in the case of native CS requested with CS > 1
 		 * officially there is a CS2, but it is not documented
@@ -1286,10 +1339,25 @@ static int bcm2835_spi_setup(struct spi_device *spi)
 	return 0;
 }
 
+
+#ifdef CONFIG_OF
+static int bcm2835_spi_get_num_chipselect(struct platform_device *pdev)
+{
+	return max_t(int, of_gpio_named_count(pdev->dev.of_node, "cs-gpios"), BCM2835_SPI_NUM_CS);
+}
+#else
+static int bcm2835_spi_get_num_chipselect(struct platform_device *pdev)
+{
+	return BCM2835_SPI_NUM_CS;
+}
+#endif
+
+
 static int bcm2835_spi_probe(struct platform_device *pdev)
 {
 	struct spi_controller *ctlr;
 	struct bcm2835_spi *bs;
+	int num_chipselect;
 	int err;
 
 	ctlr = devm_spi_alloc_master(&pdev->dev, ALIGN(sizeof(*bs),
@@ -1297,12 +1365,14 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 	if (!ctlr)
 		return -ENOMEM;
 
+	num_chipselect = bcm2835_spi_get_num_chipselect(pdev);
+
 	platform_set_drvdata(pdev, ctlr);
 
 	ctlr->use_gpio_descriptors = true;
 	ctlr->mode_bits = BCM2835_SPI_MODE_BITS;
 	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
-	ctlr->num_chipselect = BCM2835_SPI_NUM_CS;
+	ctlr->num_chipselect = num_chipselect;
 	ctlr->setup = bcm2835_spi_setup;
 	ctlr->transfer_one = bcm2835_spi_transfer_one;
 	ctlr->handle_err = bcm2835_spi_handle_err;
@@ -1311,6 +1381,20 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 
 	bs = spi_controller_get_devdata(ctlr);
 	bs->ctlr = ctlr;
+	bs->allocated_cs_num = num_chipselect;
+
+	bs->prepare_cs = kmalloc_array(num_chipselect, sizeof(u32), GFP_KERNEL);
+	if (!bs->prepare_cs)
+		return -ENOMEM;
+
+	bs->clear_rx_desc = kmalloc_array(num_chipselect,
+		sizeof(struct dma_async_tx_descriptor *), GFP_KERNEL);
+	if (!bs->clear_rx_desc)
+		return -ENOMEM;
+
+	bs->clear_rx_cs = kmalloc_array(num_chipselect, sizeof(u32), GFP_DMA);
+	if (!bs->clear_rx_cs)
+		return -ENOMEM;
 
 	bs->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(bs->regs))
@@ -1378,6 +1462,10 @@ static int bcm2835_spi_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(bs->clk);
 
+	kfree(bs->prepare_cs);
+	kfree(bs->clear_rx_desc);
+	kfree(bs->clear_rx_cs);
+
 	return 0;
 }
 
-- 
2.30.2

Re: Multi driver SPI chip select count unenforced.

[Lukas Wunner]

On Fri, Apr 16, 2021 at 11:55:45AM +0100, Joe Burmeister wrote:
> However, after I found the same broken pattern in a few places.
> You can find suspects by grep'ing in the SPI folder for "->chip_select]".

You need to cc the maintainers of the affected drivers.
You can retrieve them with "scripts/get_maintainer.pl drivers/spi/spi-...".

Please don't attach patches but rather submit each one separately.
Pass a commit range (abcdefg..0123456) to "git format-patch" to
get numbered patches, then submit them with msmtp or "git send-email".


> Subject: [PATCH] Handle SPI device setup callback failure.

Prepend the subsystem to the subject and drop the period, e.g.:

spi: Handle SPI device setup callback failure

Patch otherwise LGTM.


> Subject: [PATCH] Remove BCM2835 SPI chipselect limit.

Prepend "spi: bcm2835: " in this case and drop the period.
Use "git log --oneline <filename>" to see what the prefix should look
like for a particular file.


> The limit of 4 chipselects for the BCM2835 was not required and also was
> not inforced. Without inforcement it was possible to make a device tree
> over this limit which would trample memory.
> 
> The chipselect count is now obtained from the device tree and expanded
> if more devices are added.

I'd prefer it if you could just raise BCM2835_SPI_NUM_CS to 6
(or 8 or whatever you need).  Use commit 603e92ff10a8 as a template.

Then submit a separate patch to error out of bcm2835_spi_setup()
if bcm2835_spi_setup >= BCM2835_SPI_NUM_CS.

Honestly I think the additional code complexity isn't worth it to allow
for dynamic resizing.  Raising BCM2835_SPI_NUM_CS results in just a
few additional bytes, that's probably smaller than the increase of the
text segment due to the additional resizing code.

Nit: I think the correct spelling is "enforce".  (Not an English native
speaker but my dictionary says so.)

Thanks!

Lukas

Re: Multi driver SPI chip select count unenforced.

[Joe Burmeister]

Thanks Lukas,

I'll tweak and resubmit properly.

Regards,

Joe

On 16/04/2021 16:30, Lukas Wunner wrote:
> On Fri, Apr 16, 2021 at 11:55:45AM +0100, Joe Burmeister wrote:
>> However, after I found the same broken pattern in a few places.
>> You can find suspects by grep'ing in the SPI folder for "->chip_select]".
> You need to cc the maintainers of the affected drivers.
> You can retrieve them with "scripts/get_maintainer.pl drivers/spi/spi-...".
>
> Please don't attach patches but rather submit each one separately.
> Pass a commit range (abcdefg..0123456) to "git format-patch" to
> get numbered patches, then submit them with msmtp or "git send-email".
>
>
>> Subject: [PATCH] Handle SPI device setup callback failure.
> Prepend the subsystem to the subject and drop the period, e.g.:
>
> spi: Handle SPI device setup callback failure
>
> Patch otherwise LGTM.
>
>
>> Subject: [PATCH] Remove BCM2835 SPI chipselect limit.
> Prepend "spi: bcm2835: " in this case and drop the period.
> Use "git log --oneline <filename>" to see what the prefix should look
> like for a particular file.
>
>
>> The limit of 4 chipselects for the BCM2835 was not required and also was
>> not inforced. Without inforcement it was possible to make a device tree
>> over this limit which would trample memory.
>>
>> The chipselect count is now obtained from the device tree and expanded
>> if more devices are added.
> I'd prefer it if you could just raise BCM2835_SPI_NUM_CS to 6
> (or 8 or whatever you need).  Use commit 603e92ff10a8 as a template.
>
> Then submit a separate patch to error out of bcm2835_spi_setup()
> if bcm2835_spi_setup >= BCM2835_SPI_NUM_CS.
>
> Honestly I think the additional code complexity isn't worth it to allow
> for dynamic resizing.  Raising BCM2835_SPI_NUM_CS results in just a
> few additional bytes, that's probably smaller than the increase of the
> text segment due to the additional resizing code.
>
> Nit: I think the correct spelling is "enforce".  (Not an English native
> speaker but my dictionary says so.)
>
> Thanks!
>
> Lukas

Re: Multi driver SPI chip select count unenforced.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

On Fri, Apr 16, 2021 at 11:55:45AM +0100, Joe Burmeister wrote:

> I think I've stubbled into an issue with some SPI drivers.

Like Lukas says you need to copy the maintainers of those drivers.

> Even if the driver did enforce its limit, the returned status could be ignored.
> Patch for that also attached.

See submitting-patches.rst for how to submit a patch to the kernel,
Lukas flagged up most of the issues here.

> So question is, what is the fix. Currently num_chipselect is not only not enforced
> but changed to what is in the device tree by_spi_get_gpio_numbers and
> spi_get_gpio_descs. Some drivers may not really have a limit anyway.

It's really driver specific if it needs or uses the property so any
issues should be at the driver level.  Ideally the property should be
completely redundant, we can figure out how many chip selects we need to
handle based on knowing what devices are connected and what the
controller is but the binding was standardised ages ago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]