Skip to contentSkip to conversation

spi: fix resource leaks on device setup failure

PATCH In review Johan Hovold 2 participants 5 messages Apr 10 – Apr 22

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover spi: fix resource leaks on device setup failure

Make sure to call controller cleanup() if spi_setup() fails while registering a device to avoid leaking any resources allocated by setup().

Johan

Changes in v2:
- only call cleanup() if spi_setup() fails during registration
- use spi_cleanup() helper
- fix controller cleanup() documentation (new patch)

Johan Hovold (2):
spi: fix resource leaks on device setup failure
spi: fix controller cleanup() documentation

 drivers/spi/spi.c       | 61 +++++++++++++++++++++++++----------------
 include/linux/spi/spi.h |  2 +-
 2 files changed, 38 insertions(+), 25 deletions(-)
Signature · 1 line
2.52.0
#1 lore raw

PATCH 1/2 spi: fix resource leaks on device setup failure

Make sure to call controller cleanup() if spi_setup() fails while registering a device to avoid leaking any resources allocated by setup().

Fixes c7299fea6769 ("spi: Fix spi device unregister flow")
Cc Saravana Kannan <[email protected]>
---
 drivers/spi/spi.c | 61 ++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 24 deletions(-)
drivers/spi/spi.c +37 -24
index 272b45d6b5aa..c14f29457c29 100644
@@ -43,6 +43,8 @@ EXPORT_TRACEPOINT_SYMBOL(spi_transfer_stop);
 
 #include "internals.h"
 
+static int __spi_setup(struct spi_device *spi, bool initial_setup);
+
 static DEFINE_IDR(spi_controller_idr);
 
 static void spidev_release(struct device *dev)
@@ -726,7 +728,7 @@ static int __spi_add_device(struct spi_device *spi)
 	 * normally rely on the device being setup.  Devices
 	 * using SPI_CS_HIGH can't coexist well otherwise...
 	 */
-	status = spi_setup(spi);
+	status = __spi_setup(spi, true);
 	if (status < 0) {
 		dev_err(dev, "can't setup %s, status %d\n",
 				dev_name(&spi->dev), status);
@@ -3992,27 +3994,7 @@ static int spi_set_cs_timing(struct spi_device *spi)
 	return status;
 }
 
-/**
- * spi_setup - setup SPI mode and clock rate
- * @spi: the device whose settings are being modified
- * Context: can sleep, and no requests are queued to the device
- *
- * SPI protocol drivers may need to update the transfer mode if the
- * device doesn't work with its default.  They may likewise need
- * to update clock rates or word sizes from initial values.  This function
- * changes those settings, and must be called from a context that can sleep.
- * Except for SPI_CS_HIGH, which takes effect immediately, the changes take
- * effect the next time the device is selected and data is transferred to
- * or from it.  When this function returns, the SPI device is deselected.
- *
- * Note that this call will fail if the protocol driver specifies an option
- * that the underlying controller or its driver does not support.  For
- * example, not all hardware supports wire transfers using nine bit words,
- * LSB-first wire encoding, or active-high chipselects.
- *
- * Return: zero on success, else a negative error code.
- */
-int spi_setup(struct spi_device *spi)
+static int __spi_setup(struct spi_device *spi, bool initial_setup)
 {
 	unsigned	bad_bits, ugly_bits;
 	int		status;
@@ -4097,7 +4079,7 @@ int spi_setup(struct spi_device *spi)
 	status = spi_set_cs_timing(spi);
 	if (status) {
 		mutex_unlock(&spi->controller->io_mutex);
-		return status;
+		goto err_cleanup;
 	}
 
 	if (spi->controller->auto_runtime_pm && spi->controller->set_cs) {
@@ -4106,7 +4088,7 @@ int spi_setup(struct spi_device *spi)
 			mutex_unlock(&spi->controller->io_mutex);
 			dev_err(&spi->controller->dev, "Failed to power device: %d\n",
 				status);
-			return status;
+			goto err_cleanup;
 		}
 
 		/*
@@ -4142,6 +4124,37 @@ int spi_setup(struct spi_device *spi)
 			status);
 
 	return status;
+
+err_cleanup:
+	if (initial_setup)
+		spi_cleanup(spi);
+
+	return status;
+}
+
+/**
+ * spi_setup - setup SPI mode and clock rate
+ * @spi: the device whose settings are being modified
+ * Context: can sleep, and no requests are queued to the device
+ *
+ * SPI protocol drivers may need to update the transfer mode if the
+ * device doesn't work with its default.  They may likewise need
+ * to update clock rates or word sizes from initial values.  This function
+ * changes those settings, and must be called from a context that can sleep.
+ * Except for SPI_CS_HIGH, which takes effect immediately, the changes take
+ * effect the next time the device is selected and data is transferred to
+ * or from it.  When this function returns, the SPI device is deselected.
+ *
+ * Note that this call will fail if the protocol driver specifies an option
+ * that the underlying controller or its driver does not support.  For
+ * example, not all hardware supports wire transfers using nine bit words,
+ * LSB-first wire encoding, or active-high chipselects.
+ *
+ * Return: zero on success, else a negative error code.
+ */
+int spi_setup(struct spi_device *spi)
+{
+	return __spi_setup(spi, false);
 }
 EXPORT_SYMBOL_GPL(spi_setup);
 
Signature · 1 line
2.52.0
#2 lore raw

PATCH 2/2 spi: fix controller cleanup() documentation

In reply to Johan Hovold

The controller cleanup() callback is no longer called when releasing a device, but rather when deregistering it (and on registration failures).

Fixes c7299fea6769 ("spi: Fix spi device unregister flow")
Cc Saravana Kannan <[email protected]>
---
 include/linux/spi/spi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/spi/spi.h +1 -1
index 0dc671c07d3a..0de636484f3d 100644
@@ -700,7 +700,7 @@ struct spi_controller {
 	int			(*transfer)(struct spi_device *spi,
 						struct spi_message *mesg);
 
-	/* Called on release() to free memory provided by spi_controller */
+	/* Called on deregistration to free memory provided by spi_controller */
 	void			(*cleanup)(struct spi_device *spi);
 
 	/*
Signature · 1 line
2.52.0
#3 lore raw
In reply to Johan Hovold
Quoted 8 lines · Johan Hovold

Make sure to call controller cleanup() if spi_setup() fails while registering a device to avoid leaking any resources allocated by setup().

Changes in v2:
- only call cleanup() if spi_setup() fails during registration
- use spi_cleanup() helper
- fix controller cleanup() documentation (new patch)

Bah, I missed to add "v2" to Subject. Let me know if you prefer me to send a v3...

Johan

maintainer
#4 lore raw
In reply to Johan Hovold
Quoted 9 lines · Johan Hovold

spi: fix resource leaks on device setup failure

Make sure to call controller cleanup() if spi_setup() fails while registering a device to avoid leaking any resources allocated by setup().

Johan

[...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-7.1

Thanks!

[1/2] spi: fix resource leaks on device setup failure

      https://git.kernel.org/broonie/spi/c/db357034f7e0
[2/2] spi: fix controller cleanup() documentation
      https://git.kernel.org/broonie/spi/c/a6e23843e949

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