Re: [PATCH v2 0/2] Add log related mailbox commands

[Dan Williams <dan.j.williams@intel.com>]

Any specific reason to include linux-mm on this enabling? I suspect this
topic is only of interest to linux-cxl. Lets drop linux-mm from a v3
posting.

sthanneeru.opensrc@ wrote:
> From: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
> 
> Add support to expose following mailbox commands to userspace
> for clearing and populating the Vendor debug log and
> Component State dump log in certain scenarios,
> allowing for the aggregation of results over time.

What is missing for me here is why the ioctl() ABI is suitable for both
of these. The Vendor Debug Log seems straightforward to enable via
ioctl() since there is no background operation entanglement, no
population complexity, and no reference to other payloads.

The Component State Dump has several caveats in my mind for ioctl() not
being a suitable ABI:

1/ Populate Log has an unreasonable expectation that the submitter can
monopolize the mailbox indefinitely. At least that can be solved by
Linux only supporting automatically populated Component State Dump logs.

2/ Automatic log population requires the caller to handle races with
auto-population. The kernel need not export that complexity to
userspace. This is not fatal to the proposal, but it has interactions
with caveat 3.

3/ The Component State Dump format wants to reference events in the
Event Log, if the Event Log has been cleared then, per the spec, the
Component State Dump must not reference the Event Handle. To me that
implies that the code that clears event records must be careful to not
destroy linkage to component state information. That suggests that the
proper place to dump the component state log is an addendum to the
current trace events, before that code clears the event record.

Something roughly like this:

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 9adda4795eb7..498b2a0b3e76 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -941,7 +941,8 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
 }
 
 static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
-                                   enum cxl_event_log_type type)
+                                   enum cxl_event_log_type type,
+                                   struct cxl_component_state_dump *csd)
 {
        struct cxl_memdev *cxlmd = mds->cxlds.cxlmd;
        struct device *dev = mds->cxlds.dev;
@@ -977,9 +978,12 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
                if (!nr_rec)
                        break;
 
-               for (i = 0; i < nr_rec; i++)
+               for (i = 0; i < nr_rec; i++) {
                        __cxl_event_trace_record(cxlmd, type,
                                                 &payload->records[i]);
+                       if (is_event_referenced(csd, type, &payload->records[i]))
+                               trace_csd(csd, ...);
+               }
 
                if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
                        trace_cxl_overflow(cxlmd, type, payload);

...but in general this cover letter needs to comment on the long term
suitability of the ABI.