On Fri, Jun 5, 2020 at 8:22 AM Vaibhav Jain <vaibhav(a)linux.ibm.com> wrote:
> Oh, why not define a maximal health payload with all the
> you know about today, leave some room for future expansion, and then
> report a validity flag for each attribute? This is how the "intel"
> smart-health payload works. If they ever needed to extend the payload
> they would increase the size and add more validity flags. Old
> userspace never groks the new fields, new userspace knows to ask for
> and parse the larger payload.
> See the flags field in 'struct nd_intel_smart' (in ndctl) and the
> translation of those flags to ndctl generic attribute flags
> In general I'd like ndctl to understand the superset of all health
> attributes across all vendors. For the truly vendor specific ones it
> would mean that the health flags with a specific "papr_scm" back-end
> just would never be set on an "intel" device. I.e. look at the
> and "msft" health backends. They only set a subset of the valid flags
> that could be reported.
Thanks, this sounds good. Infact papr_scm implementation in ndctl does
advertises support for only a subset of ND_SMART_* flags right now.
Using 'flags' instead of 'version' was indeed discussed during
v7..v9. However re-looking at the 'msft' and 'hpe' implementations the
approach of maximal health payload tagged with a flags field looks more
intuitive and I would prefer implementing this scheme in this patch-set.
The current set health data exchanged with between libndctl and
papr_scm via 'struct nd_papr_pdsm_health' (e.g various health status
bits , nvdimm arming status etc) are guaranteed to be always available
hence associating their availability with a flag wont be much useful as
the flag will be always set.
However as you suggested, extending the 'struct nd_papr_pdsm_health' in
future to accommodate new attributes like 'life-remaining' can be done
via adding them to the end of the struct and setting a flag field to
indicate its presence.
So I have the following proposal:
* Add a new '__u32 extension_flags' field at beginning of 'struct
* Set the size of the struct to 184-bytes which is the maximum possible
size for a pdsm payload.
* 'papr_scm' kernel driver will currently set 'extension_flag' to 0
indicating no extension fields.
* Future patch that adds support for 'life-remaining' add the new-field
at the end of known fields in 'struct nd_papr_pdsm_health'.
* When provided to papr_scm kernel module, if 'life-remaining' data is
available its populated and corresponding flag set in
'extension_flags' field indicating its presence.
* When received by libndctl papr_scm implementation its tests if the
extension_flags have associated 'life-remaining' flag set and if yes
then return ND_SMART_USED_VALID flag back from
Implementing first 3 items above in the current patchset should be
Does that sounds reasonable ?
This sounds good to me.