Re: [PATCH 03/11] cxl/mem: Implement Clear Event Records command

From: Ira Weiny
Date: Wed Nov 16 2022 - 20:07:40 EST


On Wed, Nov 16, 2022 at 03:24:26PM +0000, Jonathan Cameron wrote:
> On Thu, 10 Nov 2022 10:57:50 -0800
> ira.weiny@xxxxxxxxx wrote:
>
> > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> > CXL rev 3.0 section 8.2.9.2.3 defines the Clear Event Records mailbox
> > command. After an event record is read it needs to be cleared from the
> > event log.
> >
> > Implement cxl_clear_event_record() and call it for each record retrieved
> > from the device.
> >
> > Each record is cleared individually. A clear all bit is specified but
> > events could arrive between a get and the final clear all operation.
> > Therefore each event is cleared specifically.
> >
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> Some follow through comment updates needed from changes in earlier patches +
> one comment you can ignore if you prefer to keep it as is.
>
> > static void cxl_mem_get_records_log(struct cxl_dev_state *cxlds,
> > enum cxl_event_log_type type)
> > {
> > @@ -728,14 +750,23 @@ static void cxl_mem_get_records_log(struct cxl_dev_state *cxlds,
> > }
> >
> > pl_nr = le16_to_cpu(payload.record_count);
> > - if (trace_cxl_generic_event_enabled()) {
>
> To simplify this patch, maybe push this check down in the previous patch so this
> one doesn't move code around? It'll look a tiny bit odd there of course..

That is the issue I think the oddness is easier to defend here vs having it in
the previous patch.

>
> > + if (pl_nr > 0) {
> > u16 nr_rec = min_t(u16, pl_nr, CXL_GET_EVENT_NR_RECORDS);
> > int i;
> >
> > - for (i = 0; i < nr_rec; i++)
> > - trace_cxl_generic_event(dev_name(cxlds->dev),
> > - type,
> > - &payload.record[i]);
> > + if (trace_cxl_generic_event_enabled()) {
> > + for (i = 0; i < nr_rec; i++)
> > + trace_cxl_generic_event(dev_name(cxlds->dev),
> > + type,
> > + &payload.record[i]);
> > + }
> > +
> > + rc = cxl_clear_event_record(cxlds, type, &payload, nr_rec);
> > + if (rc) {
> > + dev_err(cxlds->dev, "Event log '%s': Failed to clear events : %d",
> > + cxl_event_log_type_str(type), rc);
> > + return;
> > + }
> > }
> >
>
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index da64ba0f156b..28a114c7cf69 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
>
> >
> > +/*
> > + * Clear Event Records input payload
> > + * CXL rev 3.0 section 8.2.9.2.3; Table 8-51
> > + *
> > + * Space given for 1 record
>
> Nope...

<sigh> yep... ;-)

>
>
> > + */
> > +struct cxl_mbox_clear_event_payload {
> > + u8 event_log; /* enum cxl_event_log_type */
> > + u8 clear_flags;
> > + u8 nr_recs; /* 1 for this struct */
> Nope :) Delete the comments so they can't be wrong if this changes in future!

Yep. :-/

Ira