| Message ID | 20230807063549.5942-3-jehoon.park@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Vishal Verma |
| Headers | show |
| Series | Fix accessors for temperature field when it is negative | expand |
On Mon, 7 Aug 2023 15:35:48 +0900 Jehoon Park <jehoon.park@samsung.com> wrote: > Add a new macro function to retrieve a signed value such as a temperature. > Modify accessors for signed value to return INT_MAX when error occurs and > set errno to corresponding errno codes. None of the callers have been modified to deal with INTMAX until next patch. So I think you need to combine the two to avoid temporary breakage. Also you seem to be also changing the health status. That seems to be unrelated to the negative temperature support so shouldn't really be in same patch. > > Signed-off-by: Jehoon Park <jehoon.park@samsung.com> > --- > cxl/lib/libcxl.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c > index af4ca44..fc64de1 100644 > --- a/cxl/lib/libcxl.c > +++ b/cxl/lib/libcxl.c > @@ -3661,11 +3661,23 @@ 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) { \ > + errno = -rc; \ > + return INT_MAX; \ > + } \ > + 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); > } > > @@ -3673,7 +3685,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); > } > > @@ -3681,7 +3693,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); > } > > @@ -3689,7 +3701,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); > } > > @@ -3905,8 +3917,6 @@ CXL_EXPORT int cxl_cmd_health_info_get_life_used(struct cxl_cmd *cmd) > { > int rc = health_info_get_life_used_raw(cmd); > > - if (rc < 0) > - return rc; Why has this one changed? It's a u8 so not as far as I can see affected by your new signed accessor. > if (rc == CXL_CMD_HEALTH_INFO_LIFE_USED_NOT_IMPL) > return -EOPNOTSUPP; > return rc; > @@ -3914,7 +3924,7 @@ CXL_EXPORT int cxl_cmd_health_info_get_life_used(struct cxl_cmd *cmd) > > 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); > } > > @@ -3922,10 +3932,10 @@ CXL_EXPORT int cxl_cmd_health_info_get_temperature(struct cxl_cmd *cmd) > { > int rc = health_info_get_temperature_raw(cmd); > > - if (rc < 0) > - return rc; > - if (rc == CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL) > - return -EOPNOTSUPP; > + if (rc == CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL) { > + errno = EOPNOTSUPP; > + return INT_MAX; > + } > return rc; > } >
On Mon, Aug 07, 2023 at 02:14:35PM +0100, Jonathan Cameron wrote: > On Mon, 7 Aug 2023 15:35:48 +0900 > Jehoon Park <jehoon.park@samsung.com> wrote: > > > Add a new macro function to retrieve a signed value such as a temperature. > > Modify accessors for signed value to return INT_MAX when error occurs and > > set errno to corresponding errno codes. > > None of the callers have been modified to deal with INTMAX until next patch. > So I think you need to combine the two to avoid temporary breakage. > > Also you seem to be also changing the health status. That seems > to be unrelated to the negative temperature support so shouldn't > really be in same patch. > Thank you for comments, I will re-organize these patches in the next revision. > > > > Signed-off-by: Jehoon Park <jehoon.park@samsung.com> > > --- > > cxl/lib/libcxl.c | 32 +++++++++++++++++++++----------- > > 1 file changed, 21 insertions(+), 11 deletions(-) > > > > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c > > index af4ca44..fc64de1 100644 > > --- a/cxl/lib/libcxl.c > > +++ b/cxl/lib/libcxl.c > > @@ -3661,11 +3661,23 @@ 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) { \ > > + errno = -rc; \ > > + return INT_MAX; \ > > + } \ > > + 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); > > } > > > > @@ -3673,7 +3685,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); > > } > > > > @@ -3681,7 +3693,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); > > } > > > > @@ -3689,7 +3701,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); > > } > > > > @@ -3905,8 +3917,6 @@ CXL_EXPORT int cxl_cmd_health_info_get_life_used(struct cxl_cmd *cmd) > > { > > int rc = health_info_get_life_used_raw(cmd); > > > > - if (rc < 0) > > - return rc; > > Why has this one changed? It's a u8 so not as far as I can see affected by > your new signed accessor. > > I removed it because it was unnecessary code. (No action after error checking) However, as you stated, this code cleaning is irrelevant to this patch. I will revert this in the next patch. > > if (rc == CXL_CMD_HEALTH_INFO_LIFE_USED_NOT_IMPL) > > return -EOPNOTSUPP; > > return rc; > > @@ -3914,7 +3924,7 @@ CXL_EXPORT int cxl_cmd_health_info_get_life_used(struct cxl_cmd *cmd) > > > > 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); > > } > > > > @@ -3922,10 +3932,10 @@ CXL_EXPORT int cxl_cmd_health_info_get_temperature(struct cxl_cmd *cmd) > > { > > int rc = health_info_get_temperature_raw(cmd); > > > > - if (rc < 0) > > - return rc; > > - if (rc == CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL) > > - return -EOPNOTSUPP; > > + if (rc == CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL) { > > + errno = EOPNOTSUPP; > > + return INT_MAX; > > + } > > return rc; > > } > > >
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c index af4ca44..fc64de1 100644 --- a/cxl/lib/libcxl.c +++ b/cxl/lib/libcxl.c @@ -3661,11 +3661,23 @@ 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) { \ + errno = -rc; \ + return INT_MAX; \ + } \ + 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); } @@ -3673,7 +3685,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); } @@ -3681,7 +3693,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); } @@ -3689,7 +3701,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); } @@ -3905,8 +3917,6 @@ CXL_EXPORT int cxl_cmd_health_info_get_life_used(struct cxl_cmd *cmd) { int rc = health_info_get_life_used_raw(cmd); - if (rc < 0) - return rc; if (rc == CXL_CMD_HEALTH_INFO_LIFE_USED_NOT_IMPL) return -EOPNOTSUPP; return rc; @@ -3914,7 +3924,7 @@ CXL_EXPORT int cxl_cmd_health_info_get_life_used(struct cxl_cmd *cmd) 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); } @@ -3922,10 +3932,10 @@ CXL_EXPORT int cxl_cmd_health_info_get_temperature(struct cxl_cmd *cmd) { int rc = health_info_get_temperature_raw(cmd); - if (rc < 0) - return rc; - if (rc == CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL) - return -EOPNOTSUPP; + if (rc == CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL) { + errno = EOPNOTSUPP; + return INT_MAX; + } return rc; }
Add a new macro function to retrieve a signed value such as a temperature. Modify accessors for signed value to return INT_MAX when error occurs and set errno to corresponding errno codes. Signed-off-by: Jehoon Park <jehoon.park@samsung.com> --- cxl/lib/libcxl.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)