| Message ID | 20230717062908.8292-3-jehoon.park@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix accessors for temperature field when it is negative | expand |
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; > } >
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; > > } > > >
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 --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; }
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(-)