linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/9] pmdomain: Convert to platform remove callback returning void
@ 2023-11-24  8:06 Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 1/9] pmdomain: imx-pgc: " Uwe Kleine-König
                   ` (8 more replies)
  0 siblings, 9 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2023-11-24  8:06 UTC (permalink / raw)
  To: Ulf Hansson, Shawn Guo, Sascha Hauer, Linus Walleij,
	Arnd Bergmann, Pengfei Li, Rob Herring, Rafael J. Wysocki,
	Saravana Kannan, Marco Felsch, Peng Fan, Jindong Yue, Lucas Stach,
	Marek Vasut, Deepak R Varma, Bjorn Andersson, Konrad Dybcio,
	Andy Gross, Michal Simek, Heiko Stuebner
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, linux-pm,
	linux-arm-kernel, linux-arm-msm

Hello,

this patch set converts all drivers below drivers/pmdomain to use struct
platform_driver::remove_new(). See commit 5c5a7680e67b ("platform:
Provide a remove callback that returns no value") for an extended
explanation and the eventual goal.

While working on drivers/pmdomain/imx/gpc.c I noticed three issues, but
didn't address them:

 - The driver uses builtin_platform_driver twice. The documentation
   however mandates that "Each driver may only use this macro once".
   I don't know if the documentation is wrong and using it twice works
   as intended.

 - imx_gpc_remove() only removes two PDs, but there might be up to four?!

 - In imx_gpc_remove() if
   pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base) fails,
   removing the ARM PD is skipped. So together with the previous item
   the driver leaks up to three genpd instances.

Maybe someone caring for this driver will pick these up and prepare
patches? Ideally pm_genpd_remove() should return void caring for still
existing providers, parents and devices in generic code. I think that
erroring out in genpd_remove() before the PM domain is removed from the
various lists might result in use-after-free errors.

Best regards
Uwe

Uwe Kleine-König (9):
  pmdomain: imx-pgc: Convert to platform remove callback returning void
  pmdomain: imx-gpc: Convert to platform remove callback returning void
  pmdomain: imx-gpcv2: Convert to platform remove callback returning
    void
  pmdomain: imx8m-blk-ctrl: Convert to platform remove callback
    returning void
  pmdomain: imx8mp-blk-ctrl: Convert to platform remove callback
    returning void
  pmdomain: imx93-blk-ctrl: Convert to platform remove callback
    returning void
  pmdomain: imx93-pd: Convert to platform remove callback returning void
  pmdomain: qcom-cpr: Convert to platform remove callback returning void
  pmdomain: xilinx/zynqmp: Convert to platform remove callback returning
    void

 drivers/pmdomain/imx/gpc.c                  | 28 +++++++++++----------
 drivers/pmdomain/imx/gpcv2.c                |  6 ++---
 drivers/pmdomain/imx/imx8m-blk-ctrl.c       |  6 ++---
 drivers/pmdomain/imx/imx8mp-blk-ctrl.c      |  6 ++---
 drivers/pmdomain/imx/imx93-blk-ctrl.c       |  6 ++---
 drivers/pmdomain/imx/imx93-pd.c             |  6 ++---
 drivers/pmdomain/qcom/cpr.c                 |  6 ++---
 drivers/pmdomain/xilinx/zynqmp-pm-domains.c |  6 ++---
 8 files changed, 29 insertions(+), 41 deletions(-)

base-commit: 8c9660f6515396aba78d1168d2e17951d653ebf2
-- 
2.42.0


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/9] pmdomain: imx-pgc: Convert to platform remove callback returning void
  2023-11-24  8:06 [PATCH 0/9] pmdomain: Convert to platform remove callback returning void Uwe Kleine-König
@ 2023-11-24  8:06 ` Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 2/9] pmdomain: imx-gpc: " Uwe Kleine-König
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2023-11-24  8:06 UTC (permalink / raw)
  To: Ulf Hansson, Shawn Guo, Sascha Hauer, Linus Walleij,
	Arnd Bergmann, Pengfei Li, Rob Herring
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, linux-pm,
	linux-arm-kernel

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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/pmdomain/imx/gpc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pmdomain/imx/gpc.c b/drivers/pmdomain/imx/gpc.c
index 7d81e3171d39..d6cf3759570b 100644
--- a/drivers/pmdomain/imx/gpc.c
+++ b/drivers/pmdomain/imx/gpc.c
@@ -212,7 +212,7 @@ static int imx_pgc_power_domain_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int imx_pgc_power_domain_remove(struct platform_device *pdev)
+static void imx_pgc_power_domain_remove(struct platform_device *pdev)
 {
 	struct imx_pm_domain *domain = pdev->dev.platform_data;
 
@@ -221,8 +221,6 @@ static int imx_pgc_power_domain_remove(struct platform_device *pdev)
 		pm_genpd_remove(&domain->base);
 		imx_pgc_put_clocks(domain);
 	}
