* [ndctl PATCH v2 0/3] Fix accessors for temperature field when it is negative
[not found] <CGME20230807063513epcas2p261ba4dfbfff34e99077596128eb6fc48@epcas2p2.samsung.com>
@ 2023-08-07 6:35 ` Jehoon Park
2023-08-07 6:35 ` [ndctl PATCH v2 1/3] libcxl: Update a revision by CXL 3.0 specification Jehoon Park
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jehoon Park @ 2023-08-07 6:35 UTC (permalink / raw)
To: linux-cxl
Cc: nvdimm, Alison Schofield, Vishal Verma, Ira Weiny, Dan Williams,
Dave Jiang, Davidlohr Bueso, Jonathan Cameron, Kyungsan Kim,
Junhyeok Im, 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. INT_MAX is used to indicate errors because negative
errno codes are not distinguishable from the retrieved values when they are
negative. Caller should check errno to know what kind of error occurs.
The third patch fixes the checking value when listing device's health info.
Changes in v2:
- Rebase on the latest pending branch
- Remove dbg() messages in libcxl accessors (Vishal)
- Make signed value accessors to return INT_MAX when error occurs and set
errno as proper errno codes (Vishal)
- Use proper value for checking "life_used" and "device_temperature" fields
are implemented
- Link to v1: https://lore.kernel.org/r/20230717062908.8292-1-jehoon.park@samsung.com/
Jehoon Park (3):
libcxl: Update a revision by CXL 3.0 specification
libcxl: Fix accessors for temperature field to support negative value
cxl: Fix the checking value when listing device's health info
cxl/json.c | 9 +++++----
cxl/lib/libcxl.c | 32 +++++++++++++++++++++-----------
cxl/lib/private.h | 2 +-
3 files changed, 27 insertions(+), 16 deletions(-)
base-commit: a871e6153b11fe63780b37cdcb1eb347b296095c
--
2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [ndctl PATCH v2 1/3] libcxl: Update a revision by CXL 3.0 specification
2023-08-07 6:35 ` [ndctl PATCH v2 0/3] Fix accessors for temperature field when it is negative Jehoon Park
@ 2023-08-07 6:35 ` Jehoon Park
2023-08-07 13:02 ` Jonathan Cameron
2023-08-25 1:45 ` Davidlohr Bueso
2023-08-07 6:35 ` [ndctl PATCH v2 2/3] libcxl: Fix accessors for temperature field to support negative value Jehoon Park
2023-08-07 6:35 ` [ndctl PATCH v2 3/3] cxl: Fix the checking value when listing device's health info Jehoon Park
2 siblings, 2 replies; 9+ messages in thread
From: Jehoon Park @ 2023-08-07 6:35 UTC (permalink / raw)
To: linux-cxl
Cc: nvdimm, Alison Schofield, Vishal Verma, Ira Weiny, Dan Williams,
Dave Jiang, Davidlohr Bueso, Jonathan Cameron, Kyungsan Kim,
Junhyeok Im, Jehoon Park
Update the predefined value for 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/lib/private.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index a641727..a692fd5 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -360,7 +360,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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [ndctl PATCH v2 2/3] libcxl: Fix accessors for temperature field to support negative value
2023-08-07 6:35 ` [ndctl PATCH v2 0/3] Fix accessors for temperature field when it is negative Jehoon Park
2023-08-07 6:35 ` [ndctl PATCH v2 1/3] libcxl: Update a revision by CXL 3.0 specification Jehoon Park
@ 2023-08-07 6:35 ` Jehoon Park
2023-08-07 13:14 ` Jonathan Cameron
2023-08-07 6:35 ` [ndctl PATCH v2 3/3] cxl: Fix the checking value when listing device's health info Jehoon Park
2 siblings, 1 reply; 9+ messages in thread
From: Jehoon Park @ 2023-08-07 6:35 UTC (permalink / raw)
To: linux-cxl
Cc: nvdimm, Alison Schofield, Vishal Verma, Ira Weiny, Dan Williams,
Dave Jiang, Davidlohr Bueso, Jonathan Cameron, Kyungsan Kim,
Junhyeok Im, Jehoon Park
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(-)
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;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [ndctl PATCH v2 3/3] cxl: Fix the checking value when listing device's health info
2023-08-07 6:35 ` [ndctl PATCH v2 0/3] Fix accessors for temperature field when it is negative Jehoon Park
2023-08-07 6:35 ` [ndctl PATCH v2 1/3] libcxl: Update a revision by CXL 3.0 specification Jehoon Park
2023-08-07 6:35 ` [ndctl PATCH v2 2/3] libcxl: Fix accessors for temperature field to support negative value Jehoon Park
@ 2023-08-07 6:35 ` Jehoon Park
2 siblings, 0 replies; 9+ messages in thread
From: Jehoon Park @ 2023-08-07 6:35 UTC (permalink / raw)
To: linux-cxl
Cc: nvdimm, Alison Schofield, Vishal Verma, Ira Weiny, Dan Williams,
Dave Jiang, Davidlohr Bueso, Jonathan Cameron, Kyungsan Kim,
Junhyeok Im, Jehoon Park
Fix the value for checking device's life used and temperature fields are
implemented.
Signed-off-by: Jehoon Park <jehoon.park@samsung.com>
---
cxl/json.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/cxl/json.c b/cxl/json.c
index 7678d02..102bfaf 100644
--- a/cxl/json.c
+++ b/cxl/json.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
// Copyright (C) 2015-2021 Intel Corporation. All rights reserved.
+#include <errno.h>
#include <limits.h>
#include <util/json.h>
#include <uuid/uuid.h>
@@ -238,15 +239,15 @@ static struct json_object *util_cxl_memdev_health_to_json(
json_object_object_add(jhealth, "ext_corrected_persistent", jobj);
/* other fields */
- field = cxl_cmd_health_info_get_life_used(cmd);
- if (field != 0xff) {
- jobj = json_object_new_int(field);
+ rc = cxl_cmd_health_info_get_life_used(cmd);
+ if (rc != -EOPNOTSUPP) {
+ jobj = json_object_new_int(rc);
if (jobj)
json_object_object_add(jhealth, "life_used_percent", jobj);
}
field = cxl_cmd_health_info_get_temperature(cmd);
- if (field != 0xffff) {
+ if (field != INT_MAX) {
jobj = json_object_new_int(field);
if (jobj)
json_object_object_add(jhealth, "temperature", jobj);
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH v2 1/3] libcxl: Update a revision by CXL 3.0 specification
2023-08-07 6:35 ` [ndctl PATCH v2 1/3] libcxl: Update a revision by CXL 3.0 specification Jehoon Park
@ 2023-08-07 13:02 ` Jonathan Cameron
2023-08-08 7:38 ` Jehoon Park
2023-08-25 1:45 ` Davidlohr Bueso
1 sibling, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2023-08-07 13:02 UTC (permalink / raw)
To: Jehoon Park
Cc: linux-cxl, nvdimm, Alison Schofield, Vishal Verma, Ira Weiny,
Dan Williams, Dave Jiang, Davidlohr Bueso, Kyungsan Kim,
Junhyeok Im
On Mon, 7 Aug 2023 15:35:47 +0900
Jehoon Park <jehoon.park@samsung.com> wrote:
> Update the predefined value for 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>
Hi Jehoon,
Key here is not that it was in 3.0, but that it was changed in 2.0 Errata F38
and as such software doesn't need to cope with the old (wrong) value.
Good to state that clearly in the patch description. If it had been merely
a change for 3.0 there would have needed to be an enable bit to change the
default behavior (or something like that).
Otherwise LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> cxl/lib/private.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index a641727..a692fd5 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -360,7 +360,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)
> {
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH v2 2/3] libcxl: Fix accessors for temperature field to support negative value
2023-08-07 6:35 ` [ndctl PATCH v2 2/3] libcxl: Fix accessors for temperature field to support negative value Jehoon Park
@ 2023-08-07 13:14 ` Jonathan Cameron
2023-08-08 7:41 ` Jehoon Park
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2023-08-07 13:14 UTC (permalink / raw)
To: Jehoon Park
Cc: linux-cxl, nvdimm, Alison Schofield, Vishal Verma, Ira Weiny,
Dan Williams, Dave Jiang, Davidlohr Bueso, Kyungsan Kim,
Junhyeok Im
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;
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH v2 1/3] libcxl: Update a revision by CXL 3.0 specification
2023-08-07 13:02 ` Jonathan Cameron
@ 2023-08-08 7:38 ` Jehoon Park
0 siblings, 0 replies; 9+ messages in thread
From: Jehoon Park @ 2023-08-08 7:38 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-cxl, nvdimm, Alison Schofield, Vishal Verma, Ira Weiny,
Dan Williams, Dave Jiang, Davidlohr Bueso, Kyungsan Kim,
Junhyeok Im, Jehoon Park
[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]
On Mon, Aug 07, 2023 at 02:02:06PM +0100, Jonathan Cameron wrote:
> On Mon, 7 Aug 2023 15:35:47 +0900
> Jehoon Park <jehoon.park@samsung.com> wrote:
>
> > Update the predefined value for 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>
> Hi Jehoon,
>
> Key here is not that it was in 3.0, but that it was changed in 2.0 Errata F38
> and as such software doesn't need to cope with the old (wrong) value.
>
> Good to state that clearly in the patch description. If it had been merely
> a change for 3.0 there would have needed to be an enable bit to change the
> default behavior (or something like that).
>
> Otherwise LGTM
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
Hi Jonathan, thanks for the reviews.
I will correct the revision history in the next patch.
> > ---
> > cxl/lib/private.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> > index a641727..a692fd5 100644
> > --- a/cxl/lib/private.h
> > +++ b/cxl/lib/private.h
> > @@ -360,7 +360,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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH v2 2/3] libcxl: Fix accessors for temperature field to support negative value
2023-08-07 13:14 ` Jonathan Cameron
@ 2023-08-08 7:41 ` Jehoon Park
0 siblings, 0 replies; 9+ messages in thread
From: Jehoon Park @ 2023-08-08 7:41 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-cxl, nvdimm, Alison Schofield, Vishal Verma, Ira Weiny,
Dan Williams, Dave Jiang, Davidlohr Bueso, Kyungsan Kim,
Junhyeok Im, Jehoon Park
[-- Attachment #1: Type: text/plain, Size: 4394 bytes --]
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;
> > }
> >
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ndctl PATCH v2 1/3] libcxl: Update a revision by CXL 3.0 specification
2023-08-07 6:35 ` [ndctl PATCH v2 1/3] libcxl: Update a revision by CXL 3.0 specification Jehoon Park
2023-08-07 13:02 ` Jonathan Cameron
@ 2023-08-25 1:45 ` Davidlohr Bueso
1 sibling, 0 replies; 9+ messages in thread
From: Davidlohr Bueso @ 2023-08-25 1:45 UTC (permalink / raw)
To: Jehoon Park
Cc: linux-cxl, nvdimm, Alison Schofield, Vishal Verma, Ira Weiny,
Dan Williams, Dave Jiang, Jonathan Cameron, Kyungsan Kim,
Junhyeok Im
On Mon, 07 Aug 2023, Jehoon Park wrote:
>Update the predefined value for 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>
With Jonathan's feedback in the changelog,
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>---
> cxl/lib/private.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/cxl/lib/private.h b/cxl/lib/private.h
>index a641727..a692fd5 100644
>--- a/cxl/lib/private.h
>+++ b/cxl/lib/private.h
>@@ -360,7 +360,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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-08-25 2:25 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20230807063513epcas2p261ba4dfbfff34e99077596128eb6fc48@epcas2p2.samsung.com>
2023-08-07 6:35 ` [ndctl PATCH v2 0/3] Fix accessors for temperature field when it is negative Jehoon Park
2023-08-07 6:35 ` [ndctl PATCH v2 1/3] libcxl: Update a revision by CXL 3.0 specification Jehoon Park
2023-08-07 13:02 ` Jonathan Cameron
2023-08-08 7:38 ` Jehoon Park
2023-08-25 1:45 ` Davidlohr Bueso
2023-08-07 6:35 ` [ndctl PATCH v2 2/3] libcxl: Fix accessors for temperature field to support negative value Jehoon Park
2023-08-07 13:14 ` Jonathan Cameron
2023-08-08 7:41 ` Jehoon Park
2023-08-07 6:35 ` [ndctl PATCH v2 3/3] cxl: Fix the checking value when listing device's health info Jehoon Park
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.