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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux