| Message ID | 20230401120000.2487153-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 48f2c681df4329b50fc92516c10e0398ca127242 |
| Headers | show |
| Series | pstore/ram: Convert to platform remove callback returning void | expand |
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
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
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
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
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!
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
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,
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(-) base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6