-
-	return 0;
 }
 
 static const struct platform_device_id imx_pgc_power_domain_id[] = {
@@ -235,7 +233,7 @@ static struct platform_driver imx_pgc_power_domain_driver = {
 		.name = "imx-pgc-pd",
 	},
 	.probe = imx_pgc_power_domain_probe,
-	.remove = imx_pgc_power_domain_remove,
+	.remove_new = imx_pgc_power_domain_remove,
 	.id_table = imx_pgc_power_domain_id,
 };
 builtin_platform_driver(imx_pgc_power_domain_driver)
-- 
2.42.0


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/9] pmdomain: imx-gpc: Convert to platform remove callback returning void
  2023-11-24  8:06 [PATCH 0/9] pmdomain: Convert to platform remove callback returning void Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 1/9] pmdomain: imx-pgc: " Uwe Kleine-König
@ 2023-11-24  8:06 ` Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 3/9] pmdomain: imx-gpcv2: " Uwe Kleine-König
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2023-11-24  8:06 UTC (permalink / raw)
  To: Ulf Hansson, Shawn Guo, Sascha Hauer, Linus Walleij,
	Rafael J. Wysocki, Pengfei Li, Rob Herring
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, linux-pm,
	linux-arm-kernel

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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

In the error path emit an error message replacing the (less useful)
message by the core. Apart from the improved error message there is no
change in behaviour.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pmdomain/imx/gpc.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/pmdomain/imx/gpc.c b/drivers/pmdomain/imx/gpc.c
index d6cf3759570b..9517cce93d8a 100644
--- a/drivers/pmdomain/imx/gpc.c
+++ b/drivers/pmdomain/imx/gpc.c
@@ -509,7 +509,7 @@ static int imx_gpc_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int imx_gpc_remove(struct platform_device *pdev)
+static void imx_gpc_remove(struct platform_device *pdev)
 {
 	struct device_node *pgc_node;
 	int ret;
@@ -519,7 +519,7 @@ static int imx_gpc_remove(struct platform_device *pdev)
 	/* bail out if DT too old and doesn't provide the necessary info */
 	if (!of_property_read_bool(pdev->dev.of_node, "#power-domain-cells") &&
 	    !pgc_node)
-		return 0;
+		return;
 
 	/*
 	 * If the old DT binding is used the toplevel driver needs to
@@ -529,16 +529,20 @@ static int imx_gpc_remove(struct platform_device *pdev)
 		of_genpd_del_provider(pdev->dev.of_node);
 
 		ret = pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base);
-		if (ret)
-			return ret;
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to remove PU power domain (%pe)\n",
+				ERR_PTR(ret));
+			return;
+		}
 		imx_pgc_put_clocks(&imx_gpc_domains[GPC_PGC_DOMAIN_PU]);
 
 		ret = pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_ARM].base);
-		if (ret)
-			return ret;
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to remove ARM power domain (%pe)\n",
+				ERR_PTR(ret));
+			return;
+		}
 	}
-
-	return 0;
 }
 
 static struct platform_driver imx_gpc_driver = {
@@ -547,6 +551,6 @@ static struct platform_driver imx_gpc_driver = {
 		.of_match_table = imx_gpc_dt_ids,
 	},
 	.probe = imx_gpc_probe,
-	.remove = imx_gpc_remove,
+	.remove_new = imx_gpc_remove,
 };
 builtin_platform_driver(imx_gpc_driver)
-- 
2.42.0


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/9] pmdomain: imx-gpcv2: Convert to platform remove callback returning void
  2023-11-24  8:06 [PATCH 0/9] pmdomain: Convert to platform remove callback returning void Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 1/9] pmdomain: imx-pgc: " Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 2/9] pmdomain: imx-gpc: " Uwe Kleine-König
@ 2023-11-24  8:06 ` Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 4/9] pmdomain: imx8m-blk-ctrl: " Uwe Kleine-König
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2023-11-24  8:06 UTC (permalink / raw)
  To: Ulf Hansson, Shawn Guo, Sascha Hauer, Rafael J. Wysocki,
	Saravana Kannan, Linus Walleij
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, linux-pm,
	linux-arm-kernel

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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/pmdomain/imx/gpcv2.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pmdomain/imx/gpcv2.c b/drivers/pmdomain/imx/gpcv2.c
index fbd3d92f8cd8..4b828d74a606 100644
--- a/drivers/pmdomain/imx/gpcv2.c
+++ b/drivers/pmdomain/imx/gpcv2.c
@@ -1373,7 +1373,7 @@ static int imx_pgc_domain_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int imx_pgc_domain_remove(struct platform_device *pdev)
+static void imx_pgc_domain_remove(struct platform_device *pdev)
 {
 	struct imx_pgc_domain *domain = pdev->dev.platform_data;
 
@@ -1385,8 +1385,6 @@ static int imx_pgc_domain_remove(struct platform_device *pdev)
 				   domain->bits.map, 0);
 
 	pm_runtime_disable(domain->dev);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1430,7 +1428,7 @@ static struct platform_driver imx_pgc_domain_driver = {
 		.pm = &imx_pgc_domain_pm_ops,
 	},
 	.probe    = imx_pgc_domain_probe,
