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

[Jehoon Park <jehoon.park@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 5682 bytes --]

On Mon, Jul 24, 2023 at 09:08:21PM +0000, Verma, Vishal L wrote:
> 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.
> 

Hi, Vishal.

Thank you for comment. I agree with the behavior of libcxl accessors as you
explained. FYI, the reason I replaced errno codes with dbg messages is that
those accessors are retreiving signed values. I thought returning errno codes
is not distinguishable from retrieved values when they are negative.
However, it looks like an overkill because a memory device works below-zero
temperature would not make sense in real world.

I'll send revised patch soon after reverting to errno codes and fixing
related codes in cxl/json.c.

Jehoon

> >         return rc;
> >  }
> >  
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]