Re: [PATCH v12 1/6] cxl/mbox: Add GET_POISON_LIST mailbox command

[Alison Schofield <alison.schofield@intel.com>]

On Tue, Apr 11, 2023 at 10:18:40PM -0700, Dan Williams wrote:
> Alison Schofield wrote:
> > On Tue, Apr 11, 2023 at 06:47:56PM -0700, Dan Williams wrote:
> > > alison.schofield@ wrote:
> > > > From: Alison Schofield <alison.schofield@intel.com>
> > > > 

snip

> > > 
> > > With this new interface I do not expect we want to support user tooling
> > > that wants to retrieve the list via ioctl. So I think this wants a
> > > lead-in patch that deprecates the poison command support so that the
> > > linux-cxl community only has one mechanism to maintain going forward.
> > > 
> > > Something like the below as a lead-in, and then you would add code to
> > > cxl_walk_cel() to set a flag for the "get poison" machinery.
> > 
> > In the inject & clear series, I made the commands kernel exclusive (and
> > also blocked their usage in raw mode).
> > 
> > https://lore.kernel.org/linux-cxl/1576040e-e8db-bc78-2fa3-622c8f7da8ec@intel.com/T/#m5b86f3e88ee7ad5b92843babdb9fd41b7f03cf36
> > 
> > Is that not enough or not the right protection?
> 
> I think I'm having deja vu and maybe we talked about this already and I
> forgot the outcome?
> 
> The concern is having a consistent approach across the commands that are
> valid from userspace all the time, those that are valid sometimes
> (temporarily marked exclusive), and those that are unsupported from
> userspace (unsupported via raw command being a special case of that).
> 
> For recent new mailbox functionality like CXL_MBOX_OP_GET_EVENT_RECORD
> and the DCD commands, that are only expected to be executed from the
> kernel, those are kept out of the CXL_CMDS list, not marked exclusive.
> 
> The commands marked exclusive like the label commands return EBUSY, but
> only while libnvdimm owns the label area. If libnvdimm is disconnected
> (echo pmemX > /sys/bus/cxl/drivers/cxl_pmem/unbind), then those commands
> stop being exclusive.
> 
> So, I think I want to keep "exclusive" as a set of commands that are
> suitable to build into a user tool just that the tool needs to know the
> rules about when those commands might be busy.
> 
> For poison the user tool is expected to use sysfs + trace-events, so
> including it in the list of available commands returned by
> CXL_MEM_QUERY_COMMANDS only to never be able to execute it seems wrong.
> 
> Marking it deprecated as a "whoops" feels more honest and brings it in
> line with the other commands that are permanently kernel exclusive.


Yes, we were here previously w Inject & Clear series, and didn't close on
it. My last words were were in a inject/clear changelog:

>>  I didn't deprecate the ioctls as Dan suggested. Instead, I followed
>>  on Ira's recent clean up wrt enabled and exclusive commands, deciding
>>  that the honest response to a cxl_query() would be 'Enabled' (if hardware
>>  supports) and kernel 'Exclusive' (always). Please take a look.

Once we deprecate, is there a way for users to determine what commands a
device supports?

Can you post that deprecate patch preemptively? I'll update the affected
series to follow it.

Please include SCAN_MEDIA & GET_SCAN_MEDIA in your patch for the same reason. 
SCAN_MEDIA_CAPS is harmless.

 	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
-	___C(SCAN_MEDIA, "Scan Media"),                                   \
-	___C(GET_SCAN_MEDIA, "Get Scan Media Results"),                   \
+	___DEPRECATED(SCAN_MEDIA, "Scan Media"),                          \
+	___DEPRECATED(GET_SCAN_MEDIA, "Get Scan Media Results"),          \
 	___C(MAX, "invalid / last command")

Alison