| Message ID | 20221220154447.12341-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] firmware: arm_sdei: Make sdei_unregister_ghes() return void | expand |
On Tue, Dec 20, 2022 at 4:45 PM Uwe Kleine-König <u.kleine-koenig@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@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 >
Hello Rafael, On Wed, Dec 21, 2022 at 02:53:05PM +0100, Rafael J. Wysocki wrote: > On Tue, Dec 20, 2022 at 4:45 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > [...] > > 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. It's you who cares for this code, but I'd prefer my option. If we assume the describing comment would have a similar length, we're saving 3 or four lines of code here but need 3 lines for the #if / #else / #endif plus the stub definition. And compared to my suggested solution we don't catch someone introducing a (bogus) call to apei_sdei_unregister_ghes() (or sdei_unregister_ghes()). And (again IMHO) two different implementations are harder to grasp than a single with an if. If you don't like the BUG, a plain return is in my eyes the next best option which is semantically equivalent to an empty stub. If you still like the stub better (or a return instead of the BUG), I can send a v3, just tell me your preference. Best regards Uwe
Hello Rafael, On Wed, Dec 21, 2022 at 07:21:38PM +0100, Uwe Kleine-König wrote: > On Wed, Dec 21, 2022 at 02:53:05PM +0100, Rafael J. Wysocki wrote: > > On Tue, Dec 20, 2022 at 4:45 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > [...] > > > 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. > > It's you who cares for this code, but I'd prefer my option. If we assume > the describing comment would have a similar length, we're saving 3 or > four lines of code here but need 3 lines for the #if / #else / #endif > plus the stub definition. And compared to my suggested solution we don't > catch someone introducing a (bogus) call to apei_sdei_unregister_ghes() > (or sdei_unregister_ghes()). And (again IMHO) two different > implementations are harder to grasp than a single with an if. > > If you don't like the BUG, a plain return is in my eyes the next best > option which is semantically equivalent to an empty stub. > > If you still like the stub better (or a return instead of the BUG), I > can send a v3, just tell me your preference. I work on changes that depend on a solution here. However you didn't tell me your preference here. I'm unsure if this means that this discussion fell through the cracks, or if it annoys you and you still prefer the cpp #ifdef solution. A note from your side would be very welcome. Best regards Uwe
On Wed, Apr 12, 2023 at 4:55 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Rafael, > > On Wed, Dec 21, 2022 at 07:21:38PM +0100, Uwe Kleine-König wrote: > > On Wed, Dec 21, 2022 at 02:53:05PM +0100, Rafael J. Wysocki wrote: > > > On Tue, Dec 20, 2022 at 4:45 PM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > [...] > > > > 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. > > > > It's you who cares for this code, but I'd prefer my option. If we assume > > the describing comment would have a similar length, we're saving 3 or > > four lines of code here but need 3 lines for the #if / #else / #endif > > plus the stub definition. And compared to my suggested solution we don't > > catch someone introducing a (bogus) call to apei_sdei_unregister_ghes() > > (or sdei_unregister_ghes()). And (again IMHO) two different > > implementations are harder to grasp than a single with an if. > > > > If you don't like the BUG, a plain return is in my eyes the next best > > option which is semantically equivalent to an empty stub. > > > > If you still like the stub better (or a return instead of the BUG), I > > can send a v3, just tell me your preference. > > I work on changes that depend on a solution here. However you didn't > tell me your preference here. I'm unsure if this means that this > discussion fell through the cracks, or if it annoys you and you still > prefer the cpp #ifdef solution. A note from your side would be very > welcome. Well, every time I see BUG() in the code I wonder if crashing the kernel really is the best thing one can do should the execution reach that point. In any case, it's not my opinion that matters the most regarding APEI/GHES, so I would like Tony/Boris/James to speak up here.
Hello, On Wed, Apr 12, 2023 at 08:33:15PM +0200, Rafael J. Wysocki wrote: > On Wed, Apr 12, 2023 at 4:55 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Wed, Dec 21, 2022 at 07:21:38PM +0100, Uwe Kleine-König wrote: > > > On Wed, Dec 21, 2022 at 02:53:05PM +0100, Rafael J. Wysocki wrote: > > > > On Tue, Dec 20, 2022 at 4:45 PM Uwe Kleine-König > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > [...] > > > > > 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. > > > > > > It's you who cares for this code, but I'd prefer my option. If we assume > > > the describing comment would have a similar length, we're saving 3 or > > > four lines of code here but need 3 lines for the #if / #else / #endif > > > plus the stub definition. And compared to my suggested solution we don't > > > catch someone introducing a (bogus) call to apei_sdei_unregister_ghes() > > > (or sdei_unregister_ghes()). And (again IMHO) two different > > > implementations are harder to grasp than a single with an if. > > > > > > If you don't like the BUG, a plain return is in my eyes the next best > > > option which is semantically equivalent to an empty stub. > > > > > > If you still like the stub better (or a return instead of the BUG), I > > > can send a v3, just tell me your preference. > > > > I work on changes that depend on a solution here. However you didn't > > tell me your preference here. I'm unsure if this means that this > > discussion fell through the cracks, or if it annoys you and you still > > prefer the cpp #ifdef solution. A note from your side would be very > > welcome. > > Well, every time I see BUG() in the code I wonder if crashing the > kernel really is the best thing one can do should the execution reach > that point. > > In any case, it's not my opinion that matters the most regarding > APEI/GHES, so I would like Tony/Boris/James to speak up here. hmm, they didn't speak up so this patch is stalled. I added them to "To:" (instead of Cc: before) in this mail, let's see if that helps. Can you please state your preferred solution that I can properly prepare this driver for the conversion of the remove callback?! Best regards Uwe
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(); - 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. */
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@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(-)