On Fri, Jun 5, 2020 at 8:22 AM Vaibhav Jain <vaib...@linux.ibm.com> wrote: [..] > > Oh, why not define a maximal health payload with all the attributes > > 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 > > intel_cmd_smart_get_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 "hpe" > > 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 > nd_papr_pdsm_health' > * 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 > ndctl_cmd_smart_get_flags(). > > Implementing first 3 items above in the current patchset should be > fairly trivial. > > Does that sounds reasonable ?
This sounds good to me.
