Messages in this thread Patch in this message | | | Date | Thu, 1 Dec 2022 13:06:50 +0000 | From | Jonathan Cameron <> | Subject | Re: [PATCH V2 02/11] cxl/mem: Implement Get Event Records command |
| |
On Wed, 30 Nov 2022 16:27:10 -0800 ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com> > > CXL devices have multiple event logs which can be queried for CXL event > records. Devices are required to support the storage of at least one > event record in each event log type. > > Devices track event log overflow by incrementing a counter and tracking > the time of the first and last overflow event seen. > > Software queries events via the Get Event Record mailbox command; CXL > rev 3.0 section 8.2.9.2.2. > > Issue the Get Event Record mailbox command on driver load. Trace each > record found with a generic record trace. Trace any overflow > conditions. > > The device can return up to 1MB worth of event records per query. > Allocate a shared large buffer to handle the max number of records based > on the mailbox payload size. > > This patch traces a raw event record only and leaves the specific event > record types to subsequent patches. > > Macros are created to use for tracing the common CXL Event header > fields. > > Cc: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Hi Ira,
Looks good to me. A few trivial suggestions inline. Either way,
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 16176b9278b4..70b681027a3d 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -7,6 +7,9 @@
...
> + > +static void cxl_mem_free_event_buffer(void *data) > +{ > + struct cxl_dev_state *cxlds = data; > + > + kvfree(cxlds->event_buf);
Trivial, but why not just pass in the event_buf?
> +} > + > +/* > + * There is a single buffer for reading event logs from the mailbox. All logs > + * share this buffer protected by the cxlds->event_buf_lock. > + */ > +static struct cxl_get_event_payload *alloc_event_buf(struct cxl_dev_state *cxlds) > +{ > + struct cxl_get_event_payload *buf; > + > + dev_dbg(cxlds->dev, "Allocating event buffer size %zu\n", > + cxlds->payload_size); > + > + buf = kvmalloc(cxlds->payload_size, GFP_KERNEL);
huh. I assumed there would be a devm_kvmalloc() but apparently not.. Ah well - whilst it might makes sense to add one, let's not tie that up with this series.
> + if (buf && devm_add_action_or_reset(cxlds->dev, > + cxl_mem_free_event_buffer, cxlds)) > + return NULL;
Trivial, but I'd go for a more wordy but more conventional pattern of if (!buf) return NULL;
if (devm_add_action_or_reset()) return NULL return buff; > + return buf; > +} > +
...
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index cd35f43fedd4..55d57f5a64bc 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -4,6 +4,7 @@ > #define __CXL_MEM_H__ > #include <uapi/linux/cxl_mem.h> > #include <linux/cdev.h> > +#include <linux/uuid.h> > #include "cxl.h" > > /* CXL 2.0 8.2.8.5.1.1 Memory Device Status Register */ > @@ -250,12 +251,16 @@ struct cxl_dev_state { > > bool msi_enabled; > > + struct cxl_get_event_payload *event_buf; Whilst it is obvious (and document at point of allocation), I think one of the static checkers still warns that all locks must have comments. Probably easier to add one now than wait for the inevitable warning report.
> + struct mutex event_buf_lock; > + > int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd); > }; >
| |