-	.remove   = imx_pgc_domain_remove,
+	.remove_new = imx_pgc_domain_remove,
 	.id_table = imx_pgc_domain_id,
 };
 builtin_platform_driver(imx_pgc_domain_driver)
-- 
2.42.0


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/9] pmdomain: imx8m-blk-ctrl: Convert to platform remove callback returning void
  2023-11-24  8:06 [PATCH 0/9] pmdomain: Convert to platform remove callback returning void Uwe Kleine-König
                   ` (2 preceding siblings ...)
  2023-11-24  8:06 ` [PATCH 3/9] pmdomain: imx-gpcv2: " Uwe Kleine-König
@ 2023-11-24  8:06 ` Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 5/9] pmdomain: imx8mp-blk-ctrl: " Uwe Kleine-König
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2023-11-24  8:06 UTC (permalink / raw)
  To: Ulf Hansson, Shawn Guo, Sascha Hauer, Marco Felsch, Peng Fan,
	Jindong Yue, Lucas Stach, Marek Vasut
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, linux-pm,
	linux-arm-kernel

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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/pmdomain/imx/imx8m-blk-ctrl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
index cc5ef6e2f0a8..1341a707f61b 100644
--- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
+++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
@@ -330,7 +330,7 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int imx8m_blk_ctrl_remove(struct platform_device *pdev)
+static void imx8m_blk_ctrl_remove(struct platform_device *pdev)
 {
 	struct imx8m_blk_ctrl *bc = dev_get_drvdata(&pdev->dev);
 	int i;
@@ -347,8 +347,6 @@ static int imx8m_blk_ctrl_remove(struct platform_device *pdev)
 	dev_pm_genpd_remove_notifier(bc->bus_power_dev);
 
 	dev_pm_domain_detach(bc->bus_power_dev, true);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -888,7 +886,7 @@ MODULE_DEVICE_TABLE(of, imx8m_blk_ctrl_of_match);
 
 static struct platform_driver imx8m_blk_ctrl_driver = {
 	.probe = imx8m_blk_ctrl_probe,
-	.remove = imx8m_blk_ctrl_remove,
+	.remove_new = imx8m_blk_ctrl_remove,
 	.driver = {
 		.name = "imx8m-blk-ctrl",
 		.pm = &imx8m_blk_ctrl_pm_ops,
-- 
2.42.0


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 5/9] pmdomain: imx8mp-blk-ctrl: Convert to platform remove callback returning void
  2023-11-24  8:06 [PATCH 0/9] pmdomain: Convert to platform remove callback returning void Uwe Kleine-König
                   ` (3 preceding siblings ...)
  2023-11-24  8:06 ` [PATCH 4/9] pmdomain: imx8m-blk-ctrl: " Uwe Kleine-König
@ 2023-11-24  8:06 ` Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 6/9] pmdomain: imx93-blk-ctrl: " Uwe Kleine-König
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2023-11-24  8:06 UTC (permalink / raw)
  To: Ulf Hansson, Shawn Guo, Sascha Hauer, Lucas Stach, Peng Fan,
	Jindong Yue, Marco Felsch
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, linux-pm,
	linux-arm-kernel

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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/pmdomain/imx/imx8mp-blk-ctrl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pmdomain/imx/imx8mp-blk-ctrl.c b/drivers/pmdomain/imx/imx8mp-blk-ctrl.c
index c6ac32c1a8c1..e3203eb6a022 100644
--- a/drivers/pmdomain/imx/imx8mp-blk-ctrl.c
+++ b/drivers/pmdomain/imx/imx8mp-blk-ctrl.c
@@ -760,7 +760,7 @@ static int imx8mp_blk_ctrl_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int imx8mp_blk_ctrl_remove(struct platform_device *pdev)
+static void imx8mp_blk_ctrl_remove(struct platform_device *pdev)
 {
 	struct imx8mp_blk_ctrl *bc = dev_get_drvdata(&pdev->dev);
 	int i;
@@ -777,8 +777,6 @@ static int imx8mp_blk_ctrl_remove(struct platform_device *pdev)
 	dev_pm_genpd_remove_notifier(bc->bus_power_dev);
 
 	dev_pm_domain_detach(bc->bus_power_dev, true);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -856,7 +854,7 @@ MODULE_DEVICE_TABLE(of, imx8mp_blk_ctrl_of_match);
 
 static struct platform_driver imx8mp_blk_ctrl_driver = {
 	.probe = imx8mp_blk_ctrl_probe,
-	.remove = imx8mp_blk_ctrl_remove,
+	.remove_new = imx8mp_blk_ctrl_remove,
 	.driver = {
 		.name = "imx8mp-blk-ctrl",
 		.pm = &imx8mp_blk_ctrl_pm_ops,
-- 
2.42.0


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 6/9] pmdomain: imx93-blk-ctrl: Convert to platform remove callback returning void
  2023-11-24  8:06 [PATCH 0/9] pmdomain: Convert to platform remove callback returning void Uwe Kleine-König
                   ` (4 preceding siblings ...)
  2023-11-24  8:06 ` [PATCH 5/9] pmdomain: imx8mp-blk-ctrl: " Uwe Kleine-König
@ 2023-11-24  8:06 ` Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 7/9] pmdomain: imx93-pd: " Uwe Kleine-König
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2023-11-24  8:06 UTC (permalink / raw)
  To: Ulf Hansson, Shawn Guo, Sascha Hauer, Linus Walleij,
	Arnd Bergmann, Rafael J. Wysocki
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, linux-pm,
	linux-arm-kernel

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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/pmdomain/imx/imx93-blk-ctrl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pmdomain/imx/imx93-blk-ctrl.c b/drivers/pmdomain/imx/imx93-blk-ctrl.c
index 40bd90f8b977..904ffa55b8f4 100644
--- a/drivers/pmdomain/imx/imx93-blk-ctrl.c
+++ b/drivers/pmdomain/imx/imx93-blk-ctrl.c
@@ -306,7 +306,7 @@ static int imx93_blk_ctrl_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int imx93_blk_ctrl_remove(struct platform_device *pdev)
+static void imx93_blk_ctrl_remove(struct platform_device *pdev)
 {
 	struct imx93_blk_ctrl *bc = dev_get_drvdata(&pdev->dev);
 	int i;
@@ -318,8 +318,6 @@ static int imx93_blk_ctrl_remove(struct platform_device *pdev)
 
 		pm_genpd_remove(&domain->genpd);
 	}
-
-	return 0;
 }
 
 static const struct imx93_blk_ctrl_domain_data imx93_media_blk_ctl_domain_data[] = {
@@ -438,7 +436,7 @@ MODULE_DEVICE_TABLE(of, imx93_blk_ctrl_of_match);
 
 static struct platform_driver imx93_blk_ctrl_driver = {
 	.probe = imx93_blk_ctrl_probe,
-	.remove = imx93_blk_ctrl_remove,
+	.remove_new = imx93_blk_ctrl_remove,
 	.driver = {
 		.name = "imx93-blk-ctrl",
 		.of_match_table = imx93_blk_ctrl_of_match,
-- 
2.42.0


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 7/9] pmdomain: imx93-pd: Convert to platform remove callback returning void
  2023-11-24  8:06 [PATCH 0/9] pmdomain: Convert to platform remove callback returning void Uwe Kleine-König
                   ` (5 preceding siblings ...)
  2023-11-24  8:06 ` [PATCH 6/9] pmdomain: imx93-blk-ctrl: " Uwe Kleine-König
@ 2023-11-24  8:06 ` Uwe Kleine-König
  2023-11-24  8:06 ` [PATCH 9/9] pmdomain: xilinx/zynqmp: " Uwe Kleine-König
  2023-11-30 11:30 ` [PATCH 0/9] pmdomain: " Ulf Hansson
  8 siblings, 0 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2023-11-24  8:06 UTC (permalink / raw)
  To: Ulf Hansson, Shawn Guo, Sascha Hauer, Linus Walleij,
	Arnd Bergmann, Deepak R Varma
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, linux-pm,
	linux-arm-kernel

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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/pmdomain/imx/imx93-pd.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pmdomain/imx/imx93-pd.c b/drivers/pmdomain/imx/imx93-pd.c
index b9e60d136875..1e94b499c19b 100644
--- a/drivers/pmdomain/imx/imx93-pd.c
+++ b/drivers/pmdomain/imx/imx93-pd.c
@@ -83,7 +83,7 @@ static int imx93_pd_off(struct generic_pm_domain *genpd)
 	return 0;
 };
 
-static int imx93_pd_remove(struct platform_device *pdev)
+static void imx93_pd_remove(struct platform_device *pdev)
 {
 	struct imx93_power_domain *domain = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
@@ -94,8 +94,6 @@ static int imx93_pd_remove(struct platform_device *pdev)
 
 	of_genpd_del_provider(np);
 	pm_genpd_remove(&domain->genpd);
-
-	return 0;
 }
 
 static int imx93_pd_probe(struct platform_device *pdev)
@@ -167,7 +165,7 @@ static struct platform_driver imx93_power_domain_driver = {
 		.of_match_table = imx93_pd_ids,
 	},
 	.probe = imx93_pd_probe,
-	.remove = imx93_pd_remove,
+	.remove_new = imx93_pd_remove,
 };
 module_platform_driver(imx93_power_domain_driver);
 
