diff mbox series

[ndctl,1/2,v2,RESEND] libndctl/msft: Cleanup the code

Message ID 20230113172732.1122643-1-mav@ixsystems.com (mailing list archive)
State New, archived
Headers show
Series [ndctl,1/2,v2,RESEND] libndctl/msft: Cleanup the code | expand

Commit Message

Alexander Motin Jan. 13, 2023, 5:27 p.m. UTC
Clean up the code, making it more uniform with others and
allowing more methods to be implemented later:
 - remove nonsense NDN_MSFT_CMD_SMART definition, replacing it
with real commands, primarity NDN_MSFT_CMD_NHEALTH;
 - allow sending arbitrary commands and add their descriptions;
 - add custom cmd_is_supported method to allow monitor mode.

Signed-off-by: Alexander Motin <mav@ixsystems.com>
---
 ndctl/lib/msft.c | 58 ++++++++++++++++++++++++++++++++++++++----------
 ndctl/lib/msft.h | 13 ++++-------
 2 files changed, 50 insertions(+), 21 deletions(-)

Comments

Verma, Vishal L Feb. 15, 2023, 5 a.m. UTC | #1
On Fri, 2023-01-13 at 12:27 -0500, Alexander Motin wrote:
> Clean up the code, making it more uniform with others and
> allowing more methods to be implemented later:
>  - remove nonsense NDN_MSFT_CMD_SMART definition, replacing it
> with real commands, primarity NDN_MSFT_CMD_NHEALTH;
>  - allow sending arbitrary commands and add their descriptions;
>  - add custom cmd_is_supported method to allow monitor mode.

Hi Alexander,

I think I had similar feedback earlier, but these would be reviewable
more easily if these three unrelated things are broken up into three
separate patches.

A few other minor comments below.

