[PATCH 0/8] interconnect: Prepare making platform remove callbacks return void

[PATCH 0/8] interconnect: Prepare making platform remove callbacks return void

[Uwe Kleine-König]

Hello,

today remove callbacks of platform devices return an int. This is unfortunate
because the device core ignores the return value and so the platform code only
emits a warning (and still removes the device).

My longterm quest is to make these remove callbacks return void instead.
This series is a preparation for that, with the goal to make the remove
callbacks obviously always return 0. This way when the prototype of
these functions is changed to return void, the change is straigt forward
and easy to review.

Best regards
Uwe

Uwe Kleine-König (8):
  interconnect: imx: Ignore return value of icc_provider_del() in
    .remove()
  interconnect: icc-rpm: Ignore return value of icc_provider_del() in
    .remove()
  interconnect: icc-rpmh: Ignore return value of icc_provider_del() in
    .remove()
  interconnect: msm8974: Ignore return value of icc_provider_del() in
    .remove()
  interconnect: osm-l3: Ignore return value of icc_provider_del() in
    .remove()
  interconnect: sm8450: Ignore return value of icc_provider_del() in
    .remove()
  interconnect: Make icc_provider_del() return void
  interconnect: imx: Make imx_icc_unregister() return void

 drivers/interconnect/core.c           | 10 +++-------
 drivers/interconnect/imx/imx.c        |  4 ++--
 drivers/interconnect/imx/imx.h        |  2 +-
 drivers/interconnect/imx/imx8mm.c     |  4 +++-
 drivers/interconnect/imx/imx8mn.c     |  4 +++-
 drivers/interconnect/imx/imx8mq.c     |  4 +++-
 drivers/interconnect/qcom/icc-rpm.c   |  4 +++-
 drivers/interconnect/qcom/icc-rpmh.c  |  4 +++-
 drivers/interconnect/qcom/msm8974.c   |  4 +++-
 drivers/interconnect/qcom/osm-l3.c    |  4 +++-
 drivers/interconnect/qcom/sm8450.c    |  4 +++-
 include/linux/interconnect-provider.h |  2 +-
 12 files changed, 31 insertions(+), 19 deletions(-)


base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.36.1

[PATCH 1/8] interconnect: imx: Ignore return value of icc_provider_del() in .remove()

[Uwe Kleine-König]

icc_provider_del() already emits an error message on failure. In this
case letting .remove() return the corresponding error code results in
another error message and the device is removed anyhow. (See
platform_remove().)

So ignore the return value of icc_provider_del() and return 0
unconditionally.

This is a preparation for making platform remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/interconnect/imx/imx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c
index 249ca25d1d55..4c70530e3064 100644
--- a/drivers/interconnect/imx/imx.c
+++ b/drivers/interconnect/imx/imx.c
@@ -280,7 +280,9 @@ int imx_icc_unregister(struct platform_device *pdev)
 
 	imx_icc_unregister_nodes(provider);
 
-	return icc_provider_del(provider);
+	icc_provider_del(provider);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(imx_icc_unregister);
 
-- 
2.36.1

[PATCH 2/8] interconnect: icc-rpm: Ignore return value of icc_provider_del() in .remove()

[Uwe Kleine-König]

icc_provider_del() already emits an error message on failure. In this
case letting .remove() return the corresponding error code results in
another error message and the device is removed anyhow. (See
platform_remove().)

So ignore the return value of icc_provider_del() and return 0
unconditionally.

This is a preparation for making platform remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/interconnect/qcom/icc-rpm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index fb013191c29b..189c25f8207a 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -447,6 +447,8 @@ int qnoc_remove(struct platform_device *pdev)
 
 	icc_nodes_remove(&qp->provider);
 	clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
-	return icc_provider_del(&qp->provider);
+	icc_provider_del(&qp->provider);
+
+	return 0;
 }
 EXPORT_SYMBOL(qnoc_remove);
-- 
2.36.1

[PATCH 3/8] interconnect: icc-rpmh: Ignore return value of icc_provider_del() in .remove()

