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

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

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