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

[Dave Jiang <dave.jiang@intel.com>]

On 6/15/23 12:53, Dan Williams wrote:
> commit eb0764b822b9 ("cxl/port: Enable the HDM decoder capability for switch ports")
> 
> ...was added on the observation of CXL memory not being accessible after
> setting up a region on a "cold-plugged" device. A "cold-plugged" CXL
> device is one that was not present at boot, so platform-firmware/BIOS
> has no chance to set it up.
> 
> While it is true that the debug found the enable bit clear in the
> host-bridge's instance of the global control register (CXL 3.0
> 8.2.4.19.2 CXL HDM Decoder Global Control Register), that bit is
> described as:
> 
> "This bit is only applicable to CXL.mem devices and shall
> return 0 on CXL Host Bridges and Upstream Switch Ports."
> 
> So it is meant to be zero, and further testing confirmed that this "fix"
> had no effect on the failure. Revert it, and be more vigilant about
> proposed fixes in the future. Since the original copied stable@, flag
> this revert for stable@ as well.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: eb0764b822b9 ("cxl/port: Enable the HDM decoder capability for switch ports")
> 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, 9 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 7440f84be6c8..552203c13b39 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -308,36 +308,17 @@ static void disable_hdm(void *_cxlhdm)
>   	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
>   }
>   
> -int devm_cxl_enable_hdm(struct cxl_port *port, struct cxl_hdm *cxlhdm)
> +static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
>   {
> -	void __iomem *hdm;
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>   	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(&port->dev, disable_hdm, cxlhdm);
> +	return devm_add_action_or_reset(host, 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)
> @@ -511,7 +492,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, cxlhdm);
> +	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
>   	if (rc)
>   		return rc;
>   
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 74548f8f5f4c..d743df66a582 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -717,7 +717,6 @@ 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 5ffe3c7d2f5e..43718d0396d7 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -60,17 +60,13 @@ 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, nr_dports;
> -
> -	nr_dports = devm_cxl_port_enumerate_dports(port);
> -	if (nr_dports < 0)
> -		return nr_dports;
> +	int rc;
>   
> -	cxlhdm = devm_cxl_setup_hdm(port, NULL);
> -	rc = devm_cxl_enable_hdm(port, cxlhdm);
> -	if (rc)
> +	rc = devm_cxl_port_enumerate_dports(port);
> +	if (rc < 0)
>   		return rc;
>   
> +	cxlhdm = devm_cxl_setup_hdm(port, NULL);
>   	if (!IS_ERR(cxlhdm))
>   		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
>   
> @@ -79,7 +75,7 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>   		return PTR_ERR(cxlhdm);
>   	}
>   
> -	if (nr_dports == 1) {
> +	if (rc == 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 6f9347ade82c..fba7bec96acd 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -6,7 +6,6 @@ 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 284416527644..de3933a776fd 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -149,21 +149,6 @@ 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;
>