[PATCH 0/3] CXL: Miscellaneous fixes

[PATCH 0/3] CXL: Miscellaneous fixes

[Ira Weiny]

These are minor fixes I have seen along the way in the CXL code.

To: "Dan Williams" <dan.j.williams@intel.com>
Cc: "Jiang, Dave" <dave.jiang@intel.com>
Cc: "Alison Schofield" <alison.schofield@intel.com>
Cc: "Vishal Verma" <vishal.l.verma@intel.com>
Cc: "Ben Widawsky" <bwidawsk@kernel.org>
Cc: linux-cxl@vger.kernel.org
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Ira Weiny (3):
      cxl/mem: Fix command comment
      cxl/mem: Remove unused CXL_CMD_FLAG_NONE define
      cxl/uapi: Add warning on CXL command enum

 drivers/cxl/cxlmem.h         |  1 -
 include/uapi/linux/cxl_mem.h | 10 +++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)
---
base-commit: 8395ae05cb5a2e31d36106e8c85efa11cda849be
change-id: 20221222-cxl-misc-793ec2442455

Best regards,
-- 
Ira Weiny <ira.weiny@intel.com>

[PATCH 1/3] cxl/mem: Fix command comment

[Ira Weiny]

The command comment had some minor grammatical errors.

Fix them.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 include/uapi/linux/cxl_mem.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index c71021a2a9ed..555f9140e2bc 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -11,9 +11,9 @@
 /**
  * DOC: UAPI
  *
- * Not all of all commands that the driver supports are always available for use
- * by userspace. Userspace must check the results from the QUERY command in
- * order to determine the live set of commands.
+ * Not all of the commands that the driver supports are available for use by
+ * userspace at all times.  Userspace must check the results from the QUERY
+ * command in order to determine the live set of commands.
  */
 
 #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands)

-- 
2.38.1

RE: [PATCH 1/3] cxl/mem: Fix command comment

[Dan Williams]

Ira Weiny wrote:
> The command comment had some minor grammatical errors.
> 
> Fix them.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  include/uapi/linux/cxl_mem.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index c71021a2a9ed..555f9140e2bc 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -11,9 +11,9 @@
>  /**
>   * DOC: UAPI
>   *
> - * Not all of all commands that the driver supports are always available for use
> - * by userspace. Userspace must check the results from the QUERY command in
> - * order to determine the live set of commands.
> + * Not all of the commands that the driver supports are available for use by
> + * userspace at all times.  Userspace must check the results from the QUERY
> + * command in order to determine the live set of commands.
>   */

It's interesting that these grammatical fixups further highlight that
the existing description was a lie. This new wording makes it seem like
QUERY informs about temporarily disabled commands (like those in the
cxlds->exclusive_cmds set), in addition to the device enabled commands
(those in the cxlds->enabled_cmds set).

It turns out this has always been a lie and the cxl tool checks if a
command is supported and enabled by trying to execute it if it exists in
the cxl_query_cmd() payload.

Now we could either go fix that, or change this comment to reflect the
current reality that cxl_command_info.flags is always zero and the
command info array is just telling you if the driver knows how to
attempt the given command.

>  
>  #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands)
> 
> -- 
> 2.38.1

Re: [PATCH 1/3] cxl/mem: Fix command comment

[Jonathan Cameron]

On Tue, 3 Jan 2023 19:02:38 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Ira Weiny wrote:
> > The command comment had some minor grammatical errors.
> > 
> > Fix them.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  include/uapi/linux/cxl_mem.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> > index c71021a2a9ed..555f9140e2bc 100644
> > --- a/include/uapi/linux/cxl_mem.h
> > +++ b/include/uapi/linux/cxl_mem.h
> > @@ -11,9 +11,9 @@
> >  /**
> >   * DOC: UAPI
> >   *
> > - * Not all of all commands that the driver supports are always available for use
> > - * by userspace. Userspace must check the results from the QUERY command in
> > - * order to determine the live set of commands.
> > + * Not all of the commands that the driver supports are available for use by
> > + * userspace at all times.  Userspace must check the results from the QUERY
> > + * command in order to determine the live set of commands.
> >   */  
> 
> It's interesting that these grammatical fixups further highlight that
> the existing description was a lie. This new wording makes it seem like
> QUERY informs about temporarily disabled commands (like those in the
> cxlds->exclusive_cmds set), in addition to the device enabled commands
> (those in the cxlds->enabled_cmds set).
> 
> It turns out this has always been a lie and the cxl tool checks if a
> command is supported and enabled by trying to execute it if it exists in
> the cxl_query_cmd() payload.
> 
> Now we could either go fix that, or change this comment to reflect the
> current reality that cxl_command_info.flags is always zero and the
> command info array is just telling you if the driver knows how to
> attempt the given command.

I'd really like to see a query mechanism that reflects whether the hardware
supports the command (plus all the other reasons why we might not be able to use
it - ultimately the question is 'can I issue this'). Some commands may
'take a while' so it's not nice to have to issue them to find out if they
exist given the discoverable nature hardware side.

Possible that mechanism is different from this one though...

Jonathan

> 
> >  
> >  #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands)
> > 
> > -- 
> > 2.38.1  
> 
> 

Re: [PATCH 1/3] cxl/mem: Fix command comment

[Ira Weiny]

