Re: [ndctl patch RFC 0/2] add support for IDENTIFY command

[Ira Weiny <ira.weiny@intel.com>]

Jehoon Park wrote:
> On Tue, Mar 07, 2023 at 12:18:38PM -0800, Alison Schofield wrote:
> > On Tue, Mar 07, 2023 at 05:21:00PM +0900, Jehoon Park wrote:
> > > From: jehoon park <jehoon.park@samsung.com>
> > > 
> > > This patchset supports CXL IDENTIFY mailbox command and corresponding
> > > cxl tool interface command.
> > > 
> > > CXL 3.0 Spec 8.2.9.8.1 defines IDENTIFY command which retrieves basic
> > > information about CXL memory device. The information consist of device's
> > > firmware version, capacity, LSA size, event log size, poison list size,
> > > inject poison limit, poison handling capabilities and QoS telemetry
> > > capabilities. Firmware version, capacity and LSA size are already supported
> > > and used for partition commands or sysfs attributes while others are not.
> > > Since patches about event log [1] and poison [2] are discussed recently,
> > > support for those information will be helpful.
> > 
> > Hi Jehoon,
> > 
> > Does this need to be a separate command? Identify fields can be included
> > in cxl list options. For example, the -I option to cxl list, issues the
> > identify command and includes the partition related entries in that json
> > output.
> > 
> > There are other identify fields that need to be picked up, like the
> > poison related fields.  They need to be added to the cxl list 
> > options. We may want to include some when we list the poison, and
> > some as an option in the memdev listing.
> > 
> > Is there some reasoning behind separating this out? If not, can we look
> > to add the missing fields to the various cxl-list options and add
> > new cxl-list options where needed?
> > 
> > Alison
> > 
> 
> Hi Alison, thank you for comments.
> 
> I suggested separate identify command since it retrieves basic information
> about memdev. Since cxl-list command lists all cxl objects, I intended to
> focus memdev information by separating it. Also, I referred to nvme-cli
> which has id-ctrl and id-ns commands.
> 
> However, as you commented, some fields were already included in cxl-list.
> I think the idea that providing information to proper listing option also
> makes sense.
> 
> Then, by following the approach, including fields to cxl-list options,
> identify fields could be included like below. Do they look fine?
> 
> 1. FW version and LSA size are included when listing memdev. ("list -m memdev")
> 2. For poison related fields (poison_list_max size and inject_poison_limit),
> include them when listing poison. ("--media-errors" option, patch [1])
> 3. For capabilities fields, add new option "-C, --capabilities" to the
> memdev listing. (I see there exists same option for listing nvdimm device)
> 
> However, I'm confused about event_log_size fields. Could they be included
> in capabilities option too? or require new option like "--event"?

Fundamentally why does user space need to know the event log sizes?

I do like the idea of getting the 'raw' results of the identify command in
it's entirety.

What if list has an '--identify' option which adds the list of Identify
values as a child json object.

Ira