[PATCH] pstore/ram: Convert to platform remove callback returning void

[PATCH] pstore/ram: 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>
---
 fs/pstore/ram.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ade66dbe5f39..2f625e1fa8d8 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -875,7 +875,7 @@ static int ramoops_probe(struct platform_device *pdev)
 	return err;
 }
 
-static int ramoops_remove(struct platform_device *pdev)
+static void ramoops_remove(struct platform_device *pdev)
 {
 	struct ramoops_context *cxt = &oops_cxt;
 
@@ -885,8 +885,6 @@ static int ramoops_remove(struct platform_device *pdev)
 	cxt->pstore.bufsize = 0;
 
 	ramoops_free_przs(cxt);
-
-	return 0;
 }
 
 static const struct of_device_id dt_match[] = {
@@ -896,7 +894,7 @@ static const struct of_device_id dt_match[] = {
 
 static struct platform_driver ramoops_driver = {
 	.probe		= ramoops_probe,
-	.remove		= ramoops_remove,
+	.remove_new	= ramoops_remove,
 	.driver		= {
 		.name		= "ramoops",
 		.of_match_table	= dt_match,

base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
-- 
2.39.2

Re: [PATCH] pstore/ram: Convert to platform remove callback returning void

[Guilherme G. Piccoli]

On 01/04/2023 09:00, 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 (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>

Thanks, it makes sense for me! Code-wise, it looks fine.

What would be interesting it to mention a mail thread discussing this or
maybe the patch itself that added the .remove_new() idea
[https://git.kernel.org/linus/5c5a7680e67b right?].

BTW, nice idea - converting all at once would be a terrible sync effort,
I guess this way things will go smoothly.
Feel free to add my:

Reviewed-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Cheers,


Guilherme

Re: [PATCH] pstore/ram: Convert to platform remove callback returning void

[Kees Cook]

On Thu, Apr 06, 2023 at 12:57:30PM -0300, Guilherme G. Piccoli wrote:
> On 01/04/2023 09:00, 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 (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>
> 
> Thanks, it makes sense for me! Code-wise, it looks fine.
> 
> What would be interesting it to mention a mail thread discussing this or
> maybe the patch itself that added the .remove_new() idea
> [https://git.kernel.org/linus/5c5a7680e67b right?].
> 
> BTW, nice idea - converting all at once would be a terrible sync effort,
> I guess this way things will go smoothly.
> Feel free to add my:
> 
> Reviewed-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Thanks! Looks good to me.

Acked-by: Kees Cook <keescook@chromium.org>

Do you want to take these view some other tree, or should I take this
via my pstore tree?

-Kees

-- 
Kees Cook

Re: [PATCH] pstore/ram: Convert to platform remove callback returning void

[Uwe Kleine-König]

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

On Thu, Apr 06, 2023 at 03:50:49PM -0700, Kees Cook wrote:
> On Thu, Apr 06, 2023 at 12:57:30PM -0300, Guilherme G. Piccoli wrote:
> > On 01/04/2023 09:00, 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 (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>
> > 
> > Thanks, it makes sense for me! Code-wise, it looks fine.
> > 
> > What would be interesting it to mention a mail thread discussing this or
> > maybe the patch itself that added the .remove_new() idea
> > [https://git.kernel.org/linus/5c5a7680e67b right?].
> > 
> > BTW, nice idea - converting all at once would be a terrible sync effort,
> > I guess this way things will go smoothly.
> > Feel free to add my:
> > 
> > Reviewed-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 
> Thanks! Looks good to me.
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> Do you want to take these view some other tree, or should I take this
> via my pstore tree?

Take it via your tree please. There are still >1000 drivers to convert,
so I guess this patch will be in Linus' tree long before I can convert
struct platform_driver.

Best regards and 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 --]

Re: [PATCH] pstore/ram: Convert to platform remove callback returning void

[Uwe Kleine-König]

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

Hello Kees,

On Sat, Apr 08, 2023 at 10:10:07AM +0200, Uwe Kleine-König wrote:
> On Thu, Apr 06, 2023 at 03:50:49PM -0700, Kees Cook wrote:
> > On Thu, Apr 06, 2023 at 12:57:30PM -0300, Guilherme G. Piccoli wrote:
> > > On 01/04/2023 09:00, 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 (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>
> > > 
> > > Thanks, it makes sense for me! Code-wise, it looks fine.
> > > 
> > > What would be interesting it to mention a mail thread discussing this or
> > > maybe the patch itself that added the .remove_new() idea
> > > [https://git.kernel.org/linus/5c5a7680e67b right?].
> > > 
> > > BTW, nice idea - converting all at once would be a terrible sync effort,
> > > I guess this way things will go smoothly.
> > > Feel free to add my:
> > > 
> > > Reviewed-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> > 
> > Thanks! Looks good to me.
> > 
> > Acked-by: Kees Cook <keescook@chromium.org>
> > 
> > Do you want to take these view some other tree, or should I take this
> > via my pstore tree?
> 
> Take it via your tree please. There are still >1000 drivers to convert,
> so I guess this patch will be in Linus' tree long before I can convert
> struct platform_driver.

This patch didn't make it into v6.4-rc1, is it still on your radar to
queue it for the next merge window?

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] pstore/ram: Convert to platform remove callback returning void

[Kees Cook]

On Sat, 1 Apr 2023 14:00:00 +0200, 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 (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.
> 
> [...]

Applied to for-next/pstore, thanks!

[1/1] pstore/ram: Convert to platform remove callback returning void
      https://git.kernel.org/kees/c/48f2c681df43

I will send this Linus's way for -rc2. Apologies for the delay!

-- 
Kees Cook

Re: [PATCH] pstore/ram: Convert to platform remove callback returning void

[Uwe Kleine-König]

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

On Wed, May 10, 2023 at 02:07:20PM -0700, Kees Cook wrote:
> On Sat, 1 Apr 2023 14:00:00 +0200, 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 (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.
> > 
> > [...]
> 
> Applied to for-next/pstore, thanks!
> 
> [1/1] pstore/ram: Convert to platform remove callback returning void
>       https://git.kernel.org/kees/c/48f2c681df43
> 
> I will send this Linus's way for -rc2. Apologies for the delay!

Thanks. If you ask me, you don't need to annoy Linus with such a patch
after the merge window. If it goes in for 6.5-rc1 that's fine for me.
There is no urge, still >1000 drivers to convert before I start thinking
about a patch that changes struct platform_driver and so depends on this
conversion.

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