On Mon, Nov 20, 2023 at 6:31 PM Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> 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(). > > Instead of returning an error code, emit a better error message than the > core. Apart from the improved error message this patch has no effects > for the driver. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > --- > Hello, > > I tried to improve this driver before, see > > https://lore.kernel.org/linux-acpi/CAJZ5v0ifb-wvyp0JRq_4c1L6vTi_qEeXJ6P=Pmmq_56xRL74_A@xxxxxxxxxxxxxx > https://lore.kernel.org/linux-arm-kernel/20221219221439.1681770-1-u.kleine-koenig@xxxxxxxxxxxxxx > https://lore.kernel.org/linux-arm-kernel/20221220154447.12341-1-u.kleine-koenig@xxxxxxxxxxxxxx > > but this didn't result in any patch being applied. > > I think it's inarguable that there is a problem that wants to be fixed. > My tries to fix this problem fixxled out, so here comes a minimal change > that just points out the problem and otherwise makes ghes_remove() > return void without further side effects to allow me to continue my > quest to make platform_driver remove callbacks return no error. Tony, Boris, any objections against this patch? > drivers/acpi/apei/ghes.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 63ad0541db38..dd8cd10b7809 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -1438,7 +1438,7 @@ static int ghes_probe(struct platform_device *ghes_dev) > return rc; > } > > -static int ghes_remove(struct platform_device *ghes_dev) > +static void ghes_remove(struct platform_device *ghes_dev) > { > int rc; > struct ghes *ghes; > @@ -1475,8 +1475,15 @@ static int ghes_remove(struct platform_device *ghes_dev) > break; > case ACPI_HEST_NOTIFY_SOFTWARE_DELEGATED: > rc = apei_sdei_unregister_ghes(ghes); > - if (rc) > - return rc; > + if (rc) { > + /* > + * Returning early results in a resource leak, but we're > + * only here if stopping the hardware failed. > + */ > + dev_err(&ghes_dev->dev, "Failed to unregister ghes (%pe)\n", > + ERR_PTR(rc)); > + return; > + } > break; > default: > BUG(); > @@ -1490,8 +1497,6 @@ static int ghes_remove(struct platform_device *ghes_dev) > mutex_unlock(&ghes_devs_mutex); > > kfree(ghes); > - > - return 0; > } > > static struct platform_driver ghes_platform_driver = { > @@ -1499,7 +1504,7 @@ static struct platform_driver ghes_platform_driver = { > .name = "GHES", > }, > .probe = ghes_probe, > - .remove = ghes_remove, > + .remove_new = ghes_remove, > }; > > void __init acpi_ghes_init(void) > > base-commit: 5a82d69d48c82e89aef44483d2a129f869f3506a > -- > 2.42.0 >
