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(-)

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

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

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 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

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

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

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

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.