On Mon, 9 Jun 2025 12:42:53 +0200
Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Jun 09, 2025 at 11:17:58AM +0100, Jonathan Cameron wrote:
> > On Fri, 6 Jun 2025 20:32:28 -0700
> > Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> >
> > > CXL has a symbol dependency on einj_core.ko, so if einj_init() fails then
> > > cxl_core.ko fails to load. Prior to the faux_device_create() conversion,
> > > einj_probe() failures were tracked by the einj_initialized flag without
> > > failing einj_init().
> > >
> > > Revert to that behavior and always succeed einj_init() given there is no
> > > way, and no pressing need, to discern faux device-create vs device-probe
> > > failures.
> > >
> > > This situation arose because CXL knows proper kernel named objects to
> > > trigger errors against, but acpi-einj knows how to perform the error
> > > injection. The injection mechanism is shared with non-CXL use cases. The
> > > result is CXL now has a module dependency on einj-core.ko, and init/probe
> > > failures are handled at runtime.
> > >
> > > Fixes: 6cb9441bfe8d ("ACPI: APEI: EINJ: Transition to the faux device interface")
> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > > Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> > > Cc: Ben Cheatham <Benjamin.Cheatham@xxxxxxx>
> > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > ---
> > > drivers/acpi/apei/einj-core.c | 9 +++------
> > > 1 file changed, 3 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> > > index fea11a35eea3..9b041415a9d0 100644
> > > --- a/drivers/acpi/apei/einj-core.c
> > > +++ b/drivers/acpi/apei/einj-core.c
> > > @@ -883,19 +883,16 @@ static int __init einj_init(void)
> > > }
> > >
> > > einj_dev = faux_device_create("acpi-einj", NULL, &einj_device_ops);
> > > - if (!einj_dev)
> > > - return -ENODEV;
> > >
> > > - einj_initialized = true;
> > > + if (einj_dev)
> > > + einj_initialized = true;
> > >
> > > return 0;
> > > }
> > >
> > > static void __exit einj_exit(void)
> > > {
> > > - if (einj_initialized)
> > > - faux_device_destroy(einj_dev);
> > > -
> > > + faux_device_destroy(einj_dev);
> >
> > Hi Dan,
> >
> > Thi bit is sort of fine though not really related, because
> > faux_device_destroy() checks
> >
> > void faux_device_destroy(struct faux_device *faux_dev)
> > {
> > struct device *dev = &faux_dev->dev;
> >
> > if (!faux_dev)
> > return;
> >
> > Though that check is after a dereference of faux_dev
> > which doesn't look right to me. Might be fine because
> > of how the kernel is built (I can't remember where we ended
> > up on topic of compilers making undefined behavior based
> > optimizations). Still not that nice from a logical point of view!
>
> I think this is fine as we just put "0 + offset of dev" into dev, and
> didn't do anything with that (i.e. no actual read of that memory
> location happened). The compiler shouldn't be doing anything that could
> happen after the return before we check for a valid pointer here, right?
Hmm. I did some digging. Seems that was debated 10 years ago without
a huge amount of clarity on the answer beyond all sane people telling
compiler folk not to use this in optimizations :)
Comes down to whether any dereference of NULL is UB whether or not
the compiler can just do a simple offset calculation.
Anyhow, whilst fine, it's still a little ugly to my eyes :(
Jonathan
>
> thanks,
>
> greg k-h
>
