* [PATCH] cxl/mbox: Fix CEL logic for poison and security commands
@ 2023-09-03 21:42 Ira Weiny
2023-09-04 15:19 ` Jonathan Cameron
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ira Weiny @ 2023-09-03 21:42 UTC (permalink / raw)
To: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
Vishal Verma, Dan Williams
Cc: linux-cxl, linux-kernel, Ira Weiny
The following debug output was observed while testing CXL
cxl_core:cxl_walk_cel:721: cxl_mock_mem cxl_mem.0: Opcode 0x4300 unsupported by driver
opcode 0x4300 (Get Poison) is supported by the driver and the mock
device supports it. The logic should be checking that the opcode is
both not poison and not security.
Fix the logic to allow poison and security commands.
Fixes: ad64f5952ce3 ("cxl/memdev: Only show sanitize sysfs files when supported")
Cc: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
drivers/cxl/core/mbox.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index ca60bb8114f2..b315bdab9197 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -716,8 +716,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
u16 opcode = le16_to_cpu(cel_entry[i].opcode);
struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
- if (!cmd && (!cxl_is_poison_command(opcode) ||
- !cxl_is_security_command(opcode))) {
+ if (!cmd && !cxl_is_poison_command(opcode) &&
+ !cxl_is_security_command(opcode)) {
dev_dbg(dev,
"Opcode 0x%04x unsupported by driver\n", opcode);
continue;
---
base-commit: 1c59d383390f970b891b503b7f79b63a02db2ec5
change-id: 20230903-cxl-cel-fix-9da269bf0f21
Best regards,
--
Ira Weiny <ira.weiny@intel.com>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl/mbox: Fix CEL logic for poison and security commands
2023-09-03 21:42 [PATCH] cxl/mbox: Fix CEL logic for poison and security commands Ira Weiny
@ 2023-09-04 15:19 ` Jonathan Cameron
2023-09-04 17:24 ` Davidlohr Bueso
2023-09-12 22:39 ` Dan Williams
2 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2023-09-04 15:19 UTC (permalink / raw)
To: Ira Weiny
Cc: Davidlohr Bueso, Dave Jiang, Alison Schofield, Vishal Verma,
Dan Williams, linux-cxl, linux-kernel
On Sun, 03 Sep 2023 14:42:58 -0700
Ira Weiny <ira.weiny@intel.com> wrote:
> The following debug output was observed while testing CXL
>
> cxl_core:cxl_walk_cel:721: cxl_mock_mem cxl_mem.0: Opcode 0x4300 unsupported by driver
>
> opcode 0x4300 (Get Poison) is supported by the driver and the mock
> device supports it. The logic should be checking that the opcode is
> both not poison and not security.
>
> Fix the logic to allow poison and security commands.
>
> Fixes: ad64f5952ce3 ("cxl/memdev: Only show sanitize sysfs files when supported")
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Makes sense.
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/cxl/core/mbox.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index ca60bb8114f2..b315bdab9197 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -716,8 +716,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
> u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>
> - if (!cmd && (!cxl_is_poison_command(opcode) ||
> - !cxl_is_security_command(opcode))) {
> + if (!cmd && !cxl_is_poison_command(opcode) &&
> + !cxl_is_security_command(opcode)) {
> dev_dbg(dev,
> "Opcode 0x%04x unsupported by driver\n", opcode);
> continue;
>
> ---
> base-commit: 1c59d383390f970b891b503b7f79b63a02db2ec5
> change-id: 20230903-cxl-cel-fix-9da269bf0f21
>
> Best regards,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl/mbox: Fix CEL logic for poison and security commands
2023-09-03 21:42 [PATCH] cxl/mbox: Fix CEL logic for poison and security commands Ira Weiny
2023-09-04 15:19 ` Jonathan Cameron
@ 2023-09-04 17:24 ` Davidlohr Bueso
2023-09-12 22:39 ` Dan Williams
2 siblings, 0 replies; 6+ messages in thread
From: Davidlohr Bueso @ 2023-09-04 17:24 UTC (permalink / raw)
To: Ira Weiny
Cc: Jonathan Cameron, Dave Jiang, Alison Schofield, Vishal Verma,
Dan Williams, linux-cxl, linux-kernel
On Sun, 03 Sep 2023, Ira Weiny wrote:
>The following debug output was observed while testing CXL
>
>cxl_core:cxl_walk_cel:721: cxl_mock_mem cxl_mem.0: Opcode 0x4300 unsupported by driver
>
>opcode 0x4300 (Get Poison) is supported by the driver and the mock
>device supports it. The logic should be checking that the opcode is
>both not poison and not security.
>
>Fix the logic to allow poison and security commands.
Hmm yeah sorry about that.
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
>
>Fixes: ad64f5952ce3 ("cxl/memdev: Only show sanitize sysfs files when supported")
>Cc: Davidlohr Bueso <dave@stgolabs.net>
>Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>---
> drivers/cxl/core/mbox.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>index ca60bb8114f2..b315bdab9197 100644
>--- a/drivers/cxl/core/mbox.c
>+++ b/drivers/cxl/core/mbox.c
>@@ -716,8 +716,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
> u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>
>- if (!cmd && (!cxl_is_poison_command(opcode) ||
>- !cxl_is_security_command(opcode))) {
>+ if (!cmd && !cxl_is_poison_command(opcode) &&
>+ !cxl_is_security_command(opcode)) {
> dev_dbg(dev,
> "Opcode 0x%04x unsupported by driver\n", opcode);
> continue;
>
>---
>base-commit: 1c59d383390f970b891b503b7f79b63a02db2ec5
>change-id: 20230903-cxl-cel-fix-9da269bf0f21
>
>Best regards,
>--
>Ira Weiny <ira.weiny@intel.com>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] cxl/mbox: Fix CEL logic for poison and security commands
2023-09-03 21:42 [PATCH] cxl/mbox: Fix CEL logic for poison and security commands Ira Weiny
2023-09-04 15:19 ` Jonathan Cameron
2023-09-04 17:24 ` Davidlohr Bueso
@ 2023-09-12 22:39 ` Dan Williams
2023-09-13 18:17 ` Ira Weiny
2023-09-13 19:45 ` Davidlohr Bueso
2 siblings, 2 replies; 6+ messages in thread
From: Dan Williams @ 2023-09-12 22:39 UTC (permalink / raw)
To: Ira Weiny, Davidlohr Bueso, Jonathan Cameron, Dave Jiang,
Alison Schofield, Vishal Verma, Dan Williams
Cc: linux-cxl, linux-kernel, Ira Weiny
Ira Weiny wrote:
> The following debug output was observed while testing CXL
>
> cxl_core:cxl_walk_cel:721: cxl_mock_mem cxl_mem.0: Opcode 0x4300 unsupported by driver
>
> opcode 0x4300 (Get Poison) is supported by the driver and the mock
> device supports it. The logic should be checking that the opcode is
> both not poison and not security.
>
> Fix the logic to allow poison and security commands.
>
> Fixes: ad64f5952ce3 ("cxl/memdev: Only show sanitize sysfs files when supported")
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
> drivers/cxl/core/mbox.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index ca60bb8114f2..b315bdab9197 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -716,8 +716,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
> u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>
> - if (!cmd && (!cxl_is_poison_command(opcode) ||
> - !cxl_is_security_command(opcode))) {
> + if (!cmd && !cxl_is_poison_command(opcode) &&
> + !cxl_is_security_command(opcode)) {
Given that this is going to be a recurring pattern to add optional
command support, I am not a fan of continuing to expand this boolean
algebra eye exam.
How about the following to only do the validation check and enabling in
one place:
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index ca60bb8114f2..4df4f614f490 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -715,24 +715,25 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
for (i = 0; i < cel_entries; i++) {
u16 opcode = le16_to_cpu(cel_entry[i].opcode);
struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
+ int enabled = 0;
- if (!cmd && (!cxl_is_poison_command(opcode) ||
- !cxl_is_security_command(opcode))) {
- dev_dbg(dev,
- "Opcode 0x%04x unsupported by driver\n", opcode);
- continue;
- }
-
- if (cmd)
+ if (cmd) {
set_bit(cmd->info.id, mds->enabled_cmds);
+ enabled++;
+ }
- if (cxl_is_poison_command(opcode))
+ if (cxl_is_poison_command(opcode)) {
cxl_set_poison_cmd_enabled(&mds->poison, opcode);
+ enabled++;
+ }
- if (cxl_is_security_command(opcode))
+ if (cxl_is_security_command(opcode)) {
cxl_set_security_cmd_enabled(&mds->security, opcode);
+ enabled++;
+ }
- dev_dbg(dev, "Opcode 0x%04x enabled\n", opcode);
+ dev_dbg(dev, "Opcode 0x%04x %s\n", opcode,
+ enabled ? "enabled" : "unsupported by driver");
}
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH] cxl/mbox: Fix CEL logic for poison and security commands
2023-09-12 22:39 ` Dan Williams
@ 2023-09-13 18:17 ` Ira Weiny
2023-09-13 19:45 ` Davidlohr Bueso
1 sibling, 0 replies; 6+ messages in thread
From: Ira Weiny @ 2023-09-13 18:17 UTC (permalink / raw)
To: Dan Williams, Ira Weiny, Davidlohr Bueso, Jonathan Cameron,
Dave Jiang, Alison Schofield, Vishal Verma
Cc: linux-cxl, linux-kernel, Ira Weiny
Dan Williams wrote:
> Ira Weiny wrote:
> > The following debug output was observed while testing CXL
> >
> > cxl_core:cxl_walk_cel:721: cxl_mock_mem cxl_mem.0: Opcode 0x4300 unsupported by driver
> >
> > opcode 0x4300 (Get Poison) is supported by the driver and the mock
> > device supports it. The logic should be checking that the opcode is
> > both not poison and not security.
> >
> > Fix the logic to allow poison and security commands.
> >
> > Fixes: ad64f5952ce3 ("cxl/memdev: Only show sanitize sysfs files when supported")
> > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> > drivers/cxl/core/mbox.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index ca60bb8114f2..b315bdab9197 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -716,8 +716,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
> > u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> > struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
> >
> > - if (!cmd && (!cxl_is_poison_command(opcode) ||
> > - !cxl_is_security_command(opcode))) {
> > + if (!cmd && !cxl_is_poison_command(opcode) &&
> > + !cxl_is_security_command(opcode)) {
>
> Given that this is going to be a recurring pattern to add optional
> command support, I am not a fan of continuing to expand this boolean
> algebra eye exam.
>
> How about the following to only do the validation check and enabling in
> one place:
I like it.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index ca60bb8114f2..4df4f614f490 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -715,24 +715,25 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
> for (i = 0; i < cel_entries; i++) {
> u16 opcode = le16_to_cpu(cel_entry[i].opcode);
> struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
> + int enabled = 0;
>
> - if (!cmd && (!cxl_is_poison_command(opcode) ||
> - !cxl_is_security_command(opcode))) {
> - dev_dbg(dev,
> - "Opcode 0x%04x unsupported by driver\n", opcode);
> - continue;
> - }
> -
> - if (cmd)
> + if (cmd) {
> set_bit(cmd->info.id, mds->enabled_cmds);
> + enabled++;
> + }
>
> - if (cxl_is_poison_command(opcode))
> + if (cxl_is_poison_command(opcode)) {
> cxl_set_poison_cmd_enabled(&mds->poison, opcode);
> + enabled++;
> + }
>
> - if (cxl_is_security_command(opcode))
> + if (cxl_is_security_command(opcode)) {
> cxl_set_security_cmd_enabled(&mds->security, opcode);
> + enabled++;
> + }
>
> - dev_dbg(dev, "Opcode 0x%04x enabled\n", opcode);
> + dev_dbg(dev, "Opcode 0x%04x %s\n", opcode,
> + enabled ? "enabled" : "unsupported by driver");
> }
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl/mbox: Fix CEL logic for poison and security commands
2023-09-12 22:39 ` Dan Williams
2023-09-13 18:17 ` Ira Weiny
@ 2023-09-13 19:45 ` Davidlohr Bueso
1 sibling, 0 replies; 6+ messages in thread
From: Davidlohr Bueso @ 2023-09-13 19:45 UTC (permalink / raw)
To: Dan Williams
Cc: Ira Weiny, Jonathan Cameron, Dave Jiang, Alison Schofield,
Vishal Verma, linux-cxl, linux-kernel
On Tue, 12 Sep 2023, Dan Williams wrote:
>Ira Weiny wrote:
>> The following debug output was observed while testing CXL
>>
>> cxl_core:cxl_walk_cel:721: cxl_mock_mem cxl_mem.0: Opcode 0x4300 unsupported by driver
>>
>> opcode 0x4300 (Get Poison) is supported by the driver and the mock
>> device supports it. The logic should be checking that the opcode is
>> both not poison and not security.
>>
>> Fix the logic to allow poison and security commands.
>>
>> Fixes: ad64f5952ce3 ("cxl/memdev: Only show sanitize sysfs files when supported")
>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>> ---
>> drivers/cxl/core/mbox.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index ca60bb8114f2..b315bdab9197 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -716,8 +716,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>> u16 opcode = le16_to_cpu(cel_entry[i].opcode);
>> struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
>>
>> - if (!cmd && (!cxl_is_poison_command(opcode) ||
>> - !cxl_is_security_command(opcode))) {
>> + if (!cmd && !cxl_is_poison_command(opcode) &&
>> + !cxl_is_security_command(opcode)) {
>
>Given that this is going to be a recurring pattern to add optional
>command support, I am not a fan of continuing to expand this boolean
>algebra eye exam.
>
>How about the following to only do the validation check and enabling in
>one place:
Agreed.
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
... this also is a reminder of the need for regression testing/CI.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-13 19:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-03 21:42 [PATCH] cxl/mbox: Fix CEL logic for poison and security commands Ira Weiny
2023-09-04 15:19 ` Jonathan Cameron
2023-09-04 17:24 ` Davidlohr Bueso
2023-09-12 22:39 ` Dan Williams
2023-09-13 18:17 ` Ira Weiny
2023-09-13 19:45 ` Davidlohr Bueso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox