diff mbox series

[ndctl,v3,09/21] libcxl: add GET_HEALTH_INFO mailbox command and accessors

Message ID 20210701201005.3065299-10-vishal.l.verma@intel.com (mailing list archive)
State New, archived
Headers show
Series Initial CXL support | expand

Commit Message

Verma, Vishal L July 1, 2021, 8:09 p.m. UTC 
Add libcxl APIs to create a new GET_HEALTH_INFO mailbox command, the
command output data structure (privately), and accessor APIs to return
the different fields in the health info output.

Cc: Ben Widawsky <ben.widawsky@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 cxl/lib/private.h  | 11 +++++++++
 cxl/lib/libcxl.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 cxl/libcxl.h       |  9 +++++++
 cxl/lib/libcxl.sym |  9 +++++++
 4 files changed, 90 insertions(+)

Comments

Dan Williams July 16, 2021, 1:11 a.m. UTC | #1 
On Thu, Jul 1, 2021 at 1:11 PM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Add libcxl APIs to create a new GET_HEALTH_INFO mailbox command, the
> command output data structure (privately), and accessor APIs to return
> the different fields in the health info output.
>
> Cc: Ben Widawsky <ben.widawsky@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  cxl/lib/private.h  | 11 +++++++++
>  cxl/lib/libcxl.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>  cxl/libcxl.h       |  9 +++++++
>  cxl/lib/libcxl.sym |  9 +++++++
>  4 files changed, 90 insertions(+)
>
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index 3273f21..2232f4c 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -73,6 +73,17 @@ struct cxl_cmd_identify {
>         u8 qos_telemetry_caps;
>  } __attribute__((packed));
>
> +struct cxl_cmd_get_health_info {
> +       u8 health_status;
> +       u8 media_status;
> +       u8 ext_status;
> +       u8 life_used;
> +       le16 temperature;
> +       le32 dirty_shutdowns;
> +       le32 volatile_errors;
> +       le32 pmem_errors;
> +} __attribute__((packed));
> +
>  static inline int check_kmod(struct kmod_ctx *kmod_ctx)
>  {
>         return kmod_ctx ? 0 : -ENXIO;
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 06a6c20..2e33c5e 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -673,6 +673,67 @@ CXL_EXPORT const char *cxl_cmd_get_devname(struct cxl_cmd *cmd)
>         return cxl_memdev_get_devname(cmd->memdev);
>  }
>
> +#define cmd_get_int(cmd, n, N, field) \
> +do { \
> +       struct cxl_cmd_##n *c = (void *)cmd->send_cmd->out.payload; \
> +       if (cmd->send_cmd->id != CXL_MEM_COMMAND_ID_##N) \
> +               return EINVAL; \
> +       if (cmd->status < 0) \
> +               return cmd->status; \
> +       return le32_to_cpu(c->field); \
> +} while(0);
> +
> +CXL_EXPORT struct cxl_cmd *cxl_cmd_new_get_health_info(
> +               struct cxl_memdev *memdev)
> +{
> +       return cxl_cmd_new_generic(memdev, CXL_MEM_COMMAND_ID_GET_HEALTH_INFO);
> +}
> +
> +#define cmd_health_get_int(c, f) \
> +do { \
> +       cmd_get_int(c, get_health_info, GET_HEALTH_INFO, f); \
> +} while (0);
> +
> +CXL_EXPORT int cxl_cmd_get_health_info_get_health_status(struct cxl_cmd *cmd)
> +{
> +       cmd_health_get_int(cmd, health_status);
> +}

I would expect this to broken up into a helper per flag like:

bool cxl_cmd_health_info_get_maintenance_needed(struct cxl_cmd *cmd)
bool cxl_cmd_health_info_get_performance_degraded(struct cxl_cmd *cmd)
bool cxl_cmd_health_info_get_replacement_needed(struct cxl_cmd *cmd)

i.e. the payload helpers should try to hide reserved fields from being
inadvertently communicated, otherwise software that's starts reserved
values being set may break. Rather than masking reserved adding
explicit helpers makes the library usage more readable... but yeah it
makes the library definition a bit more tedious.

I also think it's ok to drop verbs out of command names especially
when the collide with library verb names, i.e.
s/get_health_info_get/s/health_info_get/


> +CXL_EXPORT int cxl_cmd_get_health_info_get_media_status(struct cxl_cmd *cmd)
> +{
> +       cmd_health_get_int(cmd, media_status);
> +}

Similar feedback here, i.e. unless the payload field is literally an
integer it should have a scheme to prevent reserved values from being
passed through, and ideally have formal retrieval methods for each
defined value. In this case I think a library defined enum to mirror
the status values is warranted. No need to make the enum values match
up with payload values.

> +
> +CXL_EXPORT int cxl_cmd_get_health_info_get_ext_status(struct cxl_cmd *cmd)
> +{
> +       cmd_health_get_int(cmd, ext_status);
> +}
> +

This one is fun with enums and flags to parse...

> +CXL_EXPORT int cxl_cmd_get_health_info_get_life_used(struct cxl_cmd *cmd)
> +{
> +       cmd_health_get_int(cmd, life_used);
> +}
> +

This should probably return a positive number from 0-100 or a negative
error code.

> +CXL_EXPORT int cxl_cmd_get_health_info_get_temperature(struct cxl_cmd *cmd)
> +{
> +       cmd_health_get_int(cmd, temperature);
> +}

This is a two's complement value that needs to be cast into a positive
or negative number of degrees.

> +
> +CXL_EXPORT int cxl_cmd_get_health_info_get_dirty_shutdowns(struct cxl_cmd *cmd)
> +{
> +       cmd_health_get_int(cmd, dirty_shutdowns);
> +}
> +
> +CXL_EXPORT int cxl_cmd_get_health_info_get_volatile_errors(struct cxl_cmd *cmd)
> +{
> +       cmd_health_get_int(cmd, volatile_errors);
> +}
> +
> +CXL_EXPORT int cxl_cmd_get_health_info_get_pmem_errors(struct cxl_cmd *cmd)
> +{
> +       cmd_health_get_int(cmd, pmem_errors);
> +}
> +

These 3 seem ok to not need translation helpers, but only as unsigned integers.

>  CXL_EXPORT struct cxl_cmd *cxl_cmd_new_identify(struct cxl_memdev *memdev)
>  {
>         return cxl_cmd_new_generic(memdev, CXL_MEM_COMMAND_ID_IDENTIFY);
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index 9ed8c83..56ae4af 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -62,6 +62,15 @@ struct cxl_cmd *cxl_cmd_new_identify(struct cxl_memdev *memdev);
>  int cxl_cmd_identify_get_fw_rev(struct cxl_cmd *cmd, char *fw_rev, int fw_len);
>  unsigned long long cxl_cmd_identify_get_partition_align(struct cxl_cmd *cmd);
>  unsigned int cxl_cmd_identify_get_lsa_size(struct cxl_cmd *cmd);
> +struct cxl_cmd *cxl_cmd_new_get_health_info(struct cxl_memdev *memdev);
> +int cxl_cmd_get_health_info_get_health_status(struct cxl_cmd *cmd);
> +int cxl_cmd_get_health_info_get_media_status(struct cxl_cmd *cmd);
> +int cxl_cmd_get_health_info_get_ext_status(struct cxl_cmd *cmd);
> +int cxl_cmd_get_health_info_get_life_used(struct cxl_cmd *cmd);
> +int cxl_cmd_get_health_info_get_temperature(struct cxl_cmd *cmd);
> +int cxl_cmd_get_health_info_get_dirty_shutdowns(struct cxl_cmd *cmd);
> +int cxl_cmd_get_health_info_get_volatile_errors(struct cxl_cmd *cmd);
> +int cxl_cmd_get_health_info_get_pmem_errors(struct cxl_cmd *cmd);
>
>  #ifdef __cplusplus
>  } /* extern "C" */
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index d6aa0b2..e00443d 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -43,4 +43,13 @@ global:
>         cxl_cmd_identify_get_fw_rev;
>         cxl_cmd_identify_get_partition_align;
>         cxl_cmd_identify_get_lsa_size;
> +       cxl_cmd_new_get_health_info;
> +       cxl_cmd_get_health_info_get_health_status;
> +       cxl_cmd_get_health_info_get_media_status;
> +       cxl_cmd_get_health_info_get_ext_status;
> +       cxl_cmd_get_health_info_get_life_used;
> +       cxl_cmd_get_health_info_get_temperature;
> +       cxl_cmd_get_health_info_get_dirty_shutdowns;
> +       cxl_cmd_get_health_info_get_volatile_errors;
> +       cxl_cmd_get_health_info_get_pmem_errors;
>  } LIBCXL_2;
> --
> 2.31.1
>
diff mbox series

