Re: [PATCH V9 6/9] cxl/port: Read CDAT table

[Jonathan Cameron]

On Tue, 31 May 2022 08:26:29 -0700
ira.weiny@xxxxxxxxx wrote:

> From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> 
> The OS will need CDAT data from CXL devices to properly set up
> interleave sets.  Currently this is supported through a DOE mailbox
> which supports CDAT.
> 
> Cache the CDAT data for later parsing.  Provide a sysfs binary attribute
> to allow dumping of the CDAT.
> 
> Binary dumping is modeled on /sys/firmware/ACPI/tables/
> 
> The ability to dump this table will be very useful for emulation of real
> devices once they become available as QEMU CXL type 3 device emulation will
> be able to load this file in.
> 
> This does not support table updates at runtime. It will always provide
> whatever was there when first cached. Handling of table updates can be
> implemented later.
> 
> Finally create a complete list of DOE defines within cdat.h for code
> wishing to decode the CDAT table.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Co-developed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> 

Fun question of ownership inline...

...

> +void read_cdat_data(struct cxl_port *port)
> +{
> +	struct device *dev = &port->dev;
> +	size_t cdat_length;
> +	int ret;
> +
> +	if (cxl_cdat_get_length(port, &cdat_length))
> +		return;
> +
> +	port->cdat.table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);

boom. See below for why :)

> +	if (!port->cdat.table) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	port->cdat.length = cdat_length;
> +	ret = cxl_cdat_read_table(port, &port->cdat);
> +	if (ret) {
> +		devm_kfree(dev, port->cdat.table);
> +		port->cdat.table = NULL;
> +		port->cdat.length = 0;
> +		ret = -EIO;
> +		goto error;
> +	}
> +
> +	return;
> +error:
> +	dev_err(dev, "CDAT data read error (%d)\n", ret);
> +}
> +EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 2e2bd65c1024..aa4229ddc1bc 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -320,7 +320,48 @@ static void cxl_port_release(struct device *dev)
>  	kfree(port);
>  }
>  
> +static ssize_t cdat_read(struct file *filp, struct kobject *kobj,
> +			 struct bin_attribute *bin_attr, char *buf,
> +			 loff_t offset, size_t count)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cxl_port *port = to_cxl_port(dev);
> +
> +	if (!port->cdat.table)
> +		return 0;
> +
> +	return memory_read_from_buffer(buf, count, &offset,
> +				       port->cdat.table,
> +				       port->cdat.length);
> +}
> +
> +static BIN_ATTR_RO(cdat, 0);
> +
> +static umode_t cxl_port_bin_attr_is_visible(struct kobject *kobj,
> +					      struct bin_attribute *attr, int i)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cxl_port *port = to_cxl_port(dev);
> +
> +	if ((attr == &bin_attr_cdat) && port->cdat.table)
> +		return 0400;
> +
> +	return 0;
> +}
> +
> +static struct bin_attribute *cxl_cdat_bin_attributes[] = {
> +	&bin_attr_cdat,
> +	NULL,
> +};
> +
> +static struct attribute_group cxl_cdat_attribute_group = {
> +	.name = "CDAT",
> +	.bin_attrs = cxl_cdat_bin_attributes,
> +	.is_bin_visible = cxl_port_bin_attr_is_visible,
> +};
> +
>  static const struct attribute_group *cxl_port_attribute_groups[] = {
> +	&cxl_cdat_attribute_group,o
>  	&cxl_base_attribute_group,
>  	NULL,
>  };
> @@ -462,6 +503,8 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
>  		return port;
>  
>  	cxl_find_cdat_mb(port);
> +	/* Cache the data early to ensure is_visible() works */
> +	read_cdat_data(port);

This uses port as the 'device' for devm_ calls.
Unfortunately if the port driver isn't loaded, it still "successfully" runs.
Then if the port driver is probed, you get both a bunch of errors due to
devm_ allocations on a device before the driver is loaded.

For extra fun it tries to probe the ports multiple times without freeing
the index which is 'interesting'. We had this happen a while ago (unrelated
to DOE) but this may be unrelated (or maybe related to the region stuff
I'm carrying on my test tree)

As to the question of what the correct fix is...
Maybe move them into the port driver probe but then is_visible
won't work.  Or pass a pointer to the struct device *host down
into read_cdat_data and __read_cdat_data calls to handle the
allocation. (I tried this and it seems superficially fine).

Jonathan



>  
>  	dev = &port->dev;
>  	if (is_cxl_memdev(uport))
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0a86be589ffc..531b77d296c7 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -8,6 +8,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/io.h>
> +#include "cdat.h"
>  
>  /**
>   * DOC: cxl objects
> @@ -268,6 +269,7 @@ struct cxl_nvdimm {
>   * @dead: last ep has been removed, force port re-creation
>   * @depth: How deep this port is relative to the root. depth 0 is the root.
>   * @cdat_mb: Mailbox which supports the CDAT protocol
> + * @cdat: Cached CDAT data
>   */
>  struct cxl_port {
>  	struct device dev;
> @@ -280,6 +282,7 @@ struct cxl_port {
>  	bool dead;
>  	unsigned int depth;
>  	struct pci_doe_mb *cdat_mb;
> +	struct cxl_cdat cdat;
>  };
>  
>  /**
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 366b21bd1a01..35f0d4892eaa 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -75,4 +75,5 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port);
>  struct cxl_dev_state;
>  int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm);
>  void cxl_find_cdat_mb(struct cxl_port *port);
> +void read_cdat_data(struct cxl_port *port);
>  #endif /* __CXL_PCI_H__ */