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

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

[Yasunori Goto]

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.

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);
+	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);
+
+	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);
+
 	if (!ndctl_dimm_is_enabled(dimm)) {
 		jobj = json_object_new_string("disabled");
 		if (!jobj)

---
Yasunori Goto

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

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

[Dan Williams]

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);
> +       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...

> +
> +       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.

Lastly, I think these should be json_object_new_int() types since they
are numbers not strings.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Linda Knippers]

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

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

[Dan Williams]

On Wed, Jul 5, 2017 at 11:08 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
> 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?

Yes, "NVDIMM Physical ID" is defined as a 16-bit field in the NFIT.

>
>>> +       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?

Yes, if the DIMM is not defined by NFIT then we initialize the value
to -1 since only ACPI defined DIMMs have handles. I think UINT_MAX is
a safe value for an invalid handle. Otherwise, if you parse out
UINT_MAX as a valid handle, that indicates the last DIMM on 4096 node
system. If such a system is possible we can also use
ndctl_bus_has_nfit() to gate whether the NFIT-specific properties of a
DIMM are available.

>>
>>> +
>>> +       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?

I think it is safe to assume that USHORT_MAX is invalid, but we can
again use the ndctl_bus_has_nfit() if this might be a problem on some
platform.

>> 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.

Yes, that would be great. Last I checked the "json-c" library that
ndctl is using did not support this, but I don't think anything
prevents json-c from growing this support. I.e. as far as I know it
supports hex numbers for input, so I think it is valid json either
way.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Yasunori Goto]

> >>>    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?
> 
> Yes, "NVDIMM Physical ID" is defined as a 16-bit field in the NFIT.

Ah, OK. I'll fix it.


> 
> >
> >>> +       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?
> 
> Yes, if the DIMM is not defined by NFIT then we initialize the value
> to -1 since only ACPI defined DIMMs have handles. I think UINT_MAX is
> a safe value for an invalid handle. Otherwise, if you parse out
> UINT_MAX as a valid handle, that indicates the last DIMM on 4096 node
> system. If such a system is possible we can also use
> ndctl_bus_has_nfit() to gate whether the NFIT-specific properties of a
> DIMM are available.

Ok. 

> >>> +       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?
> 
> I think it is safe to assume that USHORT_MAX is invalid, but we can
> again use the ndctl_bus_has_nfit() if this might be a problem on some
> platform.

Ok.

> 
> >> 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.
> 
> Yes, that would be great. Last I checked the "json-c" library that
> ndctl is using did not support this, but I don't think anything
> prevents json-c from growing this support. I.e. as far as I know it
> supports hex numbers for input, so I think it is valid json either
> way.

Sorry, maybe I could not follow here. Please let me confirm.
In conclusion, json_object_new_int() should be used at here?

Since I tried to output hex values for user to compare with dmidecode's output easily,
I translated to hex strings at here. 
But, I should json_object_new_int() in conclusion, and should expect that json-c
library will be able to output hex value in future?

Bye,

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

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

[Dan Williams]

On Wed, Jul 5, 2017 at 9:13 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
[..]
>> Yes, that would be great. Last I checked the "json-c" library that
>> ndctl is using did not support this, but I don't think anything
>> prevents json-c from growing this support. I.e. as far as I know it
>> supports hex numbers for input, so I think it is valid json either
>> way.
>
> Sorry, maybe I could not follow here. Please let me confirm.
> In conclusion, json_object_new_int() should be used at here?
>

Yes.

> Since I tried to output hex values for user to compare with dmidecode's output easily,
> I translated to hex strings at here.
> But, I should json_object_new_int() in conclusion, and should expect that json-c
> library will be able to output hex value in future?

I checked, it seems hex output is not valid json. However, I think
what I will do is add a "--hex" option to "ndctl list" that will make
util_display_json_array() manually print the json output object and
convert all integers to hex in the process. While that option will
make the output not machine readable by standards compliant json
parsers it will make it easier for humans to read in some cases.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Yasunori Goto]

> On Wed, Jul 5, 2017 at 9:13 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> [..]
> >> Yes, that would be great. Last I checked the "json-c" library that
> >> ndctl is using did not support this, but I don't think anything
> >> prevents json-c from growing this support. I.e. as far as I know it
> >> supports hex numbers for input, so I think it is valid json either
> >> way.
> >
> > Sorry, maybe I could not follow here. Please let me confirm.
> > In conclusion, json_object_new_int() should be used at here?
> >
> 
> Yes.
> 
> > Since I tried to output hex values for user to compare with dmidecode's output easily,
> > I translated to hex strings at here.
> > But, I should json_object_new_int() in conclusion, and should expect that json-c
> > library will be able to output hex value in future?
> 
> I checked, it seems hex output is not valid json. However, I think
> what I will do is add a "--hex" option to "ndctl list" that will make
> util_display_json_array() manually print the json output object and
> convert all integers to hex in the process. While that option will
> make the output not machine readable by standards compliant json
> parsers it will make it easier for humans to read in some cases.

Ok! I understood!
Thank you for your comments.

Bye,


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

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

[Linda Knippers]

On 7/6/2017 1:41 AM, Dan Williams wrote:
> On Wed, Jul 5, 2017 at 9:13 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> [..]
>>> Yes, that would be great. Last I checked the "json-c" library that
>>> ndctl is using did not support this, but I don't think anything
>>> prevents json-c from growing this support. I.e. as far as I know it
>>> supports hex numbers for input, so I think it is valid json either
>>> way.
>>
>> Sorry, maybe I could not follow here. Please let me confirm.
>> In conclusion, json_object_new_int() should be used at here?
>>
>
> Yes.
>
>> Since I tried to output hex values for user to compare with dmidecode's output easily,
>> I translated to hex strings at here.
>> But, I should json_object_new_int() in conclusion, and should expect that json-c
>> library will be able to output hex value in future?
>
> I checked, it seems hex output is not valid json. However, I think
> what I will do is add a "--hex" option to "ndctl list" that will make
> util_display_json_array() manually print the json output object and
> convert all integers to hex in the process. While that option will
> make the output not machine readable by standards compliant json
> parsers it will make it easier for humans to read in some cases.

Will that convert all integers to hex if that option is used?  Some values
are more useful as decimal and some are more useful as hex.  In this case
where the value is never really useful as decimal, I thought the original
code did pretty well.  I'd prefer that to yet another option.

-- ljk


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

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

[Dan Williams]

On Thu, Jul 6, 2017 at 6:31 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>
>
> On 7/6/2017 1:41 AM, Dan Williams wrote:
>>
>> On Wed, Jul 5, 2017 at 9:13 PM, Yasunori Goto <y-goto@jp.fujitsu.com>
>> wrote:
>> [..]
>>>>
>>>> Yes, that would be great. Last I checked the "json-c" library that
>>>> ndctl is using did not support this, but I don't think anything
>>>> prevents json-c from growing this support. I.e. as far as I know it
>>>> supports hex numbers for input, so I think it is valid json either
>>>> way.
>>>
>>>
>>> Sorry, maybe I could not follow here. Please let me confirm.
>>> In conclusion, json_object_new_int() should be used at here?
>>>
>>
>> Yes.
>>
>>> Since I tried to output hex values for user to compare with dmidecode's
>>> output easily,
>>> I translated to hex strings at here.
>>> But, I should json_object_new_int() in conclusion, and should expect that
>>> json-c
>>> library will be able to output hex value in future?
>>
>>
>> I checked, it seems hex output is not valid json. However, I think
>> what I will do is add a "--hex" option to "ndctl list" that will make
>> util_display_json_array() manually print the json output object and
>> convert all integers to hex in the process. While that option will
>> make the output not machine readable by standards compliant json
>> parsers it will make it easier for humans to read in some cases.
>
>
> Will that convert all integers to hex if that option is used?  Some values
> are more useful as decimal and some are more useful as hex.  In this case
> where the value is never really useful as decimal, I thought the original
> code did pretty well.  I'd prefer that to yet another option.

True, converting all of the integers to hex is not entirely useful.
What would be better I think is a --human option that turns sizes into
values with units i.e. "805306368000" -> "750G", but display values
like handles and ids as hex. The display conversion can be customized
with json_object_set_serializer() on a per-object / per-field basis.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm