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

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

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.

> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);


>