On Tue, Dec 20, 2022 at 4:45 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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
>
