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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6627360078428_bd77929462@iweiny-mobl.notmuch>
Date: Mon, 22 Apr 2024 21:16:00 -0700
From: Ira Weiny <ira.weiny@...el.com>
To: Ira Weiny <ira.weiny@...el.com>, Dan Williams <dan.j.williams@...el.com>,
	Dave Jiang <dave.jiang@...el.com>, Jonathan Cameron
	<jonathan.cameron@...wei.com>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@....com>, Shiju Jose <shiju.jose@...wei.com>
CC: Dan Carpenter <dan.carpenter@...aro.org>, Yazen Ghannam
	<yazen.ghannam@....com>, Davidlohr Bueso <dave@...olabs.net>, "Alison
 Schofield" <alison.schofield@...el.com>, Vishal Verma
	<vishal.l.verma@...el.com>, Ard Biesheuvel <ardb@...nel.org>,
	<linux-efi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, Ira Weiny <ira.weiny@...el.com>, "Rafael J.
 Wysocki" <rafael@...nel.org>, Tony Luck <tony.luck@...el.com>, "Borislav
 Petkov" <bp@...en8.de>
Subject: RE: [PATCH v2 1/3] acpi/ghes: Process CXL Component Events

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
}