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

[Ira Weiny]

Ira Weiny wrote:
> Dan Williams wrote:
> > Ira Weiny wrote:
> 

[snip]

> > 
> > > +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.

Looking more closely I see that AER does something similar but it uses
constructs which I thought we should avoid.  For example all of
ghes_handle_aer is predicated on CONFIG_ACPI_APEI_PCIEAER.[0]

What I have is a bit complicated, with the extra pointer.  Keeping the
kfifo within the ghes module is much easier to follow IMO.

Let me know if you really want me to pursue this separation but my late
night gut feeling is this is going to be more trouble than it is worth to
keep the separation amongst the modules.

Ira

[0]
static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
{                                        
#ifdef CONFIG_ACPI_APEI_PCIEAER
	...
#endif
}