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

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

Alison Schofield wrote:
> 
> Dan, Vishal,
> 
> Following up on using the new deprecated list ...
> 
> On Tue, Apr 11, 2023 at 06:47:56PM -0700, Dan Williams wrote:
> > alison.schofield@ wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > CXL devices maintain a list of locations that are poisoned or result
> > > in poison if the addresses are accessed by the host.
> > > 
> > > Per the spec, (CXL 3.0 8.2.9.8.4.1), the device returns this Poison
> > > list as a set of Media Error Records that include the source of the
> > > error, the starting device physical address, and length. The length is
> > > the number of adjacent DPAs in the record and is in units of 64 bytes.
> > > 
> > > Retrieve the poison list.
> > > 
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > > ---
> > >  drivers/cxl/core/mbox.c | 71 +++++++++++++++++++++++++++++++++++++++++
> > >  drivers/cxl/cxlmem.h    | 67 ++++++++++++++++++++++++++++++++++++++
> > >  drivers/cxl/pci.c       |  4 +++
> > >  3 files changed, 142 insertions(+)
> > > 
> > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > > index f2addb457172..69a5d69dd53b 100644
> > > --- a/drivers/cxl/core/mbox.c
> > > +++ b/drivers/cxl/core/mbox.c
> > > @@ -5,6 +5,8 @@
> > >  #include <linux/debugfs.h>
> > >  #include <linux/ktime.h>
> > >  #include <linux/mutex.h>
> > > +#include <asm/unaligned.h>
> > > +#include <cxlpci.h>
> > >  #include <cxlmem.h>
> > >  #include <cxl.h>
> > >  
> > > @@ -994,6 +996,7 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
> > >  	/* See CXL 2.0 Table 175 Identify Memory Device Output Payload */
> > >  	struct cxl_mbox_identify id;
> > >  	struct cxl_mbox_cmd mbox_cmd;
> > > +	u32 val;
> > >  	int rc;
> > >  
> > >  	mbox_cmd = (struct cxl_mbox_cmd) {
> > > @@ -1017,6 +1020,11 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
> > >  	cxlds->lsa_size = le32_to_cpu(id.lsa_size);
> > >  	memcpy(cxlds->firmware_version, id.fw_revision, sizeof(id.fw_revision));
> > >  
> > > +	if (test_bit(CXL_MEM_COMMAND_ID_GET_POISON, cxlds->enabled_cmds)) {
> > > +		val = get_unaligned_le24(id.poison_list_max_mer);
> > > +		cxlds->poison.max_errors = min_t(u32, val, CXL_POISON_LIST_MAX);
> > > +	}
> > > +
> > 
> > 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.
> > 
> > -- >8 --
> > From f2cd1d1e09fe6f36255f3b8cd831b2b4903045d4 Mon Sep 17 00:00:00 2001
> > From: Dan Williams <dan.j.williams@intel.com>
> > Date: Tue, 11 Apr 2023 17:48:45 -0700
> > Subject: [PATCH] cxl/mbox: Deprecate poison commands
> > 
> > The CXL subsystem is adding a formal mechanism for retrieving the poison
> > list. Minimize the maintenance burden going forward, and maximize the
> > investment in common tooling by deprecating direct user access to issue
> > this command outside of CXL_MEM_RAW_COMMANDS debug scenarios.
> > 
> > A new cxl_deprecated_commands[] list is created for querying which
> > command ids defined in previous kernels are now deprecated.
> 
> Dan, Vishal,
> 
> With the driver no longer returning these commands in the
> cxl_mem_commands[] list, they will appear as -EOPNOTSUPP in
> the ndctl:libcxl cxl_query().
> 
> Is there something else we need to do to actually show these
> cmds as deprecated? Something like query the deprecated list
> and add a new status to cxl_query(). 

No, because any software written before the deprecation does not know to
look to see if they are deprecated, and software written afterwards can
read the latest header file. So an EOPNOTSUPP return is all that is
needed.

> At the moment, my goal is to do what is needed in the driver
> now, and pick up any accompanying libcxl changes after the
> driver changes are merged. A bit worried I'm leaving something
> undone in driver today.

As long as there are users with new needs the driver will always have
undone corner cases. I think this series is in good shape.