Skip to contentSkip to conversation

spi: fix resource leaks on device setup failure

PATCH In review Johan Hovold 2 participants 3 messages Mar 24

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

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

Make sure to call controller cleanup() on late device setup failures to avoid leaking resources allocated by setup().

Fixes c7299fea6769 ("spi: Fix spi device unregister flow")
Cc Saravana Kannan <[email protected]>
---
 drivers/spi/spi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/spi/spi.c +8 -2
index 9fe9f99183bf..cb00619864cf 100644
@@ -4091,7 +4091,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) {
@@ -4100,7 +4100,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;
 		}
 
 		/*
@@ -4136,6 +4136,12 @@ int spi_setup(struct spi_device *spi)
 			status);
 
 	return status;
+
+err_cleanup:
+	if (spi->controller->cleanup)
+		spi->controller->cleanup(spi);
+
+	return status;
 }
 EXPORT_SYMBOL_GPL(spi_setup);
 
Signature · 1 line
2.52.0
maintainer
#1 lore raw

drivers/spi/spi.c int spi_setup(struct spi_device *spi)

Quoted 6 lines · Johan Hovold

Make sure to call controller cleanup() on late device setup failures to avoid leaking resources allocated by setup().

drivers/spi/spi.c +0 -0
@@ -4091,7 +4091,7 @@ int spi_setup(struct spi_device *spi)

This is specifically setup(). This is called repeatedly at runtime, it's not part of the device registration flow like the issues that were addressed by the commit you tagged as a fix. Can you be more specific about the leak you're trying to fix here?

drivers/spi/spi.c int spi_setup(struct spi_device *spi) · line 4142

Quoted 3 lines
drivers/spi/spi.c +3 -0
+err_cleanup:
+       if (spi->controller->cleanup)
+               spi->controller->cleanup(spi);

spi_cleanup()

#2 lore raw
Quoted 5 lines · Mark Brown
Quoted 2 lines · Johan Hovold

Make sure to call controller cleanup() on late device setup failures to avoid leaking resources allocated by setup().

Quoted 8 lines

drivers/spi/spi.c int spi_setup(struct spi_device *spi)

Quoted 3 lines
drivers/spi/spi.c +0 -0
@@ -4091,7 +4091,7 @@ int spi_setup(struct spi_device *spi)

This is specifically setup(). This is called repeatedly at runtime, it's not part of the device registration flow like the issues that were addressed by the commit you tagged as a fix. Can you be more specific about the leak you're trying to fix here?

It's when spi_add_device() device fails in spi_setup() that cleanup() may no longer be called.

	spi_add_device()
	  __spi_add_device()
	    spi_setup()
	      controller->setup()
	      return -<errno>;

And there are drivers that allocate memory in the controller->setup() callback which is released in ->cleanup() (e.g. atmel_spi_setup() and atmel_spi_cleanup()).

From a quick look at some of the drivers it did not seem limited to memory allocations.

But I see now that not all of them can handle cleanup() being called more than once, which could indeed happen if a later call to spi_setup() fails after the device has been added.

We could set a flag to track whether setup() has ever been called successfully, or make sure that cleanup() can be called more than once for all drivers.

Quoted 5 lines

drivers/spi/spi.c int spi_setup(struct spi_device *spi) · line 4142

Quoted 3 lines
drivers/spi/spi.c +3 -0
+err_cleanup:
+       if (spi->controller->cleanup)
+               spi->controller->cleanup(spi);

spi_cleanup()

I open-coded as I'm matching the call to ->setup() inside spi_setup() with ->cleanup() (i.e. not spi_setup() with spi_cleanup()).

Johan