Messages in this thread Patch in this message | | | Date | Thu, 1 Dec 2022 07:10:59 -0800 | From | Ira Weiny <> | Subject | Re: [PATCH V2 02/11] cxl/mem: Implement Get Event Records command |
| |
On Thu, Dec 01, 2022 at 01:06:50PM +0000, Jonathan Cameron wrote: > 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?
Just following the pattern that 'cxl_mem_*' functions take a cxlds parameter. <shrug>
I'm going to leave this because it is tested.
> > > +} > > + > > +/* > > + * 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
Nope I've learned my lesson and checked first!
> - whilst it might makes sense to add one, let's not tie that up with this series.
Yep I did not want to hold this up for something like that. > > > + 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
I've been beat up in the past for not combining statements before. So I've a bad habit sometimes.
This pattern is a bit more clear. Since I'm adding the comment below I'll change it.
> > 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.
Well 0-day did not complain. :-/ But I know there are other checkers out there; better to add now, thanks.
Thanks for the review, Ira
> > > + struct mutex event_buf_lock; > > + > > int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd); > > }; > > > >
| |