| Message ID | 20221222-cxl-misc-v1-1-9343bab16e72@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CXL: Miscellaneous fixes | expand |
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
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 > >
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 > > > > >
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)
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(-)