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

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

On Wed, Mar 08, 2023 at 10:22:41AM -0800, Ira Weiny wrote:
> 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.
>
That's a good way to add it. That'll give you all the fields in one
place, and then, we can still think about if we want to spit out
specific fields (ie poison related) when we are doing a poison command.
You will have added all the libcxl accessors, making those easier to add.
Alison


> Ira