Messages in this thread Patch in this message | | | Date | Fri, 2 Dec 2022 13:18:36 +0000 | From | Jonathan Cameron <> | Subject | Re: [PATCH V2 03/11] cxl/mem: Implement Clear Event Records command |
| |
> > +static int cxl_clear_event_record(struct cxl_dev_state *cxlds, > > + enum cxl_event_log_type log, > > + struct cxl_get_event_payload *get_pl, > > + u16 total) > > +{ > > + struct cxl_mbox_clear_event_payload payload = { > > + .event_log = log, > > + }; > > + int cnt; > > + > > + /* > > + * Clear Event Records uses u8 for the handle cnt while Get Event > > + * Record can return up to 0xffff records. > > + */ > > + for (cnt = 0; cnt < total; /* cnt incremented internally */) { > > + u8 nr_recs = min_t(u8, (total - cnt), > > + CXL_CLEAR_EVENT_MAX_HANDLES); > > This seems overly complicated. @total is a duplicate of > @get_pl->record_count, and the 2 loops feel like it could be cut > down to one.
You could do something nasty like for (i = 0; i < total; i++) {
... payload.handle[i % CLEAR_EVENT_MAX_HANDLES] = ... if (i % CXL_CLEAR_EVENT_MAX_HANDLES == CXL_CLEAR_EVENT_MAX_HANDLE - 1) { send command. } }
but that looks worse to me than the double loop.
Making outer loop for (j = 0; j <= total / CXL_CLEAR_EVENT_MAX_HANDLES; j++) might bet clearer but then you'd have to do records[j * CXL_CLEAR_EVENT_MAX_HANDLES + i] which isn't nice.
Ah well, Ira gets to try and find a happy compromise.
...
> > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h > > index 70459be5bdd4..7c1ad8062792 100644 > > --- a/include/uapi/linux/cxl_mem.h > > +++ b/include/uapi/linux/cxl_mem.h > > @@ -25,6 +25,7 @@ > > ___C(RAW, "Raw device command"), \ > > ___C(GET_SUPPORTED_LOGS, "Get Supported Logs"), \ > > ___C(GET_EVENT_RECORD, "Get Event Record"), \ > > + ___C(CLEAR_EVENT_RECORD, "Clear Event Record"), \ > > ___C(GET_FW_INFO, "Get FW Info"), \ > > ___C(GET_PARTITION_INFO, "Get Partition Information"), \ > > ___C(GET_LSA, "Get Label Storage Area"), \ > > Same, "yikes" / "must be at the end of the enum" feedback.
Macro magic makes that non obvious.. Not that I'd ever said I thought this trick was a bad idea ;)
| |