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

[Jehoon Park <jehoon.park@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 4155 bytes --]

On Wed, Mar 08, 2023 at 01:58:11PM -0800, Alison Schofield wrote:
> 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

The idea adding new list option “—identify” to display raw data from Identify
looks good! Providing proper fields to other options will be helpful for users,
however, I think it may be covered by different patchset after basic support
for Identify command is fully handled.

I will revise this patchset by applying your valuable comments. Thank you.

Jehoon

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]