RE: [PATCH v2 3/4] cxl/uapi: Only return valid commands from cxl_query_cmd()

[Dan Williams <dan.j.williams@intel.com>]

Ira Weiny wrote:
> > There is value, similar to the motivation to print the status of enabled
> > commands. It is useful to know what functionality is limited by the
> > kernel and what functionality is limited by the device.
> 
> Ok.  Do you think we should then update libcxl to use these flags?  Or are
> you ok with the try and fail behavior now?

The try and fail behavior needs to stay if only to maintain backward
compatibility.

> 
> [snip]
> 
> > 
> > > 
> > > >  
> > > > -               if (copy_to_user(&q->commands[j++], info, sizeof(*info)))
> > > > +               /* reflect exclusive and enabled */
> > > > +               if (test_bit(info.id, cxlds->enabled_cmds))
> > > > +                       info.flags |= CXL_MEM_COMMAND_FLAG_ENABLED;
> > > > +               if (test_bit(info.id, cxlds->exclusive_cmds))
> > > > +                       info.flags |= CXL_MEM_COMMAND_FLAG_EXCLUSIVE;
> > > 
> > > Ok Just to make sure I'm following you.  This is then expecting user space
> > > to check these flags to know if this command is available or not?
> > 
> > No, there is no expectation that userspace check these, especially since
> > they were not mandated before, but they are optional useful information
> > for future debug.
> 
> Ok this is obviously easy to incorporate.
> 
> > 
> > > This means additional user space changes to use these.  Which is ok.  But
> > > what is the value?  Current user space will still be broken and future
> > > user space will be more complicated.
> > 
> > Value is as above, more user visible debug information which will be
> > important as different levels of enabling make it out to different
> > backport kernels.
> > 
> > > Right now libcxl issues a query before each command to ensure the command
> > > is available.  These flags can't be cached.
> > > 
> > > The patch as I have proposed simply short circuits the already failing
> > > path in libcxl with minimal changes.
> > > 
> > > Do we have a use case for user space to report the current disabled and
> > > exclusive commands?
> > 
> > Nothing beyond future debug flexibility.
> 
> I'll respin with this change.  Let me know on any user space change you
> might want.  Another option would be to add a libcxl 'info' functionality
> which reports the above flags?  Not quite sure how that would work exactly
> but I could put some thought into that if you want.

I think it can wait until there is a bigger need to enumerate device
capabilities. All the current commands have sysfs and other sideband
ways to enumerate their presence, but going forward I can see this being
useful. I.e. legacy commands are enumerated by try and fail, but new
commands (post this enabling) can reliably depend on the flags to be
there.