Re: [PATCH v3 3/4] cxl/uapi: Tag commands from cxl_query_cmd()

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

On Thu, 2 Feb 2023 09:58:21 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 01 Feb 2023 17:23:12 -0800
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > It was pointed out that commands not supported by the device or excluded
> > by the kernel were being returned in cxl_query_cmd().[1]
> > 
> > While libcxl correctly handles failing commands, it is more efficient to
> > not issue an invalid command in the first place.  This can't be done
> > without additional information being returned from cxl_query_cmd().  In
> > addition, information about the availability of commands can be useful
> > for debugging.
> > 
> > Add flags to struct cxl_command_info which reflect if a command is
> > enabled and/or exclusive to the kernel.
> > 
> > [1] https://lore.kernel.org/all/63b4ec4e37cc1_5178e2941d@dwillia2-xfh.jf.intel.com.notmuch/
> > 
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>  
> 
> No mention of why we can use space in the existing defined flag mask?
> 
> I couldn't find a reason why we couldn't previously but Dan's proposal
> was to reserve that first bit so I'm wondering on the reasoning one
> way or the other!
> 
> Other than that detail, this looks like a good solution to me.

On second thoughts. Some docs for the two flags explaining what they
mean would be good to have (as there isn't a reference to how to use
them in the Docs patch that follows this).

> 
> Jonathan
> 
> 
> > 
> > ---
> > Changes for v3:
> > 	Dan
> > 		Pass enabled/exclusive flags to user space rather than
> > 		masking those commands.
> > ---
> >  drivers/cxl/core/mbox.c      | 9 +++++++--
> >  include/uapi/linux/cxl_mem.h | 4 +++-
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index b03fba212799..6ab4613f3cf9 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -445,9 +445,14 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
> >  	 * structures.
> >  	 */
> >  	cxl_for_each_cmd(cmd) {
> > -		const struct cxl_command_info *info = &cmd->info;
> > +		struct cxl_command_info info = cmd->info;
> >  
> > -		if (copy_to_user(&q->commands[j++], info, sizeof(*info)))
> > +		if (test_bit(info.id, cxlmd->cxlds->enabled_cmds))
> > +			info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED;
> > +		if (test_bit(info.id, cxlmd->cxlds->exclusive_cmds))
> > +			info.flags |= CXL_MEM_COMMAND_FLAG_EXCLUSIVE;
> > +
> > +		if (copy_to_user(&q->commands[j++], &info, sizeof(info)))
> >  			return -EFAULT;
> >  
> >  		if (j == n_commands)
> > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> > index 459a3f7f764b..4aee64b7cc1a 100644
> > --- a/include/uapi/linux/cxl_mem.h
> > +++ b/include/uapi/linux/cxl_mem.h
> > @@ -91,7 +91,9 @@ struct cxl_command_info {
> >  	__u32 id;
> >  
> >  	__u32 flags;
> > -#define CXL_MEM_COMMAND_FLAG_MASK GENMASK(0, 0)
> > +#define CXL_MEM_COMMAND_FLAG_MASK		GENMASK(1, 0)
> > +#define CXL_MEM_COMMAND_FLAG_ENABLED		BIT(0)
> > +#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE		BIT(1)
> >  
> >  	__u32 size_in;
> >  	__u32 size_out;
> >   
>