[PATCH] cxl/port: Enable the HDM decoder capability for switch ports

[PATCH] cxl/port: Enable the HDM decoder capability for switch ports

[Dan Williams]

Derick noticed, when testing hot plug, that hot-add behaves nominally
after a removal. However, if the hot-add is done without a prior
removal, CXL.mem accesses fail. It turns out that the original
implementation of the port driver and region programming wrongly assumed
that platform-firmware always enables the host-bridge HDM decoder
capability. Add support turning on switch-level HDM decoders in the case
where platform-firmware has not.

The implementation is careful to only arrange for the enable to be
undone if the current instance of the driver was the one that did the
enable. This is to interoperate with platform-firmware that may expect
CXL.mem to remain active after the driver is shutdown. This comes at the
cost of potentially not shutting down the enable on kexec flows, but it
is mitigated by the fact that the related HDM decoders still need to be
enabled on an individual basis.

Cc: <stable@vger.kernel.org>
Reported-by: Derick Marks <derick.w.marks@intel.com>
Fixes: 54cdbf845cf7 ("cxl/port: Add a driver for 'struct cxl_port' objects")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c        |   27 +++++++++++++++++++++++----
 drivers/cxl/cxl.h             |    1 +
 drivers/cxl/port.c            |   14 +++++++++-----
 tools/testing/cxl/Kbuild      |    1 +
 tools/testing/cxl/test/mock.c |   15 +++++++++++++++
 5 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index f332fe7af92b..c35c002b65dd 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -241,17 +241,36 @@ static void disable_hdm(void *_cxlhdm)
 	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 }
 
-static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
+int devm_cxl_enable_hdm(struct cxl_port *port, struct cxl_hdm *cxlhdm)
 {
-	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	void __iomem *hdm;
 	u32 global_ctrl;
 
+	/*
+	 * If the hdm capability was not mapped there is nothing to enable and
+	 * the caller is responsible for what happens next.  For example,
+	 * emulate a passthrough decoder.
+	 */
+	if (IS_ERR(cxlhdm))
+		return 0;
+
+	hdm = cxlhdm->regs.hdm_decoder;
 	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+
+	/*
+	 * If the HDM decoder capability was enabled on entry, skip
+	 * registering disable_hdm() since this decode capability may be
+	 * owned by platform firmware.
+	 */
+	if (global_ctrl & CXL_HDM_DECODER_ENABLE)
+		return 0;
+
 	writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
 	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 
-	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
+	return devm_add_action_or_reset(&port->dev, disable_hdm, cxlhdm);
 }
+EXPORT_SYMBOL_NS_GPL(devm_cxl_enable_hdm, CXL);
 
 int cxl_dvsec_rr_decode(struct device *dev, int d,
 			struct cxl_endpoint_dvsec_info *info)
@@ -425,7 +444,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	if (info->mem_enabled)
 		return 0;
 
-	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
+	rc = devm_cxl_enable_hdm(port, cxlhdm);
 	if (rc)
 		return rc;
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 044a92d9813e..f93a28538962 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -710,6 +710,7 @@ struct cxl_endpoint_dvsec_info {
 struct cxl_hdm;
 struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
 				   struct cxl_endpoint_dvsec_info *info);
+int devm_cxl_enable_hdm(struct cxl_port *port, struct cxl_hdm *cxlhdm);
 int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 				struct cxl_endpoint_dvsec_info *info);
 int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index eb57324c4ad4..17a95f469c26 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -60,13 +60,17 @@ static int discover_region(struct device *dev, void *root)
 static int cxl_switch_port_probe(struct cxl_port *port)
 {
 	struct cxl_hdm *cxlhdm;
-	int rc;
+	int rc, nr_dports;
 
-	rc = devm_cxl_port_enumerate_dports(port);
-	if (rc < 0)
-		return rc;
+	nr_dports = devm_cxl_port_enumerate_dports(port);
+	if (nr_dports < 0)
+		return nr_dports;
 
 	cxlhdm = devm_cxl_setup_hdm(port, NULL);
+	rc = devm_cxl_enable_hdm(port, cxlhdm);
+	if (rc)
+		return rc;
+
 	if (!IS_ERR(cxlhdm))
 		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
 
@@ -75,7 +79,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
 		return PTR_ERR(cxlhdm);
 	}
 