-- 
2.42.0


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 9/9] pmdomain: xilinx/zynqmp: Convert to platform remove callback returning void
  2023-11-24  8:06 [PATCH 0/9] pmdomain: Convert to platform remove callback returning void Uwe Kleine-König
                   ` (6 preceding siblings ...)
  2023-11-24  8:06 ` [PATCH 7/9] pmdomain: imx93-pd: " Uwe Kleine-König
@ 2023-11-24  8:06 ` Uwe Kleine-König
  2023-11-24  8:19   ` Michal Simek
  2023-11-30 11:30 ` [PATCH 0/9] pmdomain: " Ulf Hansson
  8 siblings, 1 reply; 11+ messages in thread
From: Uwe Kleine-König @ 2023-11-24  8:06 UTC (permalink / raw)
  To: Ulf Hansson, Michal Simek, Linus Walleij, Heiko Stuebner,
	Arnd Bergmann, Rafael J. Wysocki
  Cc: linux-pm, linux-arm-kernel, Pengutronix Kernel Team

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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

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/pmdomain/xilinx/zynqmp-pm-domains.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pmdomain/xilinx/zynqmp-pm-domains.c b/drivers/pmdomain/xilinx/zynqmp-pm-domains.c
index 69d03ad4cf1e..6fd514286d82 100644
--- a/drivers/pmdomain/xilinx/zynqmp-pm-domains.c
+++ b/drivers/pmdomain/xilinx/zynqmp-pm-domains.c
@@ -293,11 +293,9 @@ static int zynqmp_gpd_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int zynqmp_gpd_remove(struct platform_device *pdev)
+static void zynqmp_gpd_remove(struct platform_device *pdev)
 {
 	of_genpd_del_provider(pdev->dev.parent->of_node);
-
-	return 0;
 }
 
 static void zynqmp_gpd_sync_state(struct device *dev)
@@ -315,7 +313,7 @@ static struct platform_driver zynqmp_power_domain_driver = {
 		.sync_state = zynqmp_gpd_sync_state,
 	},
 	.probe = zynqmp_gpd_probe,
-	.remove = zynqmp_gpd_remove,
+	.remove_new = zynqmp_gpd_remove,
 };
 module_platform_driver(zynqmp_power_domain_driver);
 
-- 
2.42.0


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 9/9] pmdomain: xilinx/zynqmp: Convert to platform remove callback returning void
  2023-11-24  8:06 ` [PATCH 9/9] pmdomain: xilinx/zynqmp: " Uwe Kleine-König
