diff mbox series

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

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

Commit Message

Alexander Motin Nov. 17, 2022, 10:37 p.m. UTC 
Clean up the code, making it more uniform with others and
allowing more methods to be implemented later.

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 Dec. 14, 2022, 12:33 a.m. UTC | #1 
On Thu, 2022-11-17 at 17:37 -0500, Alexander Motin wrote:
> Clean up the code, making it more uniform with others and
> allowing more methods to be implemented later.

Hi Alexander,

I think this would be easier to review if the the cleanups were
summarized in the commit message - and if there are different types of
cleanups, then possibly break them down into a different patch per type.

I'd also consider CC'ing the original authors of this file for some Acks
and reviews.

> 
> 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;
>  
>         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
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