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

[Uwe Kleine-König]

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 at 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 at 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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20230530/55bca75d/attachment.sig>