[Uwe Kleine-König]

icc_provider_del() already emits an error message on failure. In this
case letting .remove() return the corresponding error code results in
another error message and the device is removed anyhow. (See
platform_remove().)

So ignore the return value of icc_provider_del() and return 0
unconditionally.

This is a preparation for making platform remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/interconnect/qcom/icc-rpmh.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
index 3c40076eb5fb..5846dafbe40e 100644
--- a/drivers/interconnect/qcom/icc-rpmh.c
+++ b/drivers/interconnect/qcom/icc-rpmh.c
@@ -271,7 +271,9 @@ int qcom_icc_rpmh_remove(struct platform_device *pdev)
 	struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
 
 	icc_nodes_remove(&qp->provider);
-	return icc_provider_del(&qp->provider);
+	icc_provider_del(&qp->provider);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(qcom_icc_rpmh_remove);
 
-- 
2.36.1

[PATCH 4/8] interconnect: msm8974: Ignore return value of icc_provider_del() in .remove()

[Uwe Kleine-König]

icc_provider_del() already emits an error message on failure. In this
case letting .remove() return the corresponding error code results in
another error message and the device is removed anyhow. (See
platform_remove().)

So ignore the return value of icc_provider_del() and return 0
unconditionally.

This is a preparation for making platform remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/interconnect/qcom/msm8974.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/qcom/msm8974.c b/drivers/interconnect/qcom/msm8974.c
index 6fa0ad90fc3d..5ea192f1141d 100644
--- a/drivers/interconnect/qcom/msm8974.c
+++ b/drivers/interconnect/qcom/msm8974.c
@@ -749,7 +749,9 @@ static int msm8974_icc_remove(struct platform_device *pdev)
 
 	icc_nodes_remove(&qp->provider);
 	clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
-	return icc_provider_del(&qp->provider);
+	icc_provider_del(&qp->provider);
+
+	return 0;
 }
 
 static const struct of_device_id msm8974_noc_of_match[] = {
-- 
2.36.1

[PATCH 5/8] interconnect: osm-l3: Ignore return value of icc_provider_del() in .remove()

[Uwe Kleine-König]

icc_provider_del() already emits an error message on failure. In this
case letting .remove() return the corresponding error code results in
another error message and the device is removed anyhow. (See
platform_remove().)

So ignore the return value of icc_provider_del() and return 0
unconditionally.

This is a preparation for making platform remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/interconnect/qcom/osm-l3.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index 4198656f4e59..ddbdf0943f94 100644
--- a/drivers/interconnect/qcom/osm-l3.c
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -217,7 +217,9 @@ static int qcom_osm_l3_remove(struct platform_device *pdev)
 	struct qcom_osm_l3_icc_provider *qp = platform_get_drvdata(pdev);
 
 	icc_nodes_remove(&qp->provider);
-	return icc_provider_del(&qp->provider);
+	icc_provider_del(&qp->provider);
+
+	return 0;
 }
 
 static int qcom_osm_l3_probe(struct platform_device *pdev)
-- 
2.36.1

[PATCH 6/8] interconnect: sm8450: Ignore return value of icc_provider_del() in .remove()

[Uwe Kleine-König]

icc_provider_del() already emits an error message on failure. In this
case letting .remove() return the corresponding error code results in
another error message and the device is removed anyhow. (See
platform_remove().)

So ignore the return value of icc_provider_del() and return 0
unconditionally.

This is a preparation for making platform remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/interconnect/qcom/sm8450.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/qcom/sm8450.c b/drivers/interconnect/qcom/sm8450.c
index 7e3d372b712f..613c30df8aed 100644
--- a/drivers/interconnect/qcom/sm8450.c
+++ b/drivers/interconnect/qcom/sm8450.c
@@ -1932,7 +1932,9 @@ static int qnoc_remove(struct platform_device *pdev)
 	struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
 
 	icc_nodes_remove(&qp->provider);
