[PATCH v2] firmware: arm_sdei: Make sdei_unregister_ghes() return void

[Rafael J. Wysocki]

On Tue, Dec 20, 2022 at 4:45 PM Uwe Kleine-König
<u.kleine-koenig at pengutronix.de> wrote:
>
> Unregistering a ghes shouldn't fail (because how can firmware refuse to
> disable an event on unregister). And the callers are not really in a
> position to handle errors. (Note: The return value of platform remove
> callbacks is ignored.) So make sdei_unregister_ghes() return void and
> add warnings at the few locations that can theoretically fail.
>
> !IS_ENABLED(CONFIG_ACPI_APEI_GHES) and
> !IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) cannot be hit here, because if
> these aren't given, ghex_probe() already fails and so ghes_remove()
> isn't called.
>
> This change improves the behaviour in the error case. Without it the
> platform code emits a very generic (and so very unhelpful) error
> message. Now the warning is at least helpful.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig at pengutronix.de>
> ---
> Hello,
>
> Changes since (implicit) v1: Add the if (...) BUG() part to fix a linker
> error with CONFIG_ARM_SDE_INTERFACE disabled.
>
> Best regards
> Uwe
>
>  drivers/acpi/apei/ghes.c    | 19 ++++++++++++-------
>  drivers/firmware/arm_sdei.c | 14 +++++++-------
>  include/linux/arm_sdei.h    |  2 +-
>  3 files changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 066dc1f5c235..7d705930e21b 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -1275,12 +1275,20 @@ static int apei_sdei_register_ghes(struct ghes *ghes)
>                                  ghes_sdei_critical_callback);
>  }
>
> -static int apei_sdei_unregister_ghes(struct ghes *ghes)
> +static void apei_sdei_unregister_ghes(struct ghes *ghes)
>  {
> +       /*
> +        * If CONFIG_ARM_SDE_INTERFACE isn't enabled apei_sdei_register_ghes()
> +        * cannot have been called successfully. So ghes_remove() won't be
> +        * called because either ghes_probe() failed or the notify type isn't
> +        * ACPI_HEST_NOTIFY_SOFTWARE_DELEGATED.
> +        * Note the if statement below is necessary to prevent a linker error as
> +        * the compiler has no chance to understand the above correlation.
> +        */
>         if (!IS_ENABLED(CONFIG_ARM_SDE_INTERFACE))
> -               return -EOPNOTSUPP;
> +               BUG();

Well, you could just provide an empty stub for the !CONFIG_ARM_SDE_INTERFACE.

It would be cleaner and probably fewer lines of code too.

>
> -       return sdei_unregister_ghes(ghes);
> +       sdei_unregister_ghes(ghes);
>  }
>
>  static int ghes_probe(struct platform_device *ghes_dev)
> @@ -1421,7 +1429,6 @@ static int ghes_probe(struct platform_device *ghes_dev)
>
>  static int ghes_remove(struct platform_device *ghes_dev)
>  {
> -       int rc;
>         struct ghes *ghes;
>         struct acpi_hest_generic *generic;
>
> @@ -1455,9 +1462,7 @@ static int ghes_remove(struct platform_device *ghes_dev)
>                 ghes_nmi_remove(ghes);
>                 break;
>         case ACPI_HEST_NOTIFY_SOFTWARE_DELEGATED:
> -               rc = apei_sdei_unregister_ghes(ghes);
> -               if (rc)
> -                       return rc;
> +               apei_sdei_unregister_ghes(ghes);
>                 break;
>         default:
>                 BUG();
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 1e1a51510e83..7af619464183 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -889,7 +889,7 @@ int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
>         return err;
>  }
>
> -int sdei_unregister_ghes(struct ghes *ghes)
> +void sdei_unregister_ghes(struct ghes *ghes)
>  {
>         int i;
>         int err;
> @@ -897,16 +897,15 @@ int sdei_unregister_ghes(struct ghes *ghes)
>
>         might_sleep();
>
> -       if (!IS_ENABLED(CONFIG_ACPI_APEI_GHES))
> -               return -EOPNOTSUPP;
> -
>         /*
>          * The event may be running on another CPU. Disable it
>          * to stop new events, then try to unregister a few times.
>          */
>         err = sdei_event_disable(event_num);
> -       if (err)
> -               return err;
> +       if (err) {
> +               dev_warn(ghes->dev, "Failed to disable event %u: %pe\n", event_num, ERR_PTR(err));
> +               return;
> +       }
>
>         for (i = 0; i < 3; i++) {
>                 err = sdei_event_unregister(event_num);
> @@ -916,7 +915,8 @@ int sdei_unregister_ghes(struct ghes *ghes)
>                 schedule();
>         }
>
> -       return err;
> +       if (err)
> +               dev_warn(ghes->dev, "Failed to disable event %u: %pe\n", event_num, ERR_PTR(err));
>  }
>
>  static int sdei_get_conduit(struct platform_device *pdev)
> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> index 14dc461b0e82..0812af4530a4 100644
> --- a/include/linux/arm_sdei.h
> +++ b/include/linux/arm_sdei.h
> @@ -40,7 +40,7 @@ int sdei_event_disable(u32 event_num);
>  /* GHES register/unregister helpers */
>  int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
>                        sdei_event_callback *critical_cb);
> -int sdei_unregister_ghes(struct ghes *ghes);
> +void sdei_unregister_ghes(struct ghes *ghes);
>
>  #ifdef CONFIG_ARM_SDE_INTERFACE
>  /* For use by arch code when CPU hotplug notifiers are not appropriate. */
> --
> 2.38.1
>