diff mbox series

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

Message ID 20230717062908.8292-3-jehoon.park@samsung.com
State New, archived
Headers show
Series [ndctl,RESEND,1/2] cxl: Update a revision by CXL 3.0 specification | expand

Commit Message

Jehoon Park July 17, 2023, 6:29 a.m. UTC
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(-)

Comments

Verma, Vishal L July 24, 2023, 9:08 p.m. UTC | #1
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;
>  }
>
Jehoon Park July 31, 2023, 3:17 a.m. UTC | #2
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;
> >  }
> >  
>
Verma, Vishal L July 31, 2023, 6:45 p.m. UTC | #3
On Mon, 2023-07-31 at 12:17 +0900, Jehoon Park wrote:
> 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(-)
> > > 

<..>

> > > 
> > >  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.
> 
Good point on the negative temperatures - this means we can't use the
negative = error convention, but in this case what you can do is return
something like INT_MAX to indicate an error, and set errno in the
library to whatever error we want to indicate. And adjust all the
callers to check for errors in this way.
diff mbox series

Patch

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));
 	return rc;
 }