Re: [PATCH] cxl/mbox: Add Get Log Capabilities, Clear Log and Get Supported Logs Sub-List commands

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 7 Feb 2024 16:06:34 +0530
<sthanneeru.opensrc@micron.com> wrote:

> From: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
> 
> Adding UAPI support for
> 1. CXL r3.1 8.2.9.5.3 Get Log Capabilities.
> 2. CXL r3.1 8.2.9.5.4 Clear Log commands.
> 3. CXL r3.1 8.2.9.5.6 Get Supported Logs Sub-List.
> 
> Signed-off-by: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>

Hi Srinivasulu,

Whilst I can conjecture some valid reasons to expose these to
userspace, can you add some examples to this patch description?

We might want to filter the clear in particular to avoid a clash
with the driver log handling. That is only allow it for vendor
logs.

Perhaps split the patch into 2 parts. The less controversial
GET_LOG_CAPS and GET_LOG_SUBLIST, followed by a patch for the
destructive clear log.

The memory scrub handling might well
access the ECS log for example (I don't think the current proposal
yet does this).

Jonathan


> ---
>  drivers/cxl/core/mbox.c      | 3 +++
>  drivers/cxl/cxlmem.h         | 3 +++
>  include/uapi/linux/cxl_mem.h | 3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 27166a411705..64a44e286488 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -64,6 +64,9 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>  	CXL_CMD(SET_SHUTDOWN_STATE, 0x1, 0, 0),
>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>  	CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0),
> +	CXL_CMD(GET_LOG_CAPS, 0x10, 0x4, 0),
> +	CXL_CMD(CLEAR_LOG, 0x10, 0, 0),
> +	CXL_CMD(GET_LOG_SUBLIST, 0x2, CXL_VARIABLE_PAYLOAD, 0),
>  };
>  
>  /*
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 5303d6942b88..4128c810051c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -529,6 +529,9 @@ enum cxl_opcode {
>  	CXL_MBOX_OP_SET_TIMESTAMP	= 0x0301,
>  	CXL_MBOX_OP_GET_SUPPORTED_LOGS	= 0x0400,
>  	CXL_MBOX_OP_GET_LOG		= 0x0401,
> +	CXL_MBOX_OP_GET_LOG_CAPS	= 0x0402,
> +	CXL_MBOX_OP_CLEAR_LOG		= 0x0403,
> +	CXL_MBOX_OP_GET_LOG_SUBLIST     = 0x0405,

Name should include something to make it clear this is getting
sublist of 'supported' logs.  Not the log.
 
>  	CXL_MBOX_OP_IDENTIFY		= 0x4000,
>  	CXL_MBOX_OP_GET_PARTITION_INFO	= 0x4100,
>  	CXL_MBOX_OP_SET_PARTITION_INFO	= 0x4101,
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 42066f4eb890..d2df9782a5ef 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -47,6 +47,9 @@
>  	___DEPRECATED(SCAN_MEDIA, "Scan Media"),                          \
>  	___DEPRECATED(GET_SCAN_MEDIA, "Get Scan Media Results"),          \
>  	___C(GET_TIMESTAMP, "Get Timestamp"),                             \
> +	___C(GET_LOG_CAPS, "Get Log Capabilities"),			  \
> +	___C(CLEAR_LOG, "Clear Log"),					  \
> +	___C(GET_LOG_SUBLIST, "Get Log Sublist"),			  \

Likewise, mention it's list of supported logs.

>  	___C(MAX, "invalid / last command")
>  
>  #define ___C(a, b) CXL_MEM_COMMAND_ID_##a