RE: [ndctl PATCH v2 2/4] libndctl: NVDIMM_FAMILY_HYPERV: add .smart_get_shutdown_count (Function 2)
> From: Verma, Vishal L
> Sent: Wednesday, March 20, 2019 6:34 PM
> > ...
> > My feeling is that it's not very good to directly call ndctl_cmd_submit(),
> > but by doing this we don't need to make any change to the common code,
> and
> > I'm unsure if it's good to change the common code just for Hyper-V.
>
> I thought about this, and this approach seems reasonable to me. I agree
> that there is not precedent for submitting a command from the various
> family libraries, but I think the way dimm-ops are structured, this
> seems warranted. The way I see it is a dimm op means "get X information
> for this family", and however it needs to be obtained is just a detail
> specific to that DSM family. For intel it happens to be in the smart
> information, but if it happens to be a separate DSM, that is also fine.
Thanks! I'm gald you think this is fine. :-)
> I do think this commentary in the changelog can be reworded. Consider
> changing the first-person narrative to a more 'informational' or
> 'imperative' one. For example: "The 'smart_get_shutdown_count' dimm-op
> is expected to return a shutdown count, but for the HYPERV family, this
> is only available from a separate DSM. Perform a command submission in
> the 'hyperv_cmd_smart_get_shutdown_count' dimm op to retrieve this, so
> that a smart health listing can display this information"
Will fix it.
> Apart from the commit message comments in the previous patch, also
> consider wrapping commit messages to a 72 column width [1].
Thanks for another good read! I'll fix it.
> > -static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm
> *dimm)
> > +static struct ndctl_cmd *hyperv_dimm_cmd_new_cmd(struct ndctl_dimm
> *dimm,
> > +unsigned int command)
>
> The 'new' in the function name is a bit confusing, as 'new' functions
> are also used for cmd allocation. I'd suggest following the intel.c
> convention and calling it 'alloc_hyperv_cmd'.
Will fix it.
> Maybe consider calling it this right from the start in patch 1, and also
> having the wrapper for the new smart command in patch 1 - this way there
> is less unrelated thrash in this patch, and makes it easier to review.
Ok. Will do.
> > -static int hyperv_smart_valid(struct ndctl_cmd *cmd)
> > +static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm
> *dimm)
> Similar comment as above. In general this sort of refactoring can be
> done in two ways:
>
>1. If you know the end result at the start, just create the generic
>helpers then, so future patches don't have the thrash.
>
>2. If the changes are in prior code, and if the refactoring is
>extensive, split it out into its own functionally equivalent patch,
>so that the meat of *this* change is not cluttered by unrelated
>refactoring.
Will fix it.
> > static int hyperv_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
> > {
> > return CMD_HYPERV_STATUS(cmd) == 0 ? 0 : -EINVAL;
> > }
> >
> > +static int hyperv_get_shutdown_count(struct ndctl_cmd *cmd,
> > +unsigned int *count)
>
> No need to split this line, it fits within 80 columns.
Ok. Will fix it.
> > +{
> > + unsigned int command = ND_HYPERV_CMD_GET_SHUTDOWN_INFO;
> > + struct ndctl_cmd *cmd_get_shutdown_info;
> > + int rc;
> > +
> > + cmd_get_shutdown_info = hyperv_dimm_cmd_new_cmd(cmd->dimm,
> command);
> > + if (!cmd_get_shutdown_info)
> > + return -EINVAL;
> > +
> > + if (ndctl_cmd_submit(cmd_get_shutdown_info) < 0 ||
>
> The return value from ndctl_cmd_submit() only indicates that the kernel
> was successfully able to send the DSM to the platform firmware. It does
> *not* capture any failures indicated by the platform in the 'status'
> field of the cmd struct. We should be ensuring that was also successful
> by calling the hyperv_cmd_xlat_firmware_status for the cmd.
> Alternatively, instead of the ndctl_cmd_submit() API, you could also use
> the newly added (in ndctl-64) ndctl_cmd_submit_xlat() API, which calls
> the dimm-op for 'xlat_firmware_status' if present to do a status
> translation, and present it in the return value.
Will fix it.
> > +static unsigned int hyperv_cmd_smart_get_shutdown_count(struct
> ndctl_cmd *cmd)
> > +{
> > + unsigned int count;
> > +
> > + return hyperv_get_shutdown_count(cmd, ) == 0 ? count :
> UINT_MAX;
>
> I'd prefer the long form rc = func(); if (rc) .. here.
> Also, shouldn't we set errno appropriately in the UINT_MAX case?
Will fix it.
Thanks,
-- Dexuan
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl PATCH v2 2/4] libndctl: NVDIMM_FAMILY_HYPERV: add .smart_get_shutdown_count (Function 2)
On Wed, 2019-02-20 at 05:10 +, Dexuan Cui wrote:
> With the patch, "ndctl list --dimms --health --idle" can show
> "shutdown_count" now, e.g.
>
> {
> "dev":"nmem0",
> "id":"04d5-01-1701-",
> "handle":0,
> "phys_id":0,
> "health":{
> "health_state":"ok",
> "shutdown_count":2
> }
> }
>
> The patch has to directly call ndctl_cmd_submit() in
> hyperv_cmd_smart_get_flags() and hyperv_cmd_smart_get_shutdown_count() to
> get the needed info, because util_dimm_health_to_json() only submits *one*
> command, and unluckily for Hyper-V Virtual NVDIMM we need to call both
> Function 1 and 2 to get the needed info.
>
> My feeling is that it's not very good to directly call ndctl_cmd_submit(),
> but by doing this we don't need to make any change to the common code, and
> I'm unsure if it's good to change the common code just for Hyper-V.
I thought about this, and this approach seems reasonable to me. I agree
that there is not precedent for submitting a command from the various
family libraries, but I think the way dimm-ops are structured, this
seems warranted. The way I see it is a dimm op means "get X information
for this family", and however it needs to be obtained is just a detail
specific to that DSM family. For intel it happens to be in the smart
information, but if it happens to be a separate DSM, that is also fine.
I do think this commentary in the changelog can be reworded. Consider
changing the first-person narrative to a more 'informational' or
'imperative' one. For example: "The 'smart_get_shutdown_count' dimm-op
is expected to return a shutdown count, but for the HYPERV family, this
is only available from a separate DSM. Perform a command submission in
the 'hyperv_cmd_smart_get_shutdown_count' dimm op to retrieve this, so
that a smart health listing can display this information"
Apart from the commit message comments in the previous patch, also
consider wrapping commit messages to a 72 column width [1].
[1]: https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
>
> Signed-off-by: Dexuan Cui
> ---
> ndctl/lib/hyperv.c | 62 --
> ndctl/lib/hyperv.h | 7 ++
> 2 files changed, 62 insertions(+), 7 deletions(-)
>
> diff --git a/ndctl/lib/hyperv.c b/ndctl/lib/hyperv.c
> index b303d50..e8ec142 100644
> --- a/ndctl/lib/hyperv.c
> +++ b/ndctl/lib/hyperv.c
> @@ -22,7 +22,8 @@
> #define CMD_HYPERV_STATUS(_c) (CMD_HYPERV(_c)->u.status)
> #define CMD_HYPERV_SMART_DATA(_c) (CMD_HYPERV(_c)->u.smart.data)
>
> -static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
> +static struct ndctl_cmd *hyperv_dimm_cmd_new_cmd(struct ndctl_dimm *dimm,
> + unsigned int command)
The 'new' in the function name is a bit confusing, as 'new' functions
are also used for cmd allocation. I'd suggest following the intel.c
convention and calling it 'alloc_hyperv_cmd'.
Maybe consider calling it this right from the start in patch 1, and also
having the wrapper for the new smart command in patch 1 - this way there
is less unrelated thrash in this patch, and makes it easier to review.
> {
> struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
> struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
> @@ -35,8 +36,7 @@ static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct
> ndctl_dimm *dimm)
> return NULL;
> }
>
> - if (test_dimm_dsm(dimm, ND_HYPERV_CMD_GET_HEALTH_INFO) ==
> - DIMM_DSM_UNSUPPORTED) {
> + if (test_dimm_dsm(dimm, command) == DIMM_DSM_UNSUPPORTED) {
> dbg(ctx, "unsupported function\n");
> return NULL;
> }
> @@ -54,7 +54,7 @@ static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct
> ndctl_dimm *dimm)
>
> hyperv = CMD_HYPERV(cmd);
> hyperv->gen.nd_family = NVDIMM_FAMILY_HYPERV;
> - hyperv->gen.nd_command = ND_HYPERV_CMD_GET_HEALTH_INFO;
> + hyperv->gen.nd_command = command;
> hyperv->gen.nd_fw_size = 0;
> hyperv->gen.nd_size_in = offsetof(struct nd_hyperv_smart, status);
> hyperv->gen.nd_size_out = sizeof(hyperv->u.smart);
> @@ -65,34 +65,74 @@ static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct
> ndctl_dimm *dimm)
> return cmd;
> }
>
> -static int hyperv_smart_valid(struct ndctl_cmd *cmd)
> +static struct ndctl_cmd *hyperv_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
> +{
> + return hyperv_dimm_cmd_new_cmd(dimm, ND_HYPERV_CMD_GET_HEALTH_INFO);
> +}
> +
> +static int hyperv_cmd_valid(struct ndctl_cmd *cmd, unsigned int command)
> {
> if (cmd->type != ND_CMD_CALL ||
> cmd->size != sizeof(*cmd) + sizeof(struct nd_pkg_hyperv) ||
> CMD_HYPERV(cmd)->gen.nd_family != NVDIMM_FAMILY_HYPERV ||
> - CMD_HYPERV(cmd)->gen.nd_command != ND_HYPERV_CMD_GET_HEALTH_INFO ||
> + CMD_HYPERV(cmd)->gen.nd_command != command ||
> cmd->status != 0 ||
>
