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

[Srinivasulu Opensrc <sthanneeru.opensrc@micron.com>]

Hi Jonathan,

Thank you, for the inputs, please find comments inline.

Regards,
Srini
> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Thursday, February 8, 2024 6:27 PM
> To: Srinivasulu Opensrc <sthanneeru.opensrc@micron.com>
> Cc: linux-cxl@vger.kernel.org; linux-mm@kvack.org;
> dan.j.williams@intel.com; john@jagalactic.com; Eishan Mirakhur
> <emirakhur@micron.com>; Ajay Joshi <ajayjoshi@micron.com>; Ravis
> OpenSrc <Ravis.OpenSrc@micron.com>; Srinivasulu Thanneeru
> <sthanneeru@micron.com>; shiju.jose@huawei.com
> Subject: [EXT] Re: [PATCH] cxl/mbox: Add Get Log Capabilities, Clear Log and
> Get Supported Logs Sub-List commands
> 
> CAUTION: EXTERNAL EMAIL. Do not click links or open attachments unless
> you recognize the sender and were expecting this message.
> 
> 
> 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?
> 

Sure, I will add description and send V2.

> 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.
> 
Allowing for both of the following.
* 5e1819d9-11a9-400c-811f-d60719403d86 - Vendor Debug Log
* b3fab4cf-01b6-4332-943e-5e9962f23567 - Component State Dump Log

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

Sure, I will split the patches as suggested.

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

Sure, will update "supported log" in all places in the next version V2.

--
Srini

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