Re: [PATCH v15 6/7] cxl/port: Read CDAT table

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

Jonathan Cameron wrote:
> On Mon, 18 Jul 2022 18:55:01 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The per-device CDAT data provides performance data that is relevant for
> > mapping which CXL devices can participate in which CXL ranges by QTG
> > (QoS Throttling Group) (per ECN: CXL 2.0 CEDT CFMWS & QTG_DSM) [1]. The
> > QTG association specified in the ECN is advisory. Until the
> > cxl_acpi driver grows support for invoking the QTG _DSM method the CDAT
> > data is only of interest to userspace that may need it for debug
> > purposes.
> > 
> > Search the DOE mailboxes available, query CDAT data, cache the data and
> > make it available via a sysfs binary attribute per endpoint at:
> > 
> > /sys/bus/cxl/devices/endpointX/CDAT
> > 
> > ...similar to other ACPI-structured table data in
> > /sys/firmware/ACPI/tables. The CDAT is relative to 'struct cxl_port'
> > objects since switches in addition to endpoints can host a CDAT
> > instance. Switch CDAT support is not implemented.
> > 
> > This does not support table updates at runtime. It will always provide
> > whatever was there when first cached. It is also the case that table
> > updates are not expected outside of explicit DPA address map affecting
> > commands like Set Partition with the immediate flag set. Given that the
> > driver does not support Set Partition with the immediate flag set there
> > is no current need for update support.
> > 
> > Link: https://www.computeexpresslink.org/spec-landing [1]
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > [djbw: drop in-kernel parsing infra for now, and other minor fixups]
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> One comment inline that basically says the error message will do for now
> if we race with an autonomous update of CDAT by the device. (see response
> to earlier version on why that can happen).
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 7672789c3225..9240df53ed87 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> 
> 
> > +/**
> > + * read_cdat_data - Read the CDAT data on this port
> > + * @port: Port to read data from
> > + *
> > + * This call will sleep waiting for responses from the DOE mailbox.
> > + */
> > +void read_cdat_data(struct cxl_port *port)
> > +{
> > +	struct pci_doe_mb *cdat_doe;
> > +	struct device *dev = &port->dev;
> > +	struct device *uport = port->uport;
> > +	size_t cdat_length;
> > +	int rc;
> > +
> > +	cdat_doe = find_cdat_doe(uport);
> > +	if (!cdat_doe) {
> > +		dev_dbg(dev, "No CDAT mailbox\n");
> > +		return;
> > +	}
> > +
> > +	port->cdat_available = true;
> > +
> > +	if (cxl_cdat_get_length(dev, cdat_doe, &cdat_length)) {
> > +		dev_dbg(dev, "No CDAT length\n");
> > +		return;
> > +	}
> > +
> > +	port->cdat.table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);
> > +	if (!port->cdat.table)
> > +		return;
> > +
> > +	port->cdat.length = cdat_length;
> > +	rc = cxl_cdat_read_table(dev, cdat_doe, &port->cdat);
> > +	if (rc) {
> > +		/* Don't leave table data allocated on error */
> > +		devm_kfree(dev, port->cdat.table);
> > +		port->cdat.table = NULL;
> > +		port->cdat.length = 0;
> > +		dev_err(dev, "CDAT data read error\n");
> 
> This will be sufficient for now for the race against autonomous CDAT
> update potential issue mentioned in earlier review.  If it happens
> and someone really cares they can unbind and rebind the device.

Even it is too late to change the spec, which I do not think it is, it
is not too late to make a public statement that Linux has no interest in
supporting such shenanigans outside of a documented set of explicit
commands that have a CDAT-update side effect.