-	if (rc == 1) {
+	if (nr_dports == 1) {
 		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
 		return devm_cxl_add_passthrough_decoder(port);
 	}
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index fba7bec96acd..6f9347ade82c 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -6,6 +6,7 @@ ldflags-y += --wrap=acpi_pci_find_root
 ldflags-y += --wrap=nvdimm_bus_register
 ldflags-y += --wrap=devm_cxl_port_enumerate_dports
 ldflags-y += --wrap=devm_cxl_setup_hdm
+ldflags-y += --wrap=devm_cxl_enable_hdm
 ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
 ldflags-y += --wrap=devm_cxl_enumerate_decoders
 ldflags-y += --wrap=cxl_await_media_ready
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index de3933a776fd..284416527644 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -149,6 +149,21 @@ struct cxl_hdm *__wrap_devm_cxl_setup_hdm(struct cxl_port *port,
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_setup_hdm, CXL);
 
+int __wrap_devm_cxl_enable_hdm(struct cxl_port *port, struct cxl_hdm *cxlhdm)
+{
+	int index, rc;
+	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
+
+	if (ops && ops->is_mock_port(port->uport))
+		rc = 0;
+	else
+		rc = devm_cxl_enable_hdm(port, cxlhdm);
+	put_cxl_mock_ops(index);
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_enable_hdm, CXL);
+
 int __wrap_devm_cxl_add_passthrough_decoder(struct cxl_port *port)
 {
 	int rc, index;

Re: [PATCH] cxl/port: Enable the HDM decoder capability for switch ports

[Ira Weiny]

Dan Williams wrote:
> Derick noticed, when testing hot plug, that hot-add behaves nominally
> after a removal. However, if the hot-add is done without a prior
> removal, CXL.mem accesses fail. It turns out that the original
> implementation of the port driver and region programming wrongly assumed
> that platform-firmware always enables the host-bridge HDM decoder
> capability. Add support turning on switch-level HDM decoders in the case
> where platform-firmware has not.
> 
> The implementation is careful to only arrange for the enable to be
> undone if the current instance of the driver was the one that did the
> enable. This is to interoperate with platform-firmware that may expect
> CXL.mem to remain active after the driver is shutdown. This comes at the
> cost of potentially not shutting down the enable on kexec flows, but it
> is mitigated by the fact that the related HDM decoders still need to be
> enabled on an individual basis.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Derick Marks <derick.w.marks@intel.com>
> Fixes: 54cdbf845cf7 ("cxl/port: Add a driver for 'struct cxl_port' objects")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Re: [PATCH] cxl/port: Enable the HDM decoder capability for switch ports

[Dave Jiang]

On 5/17/23 8:19 PM, Dan Williams wrote:
> Derick noticed, when testing hot plug, that hot-add behaves nominally
> after a removal. However, if the hot-add is done without a prior
> removal, CXL.mem accesses fail. It turns out that the original
> implementation of the port driver and region programming wrongly assumed
> that platform-firmware always enables the host-bridge HDM decoder
> capability. Add support turning on switch-level HDM decoders in the case
> where platform-firmware has not.
> 
> The implementation is careful to only arrange for the enable to be
> undone if the current instance of the driver was the one that did the
> enable. This is to interoperate with platform-firmware that may expect
> CXL.mem to remain active after the driver is shutdown. This comes at the
> cost of potentially not shutting down the enable on kexec flows, but it
> is mitigated by the fact that the related HDM decoders still need to be
> enabled on an individual basis.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Derick Marks <derick.w.marks@intel.com>
> Fixes: 54cdbf845cf7 ("cxl/port: Add a driver for 'struct cxl_port' objects")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/core/pci.c        |   27 +++++++++++++++++++++++----
>   drivers/cxl/cxl.h             |    1 +
>   drivers/cxl/port.c            |   14 +++++++++-----
>   tools/testing/cxl/Kbuild      |    1 +
>   tools/testing/cxl/test/mock.c |   15 +++++++++++++++
>   5 files changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index f332fe7af92b..c35c002b65dd 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -241,17 +241,36 @@ static void disable_hdm(void *_cxlhdm)
>   	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
>   }
>   
> -static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
> +int devm_cxl_enable_hdm(struct cxl_port *port, struct cxl_hdm *cxlhdm)
>   {
> -	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	void __iomem *hdm;
>   	u32 global_ctrl;
>   
> +	/*
> +	 * If the hdm capability was not mapped there is nothing to enable and
> +	 * the caller is responsible for what happens next.  For example,
> +	 * emulate a passthrough decoder.
> +	 */
> +	if (IS_ERR(cxlhdm))
> +		return 0;
> +
> +	hdm = cxlhdm->regs.hdm_decoder;
>   	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> +
> +	/*
> +	 * If the HDM decoder capability was enabled on entry, skip
> +	 * registering disable_hdm() since this decode capability may be
> +	 * owned by platform firmware.
> +	 */
> +	if (global_ctrl & CXL_HDM_DECODER_ENABLE)
> +		return 0;
> +
>   	writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
>   	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
>   
> -	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
> +	return devm_add_action_or_reset(&port->dev, disable_hdm, cxlhdm);
>   }
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_enable_hdm, CXL);
>   
>   int cxl_dvsec_rr_decode(struct device *dev, int d,
>   			struct cxl_endpoint_dvsec_info *info)
> @@ -425,7 +444,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>   	if (info->mem_enabled)
>   		return 0;
>   
> -	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
> +	rc = devm_cxl_enable_hdm(port, cxlhdm);
>   	if (rc)
>   		return rc;
>   
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 044a92d9813e..f93a28538962 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -710,6 +710,7 @@ struct cxl_endpoint_dvsec_info {
>   struct cxl_hdm;
>   struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>   				   struct cxl_endpoint_dvsec_info *info);
> +int devm_cxl_enable_hdm(struct cxl_port *port, struct cxl_hdm *cxlhdm);
>   int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>   				struct cxl_endpoint_dvsec_info *info);
>   int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index eb57324c4ad4..17a95f469c26 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -60,13 +60,17 @@ static int discover_region(struct device *dev, void *root)
>   static int cxl_switch_port_probe(struct cxl_port *port)
>   {
>   	struct cxl_hdm *cxlhdm;
> -	int rc;
> +	int rc, nr_dports;
>   
> -	rc = devm_cxl_port_enumerate_dports(port);
> -	if (rc < 0)
> -		return rc;
> +	nr_dports = devm_cxl_port_enumerate_dports(port);
> +	if (nr_dports < 0)
> +		return nr_dports;
>   
>   	cxlhdm = devm_cxl_setup_hdm(port, NULL);
> +	rc = devm_cxl_enable_hdm(port, cxlhdm);
> +	if (rc)
> +		return rc;
> +
>   	if (!IS_ERR(cxlhdm))
>   		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>   
> @@ -75,7 +79,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>   		return PTR_ERR(cxlhdm);
>   	}
>   
> -	if (rc == 1) {
> +	if (nr_dports == 1) {
>   		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
>   		return devm_cxl_add_passthrough_decoder(port);
>   	}
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index fba7bec96acd..6f9347ade82c 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -6,6 +6,7 @@ ldflags-y += --wrap=acpi_pci_find_root
>   ldflags-y += --wrap=nvdimm_bus_register
>   ldflags-y += --wrap=devm_cxl_port_enumerate_dports
>   ldflags-y += --wrap=devm_cxl_setup_hdm
> +ldflags-y += --wrap=devm_cxl_enable_hdm
>   ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
>   ldflags-y += --wrap=devm_cxl_enumerate_decoders
>   ldflags-y += --wrap=cxl_await_media_ready
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index de3933a776fd..284416527644 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -149,6 +149,21 @@ struct cxl_hdm *__wrap_devm_cxl_setup_hdm(struct cxl_port *port,
>   }
>   EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_setup_hdm, CXL);
>   
> +int __wrap_devm_cxl_enable_hdm(struct cxl_port *port, struct cxl_hdm *cxlhdm)
> +{
> +	int index, rc;
> +	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
> +
> +	if (ops && ops->is_mock_port(port->uport))
> +		rc = 0;
> +	else
> +		rc = devm_cxl_enable_hdm(port, cxlhdm);
> +	put_cxl_mock_ops(index);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_enable_hdm, CXL);
> +
>   int __wrap_devm_cxl_add_passthrough_decoder(struct cxl_port *port)
>   {
>   	int rc, index;
> 

Re: [PATCH] cxl/port: Enable the HDM decoder capability for switch ports

[Jonathan Cameron]

On Wed, 17 May 2023 20:19:43 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Derick noticed, when testing hot plug, that hot-add behaves nominally
> after a removal. However, if the hot-add is done without a prior
> removal, CXL.mem accesses fail. It turns out that the original
> implementation of the port driver and region programming wrongly assumed
> that platform-firmware always enables the host-bridge HDM decoder
> capability. Add support turning on switch-level HDM decoders in the case
> where platform-firmware has not.
> 
> The implementation is careful to only arrange for the enable to be
> undone if the current instance of the driver was the one that did the
> enable. This is to interoperate with platform-firmware that may expect
> CXL.mem to remain active after the driver is shutdown. This comes at the
> cost of potentially not shutting down the enable on kexec flows, but it
> is mitigated by the fact that the related HDM decoders still need to be
> enabled on an individual basis.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Derick Marks <derick.w.marks@intel.com>
> Fixes: 54cdbf845cf7 ("cxl/port: Add a driver for 'struct cxl_port' objects")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I doubt anyone will be surprised to hear that the QEMU emulation wasn't
checking this bit when doing the topology walk.
Oops - I'll put together a fix.

I'm guessing this is already queued or upstream but if not
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


Jonathan

> ---
>  drivers/cxl/core/pci.c        |   27 +++++++++++++++++++++++----
>  drivers/cxl/cxl.h             |    1 +
>  drivers/cxl/port.c            |   14 +++++++++-----
>  tools/testing/cxl/Kbuild      |    1 +
>  tools/testing/cxl/test/mock.c |   15 +++++++++++++++
>  5 files changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index f332fe7af92b..c35c002b65dd 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -241,17 +241,36 @@ static void disable_hdm(void *_cxlhdm)
>  	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
>  }
>  
> -static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
> +int devm_cxl_enable_hdm(struct cxl_port *port, struct cxl_hdm *cxlhdm)
>  {
> -	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	void __iomem *hdm;
>  	u32 global_ctrl;
>  
> +	/*
> +	 * If the hdm capability was not mapped there is nothing to enable and
> +	 * the caller is responsible for what happens next.  For example,
> +	 * emulate a passthrough decoder.
> +	 */
> +	if (IS_ERR(cxlhdm))
> +		return 0;
> +
> +	hdm = cxlhdm->regs.hdm_decoder;
>  	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> +
> +	/*
> +	 * If the HDM decoder capability was enabled on entry, skip
> +	 * registering disable_hdm() since this decode capability may be
> +	 * owned by platform firmware.
> +	 */
> +	if (global_ctrl & CXL_HDM_DECODER_ENABLE)
> +		return 0;
> +
>  	writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
>  	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
>  
> -	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
> +	return devm_add_action_or_reset(&port->dev, disable_hdm, cxlhdm);
>  }
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_enable_hdm, CXL);
>  
>  int cxl_dvsec_rr_decode(struct device *dev, int d,
>  			struct cxl_endpoint_dvsec_info *info)
> @@ -425,7 +444,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  	if (info->mem_enabled)
>  		return 0;
>  
> -	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
> +	rc = devm_cxl_enable_hdm(port, cxlhdm);
>  	if (rc)
>  		return rc;
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 044a92d9813e..f93a28538962 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -710,6 +710,7 @@ struct cxl_endpoint_dvsec_info {
>  struct cxl_hdm;
>  struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>  				   struct cxl_endpoint_dvsec_info *info);
> +int devm_cxl_enable_hdm(struct cxl_port *port, struct cxl_hdm *cxlhdm);
>  int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  				struct cxl_endpoint_dvsec_info *info);
>  int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index eb57324c4ad4..17a95f469c26 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -60,13 +60,17 @@ static int discover_region(struct device *dev, void *root)
>  static int cxl_switch_port_probe(struct cxl_port *port)
>  {
>  	struct cxl_hdm *cxlhdm;
> -	int rc;
> +	int rc, nr_dports;
>  
> -	rc = devm_cxl_port_enumerate_dports(port);
> -	if (rc < 0)
> -		return rc;
> +	nr_dports = devm_cxl_port_enumerate_dports(port);
> +	if (nr_dports < 0)
> +		return nr_dports;
>  
>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
> +	rc = devm_cxl_enable_hdm(port, cxlhdm);
> +	if (rc)
> +		return rc;
> +
>  	if (!IS_ERR(cxlhdm))
>  		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>  
> @@ -75,7 +79,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  		return PTR_ERR(cxlhdm);
>  	}
>  
> -	if (rc == 1) {
> +	if (nr_dports == 1) {
>  		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
>  		return devm_cxl_add_passthrough_decoder(port);
>  	}

Re: [PATCH] cxl/port: Enable the HDM decoder capability for switch ports

[Dan Williams]

Jonathan Cameron wrote:
> On Wed, 17 May 2023 20:19:43 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Derick noticed, when testing hot plug, that hot-add behaves nominally
> > after a removal. However, if the hot-add is done without a prior
> > removal, CXL.mem accesses fail. It turns out that the original
> > implementation of the port driver and region programming wrongly assumed
> > that platform-firmware always enables the host-bridge HDM decoder
> > capability. Add support turning on switch-level HDM decoders in the case
> > where platform-firmware has not.
> > 
> > The implementation is careful to only arrange for the enable to be
> > undone if the current instance of the driver was the one that did the
> > enable. This is to interoperate with platform-firmware that may expect
> > CXL.mem to remain active after the driver is shutdown. This comes at the
> > cost of potentially not shutting down the enable on kexec flows, but it
> > is mitigated by the fact that the related HDM decoders still need to be
> > enabled on an individual basis.
> > 
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Derick Marks <derick.w.marks@intel.com>
> > Fixes: 54cdbf845cf7 ("cxl/port: Add a driver for 'struct cxl_port' objects")
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> I doubt anyone will be surprised to hear that the QEMU emulation wasn't
> checking this bit when doing the topology walk.
> Oops - I'll put together a fix.
> 
> I'm guessing this is already queued or upstream but if not
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

So I took it on faith from the reporter who said "the kernel is failing
to enable the HDM decoder capability in the root port", but now that I
go back and read the specification this bit is only applicable to
*endpoints*. So something else is wrong and this needs to be reverted.

Re: [PATCH] cxl/port: Enable the HDM decoder capability for switch ports

[Jonathan Cameron]

On Thu, 1 Jun 2023 10:25:13 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Wed, 17 May 2023 20:19:43 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > Derick noticed, when testing hot plug, that hot-add behaves nominally
> > > after a removal. However, if the hot-add is done without a prior
> > > removal, CXL.mem accesses fail. It turns out that the original
> > > implementation of the port driver and region programming wrongly assumed
> > > that platform-firmware always enables the host-bridge HDM decoder
> > > capability. Add support turning on switch-level HDM decoders in the case
> > > where platform-firmware has not.
> > > 
> > > The implementation is careful to only arrange for the enable to be
> > > undone if the current instance of the driver was the one that did the
> > > enable. This is to interoperate with platform-firmware that may expect
> > > CXL.mem to remain active after the driver is shutdown. This comes at the
> > > cost of potentially not shutting down the enable on kexec flows, but it
> > > is mitigated by the fact that the related HDM decoders still need to be
> > > enabled on an individual basis.
> > > 
> > > Cc: <stable@vger.kernel.org>
> > > Reported-by: Derick Marks <derick.w.marks@intel.com>
> > > Fixes: 54cdbf845cf7 ("cxl/port: Add a driver for 'struct cxl_port' objects")
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > 
> > I doubt anyone will be surprised to hear that the QEMU emulation wasn't
> > checking this bit when doing the topology walk.
> > Oops - I'll put together a fix.
> > 
> > I'm guessing this is already queued or upstream but if not
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> So I took it on faith from the reporter who said "the kernel is failing
> to enable the HDM decoder capability in the root port", but now that I
> go back and read the specification this bit is only applicable to
> *endpoints*. So something else is wrong and this needs to be reverted.

Glad you checked! :)

I'll drop the qemu patch (not posted yet)

Thanks,

Jonathan