Re: [ndctl PATCH RESEND 2/2] libcxl: Fix accessors for temperature field to support negative value

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Mon, 2023-07-17 at 15:29 +0900, Jehoon Park wrote:
> Add a new macro function to retrieve a signed value such as a temperature.
> Replace indistinguishable error numbers with debug message.
> 
> Signed-off-by: Jehoon Park <jehoon.park@samsung.com>
> ---
>  cxl/lib/libcxl.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index 769cd8a..fca7faa 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -3452,11 +3452,21 @@ cxl_cmd_alert_config_get_life_used_prog_warn_threshold(struct cxl_cmd *cmd)
>                          life_used_prog_warn_threshold);
>  }
>  
> +#define cmd_get_field_s16(cmd, n, N, field)                            \
> +do {                                                                   \
> +       struct cxl_cmd_##n *c =                                         \
> +               (struct cxl_cmd_##n *)cmd->send_cmd->out.payload;       \
> +       int rc = cxl_cmd_validate_status(cmd, CXL_MEM_COMMAND_ID_##N);  \
> +       if (rc)                                                         \
> +               return 0xffff;                                          \
> +       return (int16_t)le16_to_cpu(c->field);                                  \
> +} while(0)
> +
>  CXL_EXPORT int
>  cxl_cmd_alert_config_get_dev_over_temperature_crit_alert_threshold(
>         struct cxl_cmd *cmd)
>  {
> -       cmd_get_field_u16(cmd, get_alert_config, GET_ALERT_CONFIG,
> +       cmd_get_field_s16(cmd, get_alert_config, GET_ALERT_CONFIG,
>                           dev_over_temperature_crit_alert_threshold);
>  }
>  
> @@ -3464,7 +3474,7 @@ CXL_EXPORT int
>  cxl_cmd_alert_config_get_dev_under_temperature_crit_alert_threshold(
>         struct cxl_cmd *cmd)
>  {
> -       cmd_get_field_u16(cmd, get_alert_config, GET_ALERT_CONFIG,
> +       cmd_get_field_s16(cmd, get_alert_config, GET_ALERT_CONFIG,
>                           dev_under_temperature_crit_alert_threshold);
>  }
>  
> @@ -3472,7 +3482,7 @@ CXL_EXPORT int
>  cxl_cmd_alert_config_get_dev_over_temperature_prog_warn_threshold(
>         struct cxl_cmd *cmd)
>  {
> -       cmd_get_field_u16(cmd, get_alert_config, GET_ALERT_CONFIG,
> +       cmd_get_field_s16(cmd, get_alert_config, GET_ALERT_CONFIG,
>                           dev_over_temperature_prog_warn_threshold);
>  }
>  
> @@ -3480,7 +3490,7 @@ CXL_EXPORT int
>  cxl_cmd_alert_config_get_dev_under_temperature_prog_warn_threshold(
>         struct cxl_cmd *cmd)
>  {
> -       cmd_get_field_u16(cmd, get_alert_config, GET_ALERT_CONFIG,
> +       cmd_get_field_s16(cmd, get_alert_config, GET_ALERT_CONFIG,
>                           dev_under_temperature_prog_warn_threshold);
>  }
>  
> @@ -3695,28 +3705,34 @@ static int health_info_get_life_used_raw(struct cxl_cmd *cmd)
>  CXL_EXPORT int cxl_cmd_health_info_get_life_used(struct cxl_cmd *cmd)
>  {
>         int rc = health_info_get_life_used_raw(cmd);
> +       struct cxl_ctx *ctx = cxl_memdev_get_ctx(cmd->memdev);
>  
>         if (rc < 0)
> -               return rc;
> +               dbg(ctx, "%s: Invalid command status\n",
> +                   cxl_memdev_get_devname(cmd->memdev));
>         if (rc == CXL_CMD_HEALTH_INFO_LIFE_USED_NOT_IMPL)
> -               return -EOPNOTSUPP;
> +               dbg(ctx, "%s: Life Used not implemented\n",
> +                   cxl_memdev_get_devname(cmd->memdev));
>         return rc;
>  }
>  
>  static int health_info_get_temperature_raw(struct cxl_cmd *cmd)
>  {
> -       cmd_get_field_u16(cmd, get_health_info, GET_HEALTH_INFO,
> +       cmd_get_field_s16(cmd, get_health_info, GET_HEALTH_INFO,
>                                  temperature);
>  }
>  
>  CXL_EXPORT int cxl_cmd_health_info_get_temperature(struct cxl_cmd *cmd)
>  {
>         int rc = health_info_get_temperature_raw(cmd);
> +       struct cxl_ctx *ctx = cxl_memdev_get_ctx(cmd->memdev);
>  
> -       if (rc < 0)
> -               return rc;
> +       if (rc == 0xffff)
> +               dbg(ctx, "%s: Invalid command status\n",
> +                   cxl_memdev_get_devname(cmd->memdev));
>         if (rc == CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL)
> -               return -EOPNOTSUPP;
> +               dbg(ctx, "%s: Device Temperature not implemented\n",
> +                   cxl_memdev_get_devname(cmd->memdev));

Hi Jehoon,

libcxl tends to just return errno codes for simple accessors liek this,
and leave it up to the caller to print additional information about why
the call might have failed. Even though these are dbg() messages, I'd
prefer leaving them out of this patch, and if there is a call site
where this fails and there isn't an adequate error message printed as
to why, then add these prints there.

Rest of the conversion to s16 looks good.

>         return rc;
>  }
>