[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <66272f40420e6_bbee62945d@iweiny-mobl.notmuch>
Date: Mon, 22 Apr 2024 20:47:12 -0700
From: Ira Weiny <ira.weiny@...el.com>
To: Dan Williams <dan.j.williams@...el.com>, Ira Weiny <ira.weiny@...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
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]
Powered by blists - more mailing lists