On Thu, Jan 05, 2023 at 05:36:26PM +0000, Jonathan Cameron wrote:
> On Tue, 3 Jan 2023 19:02:38 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Ira Weiny wrote:
> > > The command comment had some minor grammatical errors.
> > > 
> > > Fix them.
> > > 
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > ---
> > >  include/uapi/linux/cxl_mem.h | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> > > index c71021a2a9ed..555f9140e2bc 100644
> > > --- a/include/uapi/linux/cxl_mem.h
> > > +++ b/include/uapi/linux/cxl_mem.h
> > > @@ -11,9 +11,9 @@
> > >  /**
> > >   * DOC: UAPI
> > >   *
> > > - * Not all of all commands that the driver supports are always available for use
> > > - * by userspace. Userspace must check the results from the QUERY command in
> > > - * order to determine the live set of commands.
> > > + * Not all of the commands that the driver supports are available for use by
> > > + * userspace at all times.  Userspace must check the results from the QUERY
> > > + * command in order to determine the live set of commands.
> > >   */  
> > 
> > It's interesting that these grammatical fixups further highlight that
> > the existing description was a lie. This new wording makes it seem like
> > QUERY informs about temporarily disabled commands (like those in the
> > cxlds->exclusive_cmds set), in addition to the device enabled commands
> > (those in the cxlds->enabled_cmds set).
> > 
> > It turns out this has always been a lie and the cxl tool checks if a
> > command is supported and enabled by trying to execute it if it exists in
> > the cxl_query_cmd() payload.
> > 
> > Now we could either go fix that, or change this comment to reflect the
> > current reality that cxl_command_info.flags is always zero and the
> > command info array is just telling you if the driver knows how to
> > attempt the given command.
> 
> I'd really like to see a query mechanism that reflects whether the hardware
> supports the command (plus all the other reasons why we might not be able to use
> it - ultimately the question is 'can I issue this'). Some commands may
> 'take a while' so it's not nice to have to issue them to find out if they
> exist given the discoverable nature hardware side.
> 
> Possible that mechanism is different from this one though...

Yea but I agree with Dan that the comment is wrong right now.  I somewhat
misinterpreted the comment and so my change was not correct despite Dan and I
being on the same page (at least in my head).

So Dan is not going to take this and I'm going to attempt to make the comment
correct per the current behavior.  If another mechanism is built then we can
document that when it comes along.

Ira

> 
> Jonathan
> 
> > 
> > >  
> > >  #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands)
> > > 
> > > -- 
> > > 2.38.1  
> > 
> > 
> 

[PATCH 2/3] cxl/mem: Remove unused CXL_CMD_FLAG_NONE define

[Ira Weiny]

CXL_CMD_FLAG_NONE is not used, remove it.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/cxlmem.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index ab138004f644..2d85776236dd 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -393,7 +393,6 @@ struct cxl_mem_command {
 	struct cxl_command_info info;
 	enum cxl_opcode opcode;
 	u32 flags;
-#define CXL_CMD_FLAG_NONE 0
 #define CXL_CMD_FLAG_FORCE_ENABLE BIT(0)
 };
 

-- 
2.38.1

Re: [PATCH 2/3] cxl/mem: Remove unused CXL_CMD_FLAG_NONE define

[Jonathan Cameron]

On Tue, 27 Dec 2022 06:52:10 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> CXL_CMD_FLAG_NONE is not used, remove it.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
That is a bit odd. Good to get rid of it.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/cxlmem.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index ab138004f644..2d85776236dd 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -393,7 +393,6 @@ struct cxl_mem_command {
>  	struct cxl_command_info info;
>  	enum cxl_opcode opcode;
>  	u32 flags;
> -#define CXL_CMD_FLAG_NONE 0
>  #define CXL_CMD_FLAG_FORCE_ENABLE BIT(0)
>  };
>  
> 

[PATCH 3/3] cxl/uapi: Add warning on CXL command enum

[Ira Weiny]

The CXL command enum is exported to user space and must maintain
backwards compatibility.

Add comment that new defines must be added to the end of the list.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 include/uapi/linux/cxl_mem.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index 555f9140e2bc..b3fd46af70f8 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -19,6 +19,10 @@
 #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands)
 #define CXL_MEM_SEND_COMMAND _IOWR(0xCE, 2, struct cxl_send_command)
 
+/*
+ * NOTE: New defines must be added to the end of the list to preserve
+ * compatibility because this enum is exported to user space.
+ */
 #define CXL_CMDS                                                          \
 	___C(INVALID, "Invalid Command"),                                 \
 	___C(IDENTIFY, "Identify Command"),                               \

-- 
2.38.1

Re: [PATCH 3/3] cxl/uapi: Add warning on CXL command enum

[Jonathan Cameron]

On Tue, 27 Dec 2022 06:52:11 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> The CXL command enum is exported to user space and must maintain
> backwards compatibility.
> 
> Add comment that new defines must be added to the end of the list.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  include/uapi/linux/cxl_mem.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 555f9140e2bc..b3fd46af70f8 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -19,6 +19,10 @@
>  #define CXL_MEM_QUERY_COMMANDS _IOR(0xCE, 1, struct cxl_mem_query_commands)
>  #define CXL_MEM_SEND_COMMAND _IOWR(0xCE, 2, struct cxl_send_command)
>  
> +/*
> + * NOTE: New defines must be added to the end of the list to preserve
> + * compatibility because this enum is exported to user space.
> + */
>  #define CXL_CMDS                                                          \
>  	___C(INVALID, "Invalid Command"),                                 \
>  	___C(IDENTIFY, "Identify Command"),                               \
>