> 
> Signed-off-by: Alexander Motin <mav@ixsystems.com>
> ---
>  ndctl/lib/msft.c | 58 ++++++++++++++++++++++++++++++++++++++----------
>  ndctl/lib/msft.h | 13 ++++-------
>  2 files changed, 50 insertions(+), 21 deletions(-)
> 
> diff --git a/ndctl/lib/msft.c b/ndctl/lib/msft.c
> index 3112799..efc7fe1 100644
> --- a/ndctl/lib/msft.c
> +++ b/ndctl/lib/msft.c
> @@ -2,6 +2,7 @@
>  // Copyright (C) 2016-2017 Dell, Inc.
>  // Copyright (C) 2016 Hewlett Packard Enterprise Development LP
>  // Copyright (C) 2016-2020, Intel Corporation.
> +/* Copyright (C) 2022 iXsystems, Inc. */
>  #include <stdlib.h>
>  #include <limits.h>
>  #include <util/log.h>
> @@ -12,12 +13,39 @@
>  #define CMD_MSFT(_c) ((_c)->msft)
>  #define CMD_MSFT_SMART(_c) (CMD_MSFT(_c)->u.smart.data)
>  
> +static const char *msft_cmd_desc(int fn)
> +{
> +       static const char * const descs[] = {
> +               [NDN_MSFT_CMD_CHEALTH] = "critical_health",
> +               [NDN_MSFT_CMD_NHEALTH] = "nvdimm_health",
> +               [NDN_MSFT_CMD_EHEALTH] = "es_health",
> +       };
> +       const char *desc;
> +
> +       if (fn >= (int) ARRAY_SIZE(descs))
> +               return "unknown";
> +       desc = descs[fn];
> +       if (!desc)
> +               return "unknown";
> +       return desc;
> +}
> +
> +static bool msft_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
> +{
> +       /* Handle this separately to support monitor mode */
> +       if (cmd == ND_CMD_SMART)
> +               return true;
> +
> +       return !!(dimm->cmd_mask & (1ULL << cmd));
> +}
> +
>  static u32 msft_get_firmware_status(struct ndctl_cmd *cmd)
>  {
>         return cmd->msft->u.smart.status;
>  }
>  
> -static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
> +static struct ndctl_cmd *alloc_msft_cmd(struct ndctl_dimm *dimm,
> +               unsigned int func, size_t in_size, size_t out_size)
>  {
>         struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
>         struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
> @@ -30,12 +58,12 @@ static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
>                 return NULL;
>         }
>  
> -       if (test_dimm_dsm(dimm, NDN_MSFT_CMD_SMART) == DIMM_DSM_UNSUPPORTED) {
> +       if (test_dimm_dsm(dimm, func) == DIMM_DSM_UNSUPPORTED) {
>                 dbg(ctx, "unsupported function\n");
>                 return NULL;
>         }
>  
> -       size = sizeof(*cmd) + sizeof(struct ndn_pkg_msft);
> +       size = sizeof(*cmd) + sizeof(struct nd_cmd_pkg) + in_size + out_size;
>         cmd = calloc(1, size);
>         if (!cmd)
>                 return NULL;
> @@ -45,25 +73,30 @@ static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
>         cmd->type = ND_CMD_CALL;
>         cmd->size = size;
>         cmd->status = 1;
> +       cmd->get_firmware_status = msft_get_firmware_status;
>  
>         msft = CMD_MSFT(cmd);
>         msft->gen.nd_family = NVDIMM_FAMILY_MSFT;
> -       msft->gen.nd_command = NDN_MSFT_CMD_SMART;
> +       msft->gen.nd_command = func;
>         msft->gen.nd_fw_size = 0;
> -       msft->gen.nd_size_in = offsetof(struct ndn_msft_smart, status);
> -       msft->gen.nd_size_out = sizeof(msft->u.smart);
> +       msft->gen.nd_size_in = in_size;
> +       msft->gen.nd_size_out = out_size;
>         msft->u.smart.status = 0;
> -       cmd->get_firmware_status = msft_get_firmware_status;

Moving this line up seems unnecessary?

>  
>         return cmd;
>  }
>  
> +static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
> +{
> +       return (alloc_msft_cmd(dimm, NDN_MSFT_CMD_NHEALTH,
> +                       0, sizeof(struct ndn_msft_smart)));

The outermost parenthesis here are unnecessary and unconventional for
the code base.

Also sizeof(*foo_ptr) is more canonical than sizeof(struct foo).
Checkpatch should warn about this.

> +}
> +
>  static int msft_smart_valid(struct ndctl_cmd *cmd)
>  {
>         if (cmd->type != ND_CMD_CALL ||
> -           cmd->size != sizeof(*cmd) + sizeof(struct ndn_pkg_msft) ||
>             CMD_MSFT(cmd)->gen.nd_family != NVDIMM_FAMILY_MSFT ||
> -           CMD_MSFT(cmd)->gen.nd_command != NDN_MSFT_CMD_SMART ||
> +           CMD_MSFT(cmd)->gen.nd_command != NDN_MSFT_CMD_NHEALTH ||
>             cmd->status != 0)
>                 return cmd->status < 0 ? cmd->status : -EINVAL;
>         return 0;
> @@ -80,9 +113,8 @@ static unsigned int msft_cmd_smart_get_flags(struct ndctl_cmd *cmd)
>         }
>  
>         /* below health data can be retrieved via MSFT _DSM function 11 */
> -       return NDN_MSFT_SMART_HEALTH_VALID |
> -               NDN_MSFT_SMART_TEMP_VALID |
> -               NDN_MSFT_SMART_USED_VALID;
> +       return ND_SMART_HEALTH_VALID | ND_SMART_TEMP_VALID |
> +           ND_SMART_USED_VALID;
>  }
>  
>  static unsigned int num_set_bit_health(__u16 num)
> @@ -171,6 +203,8 @@ static int msft_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
>  }
>  
>  struct ndctl_dimm_ops * const msft_dimm_ops = &(struct ndctl_dimm_ops) {
> +       .cmd_desc = msft_cmd_desc,
> +       .cmd_is_supported = msft_cmd_is_supported,
>         .new_smart = msft_dimm_cmd_new_smart,
>         .smart_get_flags = msft_cmd_smart_get_flags,
>         .smart_get_health = msft_cmd_smart_get_health,
> diff --git a/ndctl/lib/msft.h b/ndctl/lib/msft.h
> index 978cc11..8d246a5 100644
> --- a/ndctl/lib/msft.h
> +++ b/ndctl/lib/msft.h
> @@ -2,21 +2,16 @@
>  /* Copyright (C) 2016-2017 Dell, Inc. */
>  /* Copyright (C) 2016 Hewlett Packard Enterprise Development LP */
>  /* Copyright (C) 2014-2020, Intel Corporation. */
> +/* Copyright (C) 2022 iXsystems, Inc. */
>  #ifndef __NDCTL_MSFT_H__
>  #define __NDCTL_MSFT_H__
>  
>  enum {
> -       NDN_MSFT_CMD_QUERY = 0,
> -
> -       /* non-root commands */
> -       NDN_MSFT_CMD_SMART = 11,
> +       NDN_MSFT_CMD_CHEALTH = 10,
> +       NDN_MSFT_CMD_NHEALTH = 11,
> +       NDN_MSFT_CMD_EHEALTH = 12,
>  };
>  
> -/* NDN_MSFT_CMD_SMART */
> -#define NDN_MSFT_SMART_HEALTH_VALID    ND_SMART_HEALTH_VALID
> -#define NDN_MSFT_SMART_TEMP_VALID      ND_SMART_TEMP_VALID
> -#define NDN_MSFT_SMART_USED_VALID      ND_SMART_USED_VALID
> -
>  /*
>   * This is actually function 11 data,
>   * This is the closest I can find to match smart
Alexander Motin Feb. 15, 2023, 4:17 p.m. UTC | #2
On 15.02.2023 00:00, Verma, Vishal L wrote:
> On Fri, 2023-01-13 at 12:27 -0500, Alexander Motin wrote:
>> Clean up the code, making it more uniform with others and
>> allowing more methods to be implemented later:
>>   - remove nonsense NDN_MSFT_CMD_SMART definition, replacing it
>> with real commands, primarity NDN_MSFT_CMD_NHEALTH;
>>   - allow sending arbitrary commands and add their descriptions;
>>   - add custom cmd_is_supported method to allow monitor mode.
> 
> Hi Alexander,
> 
> I think I had similar feedback earlier, but these would be reviewable
> more easily if these three unrelated things are broken up into three
> separate patches.

Hi Vishal,

You did ask me to add more details into commit message and possibly 
split the patch.  I've added the details, but about the second I pleaded 
to save few hours of my time on reshuffling trivial patches.  Apparently 
my plea was not good enough.  Apparently you have a long line of people 
who wish to fix this area.  Whatever, doing it now, hopefully to end 
this and focus on things that actually matter.

> A few other minor comments below.

Thanks for looking deeper.

>>
>> Signed-off-by: Alexander Motin <mav@ixsystems.com>
>> ---
>>   ndctl/lib/msft.c | 58 ++++++++++++++++++++++++++++++++++++++----------
>>   ndctl/lib/msft.h | 13 ++++-------
>>   2 files changed, 50 insertions(+), 21 deletions(-)
>>
>> diff --git a/ndctl/lib/msft.c b/ndctl/lib/msft.c
>> index 3112799..efc7fe1 100644
>> --- a/ndctl/lib/msft.c
>> +++ b/ndctl/lib/msft.c
>> @@ -2,6 +2,7 @@
>>   // Copyright (C) 2016-2017 Dell, Inc.
>>   // Copyright (C) 2016 Hewlett Packard Enterprise Development LP
>>   // Copyright (C) 2016-2020, Intel Corporation.
>> +/* Copyright (C) 2022 iXsystems, Inc. */
>>   #include <stdlib.h>
>>   #include <limits.h>
>>   #include <util/log.h>
>> @@ -12,12 +13,39 @@
>>   #define CMD_MSFT(_c) ((_c)->msft)
>>   #define CMD_MSFT_SMART(_c) (CMD_MSFT(_c)->u.smart.data)
>>   
>> +static const char *msft_cmd_desc(int fn)
>> +{
>> +       static const char * const descs[] = {
>> +               [NDN_MSFT_CMD_CHEALTH] = "critical_health",
>> +               [NDN_MSFT_CMD_NHEALTH] = "nvdimm_health",
>> +               [NDN_MSFT_CMD_EHEALTH] = "es_health",
>> +       };
>> +       const char *desc;
>> +
>> +       if (fn >= (int) ARRAY_SIZE(descs))
>> +               return "unknown";
>> +       desc = descs[fn];
>> +       if (!desc)
>> +               return "unknown";
>> +       return desc;
>> +}
>> +
>> +static bool msft_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
>> +{
>> +       /* Handle this separately to support monitor mode */
>> +       if (cmd == ND_CMD_SMART)
>> +               return true;
>> +
>> +       return !!(dimm->cmd_mask & (1ULL << cmd));
>> +}
>> +
>>   static u32 msft_get_firmware_status(struct ndctl_cmd *cmd)
>>   {
>>          return cmd->msft->u.smart.status;
>>   }
>>   
>> -static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
>> +static struct ndctl_cmd *alloc_msft_cmd(struct ndctl_dimm *dimm,
>> +               unsigned int func, size_t in_size, size_t out_size)
>>   {
>>          struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
>>          struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
>> @@ -30,12 +58,12 @@ static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
>>                  return NULL;
>>          }
>>   
>> -       if (test_dimm_dsm(dimm, NDN_MSFT_CMD_SMART) == DIMM_DSM_UNSUPPORTED) {
>> +       if (test_dimm_dsm(dimm, func) == DIMM_DSM_UNSUPPORTED) {
>>                  dbg(ctx, "unsupported function\n");
>>                  return NULL;
>>          }
>>   
>> -       size = sizeof(*cmd) + sizeof(struct ndn_pkg_msft);
>> +       size = sizeof(*cmd) + sizeof(struct nd_cmd_pkg) + in_size + out_size;
>>          cmd = calloc(1, size);
>>          if (!cmd)
>>                  return NULL;
>> @@ -45,25 +73,30 @@ static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
>>          cmd->type = ND_CMD_CALL;
>>          cmd->size = size;
>>          cmd->status = 1;
>> +       cmd->get_firmware_status = msft_get_firmware_status;
>>   
>>          msft = CMD_MSFT(cmd);
>>          msft->gen.nd_family = NVDIMM_FAMILY_MSFT;
>> -       msft->gen.nd_command = NDN_MSFT_CMD_SMART;
>> +       msft->gen.nd_command = func;
>>          msft->gen.nd_fw_size = 0;
>> -       msft->gen.nd_size_in = offsetof(struct ndn_msft_smart, status);
>> -       msft->gen.nd_size_out = sizeof(msft->u.smart);
>> +       msft->gen.nd_size_in = in_size;
>> +       msft->gen.nd_size_out = out_size;
>>          msft->u.smart.status = 0;
>> -       cmd->get_firmware_status = msft_get_firmware_status;
> 
> Moving this line up seems unnecessary?

Yes, it is unnecesary, but it looked cleaner and more contained to 
assign fields of each structure together.  But once you say so, returned 
back.

>>   
>>          return cmd;
>>   }
>>   
>> +static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
>> +{
>> +       return (alloc_msft_cmd(dimm, NDN_MSFT_CMD_NHEALTH,
>> +                       0, sizeof(struct ndn_msft_smart)));
> 
> The outermost parenthesis here are unnecessary and unconventional for
> the code base.

Thanks.  Removed.

> Also sizeof(*foo_ptr) is more canonical than sizeof(struct foo).
> Checkpatch should warn about this.

Yes, I would totally agree, would I have the variable here.  Adding 
variable just for that purpose is IMO a code obfuscation.

>> +}
>> +
>>   static int msft_smart_valid(struct ndctl_cmd *cmd)
>>   {
>>          if (cmd->type != ND_CMD_CALL ||
>> -           cmd->size != sizeof(*cmd) + sizeof(struct ndn_pkg_msft) ||
>>              CMD_MSFT(cmd)->gen.nd_family != NVDIMM_FAMILY_MSFT ||
>> -           CMD_MSFT(cmd)->gen.nd_command != NDN_MSFT_CMD_SMART ||
>> +           CMD_MSFT(cmd)->gen.nd_command != NDN_MSFT_CMD_NHEALTH ||
>>              cmd->status != 0)
>>                  return cmd->status < 0 ? cmd->status : -EINVAL;
>>          return 0;
>> @@ -80,9 +113,8 @@ static unsigned int msft_cmd_smart_get_flags(struct ndctl_cmd *cmd)
>>          }
>>   
>>          /* below health data can be retrieved via MSFT _DSM function 11 */
>> -       return NDN_MSFT_SMART_HEALTH_VALID |
>> -               NDN_MSFT_SMART_TEMP_VALID |
>> -               NDN_MSFT_SMART_USED_VALID;
>> +       return ND_SMART_HEALTH_VALID | ND_SMART_TEMP_VALID |
>> +           ND_SMART_USED_VALID;
>>   }
>>   
>>   static unsigned int num_set_bit_health(__u16 num)
>> @@ -171,6 +203,8 @@ static int msft_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
>>   }
>>   
>>   struct ndctl_dimm_ops * const msft_dimm_ops = &(struct ndctl_dimm_ops) {
>> +       .cmd_desc = msft_cmd_desc,
>> +       .cmd_is_supported = msft_cmd_is_supported,
>>          .new_smart = msft_dimm_cmd_new_smart,
>>          .smart_get_flags = msft_cmd_smart_get_flags,
>>          .smart_get_health = msft_cmd_smart_get_health,
>> diff --git a/ndctl/lib/msft.h b/ndctl/lib/msft.h
>> index 978cc11..8d246a5 100644
>> --- a/ndctl/lib/msft.h
>> +++ b/ndctl/lib/msft.h
>> @@ -2,21 +2,16 @@
>>   /* Copyright (C) 2016-2017 Dell, Inc. */
>>   /* Copyright (C) 2016 Hewlett Packard Enterprise Development LP */
>>   /* Copyright (C) 2014-2020, Intel Corporation. */
>> +/* Copyright (C) 2022 iXsystems, Inc. */
>>   #ifndef __NDCTL_MSFT_H__
>>   #define __NDCTL_MSFT_H__
>>   
>>   enum {
>> -       NDN_MSFT_CMD_QUERY = 0,
>> -
>> -       /* non-root commands */
>> -       NDN_MSFT_CMD_SMART = 11,
>> +       NDN_MSFT_CMD_CHEALTH = 10,
>> +       NDN_MSFT_CMD_NHEALTH = 11,
>> +       NDN_MSFT_CMD_EHEALTH = 12,
>>   };
>>   
>> -/* NDN_MSFT_CMD_SMART */
>> -#define NDN_MSFT_SMART_HEALTH_VALID    ND_SMART_HEALTH_VALID
>> -#define NDN_MSFT_SMART_TEMP_VALID      ND_SMART_TEMP_VALID
>> -#define NDN_MSFT_SMART_USED_VALID      ND_SMART_USED_VALID
>> -
>>   /*
>>    * This is actually function 11 data,
>>    * This is the closest I can find to match smart
>
diff mbox series

Patch

diff --git a/ndctl/lib/msft.c b/ndctl/lib/msft.c
index 3112799..efc7fe1 100644
--- a/ndctl/lib/msft.c
+++ b/ndctl/lib/msft.c
@@ -2,6 +2,7 @@ 
 // Copyright (C) 2016-2017 Dell, Inc.
 // Copyright (C) 2016 Hewlett Packard Enterprise Development LP
 // Copyright (C) 2016-2020, Intel Corporation.
+/* Copyright (C) 2022 iXsystems, Inc. */
 #include <stdlib.h>
 #include <limits.h>
 #include <util/log.h>
@@ -12,12 +13,39 @@ 
 #define CMD_MSFT(_c) ((_c)->msft)
 #define CMD_MSFT_SMART(_c) (CMD_MSFT(_c)->u.smart.data)
 
+static const char *msft_cmd_desc(int fn)
+{
+	static const char * const descs[] = {
+		[NDN_MSFT_CMD_CHEALTH] = "critical_health",
+		[NDN_MSFT_CMD_NHEALTH] = "nvdimm_health",
+		[NDN_MSFT_CMD_EHEALTH] = "es_health",
+	};
+	const char *desc;
+
+	if (fn >= (int) ARRAY_SIZE(descs))
+		return "unknown";
+	desc = descs[fn];
+	if (!desc)
+		return "unknown";
+	return desc;
+}
+
+static bool msft_cmd_is_supported(struct ndctl_dimm *dimm, int cmd)
+{
+	/* Handle this separately to support monitor mode */
+	if (cmd == ND_CMD_SMART)
+		return true;
+
+	return !!(dimm->cmd_mask & (1ULL << cmd));
+}
+
 static u32 msft_get_firmware_status(struct ndctl_cmd *cmd)
 {
 	return cmd->msft->u.smart.status;
 }
 
-static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
+static struct ndctl_cmd *alloc_msft_cmd(struct ndctl_dimm *dimm,
+		unsigned int func, size_t in_size, size_t out_size)
 {
 	struct ndctl_bus *bus = ndctl_dimm_get_bus(dimm);
 	struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
@@ -30,12 +58,12 @@  static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
 		return NULL;
 	}
 
-	if (test_dimm_dsm(dimm, NDN_MSFT_CMD_SMART) == DIMM_DSM_UNSUPPORTED) {
+	if (test_dimm_dsm(dimm, func) == DIMM_DSM_UNSUPPORTED) {
 		dbg(ctx, "unsupported function\n");
 		return NULL;
 	}
 
-	size = sizeof(*cmd) + sizeof(struct ndn_pkg_msft);
+	size = sizeof(*cmd) + sizeof(struct nd_cmd_pkg) + in_size + out_size;
 	cmd = calloc(1, size);
 	if (!cmd)
 		return NULL;
@@ -45,25 +73,30 @@  static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
 	cmd->type = ND_CMD_CALL;
 	cmd->size = size;
 	cmd->status = 1;
+	cmd->get_firmware_status = msft_get_firmware_status;
 
 	msft = CMD_MSFT(cmd);
 	msft->gen.nd_family = NVDIMM_FAMILY_MSFT;
-	msft->gen.nd_command = NDN_MSFT_CMD_SMART;
+	msft->gen.nd_command = func;
 	msft->gen.nd_fw_size = 0;
-	msft->gen.nd_size_in = offsetof(struct ndn_msft_smart, status);
-	msft->gen.nd_size_out = sizeof(msft->u.smart);
+	msft->gen.nd_size_in = in_size;
+	msft->gen.nd_size_out = out_size;
 	msft->u.smart.status = 0;
-	cmd->get_firmware_status = msft_get_firmware_status;
 
 	return cmd;
 }
 
+static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
+{
+	return (alloc_msft_cmd(dimm, NDN_MSFT_CMD_NHEALTH,
+			0, sizeof(struct ndn_msft_smart)));
+}
+
 static int msft_smart_valid(struct ndctl_cmd *cmd)
 {
 	if (cmd->type != ND_CMD_CALL ||
-	    cmd->size != sizeof(*cmd) + sizeof(struct ndn_pkg_msft) ||
 	    CMD_MSFT(cmd)->gen.nd_family != NVDIMM_FAMILY_MSFT ||
-	    CMD_MSFT(cmd)->gen.nd_command != NDN_MSFT_CMD_SMART ||
+	    CMD_MSFT(cmd)->gen.nd_command != NDN_MSFT_CMD_NHEALTH ||
 	    cmd->status != 0)
 		return cmd->status < 0 ? cmd->status : -EINVAL;
 	return 0;
@@ -80,9 +113,8 @@  static unsigned int msft_cmd_smart_get_flags(struct ndctl_cmd *cmd)
 	}
 
 	/* below health data can be retrieved via MSFT _DSM function 11 */
-	return NDN_MSFT_SMART_HEALTH_VALID |
-		NDN_MSFT_SMART_TEMP_VALID |
-		NDN_MSFT_SMART_USED_VALID;
+	return ND_SMART_HEALTH_VALID | ND_SMART_TEMP_VALID |
+	    ND_SMART_USED_VALID;
 }
 
 static unsigned int num_set_bit_health(__u16 num)
