[PATCH v2 0/3] spi: mt65xx: Convert to platform remove callback returning void

[PATCH v2 0/3] spi: mt65xx: Convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

compared to (implicit) v1 sent in March with Message-Id:
<20230309094704.2568531-1-u.kleine-koenig@pengutronix.de>, I reworked
patch 1 on feedback by AngeloGioacchino Del Regno. Patches 2 and 3 got
his Reviewed-by.

Best regards
Uwe

Uwe Kleine-König (3):
  spi: mt65xx: Properly handle failures in .remove()
  spi: mt65xx: Convert to platform remove callback returning void
  spi: mt65xx: Don't disguise a "return 0" as "return ret"

 drivers/spi/spi-mt65xx.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

base-commit: ac9a78681b921877518763ba0e89202254349d1b
-- 
2.39.2

[PATCH v2 0/3] spi: mt65xx: Convert to platform remove callback returning void

[Uwe Kleine-König]

Hello,

compared to (implicit) v1 sent in March with Message-Id:
<20230309094704.2568531-1-u.kleine-koenig@pengutronix.de>, I reworked
patch 1 on feedback by AngeloGioacchino Del Regno. Patches 2 and 3 got
his Reviewed-by.

Best regards
Uwe

Uwe Kleine-König (3):
  spi: mt65xx: Properly handle failures in .remove()
  spi: mt65xx: Convert to platform remove callback returning void
  spi: mt65xx: Don't disguise a "return 0" as "return ret"

 drivers/spi/spi-mt65xx.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

base-commit: ac9a78681b921877518763ba0e89202254349d1b
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/3] spi: mt65xx: Properly handle failures in .remove()

[Uwe Kleine-König]

Returning an error code in a platform driver's remove function is wrong
most of the time and there is an effort to make the callback return
void. To prepare this rework the function not to exit early.

There wasn't a real problem because if pm runtime resume failed the only
step missing was pm_runtime_disable() which isn't an issue.

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

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 21c321f43766..9333a0e8204d 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -1275,15 +1275,21 @@ static int mtk_spi_remove(struct platform_device *pdev)
 	struct mtk_spi *mdata = spi_master_get_devdata(master);
 	int ret;
 
-	ret = pm_runtime_resume_and_get(&pdev->dev);
-	if (ret < 0)
-		return ret;
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		dev_warn(&pdev->dev, "Failed to resume hardware (%pe)\n", ERR_PTR(ret));
+	} else {
+		/*
+		 * If pm runtime resume failed, clks are disabled and
+		 * unprepared. So don't access the hardware and skip clk
+		 * unpreparing.
+		 */
+		mtk_spi_reset(mdata);
 
-	mtk_spi_reset(mdata);
-
-	if (mdata->dev_comp->no_need_unprepare) {
-		clk_unprepare(mdata->spi_clk);
-		clk_unprepare(mdata->spi_hclk);
+		if (mdata->dev_comp->no_need_unprepare) {
+			clk_unprepare(mdata->spi_clk);
+			clk_unprepare(mdata->spi_hclk);
+		}
 	}
 
 	pm_runtime_put_noidle(&pdev->dev);
-- 
2.39.2

[PATCH v2 1/3] spi: mt65xx: Properly handle failures in .remove()

[Uwe Kleine-König]

Returning an error code in a platform driver's remove function is wrong
most of the time and there is an effort to make the callback return
void. To prepare this rework the function not to exit early.

There wasn't a real problem because if pm runtime resume failed the only
step missing was pm_runtime_disable() which isn't an issue.

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

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 21c321f43766..9333a0e8204d 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -1275,15 +1275,21 @@ static int mtk_spi_remove(struct platform_device *pdev)
 	struct mtk_spi *mdata = spi_master_get_devdata(master);
 	int ret;
 
-	ret = pm_runtime_resume_and_get(&pdev->dev);
-	if (ret < 0)
-		return ret;
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		dev_warn(&pdev->dev, "Failed to resume hardware (%pe)\n", ERR_PTR(ret));
+	} else {
+		/*
+		 * If pm runtime resume failed, clks are disabled and
+		 * unprepared. So don't access the hardware and skip clk
+		 * unpreparing.
+		 */
+		mtk_spi_reset(mdata);
 
-	mtk_spi_reset(mdata);
-
-	if (mdata->dev_comp->no_need_unprepare) {
-		clk_unprepare(mdata->spi_clk);
-		clk_unprepare(mdata->spi_hclk);
+		if (mdata->dev_comp->no_need_unprepare) {
+			clk_unprepare(mdata->spi_clk);
+			clk_unprepare(mdata->spi_hclk);
+		}
 	}
 
 	pm_runtime_put_noidle(&pdev->dev);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: mt65xx: Properly handle failures in .remove()

[AngeloGioacchino Del Regno]

Il 30/05/23 10:16, Uwe Kleine-König ha scritto:
> Returning an error code in a platform driver's remove function is wrong
> most of the time and there is an effort to make the callback return
> void. To prepare this rework the function not to exit early.
> 
> There wasn't a real problem because if pm runtime resume failed the only
> step missing was pm_runtime_disable() which isn't an issue.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Cheers!

Re: [PATCH v2 1/3] spi: mt65xx: Properly handle failures in .remove()

[AngeloGioacchino Del Regno]

Il 30/05/23 10:16, Uwe Kleine-König ha scritto:
> Returning an error code in a platform driver's remove function is wrong
> most of the time and there is an effort to make the callback return
> void. To prepare this rework the function not to exit early.
> 
> There wasn't a real problem because if pm runtime resume failed the only
> step missing was pm_runtime_disable() which isn't an issue.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Cheers!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] spi: mt65xx: Properly handle failures in .remove()

[Uwe Kleine-König]

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

Hello,

On Tue, May 30, 2023 at 10:16:46AM +0200, Uwe Kleine-König wrote:
> Returning an error code in a platform driver's remove function is wrong
> most of the time and there is an effort to make the callback return
> void. To prepare this rework the function not to exit early.
> 
> There wasn't a real problem because if pm runtime resume failed the only
> step missing was pm_runtime_disable() which isn't an issue.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I just noticed there is a patch in next that conflicts with that one. My
merge resolution looks as follows:


diff --cc drivers/spi/spi-mt65xx.c
index 17162c8661b4,9333a0e8204d..000000000000
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@@ -1276,18 -1275,21 +1276,24 @@@ static int mtk_spi_remove(struct platfo
  	struct mtk_spi *mdata = spi_master_get_devdata(master);
  	int ret;
  
 +	if (mdata->use_spimem && !completion_done(&mdata->spimem_done))
 +		complete(&mdata->spimem_done);
 +
- 	ret = pm_runtime_resume_and_get(&pdev->dev);
- 	if (ret < 0)
- 		return ret;
+ 	ret = pm_runtime_get_sync(&pdev->dev);
+ 	if (ret < 0) {
+ 		dev_warn(&pdev->dev, "Failed to resume hardware (%pe)\n", ERR_PTR(ret));
+ 	} else {
+ 		/*
+ 		 * If pm runtime resume failed, clks are disabled and
+ 		 * unprepared. So don't access the hardware and skip clk
+ 		 * unpreparing.
+ 		 */
+ 		mtk_spi_reset(mdata);
  
- 	mtk_spi_reset(mdata);
- 
- 	if (mdata->dev_comp->no_need_unprepare) {
- 		clk_unprepare(mdata->spi_clk);
- 		clk_unprepare(mdata->spi_hclk);
+ 		if (mdata->dev_comp->no_need_unprepare) {
+ 			clk_unprepare(mdata->spi_clk);
+ 			clk_unprepare(mdata->spi_hclk);
+ 		}
  	}
  
  	pm_runtime_put_noidle(&pdev->dev);

If it's too complicated to apply, I can resend a rebased version.
After that the other two patches apply just fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 1/3] spi: mt65xx: Properly handle failures in .remove()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2079 bytes --]

Hello,

On Tue, May 30, 2023 at 10:16:46AM +0200, Uwe Kleine-König wrote:
> Returning an error code in a platform driver's remove function is wrong
> most of the time and there is an effort to make the callback return
> void. To prepare this rework the function not to exit early.
> 
> There wasn't a real problem because if pm runtime resume failed the only
> step missing was pm_runtime_disable() which isn't an issue.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I just noticed there is a patch in next that conflicts with that one. My
merge resolution looks as follows:


diff --cc drivers/spi/spi-mt65xx.c
index 17162c8661b4,9333a0e8204d..000000000000
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@@ -1276,18 -1275,21 +1276,24 @@@ static int mtk_spi_remove(struct platfo
  	struct mtk_spi *mdata = spi_master_get_devdata(master);
  	int ret;
  
 +	if (mdata->use_spimem && !completion_done(&mdata->spimem_done))
 +		complete(&mdata->spimem_done);
 +
- 	ret = pm_runtime_resume_and_get(&pdev->dev);
- 	if (ret < 0)
- 		return ret;
+ 	ret = pm_runtime_get_sync(&pdev->dev);
+ 	if (ret < 0) {
+ 		dev_warn(&pdev->dev, "Failed to resume hardware (%pe)\n", ERR_PTR(ret));
+ 	} else {
+ 		/*
+ 		 * If pm runtime resume failed, clks are disabled and
+ 		 * unprepared. So don't access the hardware and skip clk
+ 		 * unpreparing.
+ 		 */
+ 		mtk_spi_reset(mdata);
  
- 	mtk_spi_reset(mdata);
- 
- 	if (mdata->dev_comp->no_need_unprepare) {
- 		clk_unprepare(mdata->spi_clk);
- 		clk_unprepare(mdata->spi_hclk);
+ 		if (mdata->dev_comp->no_need_unprepare) {
+ 			clk_unprepare(mdata->spi_clk);
+ 			clk_unprepare(mdata->spi_hclk);
+ 		}
  	}
  
  	pm_runtime_put_noidle(&pdev->dev);

If it's too complicated to apply, I can resend a rebased version.
After that the other two patches apply just fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/3] spi: mt65xx: 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.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-mt65xx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 9333a0e8204d..f532cee3461e 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -1269,7 +1269,7 @@ static int mtk_spi_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int mtk_spi_remove(struct platform_device *pdev)
+static void mtk_spi_remove(struct platform_device *pdev)
 {
 	struct spi_master *master = platform_get_drvdata(pdev);
 	struct mtk_spi *mdata = spi_master_get_devdata(master);
@@ -1294,8 +1294,6 @@ static int mtk_spi_remove(struct platform_device *pdev)
 
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1415,7 +1413,7 @@ static struct platform_driver mtk_spi_driver = {
 		.of_match_table = mtk_spi_of_match,
 	},
 	.probe = mtk_spi_probe,
-	.remove = mtk_spi_remove,
+	.remove_new = mtk_spi_remove,
 };
 
 module_platform_driver(mtk_spi_driver);
-- 
2.39.2

[PATCH v2 2/3] spi: mt65xx: 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.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-mt65xx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 9333a0e8204d..f532cee3461e 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -1269,7 +1269,7 @@ static int mtk_spi_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int mtk_spi_remove(struct platform_device *pdev)
+static void mtk_spi_remove(struct platform_device *pdev)
 {
 	struct spi_master *master = platform_get_drvdata(pdev);
 	struct mtk_spi *mdata = spi_master_get_devdata(master);
@@ -1294,8 +1294,6 @@ static int mtk_spi_remove(struct platform_device *pdev)
 
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1415,7 +1413,7 @@ static struct platform_driver mtk_spi_driver = {
 		.of_match_table = mtk_spi_of_match,
 	},
 	.probe = mtk_spi_probe,
-	.remove = mtk_spi_remove,
+	.remove_new = mtk_spi_remove,
 };
 
 module_platform_driver(mtk_spi_driver);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/3] spi: mt65xx: Don't disguise a "return 0" as "return ret"

[Uwe Kleine-König]

Because of the earlier

	 if (ret)
		return ret;

ret is always zero at the end of mtk_spi_suspend(). Write it as explicit
return 0 for slightly improved clearness.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-mt65xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index f532cee3461e..5a0b04c1c755 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -1312,7 +1312,7 @@ static int mtk_spi_suspend(struct device *dev)
 		clk_disable_unprepare(mdata->spi_hclk);
 	}
 
-	return ret;
+	return 0;
 }
 
 static int mtk_spi_resume(struct device *dev)
-- 
2.39.2

[PATCH v2 3/3] spi: mt65xx: Don't disguise a "return 0" as "return ret"

[Uwe Kleine-König]

Because of the earlier

	 if (ret)
		return ret;

ret is always zero at the end of mtk_spi_suspend(). Write it as explicit
return 0 for slightly improved clearness.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-mt65xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index f532cee3461e..5a0b04c1c755 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -1312,7 +1312,7 @@ static int mtk_spi_suspend(struct device *dev)
 		clk_disable_unprepare(mdata->spi_hclk);
 	}
 
-	return ret;
+	return 0;
 }
 
 static int mtk_spi_resume(struct device *dev)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/3] spi: mt65xx: Convert to platform remove callback returning void

[Mark Brown]

On Tue, 30 May 2023 10:16:45 +0200, Uwe Kleine-König wrote:
> compared to (implicit) v1 sent in March with Message-Id:
> <20230309094704.2568531-1-u.kleine-koenig@pengutronix.de>, I reworked
> patch 1 on feedback by AngeloGioacchino Del Regno. Patches 2 and 3 got
> his Reviewed-by.
> 
> Best regards
> Uwe
> 
> [...]

Applied to

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

Thanks!

[1/3] spi: mt65xx: Properly handle failures in .remove()
      commit: 22f407278ea43df46f90cece6595e5e8a0d5447c
[2/3] spi: mt65xx: Convert to platform remove callback returning void
      commit: df7e47196fcef5d5611caa65f91d813578cf3efd
[3/3] spi: mt65xx: Don't disguise a "return 0" as "return ret"
      commit: 6f089e986778d3657247fdc2b38bd38de796732b

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

Re: [PATCH v2 0/3] spi: mt65xx: Convert to platform remove callback returning void

[Mark Brown]

On Tue, 30 May 2023 10:16:45 +0200, Uwe Kleine-König wrote:
> compared to (implicit) v1 sent in March with Message-Id:
> <20230309094704.2568531-1-u.kleine-koenig@pengutronix.de>, I reworked
> patch 1 on feedback by AngeloGioacchino Del Regno. Patches 2 and 3 got
> his Reviewed-by.
> 
> Best regards
> Uwe
> 
> [...]

Applied to

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

Thanks!

[1/3] spi: mt65xx: Properly handle failures in .remove()
      commit: 22f407278ea43df46f90cece6595e5e8a0d5447c
[2/3] spi: mt65xx: Convert to platform remove callback returning void
      commit: df7e47196fcef5d5611caa65f91d813578cf3efd
[3/3] spi: mt65xx: Don't disguise a "return 0" as "return ret"
      commit: 6f089e986778d3657247fdc2b38bd38de796732b

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel