On Wed, Apr 12, 2023 at 4:55 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> 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@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.
