Messages in this thread Patch in this message | | | Date | Wed, 16 Nov 2022 17:07:28 -0800 | From | Ira Weiny <> | Subject | Re: [PATCH 03/11] cxl/mem: Implement Clear Event Records command |
| |
On Wed, Nov 16, 2022 at 03:24:26PM +0000, Jonathan Cameron wrote: > On Thu, 10 Nov 2022 10:57:50 -0800 > ira.weiny@intel.com wrote: > > > From: Ira Weiny <ira.weiny@intel.com> > > > > 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@huawei.com> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > 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
| |