Dan Williams wrote:
> Ira Weiny wrote:
> > BIOS can configure memory devices as firmware first. This will send CXL
> > events to the firmware instead of the OS. The firmware can then send
> > these events to the OS via UEFI.
> >
> > UEFI v2.10 section N.2.14 defines a Common Platform Error Record (CPER)
> > format for CXL Component Events. The format is mostly the same as the
> > CXL Common Event Record Format. The difference is a GUID is used in
> > the Section Type to identify the event type.
> >
> > Add EFI support to detect CXL CPER records and call a notifier chain
> > with the record data blobs to be processed by the CXL code.
>
> It is no longer a notifier chain in this version. I wouldn't even call
> it a notifier, it's just a registered callback.
Ah yea I missed that in my rework sorry.
[snip]
> > +DECLARE_RWSEM(cxl_cper_rw_sem);
> > +static cxl_cper_notifier cper_notifier;
> > +
> > +void cxl_cper_post_event(const char *pfx, guid_t *sec_type,
> > + struct cper_cxl_event_rec *rec)
> > +{
> > + struct cxl_cper_event_data data = {
> > + .rec = rec,
> > + };
> > +
> > + if (!(rec->hdr.validation_bits & CPER_CXL_COMP_EVENT_LOG_VALID)) {
> > + pr_err(FW_WARN "cxl event no Component Event Log present\n");
> > + return;
> > + }
> > +
> > + if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID))
> > + data.event_type = CXL_CPER_EVENT_GEN_MEDIA;
> > + else if (guid_equal(sec_type, &CPER_SEC_CXL_DRAM_GUID))
> > + data.event_type = CXL_CPER_EVENT_DRAM;
> > + else if (guid_equal(sec_type, &CPER_SEC_CXL_MEM_MODULE_GUID))
> > + data.event_type = CXL_CPER_EVENT_MEM_MODULE;
> > +
> > + down_read(&cxl_cper_rw_sem);
>
> guard(rwsem_read)(&cxl_cper_rw_sem);
Much better. Done throughout.
>
> > + if (cper_notifier)
> > + cper_notifier(&data);
> > + up_read(&cxl_cper_rw_sem);
> > +}
> > +
> > +void cxl_cper_register_notifier(cxl_cper_notifier notifier)
> > +{
> > + down_write(&cxl_cper_rw_sem);
>
> guard(rwsem_write)(&cxl_cper_rw_sem);
>
> > + cper_notifier = notifier;
>
> I would enforce that there is only one registrant and explicitly fail
> attempts to assign more than one.
Done.
>
> > + up_write(&cxl_cper_rw_sem);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_cper_register_notifier, CXL);
> > +
> > +void cxl_cper_unregister_notifier(void)
> > +{
> > + down_write(&cxl_cper_rw_sem);
>
> guard(rwsem_write)(&cxl_cper_rw_sem);
>
> > + cper_notifier = NULL;
>
> This could enforce that the same callback specified at registration time
> must also be passed at unregistration to disallow anonymous
> unregistration.
>
> Makes the code self documenting that the registrant is a singleton, and
> that unregistration must precede the next registration.
But what do we do if it does not match? Returning an error will be
ignored by the cxl_pci_driver_exit() and if we enforce the singleton in
the registration I don't see a lot of room for error here.
[snip]
> > diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> > index 18dab4d90dc8..c764ff877a6d 100644
> > --- a/include/linux/cxl-event.h
> > +++ b/include/linux/cxl-event.h
> > @@ -108,4 +108,55 @@ struct cxl_event_record_raw {
> > union cxl_event event;
> > } __packed;
> >
> > +enum cxl_event_type {
> > + CXL_CPER_EVENT_GEN_MEDIA,
> > + CXL_CPER_EVENT_DRAM,
> > + CXL_CPER_EVENT_MEM_MODULE,
> > +};
> > +
> > +#pragma pack(1)
>
> Looks like there is usage of __packed a few lines up, just pick
> one-style. Prefer __packed vs #pragma when only a small handful of
> structures need annotation as that is easier to check for correctness in
> patch form.
Ok I'll change it. Smita requested the use of pragma but keeping the
__packed is redundant right now. And I'll go with your preference of
__packed.
Ira