@ 2023-11-24  8:19   ` Michal Simek
  0 siblings, 0 replies; 11+ messages in thread
From: Michal Simek @ 2023-11-24  8:19 UTC (permalink / raw)
  To: Uwe Kleine-König, Ulf Hansson, Linus Walleij, Heiko Stuebner,
	Arnd Bergmann, Rafael J. Wysocki
  Cc: linux-pm, linux-arm-kernel, Pengutronix Kernel Team



On 11/24/23 09:06, Uwe Kleine-König wrote:
> 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 ignored (apart
> from emitting a warning) 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. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> 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/pmdomain/xilinx/zynqmp-pm-domains.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pmdomain/xilinx/zynqmp-pm-domains.c b/drivers/pmdomain/xilinx/zynqmp-pm-domains.c
> index 69d03ad4cf1e..6fd514286d82 100644
> --- a/drivers/pmdomain/xilinx/zynqmp-pm-domains.c
> +++ b/drivers/pmdomain/xilinx/zynqmp-pm-domains.c
> @@ -293,11 +293,9 @@ static int zynqmp_gpd_probe(struct platform_device *pdev)
>   	return 0;
>   }
>   
> -static int zynqmp_gpd_remove(struct platform_device *pdev)
> +static void zynqmp_gpd_remove(struct platform_device *pdev)
>   {
>   	of_genpd_del_provider(pdev->dev.parent->of_node);
> -
> -	return 0;
>   }
>   
>   static void zynqmp_gpd_sync_state(struct device *dev)
> @@ -315,7 +313,7 @@ static struct platform_driver zynqmp_power_domain_driver = {
>   		.sync_state = zynqmp_gpd_sync_state,
>   	},
>   	.probe = zynqmp_gpd_probe,
> -	.remove = zynqmp_gpd_remove,
> +	.remove_new = zynqmp_gpd_remove,
>   };
>   module_platform_driver(zynqmp_power_domain_driver);
>   

Acked-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/9] pmdomain: Convert to platform remove callback returning void
  2023-11-24  8:06 [PATCH 0/9] pmdomain: Convert to platform remove callback returning void Uwe Kleine-König
                   ` (7 preceding siblings ...)
  2023-11-24  8:06 ` [PATCH 9/9] pmdomain: xilinx/zynqmp: " Uwe Kleine-König
@ 2023-11-30 11:30 ` Ulf Hansson
  8 siblings, 0 replies; 11+ messages in thread
