RE: [PATCH v2 1/3] acpi/ghes: Process CXL Component Events

[Ira Weiny <ira.weiny@intel.com>]

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]