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

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

On Thu, 15 Jun 2023 16:37:04 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 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>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Note we also dropped the 'fix' from the QEMU emulation.

Thanks,

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