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 <[email protected]> > > --- > > 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; > > } > > >
