Dan Williams wrote: > Ira Weiny wrote: [snip] > > > > [0] > > Link: https://lore.kernel.org/all/cover.1711598777.git.alison.schofield@xxxxxxxxx/ > > [1] > > Link: https://lore.kernel.org/all/65d111eb87115_6c745294ac@xxxxxxxxxxxxxxxxxxxxxxxxx.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@xxxxxxxxx [0] > Link: http://lore.kernel.org/r/65d111eb87115_6c745294ac@xxxxxxxxxxxxxxxxxxxxxxxxx.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]
