Re: [ndctl PATCH v11 6/7] cxl/list: add --media-errors option to cxl list

[Alison Schofield <alison.schofield@intel.com>]

On Thu, Mar 14, 2024 at 08:35:01PM -0700, Dan Williams wrote:
> Alison Schofield wrote:
> > On Fri, Mar 15, 2024 at 10:09:44AM +0900, Wonjae Lee wrote:
> > > alison.schofield@intel.com wrote:
> > > > From: Alison Schofield <alison.schofield@intel.com>
> > > >
> > > > The --media-errors option to 'cxl list' retrieves poison lists from
> > > > memory devices supporting the capability and displays the returned
> > > > media_error records in the cxl list json. This option can apply to
> > > > memdevs or regions.
> > > >
> > > > Include media-errors in the -vvv verbose option.
> > > >
> > > > Example usage in the Documentation/cxl/cxl-list.txt update.
> > > >
> > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > > ---
> > > > Documentation/cxl/cxl-list.txt 62 +++++++++++++++++++++++++++++++++-
> > > > cxl/filter.h                    3 ++
> > > > cxl/list.c                      3 ++
> > > > 3 files changed, 67 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> > > > index 838de4086678..6d3ef92c29e8 100644
> > > > --- a/Documentation/cxl/cxl-list.txt
> > > > +++ b/Documentation/cxl/cxl-list.txt
> > > 
> > > [snip]
> > > 
> > > +----
> > > +In the above example, region mappings can be found using:
> > > +"cxl list -p mem9 --decoders"
> > > +----
> > > 
> > > Hi, isn't it '-m mem9' instead of -p? FYI, it's also on patch's
> > > cover letter, too.
> > 
> > Thanks for the review! I went with -p because it gives only
> > the endpoint decoder while -m gives all the decoders up to
> > the root - more than needed to discover the region.
> 
> The first thing that comes to mind to list memory devices with their
> decoders is:
> 
>     cxl list -MD -d endpoint
> 
> ...however the problem is that endpoint ports connect memdevs to their
> parent port, so the above results in:
> 
>   Warning: no matching devices found
> 
> I think I want to special case "-d endpoint" when both -M and -D are
> specified to also imply -E, "endpoint ports". However that also seems to
> have a bug at present:
> 
> # cxl list -EDM -d endpoint -iu
> {
>   "endpoint":"endpoint2",
>   "host":"mem0",
>   "parent_dport":"0000:34:00.0",
>   "depth":2
> }
> 
> That needs to be fixed up to merge:
> 
> # cxl list -ED -d endpoint -iu
> {
>   "endpoint":"endpoint2",
>   "host":"mem0",
>   "parent_dport":"0000:34:00.0",
>   "depth":2,
>   "decoders:endpoint2":[
>     {
>       "decoder":"decoder2.0",
>       "interleave_ways":1,
>       "state":"disabled"
>     }
>   ]
> }
> 
> ...and:
> 
> # cxl list -EMu
> {
>   "endpoint":"endpoint2",
>   "host":"mem0",
>   "parent_dport":"0000:34:00.0",
>   "depth":2,
>   "memdev":{
>     "memdev":"mem0",
>     "pmem_size":"512.00 MiB (536.87 MB)",
>     "serial":"0",
>     "host":"0000:35:00.0"
>   }
> }
> 
> ...so that one can get a nice listing of just endpoint ports, their
> decoders (with media errors) and their memdevs.
> 
> The reason that "cxl list -p mem9 -D" works is subtle because it filters
> the endpoint decoders by an endpoint port filter, but I think most users
> would expect to not need to enable endpoint-port listings to see their
> decoders the natural key to filter endpoint decoders is by memdev.

Wonjae, Dan,

This feedback inspires me to seek more input from future users. This
tool should be adding a convenience and I don't want to proceed without
more user feedback confirming this implementation is more convenient
than the currently available method (trace & trigger). We also want to
avoid working with or around some awkward json output for eternity.

I'm following this response with a reply to the cover letter seeking
more inputs.

Thanks,
Alison