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

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

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?
As normal I'm not keen on the macro magic if we can avoid it.


> +			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...

> +
> +			target_map = temp;

This is set to a variable that goes out of scope whilst target_map is still in
use.

> +			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...

> +		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;
> +}
> +