[ndctl PATCH RESEND 0/2] Fix accessors for temperature field when it is negative

[ndctl PATCH RESEND 0/2] Fix accessors for temperature field when it is negative

[Jehoon Park]

In CXL 3.0 SPEC, 8.2.9.8.3.1 and 8.2.9.8.3.2 define temperature fields
as a 2's complement value. However, they are retrieved by the same accessor
for unsigned value. This causes inaccuracy when the value is negative.

The first patch updates the pre-defined value for temperature field of
the Get Health Info command when it is not implemented by complying
CXL 3.0 specification. (CXL 3.0 8.2.9.8.3.1)

The second patch fixes accessors for temperature fields.
Add a new payload accessor for a signed value, then use it for retrieving
temperature properly. Remove negative error numbers since they are not
distinguishable from the retrieved value because it could be negative.
They are replaced by debug message.

Jehoon Park (2):
  cxl: Update a revision by CXL 3.0 specification
  libcxl: Fix accessors for temperature field to support negative value

 cxl/json.c        |  2 +-
 cxl/lib/libcxl.c  | 36 ++++++++++++++++++++++++++----------
 cxl/lib/private.h |  2 +-
 3 files changed, 28 insertions(+), 12 deletions(-)


base-commit: 7f75ce36ce3a0d41ed74d4e2dfcfd41a6fd7fe40
-- 
2.17.1

[ndctl PATCH RESEND 1/2] cxl: Update a revision by CXL 3.0 specification

[Jehoon Park]

Update the value of device temperature field when it is not implemented.
(CXL 3.0 8.2.9.8.3.1)

Signed-off-by: Jehoon Park <jehoon.park@samsung.com>
---
 cxl/json.c        | 2 +-
 cxl/lib/private.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cxl/json.c b/cxl/json.c
index 9a4b5c7..3661eb9 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -155,7 +155,7 @@ static struct json_object *util_cxl_memdev_health_to_json(
 	}
 
 	field = cxl_cmd_health_info_get_temperature(cmd);
-	if (field != 0xffff) {
+	if (field != 0x7fff) {
 		jobj = json_object_new_int(field);
 		if (jobj)
 			json_object_object_add(jhealth, "temperature", jobj);
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index d49b560..e92592d 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -324,7 +324,7 @@ struct cxl_cmd_set_partition {
 #define CXL_CMD_HEALTH_INFO_EXT_CORRECTED_PERSISTENT_WARNING		(1)
 
 #define CXL_CMD_HEALTH_INFO_LIFE_USED_NOT_IMPL				0xff
-#define CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL			0xffff
+#define CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL			0x7fff
 
 static inline int check_kmod(struct kmod_ctx *kmod_ctx)
 {
-- 
2.17.1

Re: [ndctl PATCH RESEND 1/2] cxl: Update a revision by CXL 3.0 specification

[Nathan Fontenot]

On 7/17/23 01:29, Jehoon Park wrote:
> Update the value of device temperature field when it is not implemented.
> (CXL 3.0 8.2.9.8.3.1)
> 
> Signed-off-by: Jehoon Park <jehoon.park@samsung.com>
> ---
>  cxl/json.c        | 2 +-
>  cxl/lib/private.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/cxl/json.c b/cxl/json.c
> index 9a4b5c7..3661eb9 100644
> --- a/cxl/json.c
> +++ b/cxl/json.c
> @@ -155,7 +155,7 @@ static struct json_object *util_cxl_memdev_health_to_json(
>  	}
>  
>  	field = cxl_cmd_health_info_get_temperature(cmd);
> -	if (field != 0xffff) {
> +	if (field != 0x7fff) {

Should you also update this field check to use CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL
instead of using 0x7fff directly?

-Nathan

>  		jobj = json_object_new_int(field);
>  		if (jobj)
>  			json_object_object_add(jhealth, "temperature", jobj);
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index d49b560..e92592d 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -324,7 +324,7 @@ struct cxl_cmd_set_partition {
>  #define CXL_CMD_HEALTH_INFO_EXT_CORRECTED_PERSISTENT_WARNING		(1)
>  
>  #define CXL_CMD_HEALTH_INFO_LIFE_USED_NOT_IMPL				0xff
> -#define CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL			0xffff
> +#define CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL			0x7fff
>  
>  static inline int check_kmod(struct kmod_ctx *kmod_ctx)
>  {

Re: [ndctl PATCH RESEND 1/2] cxl: Update a revision by CXL 3.0 specification

[Jehoon Park]

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

On Mon, Jul 17, 2023 at 08:18:55AM -0500, Nathan Fontenot wrote:
> On 7/17/23 01:29, Jehoon Park wrote:
> > Update the value of device temperature field when it is not implemented.
> > (CXL 3.0 8.2.9.8.3.1)
> > 
> > Signed-off-by: Jehoon Park <jehoon.park@samsung.com>
> > ---
> >  cxl/json.c        | 2 +-
> >  cxl/lib/private.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/cxl/json.c b/cxl/json.c
> > index 9a4b5c7..3661eb9 100644
> > --- a/cxl/json.c
> > +++ b/cxl/json.c
> > @@ -155,7 +155,7 @@ static struct json_object *util_cxl_memdev_health_to_json(
> >  	}
> >  
> >  	field = cxl_cmd_health_info_get_temperature(cmd);
> > -	if (field != 0xffff) {
> > +	if (field != 0x7fff) {
> 
> Should you also update this field check to use CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL
> instead of using 0x7fff directly?
> 
> -Nathan
>

Hi, Nathan

I agree with your suggestion since it is more understandable. However, the
constant macro is defined in "cxl/lib/private.h" which should be included only
under "cxl/lib/" (as I understand properly). To use the macro in json.c,
we have to define it somewhere under "cxl/" e.g. libcxl.h, json.h, ...

I'm not sure about this approach is right, so I followed existing
implementation that used NOT_IMPL value directly.

Jehoon

> >  		jobj = json_object_new_int(field);
> >  		if (jobj)
> >  			json_object_object_add(jhealth, "temperature", jobj);
> > diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> > index d49b560..e92592d 100644
> > --- a/cxl/lib/private.h
> > +++ b/cxl/lib/private.h
> > @@ -324,7 +324,7 @@ struct cxl_cmd_set_partition {
> >  #define CXL_CMD_HEALTH_INFO_EXT_CORRECTED_PERSISTENT_WARNING		(1)
> >  
> >  #define CXL_CMD_HEALTH_INFO_LIFE_USED_NOT_IMPL				0xff
> > -#define CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL			0xffff
> > +#define CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL			0x7fff
> >  
> >  static inline int check_kmod(struct kmod_ctx *kmod_ctx)
> >  {

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

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

[Jehoon Park]

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

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

[Verma, Vishal L]

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

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

[Jehoon Park]

[-- 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 --]

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

[Verma, Vishal L]

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.