On Thu, 15 Jun 2023 16:37:04 -0700
Dave Jiang <dave.jiang@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Fixes: eb0764b822b9 ("cxl/port: Enable the HDM decoder capability for switch ports")
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
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;
> >
