| Message ID | 20231120173053.49597-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Commit | f2f212f36a8cd3cd6763a19611d0edd2a19df51a |
| Headers | show |
| Series | ACPI: APEI: GHES: Convert to platform remove callback returning void | expand |
On Mon, Nov 20, 2023 at 6:31 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 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@pengutronix.de> > --- > Hello, > > I tried to improve this driver before, see > > https://lore.kernel.org/linux-acpi/CAJZ5v0ifb-wvyp0JRq_4c1L6vTi_qEeXJ6P=Pmmq_56xRL74_A@mail.gmail.com > https://lore.kernel.org/linux-arm-kernel/20221219221439.1681770-1-u.kleine-koenig@pengutronix.de > https://lore.kernel.org/linux-arm-kernel/20221220154447.12341-1-u.kleine-koenig@pengutronix.de > > 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 >
On Wed, Nov 22, 2023 at 04:25:30PM +0100, Rafael J. Wysocki wrote: > On Mon, Nov 20, 2023 at 6:31 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> 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@pengutronix.de> > > --- > > Hello, > > > > I tried to improve this driver before, see > > > > https://lore.kernel.org/linux-acpi/CAJZ5v0ifb-wvyp0JRq_4c1L6vTi_qEeXJ6P=Pmmq_56xRL74_A@mail.gmail.com > > https://lore.kernel.org/linux-arm-kernel/20221219221439.1681770-1-u.kleine-koenig@pengutronix.de > > https://lore.kernel.org/linux-arm-kernel/20221220154447.12341-1-u.kleine-koenig@pengutronix.de > > > > 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? SDEI is James. Moving him to To:
Hello James, On Wed, Nov 22, 2023 at 06:49:13PM +0100, Borislav Petkov wrote: > On Wed, Nov 22, 2023 at 04:25:30PM +0100, Rafael J. Wysocki wrote: > > On Mon, Nov 20, 2023 at 6:31 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> 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@pengutronix.de> > > > --- > > > Hello, > > > > > > I tried to improve this driver before, see > > > > > > https://lore.kernel.org/linux-acpi/CAJZ5v0ifb-wvyp0JRq_4c1L6vTi_qEeXJ6P=Pmmq_56xRL74_A@mail.gmail.com > > > https://lore.kernel.org/linux-arm-kernel/20221219221439.1681770-1-u.kleine-koenig@pengutronix.de > > > https://lore.kernel.org/linux-arm-kernel/20221220154447.12341-1-u.kleine-koenig@pengutronix.de > > > > > > 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? > > SDEI is James. Moving him to To: I wonder if you had a chance to look at this patch. It doesn't change anything for the SDEI driver, the only effect is to have one driver less using platform_driver's remove function. Would be great if that patch made it in. Best regards Uwe
Hello, On Mon, Dec 18, 2023 at 09:47:10PM +0100, Uwe Kleine-König wrote: > On Wed, Nov 22, 2023 at 06:49:13PM +0100, Borislav Petkov wrote: > > On Wed, Nov 22, 2023 at 04:25:30PM +0100, Rafael J. Wysocki wrote: > > > On Mon, Nov 20, 2023 at 6:31 PM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> 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@pengutronix.de> > > > > --- > > > > Hello, > > > > > > > > I tried to improve this driver before, see > > > > > > > > https://lore.kernel.org/linux-acpi/CAJZ5v0ifb-wvyp0JRq_4c1L6vTi_qEeXJ6P=Pmmq_56xRL74_A@mail.gmail.com > > > > https://lore.kernel.org/linux-arm-kernel/20221219221439.1681770-1-u.kleine-koenig@pengutronix.de > > > > https://lore.kernel.org/linux-arm-kernel/20221220154447.12341-1-u.kleine-koenig@pengutronix.de > > > > > > > > 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? > > > > SDEI is James. Moving him to To: > > I wonder if you had a chance to look at this patch. > > It doesn't change anything for the SDEI driver, the only effect is to > have one driver less using platform_driver's remove function. > > Would be great if that patch made it in. I guess it's to late for 6.8-rc1, but I wonder if this patch is still on your radar? Best regards Uwe
Hello, On Wed, Jan 10, 2024 at 09:34:53AM +0100, Uwe Kleine-König wrote: > On Mon, Dec 18, 2023 at 09:47:10PM +0100, Uwe Kleine-König wrote: > > On Wed, Nov 22, 2023 at 06:49:13PM +0100, Borislav Petkov wrote: > > > On Wed, Nov 22, 2023 at 04:25:30PM +0100, Rafael J. Wysocki wrote: > > > > On Mon, Nov 20, 2023 at 6:31 PM Uwe Kleine-König > > > > <u.kleine-koenig@pengutronix.de> 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@pengutronix.de> > > > > > --- > > > > > Hello, > > > > > > > > > > I tried to improve this driver before, see > > > > > > > > > > https://lore.kernel.org/linux-acpi/CAJZ5v0ifb-wvyp0JRq_4c1L6vTi_qEeXJ6P=Pmmq_56xRL74_A@mail.gmail.com > > > > > https://lore.kernel.org/linux-arm-kernel/20221219221439.1681770-1-u.kleine-koenig@pengutronix.de > > > > > https://lore.kernel.org/linux-arm-kernel/20221220154447.12341-1-u.kleine-koenig@pengutronix.de > > > > > > > > > > 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? > > > > > > SDEI is James. Moving him to To: > > > > I wonder if you had a chance to look at this patch. > > > > It doesn't change anything for the SDEI driver, the only effect is to > > have one driver less using platform_driver's remove function. > > > > Would be great if that patch made it in. > > I guess it's to late for 6.8-rc1, but I wonder if this patch is still on > your radar? I'm a frustrated about this patch. It already missed two merge windows while it's (IMHO) easy to understand that it doesn't change anything relevant for the driver. (There is a resource leak in this driver, the only difference my patch makes here is that it's more visible than before that the leak is there.) What must happen to make this patch go in? Best regards Uwe
On Thu, Feb 15, 2024 at 10:11 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > On Wed, Jan 10, 2024 at 09:34:53AM +0100, Uwe Kleine-König wrote: > > On Mon, Dec 18, 2023 at 09:47:10PM +0100, Uwe Kleine-König wrote: > > > On Wed, Nov 22, 2023 at 06:49:13PM +0100, Borislav Petkov wrote: > > > > On Wed, Nov 22, 2023 at 04:25:30PM +0100, Rafael J. Wysocki wrote: > > > > > On Mon, Nov 20, 2023 at 6:31 PM Uwe Kleine-König > > > > > <u.kleine-koenig@pengutronix.de> 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@pengutronix.de> > > > > > > --- > > > > > > Hello, > > > > > > > > > > > > I tried to improve this driver before, see > > > > > > > > > > > > https://lore.kernel.org/linux-acpi/CAJZ5v0ifb-wvyp0JRq_4c1L6vTi_qEeXJ6P=Pmmq_56xRL74_A@mail.gmail.com > > > > > > https://lore.kernel.org/linux-arm-kernel/20221219221439.1681770-1-u.kleine-koenig@pengutronix.de > > > > > > https://lore.kernel.org/linux-arm-kernel/20221220154447.12341-1-u.kleine-koenig@pengutronix.de > > > > > > > > > > > > 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? > > > > > > > > SDEI is James. Moving him to To: > > > > > > I wonder if you had a chance to look at this patch. > > > > > > It doesn't change anything for the SDEI driver, the only effect is to > > > have one driver less using platform_driver's remove function. > > > > > > Would be great if that patch made it in. > > > > I guess it's to late for 6.8-rc1, but I wonder if this patch is still on > > your radar? > > I'm a frustrated about this patch. It already missed two merge windows > while it's (IMHO) easy to understand that it doesn't change anything > relevant for the driver. (There is a resource leak in this driver, the > only difference my patch makes here is that it's more visible than > before that the leak is there.) > > What must happen to make this patch go in? Well, I will just have to take it without an ACK. However, the lack of a (responsive) ARM reviewer for APEI/GHES needs to be sorted out. Thanks!
On Thu, Feb 15, 2024 at 10:25 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Feb 15, 2024 at 10:11 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > Hello, > > > > On Wed, Jan 10, 2024 at 09:34:53AM +0100, Uwe Kleine-König wrote: > > > On Mon, Dec 18, 2023 at 09:47:10PM +0100, Uwe Kleine-König wrote: > > > > On Wed, Nov 22, 2023 at 06:49:13PM +0100, Borislav Petkov wrote: > > > > > On Wed, Nov 22, 2023 at 04:25:30PM +0100, Rafael J. Wysocki wrote: > > > > > > On Mon, Nov 20, 2023 at 6:31 PM Uwe Kleine-König > > > > > > <u.kleine-koenig@pengutronix.de> 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@pengutronix.de> > > > > > > > --- > > > > > > > Hello, > > > > > > > > > > > > > > I tried to improve this driver before, see > > > > > > > > > > > > > > https://lore.kernel.org/linux-acpi/CAJZ5v0ifb-wvyp0JRq_4c1L6vTi_qEeXJ6P=Pmmq_56xRL74_A@mail.gmail.com > > > > > > > https://lore.kernel.org/linux-arm-kernel/20221219221439.1681770-1-u.kleine-koenig@pengutronix.de > > > > > > > https://lore.kernel.org/linux-arm-kernel/20221220154447.12341-1-u.kleine-koenig@pengutronix.de > > > > > > > > > > > > > > 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? > > > > > > > > > > SDEI is James. Moving him to To: > > > > > > > > I wonder if you had a chance to look at this patch. > > > > > > > > It doesn't change anything for the SDEI driver, the only effect is to > > > > have one driver less using platform_driver's remove function. > > > > > > > > Would be great if that patch made it in. > > > > > > I guess it's to late for 6.8-rc1, but I wonder if this patch is still on > > > your radar? > > > > I'm a frustrated about this patch. It already missed two merge windows > > while it's (IMHO) easy to understand that it doesn't change anything > > relevant for the driver. (There is a resource leak in this driver, the > > only difference my patch makes here is that it's more visible than > > before that the leak is there.) > > > > What must happen to make this patch go in? > > Well, I will just have to take it without an ACK. > > However, the lack of a (responsive) ARM reviewer for APEI/GHES needs > to be sorted out. So applied (as 6.9 material), but I'm going to remove the record pointing James Morse as an APEI reviewer from MAINTAINERS, as he doesn't even react to responses to patches sent by himself.
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)
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@pengutronix.de> --- Hello, I tried to improve this driver before, see https://lore.kernel.org/linux-acpi/CAJZ5v0ifb-wvyp0JRq_4c1L6vTi_qEeXJ6P=Pmmq_56xRL74_A@mail.gmail.com https://lore.kernel.org/linux-arm-kernel/20221219221439.1681770-1-u.kleine-koenig@pengutronix.de https://lore.kernel.org/linux-arm-kernel/20221220154447.12341-1-u.kleine-koenig@pengutronix.de 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. Best regards Uwe drivers/acpi/apei/ghes.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) base-commit: 5a82d69d48c82e89aef44483d2a129f869f3506a