Patch

diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index 3273f21..2232f4c 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -73,6 +73,17 @@  struct cxl_cmd_identify {
 	u8 qos_telemetry_caps;
 } __attribute__((packed));
 
+struct cxl_cmd_get_health_info {
+	u8 health_status;
+	u8 media_status;
+	u8 ext_status;
+	u8 life_used;
+	le16 temperature;
+	le32 dirty_shutdowns;
+	le32 volatile_errors;
+	le32 pmem_errors;
+} __attribute__((packed));
+
 static inline int check_kmod(struct kmod_ctx *kmod_ctx)
 {
 	return kmod_ctx ? 0 : -ENXIO;
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index 06a6c20..2e33c5e 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -673,6 +673,67 @@  CXL_EXPORT const char *cxl_cmd_get_devname(struct cxl_cmd *cmd)
 	return cxl_memdev_get_devname(cmd->memdev);
 }
 
+#define cmd_get_int(cmd, n, N, field) \
+do { \
+	struct cxl_cmd_##n *c = (void *)cmd->send_cmd->out.payload; \
+	if (cmd->send_cmd->id != CXL_MEM_COMMAND_ID_##N) \
+		return EINVAL; \
+	if (cmd->status < 0) \
+		return cmd->status; \
+	return le32_to_cpu(c->field); \
+} while(0);
+
+CXL_EXPORT struct cxl_cmd *cxl_cmd_new_get_health_info(
+		struct cxl_memdev *memdev)
+{
+	return cxl_cmd_new_generic(memdev, CXL_MEM_COMMAND_ID_GET_HEALTH_INFO);
+}
+
+#define cmd_health_get_int(c, f) \
+do { \
+	cmd_get_int(c, get_health_info, GET_HEALTH_INFO, f); \
+} while (0);
+
+CXL_EXPORT int cxl_cmd_get_health_info_get_health_status(struct cxl_cmd *cmd)
+{
+	cmd_health_get_int(cmd, health_status);
+}
+
+CXL_EXPORT int cxl_cmd_get_health_info_get_media_status(struct cxl_cmd *cmd)
+{
+	cmd_health_get_int(cmd, media_status);
+}
+
+CXL_EXPORT int cxl_cmd_get_health_info_get_ext_status(struct cxl_cmd *cmd)
+{
+	cmd_health_get_int(cmd, ext_status);
+}
+
+CXL_EXPORT int cxl_cmd_get_health_info_get_life_used(struct cxl_cmd *cmd)
+{
+	cmd_health_get_int(cmd, life_used);
+}
+
+CXL_EXPORT int cxl_cmd_get_health_info_get_temperature(struct cxl_cmd *cmd)
+{
+	cmd_health_get_int(cmd, temperature);
+}
+
+CXL_EXPORT int cxl_cmd_get_health_info_get_dirty_shutdowns(struct cxl_cmd *cmd)
+{
+	cmd_health_get_int(cmd, dirty_shutdowns);
+}
+
+CXL_EXPORT int cxl_cmd_get_health_info_get_volatile_errors(struct cxl_cmd *cmd)
+{
+	cmd_health_get_int(cmd, volatile_errors);
+}
+
+CXL_EXPORT int cxl_cmd_get_health_info_get_pmem_errors(struct cxl_cmd *cmd)
+{
+	cmd_health_get_int(cmd, pmem_errors);
+}
+
 CXL_EXPORT struct cxl_cmd *cxl_cmd_new_identify(struct cxl_memdev *memdev)
 {
 	return cxl_cmd_new_generic(memdev, CXL_MEM_COMMAND_ID_IDENTIFY);
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 9ed8c83..56ae4af 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -62,6 +62,15 @@  struct cxl_cmd *cxl_cmd_new_identify(struct cxl_memdev *memdev);
 int cxl_cmd_identify_get_fw_rev(struct cxl_cmd *cmd, char *fw_rev, int fw_len);
 unsigned long long cxl_cmd_identify_get_partition_align(struct cxl_cmd *cmd);
 unsigned int cxl_cmd_identify_get_lsa_size(struct cxl_cmd *cmd);
+struct cxl_cmd *cxl_cmd_new_get_health_info(struct cxl_memdev *memdev);
+int cxl_cmd_get_health_info_get_health_status(struct cxl_cmd *cmd);
+int cxl_cmd_get_health_info_get_media_status(struct cxl_cmd *cmd);
+int cxl_cmd_get_health_info_get_ext_status(struct cxl_cmd *cmd);
+int cxl_cmd_get_health_info_get_life_used(struct cxl_cmd *cmd);
+int cxl_cmd_get_health_info_get_temperature(struct cxl_cmd *cmd);
+int cxl_cmd_get_health_info_get_dirty_shutdowns(struct cxl_cmd *cmd);
+int cxl_cmd_get_health_info_get_volatile_errors(struct cxl_cmd *cmd);
+int cxl_cmd_get_health_info_get_pmem_errors(struct cxl_cmd *cmd);
 
 #ifdef __cplusplus
 } /* extern "C" */
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index d6aa0b2..e00443d 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -43,4 +43,13 @@  global:
 	cxl_cmd_identify_get_fw_rev;
 	cxl_cmd_identify_get_partition_align;
 	cxl_cmd_identify_get_lsa_size;
+	cxl_cmd_new_get_health_info;
+	cxl_cmd_get_health_info_get_health_status;
+	cxl_cmd_get_health_info_get_media_status;
+	cxl_cmd_get_health_info_get_ext_status;
+	cxl_cmd_get_health_info_get_life_used;
+	cxl_cmd_get_health_info_get_temperature;
+	cxl_cmd_get_health_info_get_dirty_shutdowns;
+	cxl_cmd_get_health_info_get_volatile_errors;
+	cxl_cmd_get_health_info_get_pmem_errors;
 } LIBCXL_2;