| Message ID | 20230119130450.107519-1-rrichter@amd.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl/mbox: Add debug messages for supported mailbox commands | expand |
On Thu, Jan 19, 2023 at 02:04:50PM +0100, Robert Richter wrote: > Only unsupported mailbox commands are reported in debug messages. A > list of supported commands is useful too. Change debug messages to > also report the opcodes of supported commands. Hi Robert, I wonder if you can get this info another way. When I try this loading cxl_test today, I get 99 new messages. Is this going to create too much noise with debug kernels? Alison > > On that occasion also add missing trailing newlines. > > Signed-off-by: Robert Richter <rrichter@amd.com> > --- > drivers/cxl/core/mbox.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index a48ade466d6a..ffa9f84c2dce 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -629,11 +629,12 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel) > > if (!cmd) { > dev_dbg(cxlds->dev, > - "Opcode 0x%04x unsupported by driver", opcode); > + "Opcode 0x%04x unsupported by driver\n", opcode); > continue; > } > > set_bit(cmd->info.id, cxlds->enabled_cmds); > + dev_dbg(cxlds->dev, "Opcode 0x%04x supported by driver\n", opcode); > } > } > > > base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2 > -- > 2.30.2 >
Hi Alison, On 22.01.23 21:39:33, Alison Schofield wrote: > On Thu, Jan 19, 2023 at 02:04:50PM +0100, Robert Richter wrote: > > Only unsupported mailbox commands are reported in debug messages. A > > list of supported commands is useful too. Change debug messages to > > also report the opcodes of supported commands. > > Hi Robert, > I wonder if you can get this info another way. When I try this > loading cxl_test today, I get 99 new messages. Is this going to > create too much noise with debug kernels? There are 26 commands supported by the driver, so I assume there are at least 4 cards in your system? To me the number of messages looks ok for a debug kernel. And, most kernels have dyndbg enabled allowing to enable only messages of interest? Esp. if card initialization fails there is no way to get this information from userland. The list of unsupported commands is of less use than the one for supported. That is the intention for the change. Thanks, -Robert
On Thu, 19 Jan 2023 14:04:50 +0100 Robert Richter <rrichter@amd.com> wrote: > Only unsupported mailbox commands are reported in debug messages. A > list of supported commands is useful too. Change debug messages to > also report the opcodes of supported commands. > > On that occasion also add missing trailing newlines. > > Signed-off-by: Robert Richter <rrichter@amd.com> Seems reasonable to me. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/mbox.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index a48ade466d6a..ffa9f84c2dce 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -629,11 +629,12 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel) > > if (!cmd) { > dev_dbg(cxlds->dev, > - "Opcode 0x%04x unsupported by driver", opcode); > + "Opcode 0x%04x unsupported by driver\n", opcode); > continue; > } > > set_bit(cmd->info.id, cxlds->enabled_cmds); > + dev_dbg(cxlds->dev, "Opcode 0x%04x supported by driver\n", opcode); > } > } > > > base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
On Mon, Jan 23, 2023 at 01:32:55PM +0100, Robert Richter wrote: > Hi Alison, > > On 22.01.23 21:39:33, Alison Schofield wrote: > > On Thu, Jan 19, 2023 at 02:04:50PM +0100, Robert Richter wrote: > > > Only unsupported mailbox commands are reported in debug messages. A > > > list of supported commands is useful too. Change debug messages to > > > also report the opcodes of supported commands. > > > > Hi Robert, > > I wonder if you can get this info another way. When I try this > > loading cxl_test today, I get 99 new messages. Is this going to > > create too much noise with debug kernels? > > There are 26 commands supported by the driver, so I assume there are > at least 4 cards in your system? To me the number of messages looks ok > for a debug kernel. And, most kernels have dyndbg enabled allowing to > enable only messages of interest? Esp. if card initialization fails > there is no way to get this information from userland. The list of > unsupported commands is of less use than the one for supported. That > is the intention for the change. cxl_walk_cel() job is to create the enabled_cmds list for the device. How about we use that language in the message, like: set_bit(cmd->info.id, cxlds->enabled_cmds); - dev_dbg(cxlds->dev, "Opcode 0x%04x supported by driver\n", opcode); + dev_dbg(cxlds->dev, "Opcode 0x%04x enabled\n", opcode); Because when we say, "Opcode 0x%04x supported by driver\n", that comes with the assumption that the device supported it too. By saying 'enabled', it's clear device and driver are aligned. > > Thanks, > > -Robert
On 23.01.23 11:26:55, Alison Schofield wrote: > On Mon, Jan 23, 2023 at 01:32:55PM +0100, Robert Richter wrote: > > Hi Alison, > > > > On 22.01.23 21:39:33, Alison Schofield wrote: > > > On Thu, Jan 19, 2023 at 02:04:50PM +0100, Robert Richter wrote: > > > > Only unsupported mailbox commands are reported in debug messages. A > > > > list of supported commands is useful too. Change debug messages to > > > > also report the opcodes of supported commands. > > > > > > Hi Robert, > > > I wonder if you can get this info another way. When I try this > > > loading cxl_test today, I get 99 new messages. Is this going to > > > create too much noise with debug kernels? > > > > There are 26 commands supported by the driver, so I assume there are > > at least 4 cards in your system? To me the number of messages looks ok > > for a debug kernel. And, most kernels have dyndbg enabled allowing to > > enable only messages of interest? Esp. if card initialization fails > > there is no way to get this information from userland. The list of > > unsupported commands is of less use than the one for supported. That > > is the intention for the change. > > cxl_walk_cel() job is to create the enabled_cmds list for the device. > How about we use that language in the message, like: > > set_bit(cmd->info.id, cxlds->enabled_cmds); > - dev_dbg(cxlds->dev, "Opcode 0x%04x supported by driver\n", opcode); > + dev_dbg(cxlds->dev, "Opcode 0x%04x enabled\n", opcode); > > Because when we say, "Opcode 0x%04x supported by driver\n", that comes > with the assumption that the device supported it too. By saying > 'enabled', it's clear device and driver are aligned. Yes, that message is more meaningful. Thanks, -Robert
On 1/19/23 6:04 AM, Robert Richter wrote: > Only unsupported mailbox commands are reported in debug messages. A > list of supported commands is useful too. Change debug messages to > also report the opcodes of supported commands. > > On that occasion also add missing trailing newlines. > > Signed-off-by: Robert Richter <rrichter@amd.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/mbox.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index a48ade466d6a..ffa9f84c2dce 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -629,11 +629,12 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel) > > if (!cmd) { > dev_dbg(cxlds->dev, > - "Opcode 0x%04x unsupported by driver", opcode); > + "Opcode 0x%04x unsupported by driver\n", opcode); > continue; > } > > set_bit(cmd->info.id, cxlds->enabled_cmds); > + dev_dbg(cxlds->dev, "Opcode 0x%04x supported by driver\n", opcode); > } > } > > > base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
Robert Richter wrote: > Hi Alison, > > On 22.01.23 21:39:33, Alison Schofield wrote: > > On Thu, Jan 19, 2023 at 02:04:50PM +0100, Robert Richter wrote: > > > Only unsupported mailbox commands are reported in debug messages. A > > > list of supported commands is useful too. Change debug messages to > > > also report the opcodes of supported commands. > > > > Hi Robert, > > I wonder if you can get this info another way. When I try this > > loading cxl_test today, I get 99 new messages. Is this going to > > create too much noise with debug kernels? > > There are 26 commands supported by the driver, so I assume there are > at least 4 cards in your system? To me the number of messages looks ok > for a debug kernel. And, most kernels have dyndbg enabled allowing to > enable only messages of interest? Esp. if card initialization fails > there is no way to get this information from userland. The list of > unsupported commands is of less use than the one for supported. That > is the intention for the change. The debug message looks ok to me, I will just note that there has been consideration for exporting the enabled commands list via CXL_MEM_QUERY_COMMANDS [1]. I wouldn't be opposed to someone enabling that as well, just need to be clear with userspace that not all kernels will populate that status. [1]: http://lore.kernel.org/r/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch
Robert Richter wrote: > On 23.01.23 11:26:55, Alison Schofield wrote: > > On Mon, Jan 23, 2023 at 01:32:55PM +0100, Robert Richter wrote: > > > Hi Alison, > > > > > > On 22.01.23 21:39:33, Alison Schofield wrote: > > > > On Thu, Jan 19, 2023 at 02:04:50PM +0100, Robert Richter wrote: > > > > > Only unsupported mailbox commands are reported in debug messages. A > > > > > list of supported commands is useful too. Change debug messages to > > > > > also report the opcodes of supported commands. > > > > > > > > Hi Robert, > > > > I wonder if you can get this info another way. When I try this > > > > loading cxl_test today, I get 99 new messages. Is this going to > > > > create too much noise with debug kernels? > > > > > > There are 26 commands supported by the driver, so I assume there are > > > at least 4 cards in your system? To me the number of messages looks ok > > > for a debug kernel. And, most kernels have dyndbg enabled allowing to > > > enable only messages of interest? Esp. if card initialization fails > > > there is no way to get this information from userland. The list of > > > unsupported commands is of less use than the one for supported. That > > > is the intention for the change. > > > > cxl_walk_cel() job is to create the enabled_cmds list for the device. > > How about we use that language in the message, like: > > > > set_bit(cmd->info.id, cxlds->enabled_cmds); > > - dev_dbg(cxlds->dev, "Opcode 0x%04x supported by driver\n", opcode); > > + dev_dbg(cxlds->dev, "Opcode 0x%04x enabled\n", opcode); > > > > Because when we say, "Opcode 0x%04x supported by driver\n", that comes > > with the assumption that the device supported it too. By saying > > 'enabled', it's clear device and driver are aligned. > > Yes, that message is more meaningful. > Applied with this fixup.
Dan Williams wrote: > Robert Richter wrote: [snip] > > The debug message looks ok to me, I will just note that there has been > consideration for exporting the enabled commands list via > CXL_MEM_QUERY_COMMANDS [1]. I wouldn't be opposed to someone enabling that > as well, just need to be clear with userspace that not all kernels will > populate that status. > > [1]: http://lore.kernel.org/r/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch Sent as a v2 of 'CXL Misc fixes' Ira
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index a48ade466d6a..ffa9f84c2dce 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -629,11 +629,12 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel) if (!cmd) { dev_dbg(cxlds->dev, - "Opcode 0x%04x unsupported by driver", opcode); + "Opcode 0x%04x unsupported by driver\n", opcode); continue; } set_bit(cmd->info.id, cxlds->enabled_cmds); + dev_dbg(cxlds->dev, "Opcode 0x%04x supported by driver\n", opcode); } }
Only unsupported mailbox commands are reported in debug messages. A list of supported commands is useful too. Change debug messages to also report the opcodes of supported commands. On that occasion also add missing trailing newlines. Signed-off-by: Robert Richter <rrichter@amd.com> --- drivers/cxl/core/mbox.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2