Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 22 Apr 2024 20:47:12 -0700 | From | Ira Weiny <> | Subject | RE: [PATCH v2 1/3] acpi/ghes: Process CXL Component Events |
| |
Dan Williams wrote: > Ira Weiny wrote:
[snip]
> > > > [0] > > Link: https://lore.kernel.org/all/cover.1711598777.git.alison.schofield@intel.com/ > > [1] > > Link: https://lore.kernel.org/all/65d111eb87115_6c745294ac@dwillia2-xfh.jf.intel.com.notmuch/ > > [2] > > Link: https://lore.kernel.org/all/b963c490-2c13-4b79-bbe7-34c6568423c7@moroto.mountain/ > > Minor, but this can be reformatted a bit cleaner: > > Link: http://lore.kernel.org/r/cover.1711598777.git.alison.schofield@intel.com [0] > Link: http://lore.kernel.org/r/65d111eb87115_6c745294ac@dwillia2-xfh.jf.intel.com.notmuch [1] > Link: http://lore.kernel.org/r/b963c490-2c13-4b79-bbe7-34c6568423c7@moroto.mountain [2] >
Sure.
[snip]
> > +/* > > + * Memory Module Event Record > > + * CXL rev 3.0 section 8.2.9.2.1.3; Table 8-45 > > + */ > > +#define CPER_SEC_CXL_MEM_MODULE_GUID \ > > + GUID_INIT(0xfe927475, 0xdd59, 0x4339, \ > > + 0xa5, 0x86, 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74) > > + > > +struct cxl_cper_work_data { > > + enum cxl_event_type event_type; > > + struct cxl_cper_event_rec rec; > > +}; > > + > > +DEFINE_KFIFO(cxl_cper_fifo, struct cxl_cper_work_data, 32); > > Any comment on where that "32" comes from?
If anyone has any better queue depth I'm open. It is just a guess.
> > > +static DEFINE_SPINLOCK(cxl_cper_work_lock); > > Needs a comment on what it is specifically protecting.
Ok.
> > > +static cxl_cper_callback cper_callback; > > +static void cxl_cper_cb_fn(struct work_struct *work) > > +{ > > + struct cxl_cper_work_data wd; > > + > > + while (kfifo_get(&cxl_cper_fifo, &wd)) > > + cper_callback(wd.event_type, &wd.rec); > > +} > > +static DECLARE_WORK(cxl_cb_work, cxl_cper_cb_fn); > > +struct work_struct *cxl_cper_work = NULL; > > Initializing global data to NULL is redundant, however this feels like > one too many dynamic things registered. > > cxl_cper_work and cper_callback are dynamic, but from the GHES > perspective all it cares about is checking if work is registered and if > so put the data in the kfifo and trigger that work func. > > It need not care about what happens after the work is queued. So, lets > just have the CXL driver register its own cxl_cper_work instance and > skip defining one locally here. Export cxl_cper_fifo for the driver to > optionally reference.
Ok I thought we had decided not to do that. If I recall exporting the fifo had some difficulties but it may have been my unfamiliarity with it.
Ira
[snip]
| ![\](/images/icornerr.gif) |