-	return icc_provider_del(&qp->provider);
+	icc_provider_del(&qp->provider);
+
+	return 0;
 }
 
 static const struct of_device_id qnoc_of_match[] = {
-- 
2.36.1

[PATCH 7/8] interconnect: Make icc_provider_del() return void

[Uwe Kleine-König]

All users ignore the return value of icc_provider_del(). Consequently
make it not return an error code.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/interconnect/core.c           | 10 +++-------
 include/linux/interconnect-provider.h |  2 +-
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 808f6e7a8048..25debded65a8 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1057,29 +1057,25 @@ EXPORT_SYMBOL_GPL(icc_provider_add);
 /**
  * icc_provider_del() - delete previously added interconnect provider
  * @provider: the interconnect provider that will be removed from topology
- *
- * Return: 0 on success, or an error code otherwise
  */
-int icc_provider_del(struct icc_provider *provider)
+void icc_provider_del(struct icc_provider *provider)
 {
 	mutex_lock(&icc_lock);
 	if (provider->users) {
 		pr_warn("interconnect provider still has %d users\n",
 			provider->users);
 		mutex_unlock(&icc_lock);
-		return -EBUSY;
+		return;
 	}
 
 	if (!list_empty(&provider->nodes)) {
 		pr_warn("interconnect provider still has nodes\n");
 		mutex_unlock(&icc_lock);
-		return -EBUSY;
+		return;
 	}
 
 	list_del(&provider->provider_list);
 	mutex_unlock(&icc_lock);
-
-	return 0;
 }
 EXPORT_SYMBOL_GPL(icc_provider_del);
 
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index 6bd01f7159c6..191f083d1f3b 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -123,7 +123,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider);
 void icc_node_del(struct icc_node *node);
 int icc_nodes_remove(struct icc_provider *provider);
 int icc_provider_add(struct icc_provider *provider);
-int icc_provider_del(struct icc_provider *provider);
+void icc_provider_del(struct icc_provider *provider);
 struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec);
 void icc_sync_state(struct device *dev);
 
-- 
2.36.1

Re: [PATCH 7/8] interconnect: Make icc_provider_del() return void

[Georgi Djakov]

Hi Uwe,

Thanks for the patchset!

On 15.07.22 23:36, Uwe Kleine-König wrote:
> All users ignore the return value of icc_provider_del(). Consequently
> make it not return an error code.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/interconnect/core.c           | 10 +++-------
>   include/linux/interconnect-provider.h |  2 +-
>   2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 808f6e7a8048..25debded65a8 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -1057,29 +1057,25 @@ EXPORT_SYMBOL_GPL(icc_provider_add);
>   /**
>    * icc_provider_del() - delete previously added interconnect provider
>    * @provider: the interconnect provider that will be removed from topology
> - *
> - * Return: 0 on success, or an error code otherwise
>    */
> -int icc_provider_del(struct icc_provider *provider)
> +void icc_provider_del(struct icc_provider *provider)
>   {
>   	mutex_lock(&icc_lock);
>   	if (provider->users) {
>   		pr_warn("interconnect provider still has %d users\n",
>   			provider->users);
>   		mutex_unlock(&icc_lock);
> -		return -EBUSY;
> +		return;
>   	}

Looks like provider->users is now useless, so we should remove it. But that could be a separate 
clean-up.

>   
>   	if (!list_empty(&provider->nodes)) {
>   		pr_warn("interconnect provider still has nodes\n");
>   		mutex_unlock(&icc_lock);
> -		return -EBUSY;
> +		return;
>   	}
>   
>   	list_del(&provider->provider_list);
>   	mutex_unlock(&icc_lock);
> -
> -	return 0;
>   }
>   EXPORT_SYMBOL_GPL(icc_provider_del);
>   
> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
> index 6bd01f7159c6..191f083d1f3b 100644
> --- a/include/linux/interconnect-provider.h
> +++ b/include/linux/interconnect-provider.h
> @@ -123,7 +123,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider);
>   void icc_node_del(struct icc_node *node);
>   int icc_nodes_remove(struct icc_provider *provider);
>   int icc_provider_add(struct icc_provider *provider);
> -int icc_provider_del(struct icc_provider *provider);
> +void icc_provider_del(struct icc_provider *provider);
>   struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec);
>   void icc_sync_state(struct device *dev);

We should also squash the following:

--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -172,7 +172,7 @@ static inline int icc_provider_add(struct icc_provider *provider)
  	return -ENOTSUPP;
  }

-static inline int icc_provider_del(struct icc_provider *provider)
+static inline void icc_provider_del(struct icc_provider *provider)
  {
  	return -ENOTSUPP;
  }

Thanks,
Georgi

Re: [PATCH 7/8] interconnect: Make icc_provider_del() return void

[Uwe Kleine-König]

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

On Mon, Jul 18, 2022 at 12:10:34PM +0300, Georgi Djakov wrote:
> 
> Hi Uwe,
> 
> Thanks for the patchset!
> 
> On 15.07.22 23:36, Uwe Kleine-König wrote:
> > All users ignore the return value of icc_provider_del(). Consequently
> > make it not return an error code.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >   drivers/interconnect/core.c           | 10 +++-------
> >   include/linux/interconnect-provider.h |  2 +-
> >   2 files changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> > index 808f6e7a8048..25debded65a8 100644
> > --- a/drivers/interconnect/core.c
> > +++ b/drivers/interconnect/core.c
> > @@ -1057,29 +1057,25 @@ EXPORT_SYMBOL_GPL(icc_provider_add);
> >   /**
> >    * icc_provider_del() - delete previously added interconnect provider
> >    * @provider: the interconnect provider that will be removed from topology
> > - *
> > - * Return: 0 on success, or an error code otherwise
> >    */
> > -int icc_provider_del(struct icc_provider *provider)
> > +void icc_provider_del(struct icc_provider *provider)
> >   {
> >   	mutex_lock(&icc_lock);
> >   	if (provider->users) {
> >   		pr_warn("interconnect provider still has %d users\n",
> >   			provider->users);
> >   		mutex_unlock(&icc_lock);
> > -		return -EBUSY;
> > +		return;
> >   	}
> 
> Looks like provider->users is now useless, so we should remove it. But that
> could be a separate clean-up.

Well, it's still used to emit the warning. If this can trigger there is
indeed a problem though. If there are still users, they should hold a
reference to the device preventing its release.

> >   	if (!list_empty(&provider->nodes)) {
> >   		pr_warn("interconnect provider still has nodes\n");
> >   		mutex_unlock(&icc_lock);
> > -		return -EBUSY;
> > +		return;
> >   	}
> >   	list_del(&provider->provider_list);
> >   	mutex_unlock(&icc_lock);
> > -
> > -	return 0;
> >   }
> >   EXPORT_SYMBOL_GPL(icc_provider_del);
> > diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
> > index 6bd01f7159c6..191f083d1f3b 100644
> > --- a/include/linux/interconnect-provider.h
> > +++ b/include/linux/interconnect-provider.h
> > @@ -123,7 +123,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider);
> >   void icc_node_del(struct icc_node *node);
> >   int icc_nodes_remove(struct icc_provider *provider);
> >   int icc_provider_add(struct icc_provider *provider);
> > -int icc_provider_del(struct icc_provider *provider);
> > +void icc_provider_del(struct icc_provider *provider);
> >   struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec);
> >   void icc_sync_state(struct device *dev);
> 
> We should also squash the following:
> 
> --- a/include/linux/interconnect-provider.h
> +++ b/include/linux/interconnect-provider.h
> @@ -172,7 +172,7 @@ static inline int icc_provider_add(struct icc_provider *provider)
>  	return -ENOTSUPP;
>  }
> 
> -static inline int icc_provider_del(struct icc_provider *provider)
> +static inline void icc_provider_del(struct icc_provider *provider)
>  {
>  	return -ENOTSUPP;
>  }