From: Ulf Hansson @ 2023-11-30 11:30 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Shawn Guo, Sascha Hauer, Linus Walleij, Arnd Bergmann, Pengfei Li,
	Rob Herring, Rafael J. Wysocki, Saravana Kannan, Marco Felsch,
	Peng Fan, Jindong Yue, Lucas Stach, Marek Vasut, Deepak R Varma,
	Bjorn Andersson, Konrad Dybcio, Andy Gross, Michal Simek,
	Heiko Stuebner, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-pm, linux-arm-kernel, linux-arm-msm

On Fri, 24 Nov 2023 at 09:10, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> this patch set converts all drivers below drivers/pmdomain to use struct
> platform_driver::remove_new(). See commit 5c5a7680e67b ("platform:
> Provide a remove callback that returns no value") for an extended
> explanation and the eventual goal.
>
> While working on drivers/pmdomain/imx/gpc.c I noticed three issues, but
> didn't address them:
>
>  - The driver uses builtin_platform_driver twice. The documentation
>    however mandates that "Each driver may only use this macro once".
>    I don't know if the documentation is wrong and using it twice works
>    as intended.
>
>  - imx_gpc_remove() only removes two PDs, but there might be up to four?!
>
>  - In imx_gpc_remove() if
>    pm_genpd_remove(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base) fails,
>    removing the ARM PD is skipped. So together with the previous item
>    the driver leaks up to three genpd instances.
>
> Maybe someone caring for this driver will pick these up and prepare
> patches? Ideally pm_genpd_remove() should return void caring for still
> existing providers, parents and devices in generic code. I think that
> erroring out in genpd_remove() before the PM domain is removed from the
> various lists might result in use-after-free errors.
>
> Best regards
> Uwe
>
> Uwe Kleine-König (9):
>   pmdomain: imx-pgc: Convert to platform remove callback returning void
>   pmdomain: imx-gpc: Convert to platform remove callback returning void
>   pmdomain: imx-gpcv2: Convert to platform remove callback returning
>     void
>   pmdomain: imx8m-blk-ctrl: Convert to platform remove callback
>     returning void
>   pmdomain: imx8mp-blk-ctrl: Convert to platform remove callback
>     returning void
>   pmdomain: imx93-blk-ctrl: Convert to platform remove callback
>     returning void
>   pmdomain: imx93-pd: Convert to platform remove callback returning void
>   pmdomain: qcom-cpr: Convert to platform remove callback returning void
>   pmdomain: xilinx/zynqmp: Convert to platform remove callback returning
>     void
>
>  drivers/pmdomain/imx/gpc.c                  | 28 +++++++++++----------
>  drivers/pmdomain/imx/gpcv2.c                |  6 ++---
>  drivers/pmdomain/imx/imx8m-blk-ctrl.c       |  6 ++---
>  drivers/pmdomain/imx/imx8mp-blk-ctrl.c      |  6 ++---
>  drivers/pmdomain/imx/imx93-blk-ctrl.c       |  6 ++---
>  drivers/pmdomain/imx/imx93-pd.c             |  6 ++---
>  drivers/pmdomain/qcom/cpr.c                 |  6 ++---
>  drivers/pmdomain/xilinx/zynqmp-pm-domains.c |  6 ++---
>  8 files changed, 29 insertions(+), 41 deletions(-)
>

The series applied for next, thanks!

Kind regards
Uffe

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-11-30 11:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-24  8:06 [PATCH 0/9] pmdomain: Convert to platform remove callback returning void Uwe Kleine-König
2023-11-24  8:06 ` [PATCH 1/9] pmdomain: imx-pgc: " Uwe Kleine-König
2023-11-24  8:06 ` [PATCH 2/9] pmdomain: imx-gpc: " Uwe Kleine-König
2023-11-24  8:06 ` [PATCH 3/9] pmdomain: imx-gpcv2: " Uwe Kleine-König
2023-11-24  8:06 ` [PATCH 4/9] pmdomain: imx8m-blk-ctrl: " Uwe Kleine-König
2023-11-24  8:06 ` [PATCH 5/9] pmdomain: imx8mp-blk-ctrl: " Uwe Kleine-König
2023-11-24  8:06 ` [PATCH 6/9] pmdomain: imx93-blk-ctrl: " Uwe Kleine-König
2023-11-24  8:06 ` [PATCH 7/9] pmdomain: imx93-pd: " Uwe Kleine-König
2023-11-24  8:06 ` [PATCH 9/9] pmdomain: xilinx/zynqmp: " Uwe Kleine-König
2023-11-24  8:19   ` Michal Simek
2023-11-30 11:30 ` [PATCH 0/9] pmdomain: " Ulf Hansson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).