diff mbox series

[ndctl,2/4,v3] libndctl/msft: Replace nonsense NDN_MSFT_CMD_SMART command

Message ID 20230215164930.707170-2-mav@ixsystems.com (mailing list archive)
State Accepted
Commit a0955adab7bab5f417c440946e04a8061662c026
Headers show
Series [ndctl,1/4,v3] libndctl/msft: Remove NDN_MSFT_SMART_*_VALID defines. | expand

Commit Message

Alexander Motin Feb. 15, 2023, 4:49 p.m. UTC 
There is no NDN_MSFT_CMD_SMART command.  There are 3 relevant ones,
reporting different aspects of the module health.  Define those and
use NDN_MSFT_CMD_NHEALTH, while making the code more universal to
allow use of others later.

Signed-off-by:	Alexander Motin <mav@ixsystems.com>
---
 ndctl/lib/msft.c | 41 +++++++++++++++++++++++++++++++++--------
 ndctl/lib/msft.h |  8 ++++----
 2 files changed, 37 insertions(+), 12 deletions(-)

Comments

Verma, Vishal L Feb. 15, 2023, 7:36 p.m. UTC | #1 
On Wed, 2023-02-15 at 11:49 -0500, Alexander Motin wrote:
> There is no NDN_MSFT_CMD_SMART command.  There are 3 relevant ones,
> reporting different aspects of the module health.  Define those and
> use NDN_MSFT_CMD_NHEALTH, while making the code more universal to
> allow use of others later.
> 
> Signed-off-by:  Alexander Motin <mav@ixsystems.com>
> ---
>  ndctl/lib/msft.c | 41 +++++++++++++++++++++++++++++++++--------
>  ndctl/lib/msft.h |  8 ++++----
>  2 files changed, 37 insertions(+), 12 deletions(-)

Just one question below, the rest looks good. Thanks for the re-spin.
<..>
> 
> +
>  static int msft_smart_valid(struct ndctl_cmd *cmd)
>  {
>         if (cmd->type != ND_CMD_CALL ||
> -           cmd->size != sizeof(*cmd) + sizeof(struct ndn_pkg_msft) ||

Why is this size check dropped?

>             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;
> @@ -170,6 +194,7 @@ 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,
>         .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 c462612..8d246a5 100644
> --- a/ndctl/lib/msft.h
> +++ b/ndctl/lib/msft.h
> @@ -2,14 +2,14 @@
>  /* 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,
>  };
>  
>  /*
Alexander Motin Feb. 15, 2023, 7:54 p.m. UTC | #2 
On 15.02.2023 14:36, Verma, Vishal L wrote:
> On Wed, 2023-02-15 at 11:49 -0500, Alexander Motin wrote:
>> There is no NDN_MSFT_CMD_SMART command.  There are 3 relevant ones,
>> reporting different aspects of the module health.  Define those and
>> use NDN_MSFT_CMD_NHEALTH, while making the code more universal to
>> allow use of others later.
>>
>> Signed-off-by:  Alexander Motin <mav@ixsystems.com>
>> ---
>>   ndctl/lib/msft.c | 41 +++++++++++++++++++++++++++++++++--------
>>   ndctl/lib/msft.h |  8 ++++----
>>   2 files changed, 37 insertions(+), 12 deletions(-)
> 
> Just one question below, the rest looks good. Thanks for the re-spin.
> <..>
>>
>> +
>>   static int msft_smart_valid(struct ndctl_cmd *cmd)
>>   {
>>          if (cmd->type != ND_CMD_CALL ||
>> -           cmd->size != sizeof(*cmd) + sizeof(struct ndn_pkg_msft) ||
> 
> Why is this size check dropped?

Primarily because intel_smart_valid(), which I tried to mimic with this 
commit, does not check the size.  Different commands have different data 
sizes.  With addition of size arguments to alloc_msft_cmd() this check 
looks more strict, but while no other commands are used now, either way 
could probably make sense.  Any way this check is redundant, and should 
have been made an assertion to begin with.

>>              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;
>> @@ -170,6 +194,7 @@ 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,
>>          .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 c462612..8d246a5 100644
>> --- a/ndctl/lib/msft.h
>> +++ b/ndctl/lib/msft.h
>> @@ -2,14 +2,14 @@
>>   /* 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,
>>   };
>>   
>>   /*
>
diff mbox series

Patch

diff --git a/ndctl/lib/msft.c b/ndctl/lib/msft.c
index 22f72dd..b5278c5 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,30 @@ 
 #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 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 +49,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;
@@ -48,22 +67,27 @@  static struct ndctl_cmd *msft_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
 
 	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;
@@ -170,6 +194,7 @@  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,
 	.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 c462612..8d246a5 100644
--- a/ndctl/lib/msft.h
+++ b/ndctl/lib/msft.h
@@ -2,14 +2,14 @@ 
 /* 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,
 };
 
 /*