Re: [PATCH 20/23] cxl/port: Introduce a port driver

[Ben Widawsky <ben.widawsky@intel.com>]

On 21-11-22 17:41:32, Jonathan Cameron wrote:
> On Fri, 19 Nov 2021 16:02:47 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > The CXL port driver is responsible for managing the decoder resources
> > contained within the port. It will also provide APIs that other drivers
> > will consume for managing these resources.
> > 
> > There are 4 types of ports in a system:
> > 1. Platform port. This is a non-programmable entity. Such a port is
> >    named rootX. It is enumerated by cxl_acpi in an ACPI based system.
> > 2. Hostbridge port. This ports register access is defined in a platform
> 
> port's 
> 
> >    specific way (CHBS for ACPI platforms). It has n downstream ports,
> >    each of which are known as CXL 2.0 root ports. Once the platform
> >    specific mechanism to get the offset to the registers is obtained it
> >    operates just like other CXL components. The enumeration of this
> >    component is started by cxl_acpi and completed by cxl_port.
> > 3. Switch port. A switch port is similar to a hostbridge port except
> >    register access is defined in the CXL specification in a platform
> >    agnostic way. The downstream ports for a switch are simply known as
> >    downstream ports. The enumeration of these are entirely contained
> >    within cxl_port.
> > 4. Endpoint port. Endpoint ports are similar to switch ports with the
> >    exception that they have no downstream ports, only the underlying
> >    media on the device. The enumeration of these are started by cxl_pci,
> >    and completed by cxl_port.
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> A few comments inline including what looks to me memory on the stack which has
> gone out of scope when it's accessed.
> 
> Jonathan
> 
> > 
> > ---
> > Changes since RFCv2:
> > - Reword commit message tense (Dan)
> > - Reword commit message
> > - Drop SOFTDEP since it's not needed yet (Dan)
> > - Add CONFIG_CXL_PORT (Dan)
> > - s/CXL_DECODER_F_EN/CXL_DECODER_F_ENABLE (Dan)
> > - rename cxl_hdm_decoder_ functions to "to_" (Dan)
> > - remove useless inline (Dan)
> > - Check endpoint decoder based on dport list instead of driver id (Dan)
> > - Use range instead of resource per dependent patch change
> > - Use clever union packing for target list (Dan)
> > - Only check NULL from devm_cxl_iomap_block (Dan)
> > - Properly parent the created cxl decoders
> > - Move bus rescanning from cxl_acpi to here (Dan)
> > - Remove references to "CFMWS" in core (Dan)
> > - Use macro to help keep within 80 character lines
> > ---
> >  .../driver-api/cxl/memory-devices.rst         |   5 +
> >  drivers/cxl/Kconfig                           |  22 ++
> >  drivers/cxl/Makefile                          |   2 +
> >  drivers/cxl/core/bus.c                        |  67 ++++
> >  drivers/cxl/core/regs.c                       |   6 +-
> >  drivers/cxl/cxl.h                             |  34 +-
> >  drivers/cxl/port.c                            | 323 ++++++++++++++++++
> >  7 files changed, 450 insertions(+), 9 deletions(-)
> >  create mode 100644 drivers/cxl/port.c
> > 
> > diff --git a/Documentation/driver-api/cxl/memory-devices.rst b/Documentation/driver-api/cxl/memory-devices.rst
> > index 3b8f41395f6b..fbf0393cdddc 100644
> > --- a/Documentation/driver-api/cxl/memory-devices.rst
> > +++ b/Documentation/driver-api/cxl/memory-devices.rst
> > @@ -28,6 +28,11 @@ CXL Memory Device
> >  .. kernel-doc:: drivers/cxl/pci.c
> >     :internal:
> >  
> > +CXL Port
> > +--------
> > +.. kernel-doc:: drivers/cxl/port.c
> > +   :doc: cxl port
> > +
> >  CXL Core
> >  --------
> >  .. kernel-doc:: drivers/cxl/cxl.h
> > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> > index ef05e96f8f97..3aeb33bba5a3 100644
> > --- a/drivers/cxl/Kconfig
> > +++ b/drivers/cxl/Kconfig
> > @@ -77,4 +77,26 @@ config CXL_PMEM
> >  	  provisioning the persistent memory capacity of CXL memory expanders.
> >  
> >  	  If unsure say 'm'.
> > +
> > +config CXL_MEM
> > +	tristate "CXL.mem: Memory Devices"
> > +	select CXL_PORT
> > +	depends on CXL_PCI
> > +	default CXL_BUS
> > +	help
> > +	  The CXL.mem protocol allows a device to act as a provider of "System
> > +	  RAM" and/or "Persistent Memory" that is fully coherent as if the
> > +	  memory was attached to the typical CPU memory controller.  This is
> > +	  known as HDM "Host-managed Device Memory".
> > +
> > +	  Say 'y/m' to enable a driver that will attach to CXL.mem devices for
> > +	  memory expansion and control of HDM. See Chapter 9.13 in the CXL 2.0
> > +	  specification for a detailed description of HDM.
> > +
> > +	  If unsure say 'm'.
> > +
> > +
> > +config CXL_PORT
> > +	tristate
> > +
> >  endif
> > diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> > index cf07ae6cea17..56fcac2323cb 100644
> > --- a/drivers/cxl/Makefile
> > +++ b/drivers/cxl/Makefile
> > @@ -3,7 +3,9 @@ obj-$(CONFIG_CXL_BUS) += core/
> >  obj-$(CONFIG_CXL_PCI) += cxl_pci.o
> >  obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
> >  obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
> > +obj-$(CONFIG_CXL_PORT) += cxl_port.o
> >  
> >  cxl_pci-y := pci.o
> >  cxl_acpi-y := acpi.o
> >  cxl_pmem-y := pmem.o
> > +cxl_port-y := port.o
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index 9e0d7d5d9298..46a06cfe79bd 100644
> > --- a/drivers/cxl/core/bus.c
> > +++ b/drivers/cxl/core/bus.c
> > @@ -31,6 +31,8 @@ static DECLARE_RWSEM(root_port_sem);
> >  
> >  static struct device *cxl_topology_host;
> >  
> > +static bool is_cxl_decoder(struct device *dev);
> > +
> >  int cxl_register_topology_host(struct device *host)
> >  {
> >  	down_write(&topology_host_sem);
> > @@ -75,6 +77,45 @@ static void put_cxl_topology_host(struct device *dev)
> >  	up_read(&topology_host_sem);
> >  }
> >  
> > +static int decoder_match(struct device *dev, void *data)
> > +{
> > +	struct resource *theirs = (struct resource *)data;
> > +	struct cxl_decoder *cxld;
> > +
> > +	if (!is_cxl_decoder(dev))
> > +		return 0;
> > +
> > +	cxld = to_cxl_decoder(dev);
> > +	if (theirs->start <= cxld->decoder_range.start &&
> > +	    theirs->end >= cxld->decoder_range.end)
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct cxl_decoder *cxl_find_root_decoder(resource_size_t base,
> > +						 resource_size_t size)
> > +{
> > +	struct cxl_decoder *cxld = NULL;
> > +	struct device *cxldd;
> > +	struct device *host;
> > +	struct resource res = (struct resource){
> > +		.start = base,
> > +		.end = base + size - 1,
> > +	};
> > +
> > +	host = get_cxl_topology_host();
> > +	if (!host)
> > +		return NULL;
> > +
> > +	cxldd = device_find_child(host, &res, decoder_match);
> > +	if (cxldd)
> > +		cxld = to_cxl_decoder(cxldd);
> > +
> > +	put_cxl_topology_host(host);
> > +	return cxld;
> > +}
> > +
> >  static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
> >  			    char *buf)
> >  {
> > @@ -280,6 +321,11 @@ bool is_root_decoder(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(is_root_decoder, CXL);
> >  
> > +static bool is_cxl_decoder(struct device *dev)
> > +{
> > +	return dev->type->release == cxl_decoder_release;
> > +}
> > +
> >  struct cxl_decoder *to_cxl_decoder(struct device *dev)
> >  {
> >  	if (dev_WARN_ONCE(dev, dev->type->release != cxl_decoder_release,
> > @@ -327,6 +373,7 @@ struct cxl_port *to_cxl_port(struct device *dev)
> >  		return NULL;
> >  	return container_of(dev, struct cxl_port, dev);
> >  }
> > +EXPORT_SYMBOL_NS_GPL(to_cxl_port, CXL);
> >  
> >  struct cxl_dport *cxl_get_root_dport(struct device *dev)
> >  {
> > @@ -735,6 +782,24 @@ EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, CXL);
> >  
> >  static void cxld_unregister(void *dev)
> >  {
> > +	struct cxl_decoder *plat_decoder, *cxld = to_cxl_decoder(dev);
> > +	resource_size_t base, size;
> > +
> > +	if (is_root_decoder(dev)) {
> > +		device_unregister(dev);
> > +		return;
> > +	}
> > +
> > +	base = cxld->decoder_range.start;
> > +	size = range_len(&cxld->decoder_range);
> > +
> > +	if (size) {
> > +		plat_decoder = cxl_find_root_decoder(base, size);
> > +		if (plat_decoder)
> > +			__release_region(&plat_decoder->platform_res, base,
> > +					 size);
> > +	}
> > +
> >  	device_unregister(dev);
> >  }
> >  
> > @@ -789,6 +854,8 @@ static int cxl_device_id(struct device *dev)
> >  		return CXL_DEVICE_NVDIMM_BRIDGE;
> >  	if (dev->type == &cxl_nvdimm_type)
> >  		return CXL_DEVICE_NVDIMM;
> > +	if (dev->type == &cxl_port_type)
> > +		return CXL_DEVICE_PORT;
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> > index 41a0245867ea..f191b0c995a7 100644
> > --- a/drivers/cxl/core/regs.c
> > +++ b/drivers/cxl/core/regs.c
> > @@ -159,9 +159,8 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_probe_device_regs, CXL);
> >  
> > -static void __iomem *devm_cxl_iomap_block(struct device *dev,
> > -					  resource_size_t addr,
> > -					  resource_size_t length)
> > +void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> > +				   resource_size_t length)
> >  {
> >  	void __iomem *ret_val;
> >  	struct resource *res;
> > @@ -180,6 +179,7 @@ static void __iomem *devm_cxl_iomap_block(struct device *dev,
> >  
> >  	return ret_val;
> >  }
> > +EXPORT_SYMBOL_NS_GPL(devm_cxl_iomap_block, CXL);
> >  
> >  int cxl_map_component_regs(struct pci_dev *pdev,
> >  			   struct cxl_component_regs *regs,
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 3962a5e6a950..24fa16157d5e 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -17,6 +17,9 @@
> >   * (port-driver, region-driver, nvdimm object-drivers... etc).
> >   */
> >  
> > +/* CXL 2.0 8.2.4 CXL Component Register Layout and Definition */
> > +#define CXL_COMPONENT_REG_BLOCK_SIZE SZ_64K
> > +
> >  /* CXL 2.0 8.2.5 CXL.cache and CXL.mem Registers*/
> >  #define CXL_CM_OFFSET 0x1000
> >  #define CXL_CM_CAP_HDR_OFFSET 0x0
> > @@ -36,11 +39,22 @@
> >  #define CXL_HDM_DECODER_CAP_OFFSET 0x0
> >  #define   CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0)
> >  #define   CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4)
> > -#define CXL_HDM_DECODER0_BASE_LOW_OFFSET 0x10
> > -#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET 0x14
> > -#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET 0x18
> > -#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET 0x1c
> > -#define CXL_HDM_DECODER0_CTRL_OFFSET 0x20
> > +#define   CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8)
> > +#define   CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9)
> > +#define CXL_HDM_DECODER_CTRL_OFFSET 0x4
> > +#define   CXL_HDM_DECODER_ENABLE BIT(1)
> > +#define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10)
> > +#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET(i) (0x20 * (i) + 0x14)
> > +#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET(i) (0x20 * (i) + 0x18)
> > +#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(i) (0x20 * (i) + 0x1c)
> > +#define CXL_HDM_DECODER0_CTRL_OFFSET(i) (0x20 * (i) + 0x20)
> > +#define   CXL_HDM_DECODER0_CTRL_IG_MASK GENMASK(3, 0)
> > +#define   CXL_HDM_DECODER0_CTRL_IW_MASK GENMASK(7, 4)
> > +#define   CXL_HDM_DECODER0_CTRL_COMMIT BIT(9)
> > +#define   CXL_HDM_DECODER0_CTRL_COMMITTED BIT(10)
> > +#define   CXL_HDM_DECODER0_CTRL_TYPE BIT(12)
> > +#define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
> > +#define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
> >  
> >  static inline int cxl_hdm_decoder_count(u32 cap_hdr)
> >  {
> > @@ -148,6 +162,8 @@ int cxl_map_device_regs(struct pci_dev *pdev,
> >  enum cxl_regloc_type;
> >  int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> >  		      struct cxl_register_map *map);
> > +void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> > +				   resource_size_t length);
> >  
> >  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> >  #define CXL_TARGET_STRLEN 20
> > @@ -165,7 +181,8 @@ void cxl_unregister_topology_host(struct device *host);
> >  #define CXL_DECODER_F_TYPE2 BIT(2)
> >  #define CXL_DECODER_F_TYPE3 BIT(3)
> >  #define CXL_DECODER_F_LOCK  BIT(4)
> > -#define CXL_DECODER_F_MASK  GENMASK(4, 0)
> > +#define CXL_DECODER_F_ENABLE    BIT(5)
> > +#define CXL_DECODER_F_MASK  GENMASK(5, 0)
> >  
> >  enum cxl_decoder_type {
> >         CXL_DECODER_ACCELERATOR = 2,
> > @@ -255,6 +272,8 @@ struct cxl_walk_context {
> >   * @dports: cxl_dport instances referenced by decoders
> >   * @decoder_ida: allocator for decoder ids
> >   * @component_reg_phys: component register capability base address (optional)
> > + * @rescan_work: worker object for bus rescans after port additions
> > + * @data: opaque data with driver specific usage
> >   */
> >  struct cxl_port {
> >  	struct device dev;
> > @@ -263,6 +282,8 @@ struct cxl_port {
> >  	struct list_head dports;
> >  	struct ida decoder_ida;
> >  	resource_size_t component_reg_phys;
> > +	struct work_struct rescan_work;
> > +	void *data;
> >  };
> >  
> >  /**
> > @@ -325,6 +346,7 @@ void cxl_driver_unregister(struct cxl_driver *cxl_drv);
> >  
> >  #define CXL_DEVICE_NVDIMM_BRIDGE	1
> >  #define CXL_DEVICE_NVDIMM		2
> > +#define CXL_DEVICE_PORT			3
> >  
> >  #define MODULE_ALIAS_CXL(type) MODULE_ALIAS("cxl:t" __stringify(type) "*")
> >  #define CXL_MODALIAS_FMT "cxl:t%d"
> > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> > new file mode 100644
> > index 000000000000..3c03131517af
> > --- /dev/null
> > +++ b/drivers/cxl/port.c
> > @@ -0,0 +1,323 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +
> > +#include "cxlmem.h"
> > +
> > +/**
> > + * DOC: cxl port
> > + *
> > + * The port driver implements the set of functionality needed to allow full
> > + * decoder enumeration and routing. A CXL port is an abstraction of a CXL
> > + * component that implements some amount of CXL decoding of CXL.mem traffic.
> > + * As of the CXL 2.0 spec, this includes:
> > + *
> > + *	.. list-table:: CXL Components w/ Ports
> > + *		:widths: 25 25 50
> > + *		:header-rows: 1
> > + *
> > + *		* - component
> > + *		  - upstream
> > + *		  - downstream
> > + *		* - Hostbridge
> > + *		  - ACPI0016
> > + *		  - root port
> > + *		* - Switch
> > + *		  - Switch Upstream Port
> > + *		  - Switch Downstream Port
> > + *		* - Endpoint
> > + *		  - Endpoint Port
> > + *		  - N/A
> > + *
> > + * The primary service this driver provides is enumerating HDM decoders and
> > + * presenting APIs to other drivers to utilize the decoders.
> > + */
> > +
> > +static struct workqueue_struct *cxl_port_wq;
> > +
> > +struct cxl_port_data {
> > +	struct cxl_component_regs regs;
> > +
> > +	struct port_caps {
> > +		unsigned int count;
> > +		unsigned int tc;
> > +		unsigned int interleave11_8;
> > +		unsigned int interleave14_12;
> > +	} caps;
> > +};
> > +
> > +static inline int to_interleave_granularity(u32 ctrl)
> > +{
> > +	int val = FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl);
> > +
> > +	return 256 << val;
> > +}
> > +
> > +static inline int to_interleave_ways(u32 ctrl)
> > +{
> > +	int val = FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl);
> > +
> > +	return 1 << val;
> > +}
> > +
> > +static void get_caps(struct cxl_port *port, struct cxl_port_data *cpd)
> > +{
> > +	void __iomem *hdm_decoder = cpd->regs.hdm_decoder;
> > +	struct port_caps *caps = &cpd->caps;
> > +	u32 hdm_cap;
> > +
> > +	hdm_cap = readl(hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
> > +
> > +	caps->count = cxl_hdm_decoder_count(hdm_cap);
> > +	caps->tc = FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
> > +	caps->interleave11_8 =
> > +		FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap);
> > +	caps->interleave14_12 =
> > +		FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap);
> > +}
> > +
> > +static int map_regs(struct cxl_port *port, void __iomem *crb,
> > +		    struct cxl_port_data *cpd)
> > +{
> > +	struct cxl_register_map map;
> > +	struct cxl_component_reg_map *comp_map = &map.component_map;
> > +
> > +	cxl_probe_component_regs(&port->dev, crb, comp_map);
> > +	if (!comp_map->hdm_decoder.valid) {
> > +		dev_err(&port->dev, "HDM decoder registers invalid\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	cpd->regs.hdm_decoder = crb + comp_map->hdm_decoder.offset;
> > +
> > +	return 0;
> > +}
> > +
> > +static u64 get_decoder_size(void __iomem *hdm_decoder, int n)
> > +{
> > +	u32 ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(n));
> > +
> > +	if (ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED)
> > +		return 0;
> > +
> > +	return ioread64_hi_lo(hdm_decoder +
> > +			      CXL_HDM_DECODER0_SIZE_LOW_OFFSET(n));
> > +}
> > +
> > +static bool is_endpoint_port(struct cxl_port *port)
> > +{
> > +	/* Endpoints can't be ports... yet! */
> > +	return false;
> > +}
> > +
> > +static void rescan_ports(struct work_struct *work)
> > +{
> > +	if (bus_rescan_devices(&cxl_bus_type))
> > +		pr_warn("Failed to rescan\n");
> > +}
> > +
> > +/* Minor layering violation */
> > +static int dvsec_range_used(struct cxl_port *port)
> > +{
> > +	struct cxl_endpoint_dvsec_info *info;
> > +	int i, ret = 0;
> > +
> > +	if (!is_endpoint_port(port))
> > +		return 0;
> > +
> > +	info = port->data;
> > +	for (i = 0; i < info->ranges; i++)
> > +		if (info->range[i].size)
> > +			ret |= 1 << i;
> > +
> > +	return ret;
> > +}
> > +
> > +static int enumerate_hdm_decoders(struct cxl_port *port,
> > +				  struct cxl_port_data *portdata)
> > +{
> > +	void __iomem *hdm_decoder = portdata->regs.hdm_decoder;
> > +	bool global_enable;
> > +	u32 global_ctrl;
> > +	int i = 0;
> > +
> > +	global_ctrl = readl(hdm_decoder + CXL_HDM_DECODER_CTRL_OFFSET);
> > +	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
> > +	if (!global_enable) {
> > +		i = dvsec_range_used(port);
> > +		if (i) {
> > +			dev_err(&port->dev,
> > +				"Couldn't add port because device is using DVSEC range registers\n");
> > +			return -EBUSY;
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < portdata->caps.count; i++) {
> > +		int rc, target_count = portdata->caps.tc;
> > +		struct cxl_decoder *cxld;
> > +		int *target_map = NULL;
> > +		u64 size;
> > +
> > +		if (is_endpoint_port(port))
> > +			target_count = 0;
> > +
> > +		cxld = cxl_decoder_alloc(port, target_count);
> > +		if (IS_ERR(cxld)) {
> > +			dev_warn(&port->dev,
> > +				 "Failed to allocate the decoder\n");
> > +			return PTR_ERR(cxld);
> > +		}
> > +
> > +		cxld->target_type = CXL_DECODER_EXPANDER;
> > +		cxld->interleave_ways = 1;
> > +		cxld->interleave_granularity = 0;
> > +
> > +		size = get_decoder_size(hdm_decoder, i);
> > +		if (size != 0) {
> > +#define decoderN(reg, n) hdm_decoder + CXL_HDM_DECODER0_##reg(n)
> 
> Perhaps this block in the if (size != 0) would be more readable if broken out
> to a utility function?

I don't get this comment, there is already get_decoder_size(). Can you please
elaborate?

> As normal I'm not keen on the macro magic if we can avoid it.
> 

Yeah - just trying to not have to deal with wrapping long lines.

> 
> > +			int temp[CXL_DECODER_MAX_INTERLEAVE];
> > +			u64 base;
> > +			u32 ctrl;
> > +			int j;
> > +			union {
> > +				u64 value;
> > +				char target_id[8];
> > +			} target_list;
> 
> I thought we tried to avoid this union usage in kernel because of the whole
> thing about c compilers being able to do what they like with it...
> 

I wasn't aware of the restriction. I can change it back if it's required. It
does look a lot nicer this way. Is there a reference to this issue somewhere?

> > +
> > +			target_map = temp;
> 
> This is set to a variable that goes out of scope whilst target_map is still in
> use.
> 

Yikes. I'm pretty surprised the compiler didn't catch this.

> > +			ctrl = readl(decoderN(CTRL_OFFSET, i));
> > +			base = ioread64_hi_lo(decoderN(BASE_LOW_OFFSET, i));
> > +
> > +			cxld->decoder_range = (struct range){
> > +				.start = base,
> > +				.end = base + size - 1
> > +			};
> > +
> > +			cxld->flags = CXL_DECODER_F_ENABLE;
> > +			cxld->interleave_ways = to_interleave_ways(ctrl);
> > +			cxld->interleave_granularity =
> > +				to_interleave_granularity(ctrl);
> > +
> > +			if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0)
> > +				cxld->target_type = CXL_DECODER_ACCELERATOR;
> > +
> > +			target_list.value = ioread64_hi_lo(decoderN(TL_LOW, i));
> > +			for (j = 0; j < cxld->interleave_ways; j++)
> > +				target_map[j] = target_list.target_id[j];
> > +#undef decoderN
> > +		}
> > +
> > +		rc = cxl_decoder_add_locked(cxld, target_map);
> > +		if (rc)
> > +			put_device(&cxld->dev);
> > +		else
> > +			rc = cxl_decoder_autoremove(&port->dev, cxld);
> > +		if (rc)
> > +			dev_err(&port->dev, "Failed to add decoder\n");
> 
> If that fails on the autoremove registration (unlikely) this message
> will be rather confusing - as the add was fine...
> 
> This nest of carrying on when we have an error is getting ever deeper...
> 

Yeah, this is not great. I will clean it up.

Thanks.

> > +		else
> > +			dev_dbg(&cxld->dev, "Added to port %s\n",
> > +				dev_name(&port->dev));
> > +	}
> > +
> > +	/*
> > +	 * Turn on global enable now since DVSEC ranges aren't being used and
> > +	 * we'll eventually want the decoder enabled.
> > +	 */
> > +	if (!global_enable) {
> > +		dev_dbg(&port->dev, "Enabling HDM decode\n");
> > +		writel(global_ctrl | CXL_HDM_DECODER_ENABLE, hdm_decoder + CXL_HDM_DECODER_CTRL_OFFSET);
> > +	}
> > +
> > +	return 0;
> > +}
> > +