[PATCH 0/2] spi: sprd: Convert to platform remove callback returning void

[PATCH 0/2] spi: sprd: Convert to platform remove callback returning void

[Uwe Kleine-König]

From: Uwe Kleine-König <uwe@kleine-koenig.org>

Hello,

An early error return from a remove callback is usally wrong. In the
case of the spi-sprd driver it's not that critical because the skipped
steps are mainly undoing the things that a successful runtime-resume
would have done.

Still it's cleaner to not exit early and not return an (mostly ignored)
error value. The second patch converts to .remove_new (which is the
motivation for this series).

Best regards
Uwe

Uwe Kleine-König (2):
  spi: sprd: Don't skip cleanup in remove's error path
  spi: sprd: Convert to platform remove callback returning void

 drivers/spi/spi-sprd.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
-- 
2.39.1

[PATCH 1/2] spi: sprd: Don't skip cleanup in remove's error path

[Uwe Kleine-König]

If pm_runtime_resume_and_get() failed before this change, two error
messages were printed. One directly in sprd_spi_remove() and another
by the device core as the return value is non-zero.

The better handling of a failure to resume the device is to do the
software related cleanup anyhow and only skip hardware accesses.
This leaves the device in an unknown state, but there is nothing that can
be done about that.

Even in the error case, return zero to suppress the device core's error
message.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-sprd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c
index 65b8075da4eb..45b69b83a7e4 100644
--- a/drivers/spi/spi-sprd.c
+++ b/drivers/spi/spi-sprd.c
@@ -1008,17 +1008,17 @@ static int sprd_spi_remove(struct platform_device *pdev)
 	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
 	int ret;
 
-	ret = pm_runtime_resume_and_get(ss->dev);
-	if (ret < 0) {
+	ret = pm_runtime_get_sync(ss->dev);
+	if (ret < 0)
 		dev_err(ss->dev, "failed to resume SPI controller\n");
-		return ret;
-	}
 
 	spi_controller_suspend(sctlr);
 
-	if (ss->dma.enable)
-		sprd_spi_dma_release(ss);
-	clk_disable_unprepare(ss->clk);
+	if (ret >= 0) {
+		if (ss->dma.enable)
+			sprd_spi_dma_release(ss);
+		clk_disable_unprepare(ss->clk);
+	}
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
-- 
2.39.1

[PATCH 2/2] spi: sprd: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is (mostly) ignored
and this typically results in resource leaks. To improve here there is a
quest to make the remove callback return void. In the first step of this
quest all drivers are converted to .remove_new() which already returns
void.

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-sprd.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c
index 45b69b83a7e4..702acaeebab2 100644
--- a/drivers/spi/spi-sprd.c
+++ b/drivers/spi/spi-sprd.c
@@ -1002,7 +1002,7 @@ static int sprd_spi_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int sprd_spi_remove(struct platform_device *pdev)
+static void sprd_spi_remove(struct platform_device *pdev)
 {
 	struct spi_controller *sctlr = platform_get_drvdata(pdev);
 	struct sprd_spi *ss = spi_controller_get_devdata(sctlr);
@@ -1021,8 +1021,6 @@ static int sprd_spi_remove(struct platform_device *pdev)
 	}
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-
-	return 0;
 }
 
 static int __maybe_unused sprd_spi_runtime_suspend(struct device *dev)
@@ -1076,7 +1074,7 @@ static struct platform_driver sprd_spi_driver = {
 		.pm = &sprd_spi_pm_ops,
 	},
 	.probe = sprd_spi_probe,
-	.remove  = sprd_spi_remove,
+	.remove_new = sprd_spi_remove,
 };
 
 module_platform_driver(sprd_spi_driver);
-- 
2.39.1

Re: [PATCH 0/2] spi: sprd: Convert to platform remove callback returning void

[Mark Brown]

On Tue, 07 Mar 2023 22:14:24 +0100, Uwe Kleine-König wrote:
> An early error return from a remove callback is usally wrong. In the
> case of the spi-sprd driver it's not that critical because the skipped
> steps are mainly undoing the things that a successful runtime-resume
> would have done.
> 
> Still it's cleaner to not exit early and not return an (mostly ignored)
> error value. The second patch converts to .remove_new (which is the
> motivation for this series).
> 
> [...]

Applied to

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

Thanks!

[1/2] spi: sprd: Don't skip cleanup in remove's error path
      commit: 5cb79889a0bab6832662ec5b8f7d1f0e6e7c25ed
[2/2] spi: sprd: Convert to platform remove callback returning void
      commit: 3b74dc8acd5c2e59d4a1988a87d64b08fba56d5f

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