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

Jonathan Cameron
2022-12-02T13:18:57Z

> > +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 ;)