Re: [RFC/PATCH] ndctl list should show more hardware information

[Linda Knippers <linda.knippers@hpe.com>]

On 07/05/2017 01:23 PM, Dan Williams wrote:
> On Wed, Jul 5, 2017 at 2:12 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
>> Hello,
>>
>>    The current "id" data of dimm (ndctl list -D) shows the information
>>    of DIMM module vendor or serial number.  However, I think it is not enough.
>>
>>    If a NVDIMM becomes broken, users need to know its physical place
>>    rather than vendor or serial number to replace the NVDIMM.
>>
>>    There are 2 candidate of such information.
>>      a) NFIT Device Handle (_ADR of the NVDIMM device)
>>         This date includes node controller id and socket id.
>>      b) NVDIMM Physical ID (Handle for the SMBIOS Memory Device (Type 17)).
>>         The dmidecode can show this handle with more information like "Locator"
>>         So, user can find the location of NVDIMM.
>>
>>    At first, I think _ADR is enough information.
>>
>>    However, when I talked with our firmware team about this requirement,
>>    they said it may not be good data, because the node contoller ID is not
>>    stable on our server due to "partitioning" feature.
>>    (Our server can have plural partition on one box, and may have plural
>>     node 0.)
>>
>>    So, I make the ndctl shows not only NFIT Device Handle but also
>>    NVDIMM Physical ID. Then, user can find the location with dmidecode.
>>
>>    This is the first trial patch. So any comments are welcome.
> 
> This looks ok to me. The only change I would request is to not assume
> that all NVDIMMs have this NFIT-specific data available.
> 
>>
>> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
>> ---
>> diff --git a/util/json.c b/util/json.c
>> index b718d74..749a358 100644
>> --- a/util/json.c
>> +++ b/util/json.c
>> @@ -68,6 +68,9 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm)
>>  {
>>         struct json_object *jdimm = json_object_new_object();
>>         const char *id = ndctl_dimm_get_unique_id(dimm);
>> +       unsigned int handle = ndctl_dimm_get_handle(dimm);
>> +       short phys_id = ndctl_dimm_get_phys_id(dimm);

Should phys_id be an unsigned short?

>> +       char buf[11];
>>         struct json_object *jobj;
>>
>>         if (!jdimm)
>> @@ -85,6 +88,18 @@ struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm)
>>                 json_object_object_add(jdimm, "id", jobj);
>>         }
>>
>> +       snprintf(buf, 11, "0x%08x", handle);
>> +       jobj = json_object_new_string(buf);
>> +       if (!jobj)
>> +               goto err;
>> +       json_object_object_add(jdimm, "handle", jobj);
> 
> For example, this should be in a "if (handle < UINIT_MAX)" branch...

Is that check actually necessary when handle is an unsigned int?  Is there any illegal value?
> 
>> +
>> +       snprintf(buf, 7, "0x%04x", phys_id );
>> +       jobj = json_object_new_string(buf);
>> +       if (!jobj)
>> +               goto err;
>> +       json_object_object_add(jdimm, "phys_id", jobj);
>> +
> 
> ..., and this should be in a "if (phys < USHORT_MAX)" branch.

Same question here.  What's an illegal value?
> 
> Lastly, I think these should be json_object_new_int() types since they
> are numbers not strings.

Is it possible to get hex values if they're numbers?  For the device handle, hex is
helpful.

-- ljk

> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
> 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm