[PATCH] ACPI: APEI: GHES: Convert to platform remove callback returning void

[Rafael J. Wysocki]

On Mon, Nov 20, 2023 at 6:31 PM Uwe Kleine-König
<u.kleine-koenig at 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 at 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
>