Sent a v2 with this change (and also removed the return statement).

Thanks
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 --]

[PATCH 8/8] interconnect: imx: Make imx_icc_unregister() return void

[Uwe Kleine-König]

The function imx_icc_unregister() returns zero unconditionally. Make it
return void.

This is a preparation for making platform remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/interconnect/imx/imx.c    | 4 +---
 drivers/interconnect/imx/imx.h    | 2 +-
 drivers/interconnect/imx/imx8mm.c | 4 +++-
 drivers/interconnect/imx/imx8mn.c | 4 +++-
 drivers/interconnect/imx/imx8mq.c | 4 +++-
 5 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c
index 4c70530e3064..e96794acad59 100644
--- a/drivers/interconnect/imx/imx.c
+++ b/drivers/interconnect/imx/imx.c
@@ -274,15 +274,13 @@ int imx_icc_register(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(imx_icc_register);
 
-int imx_icc_unregister(struct platform_device *pdev)
+void imx_icc_unregister(struct platform_device *pdev)
 {
 	struct icc_provider *provider = platform_get_drvdata(pdev);
 
 	imx_icc_unregister_nodes(provider);
 
 	icc_provider_del(provider);
-
-	return 0;
 }
 EXPORT_SYMBOL_GPL(imx_icc_unregister);
 
diff --git a/drivers/interconnect/imx/imx.h b/drivers/interconnect/imx/imx.h
index 75da51076c68..b533c9a99710 100644
--- a/drivers/interconnect/imx/imx.h
+++ b/drivers/interconnect/imx/imx.h
@@ -56,6 +56,6 @@ struct imx_icc_node_desc {
 int imx_icc_register(struct platform_device *pdev,
 		     struct imx_icc_node_desc *nodes,
 		     int nodes_count);
-int imx_icc_unregister(struct platform_device *pdev);
+void imx_icc_unregister(struct platform_device *pdev);
 
 #endif /* __DRIVERS_INTERCONNECT_IMX_H */
diff --git a/drivers/interconnect/imx/imx8mm.c b/drivers/interconnect/imx/imx8mm.c
index 1083490bb391..fa9639c6ea37 100644
--- a/drivers/interconnect/imx/imx8mm.c
+++ b/drivers/interconnect/imx/imx8mm.c
@@ -88,7 +88,9 @@ static int imx8mm_icc_probe(struct platform_device *pdev)
 
 static int imx8mm_icc_remove(struct platform_device *pdev)
 {
-	return imx_icc_unregister(pdev);
+	imx_icc_unregister(pdev);
+
+	return 0;
 }
 
 static struct platform_driver imx8mm_icc_driver = {
diff --git a/drivers/interconnect/imx/imx8mn.c b/drivers/interconnect/imx/imx8mn.c
index ad97e55fd4e5..3b11571c23d0 100644
--- a/drivers/interconnect/imx/imx8mn.c
+++ b/drivers/interconnect/imx/imx8mn.c
@@ -77,7 +77,9 @@ static int imx8mn_icc_probe(struct platform_device *pdev)
 
 static int imx8mn_icc_remove(struct platform_device *pdev)
 {
-	return imx_icc_unregister(pdev);
+	imx_icc_unregister(pdev);
+
+	return 0;
 }
 
 static struct platform_driver imx8mn_icc_driver = {
diff --git a/drivers/interconnect/imx/imx8mq.c b/drivers/interconnect/imx/imx8mq.c
index d7768d3c6d8a..fb19b90d6767 100644
--- a/drivers/interconnect/imx/imx8mq.c
+++ b/drivers/interconnect/imx/imx8mq.c
@@ -87,7 +87,9 @@ static int imx8mq_icc_probe(struct platform_device *pdev)
 
 static int imx8mq_icc_remove(struct platform_device *pdev)
 {
-	return imx_icc_unregister(pdev);
+	imx_icc_unregister(pdev);
+
+	return 0;
 }
 
 static struct platform_driver imx8mq_icc_driver = {
-- 
2.36.1