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