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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
