diff mbox series

powercap: intel_rapl: Convert to platform remove callback returning void

Message ID 98afe4627bcfbddedfa36fb5631bb47913f5fa94.1709886922.git.u.kleine-koenig@pengutronix.de (mailing list archive)
State Mainlined, archived
Headers show
Series powercap: intel_rapl: Convert to platform remove callback returning void | expand

Commit Message

Uwe Kleine-König March 8, 2024, 8:51 a.m. UTC 
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/powercap/intel_rapl_msr.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)


base-commit: 8ffc8b1bbd505e27e2c8439d326b6059c906c9dd

Comments

Zhang Rui March 10, 2024, 1:16 p.m. UTC | #1 
On Fri, 2024-03-08 at 09:51 +0100, 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/powercap/intel_rapl_msr.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/powercap/intel_rapl_msr.c
> b/drivers/powercap/intel_rapl_msr.c
> index b4b6930cacb0..35cb152fa9aa 100644
> --- a/drivers/powercap/intel_rapl_msr.c
> +++ b/drivers/powercap/intel_rapl_msr.c
> @@ -197,11 +197,10 @@ static int rapl_msr_probe(struct
> platform_device *pdev)
>         return ret;
>  }
>  
> -static int rapl_msr_remove(struct platform_device *pdev)
> +static void rapl_msr_remove(struct platform_device *pdev)
>  {
>         cpuhp_remove_state(rapl_msr_priv->pcap_rapl_online);
>         powercap_unregister_control_type(rapl_msr_priv-
> >control_type);
> -       return 0;
>  }
>  
>  static const struct platform_device_id rapl_msr_ids[] = {
> @@ -212,7 +211,7 @@ MODULE_DEVICE_TABLE(platform, rapl_msr_ids);
>  
>  static struct platform_driver intel_rapl_msr_driver = {
>         .probe = rapl_msr_probe,
> -       .remove = rapl_msr_remove,
> +       .remove_new = rapl_msr_remove,
>         .id_table = rapl_msr_ids,
>         .driver = {
>                 .name = "intel_rapl_msr",
> 
> base-commit: 8ffc8b1bbd505e27e2c8439d326b6059c906c9dd

This "base-commit" is not an upstream commit, which makes it more
confusing.

Seems that the base support is added by commit 5c5a7680e67b ("platform:
Provide a remove callback that returns no value"). IMO, it would be
better to refer that in the changelog. But anyway, the patch LGTM.

Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui
Uwe Kleine-König March 10, 2024, 1:59 p.m. UTC | #2 
Hello,

On Sun, Mar 10, 2024 at 01:16:03PM +0000, Zhang, Rui wrote:
> On Fri, 2024-03-08 at 09:51 +0100, Uwe Kleine-König wrote:
> > base-commit: 8ffc8b1bbd505e27e2c8439d326b6059c906c9dd
> 
> This "base-commit" is not an upstream commit, which makes it more
> confusing.

FTR, 8ffc8b1bbd505e27e2c8439d326b6059c906c9dd = next-20240308.
You're the first to claim having troubles with using next as base in my
career so far.

> Seems that the base support is added by commit 5c5a7680e67b ("platform:
> Provide a remove callback that returns no value"). IMO, it would be
> better to refer that in the changelog. But anyway, the patch LGTM.

Yeah, ok. I normally mention this commit in the cover letter if there
are several drivers to convert in a subsystem.

Given that 5c5a7680e67b is contained in v6.3-rc1 I'd expect that there
is no need to explicitly point it out as a dependency.

> Reviewed-by: Zhang Rui <rui.zhang@intel.com>

Thanks

Best regards,
Uwe
Rafael J. Wysocki March 13, 2024, 7:47 p.m. UTC | #3 
On Sun, Mar 10, 2024 at 2:59 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Sun, Mar 10, 2024 at 01:16:03PM +0000, Zhang, Rui wrote:
> > On Fri, 2024-03-08 at 09:51 +0100, Uwe Kleine-König wrote:
> > > base-commit: 8ffc8b1bbd505e27e2c8439d326b6059c906c9dd
> >
> > This "base-commit" is not an upstream commit, which makes it more
> > confusing.
>
> FTR, 8ffc8b1bbd505e27e2c8439d326b6059c906c9dd = next-20240308.
> You're the first to claim having troubles with using next as base in my
> career so far.
>
> > Seems that the base support is added by commit 5c5a7680e67b ("platform:
> > Provide a remove callback that returns no value"). IMO, it would be
> > better to refer that in the changelog. But anyway, the patch LGTM.
>
> Yeah, ok. I normally mention this commit in the cover letter if there
> are several drivers to convert in a subsystem.
>
> Given that 5c5a7680e67b is contained in v6.3-rc1 I'd expect that there
> is no need to explicitly point it out as a dependency.
>
> > Reviewed-by: Zhang Rui <rui.zhang@intel.com>

Applied as 6.9-rc material, thanks!
diff mbox series

Patch

diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c
index b4b6930cacb0..35cb152fa9aa 100644
--- a/drivers/powercap/intel_rapl_msr.c
+++ b/drivers/powercap/intel_rapl_msr.c
@@ -197,11 +197,10 @@  static int rapl_msr_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int rapl_msr_remove(struct platform_device *pdev)
+static void rapl_msr_remove(struct platform_device *pdev)
 {
 	cpuhp_remove_state(rapl_msr_priv->pcap_rapl_online);
 	powercap_unregister_control_type(rapl_msr_priv->control_type);
-	return 0;
 }
 
 static const struct platform_device_id rapl_msr_ids[] = {
@@ -212,7 +211,7 @@  MODULE_DEVICE_TABLE(platform, rapl_msr_ids);
 
 static struct platform_driver intel_rapl_msr_driver = {
 	.probe = rapl_msr_probe,
-	.remove = rapl_msr_remove,
+	.remove_new = rapl_msr_remove,
 	.id_table = rapl_msr_ids,
 	.driver = {
 		.name = "intel_rapl_msr",