@@ -171,6 +203,8 @@  static int msft_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
 }
 
 struct ndctl_dimm_ops * const msft_dimm_ops = &(struct ndctl_dimm_ops) {
+	.cmd_desc = msft_cmd_desc,
+	.cmd_is_supported = msft_cmd_is_supported,
 	.new_smart = msft_dimm_cmd_new_smart,
 	.smart_get_flags = msft_cmd_smart_get_flags,
 	.smart_get_health = msft_cmd_smart_get_health,
diff --git a/ndctl/lib/msft.h b/ndctl/lib/msft.h
index 978cc11..8d246a5 100644
--- a/ndctl/lib/msft.h
+++ b/ndctl/lib/msft.h
@@ -2,21 +2,16 @@ 
 /* Copyright (C) 2016-2017 Dell, Inc. */
 /* Copyright (C) 2016 Hewlett Packard Enterprise Development LP */
 /* Copyright (C) 2014-2020, Intel Corporation. */
+/* Copyright (C) 2022 iXsystems, Inc. */
 #ifndef __NDCTL_MSFT_H__
 #define __NDCTL_MSFT_H__
 
 enum {
-	NDN_MSFT_CMD_QUERY = 0,
-
-	/* non-root commands */
-	NDN_MSFT_CMD_SMART = 11,
+	NDN_MSFT_CMD_CHEALTH = 10,
+	NDN_MSFT_CMD_NHEALTH = 11,
+	NDN_MSFT_CMD_EHEALTH = 12,
 };
 
-/* NDN_MSFT_CMD_SMART */
-#define NDN_MSFT_SMART_HEALTH_VALID	ND_SMART_HEALTH_VALID
-#define NDN_MSFT_SMART_TEMP_VALID	ND_SMART_TEMP_VALID
-#define NDN_MSFT_SMART_USED_VALID	ND_SMART_USED_VALID
-
 /*
  * This is actually function 11 data,
  * This is the closest I can find to match smart