[PATCH 0/9] CXL port prep work

[PATCH 0/9] CXL port prep work

[Ben Widawsky]

These are the first several patches from the CXL port patch series. They
primarily rework handling some of the handling in cxl_pci reworks and puts some
infrastructure in place for endpoints and ports.

The patches do not have overlap with other domains, such as PCI.

Ben Widawsky (9):
  cxl: Rename CXL_MEM to CXL_PCI
  cxl: Flesh out register names
  cxl/pci: Extract device status check
  cxl/pci: Implement Interface Ready Timeout
  cxl/pci: Don't poll doorbell for mailbox access
  cxl/pci: Add new DVSEC definitions
  cxl/acpi: Map component registers for Root Ports
  cxl: Introduce module_cxl_driver
  cxl/core: Convert decoder range to resource

 drivers/cxl/Kconfig     |  23 +++---
 drivers/cxl/Makefile    |   2 +-
 drivers/cxl/acpi.c      |  35 ++++-----
 drivers/cxl/core/bus.c  |  23 +++++-
 drivers/cxl/core/regs.c |  54 ++++++++++++++
 drivers/cxl/cxl.h       |  15 +++-
 drivers/cxl/pci.c       | 152 +++++++++++++++-------------------------
 drivers/cxl/pci.h       |  41 ++++++++---
 8 files changed, 211 insertions(+), 134 deletions(-)

-- 
2.34.1

[PATCH 1/9] cxl: Rename CXL_MEM to CXL_PCI

[Ben Widawsky]

The cxl_mem module was renamed cxl_pci in commit 21e9f76733a8 ("cxl:
Rename mem to pci"). In preparation for adding an ancillary driver for
cxl_memdev devices (registered on the cxl bus by cxl_pci), go ahead and
rename CONFIG_CXL_MEM to CONFIG_CXL_PCI. Free up the CXL_MEM name for
that new driver to manage CXL.mem endpoint operations.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/Kconfig  | 23 ++++++++++++-----------
 drivers/cxl/Makefile |  2 +-
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index 67c91378f2dd..ef05e96f8f97 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -13,25 +13,26 @@ menuconfig CXL_BUS
 
 if CXL_BUS
 
-config CXL_MEM
-	tristate "CXL.mem: Memory Devices"
+config CXL_PCI
+	tristate "PCI manageability"
 	default CXL_BUS
 	help
-	  The CXL.mem protocol allows a device to act as a provider of
-	  "System RAM" and/or "Persistent Memory" that is fully coherent
-	  as if the memory was attached to the typical CPU memory
-	  controller.
+	  The CXL specification defines a "CXL memory device" sub-class in the
+	  PCI "memory controller" base class of devices. Device's identified by
+	  this class code provide support for volatile and / or persistent
+	  memory to be mapped into the system address map (Host-managed Device
+	  Memory (HDM)).
 
-	  Say 'y/m' to enable a driver that will attach to CXL.mem devices for
-	  configuration and management primarily via the mailbox interface. See
-	  Chapter 2.3 Type 3 CXL Device in the CXL 2.0 specification for more
-	  details.
+	  Say 'y/m' to enable a driver that will attach to CXL memory expander
+	  devices enumerated by the memory device class code for configuration
+	  and management primarily via the mailbox interface. See Chapter 2.3
+	  Type 3 CXL Device in the CXL 2.0 specification for more details.
 
 	  If unsure say 'm'.
 
 config CXL_MEM_RAW_COMMANDS
 	bool "RAW Command Interface for Memory Devices"
-	depends on CXL_MEM
+	depends on CXL_PCI
 	help
 	  Enable CXL RAW command interface.
 
diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
index d1aaabc940f3..cf07ae6cea17 100644
--- a/drivers/cxl/Makefile
+++ b/drivers/cxl/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CXL_BUS) += core/
-obj-$(CONFIG_CXL_MEM) += cxl_pci.o
+obj-$(CONFIG_CXL_PCI) += cxl_pci.o
 obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
 obj-$(CONFIG_CXL_PMEM) += cxl_pmem.o
 
-- 
2.34.1

[PATCH 2/9] cxl: Flesh out register names

[Ben Widawsky]

Get a better naming scheme in place for upcoming additions. By dropping
redundant usages of CXL and DVSEC where appropriate we can get more
concise and also more grepable defines.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/pci.c | 14 +++++++-------
 drivers/cxl/pci.h | 19 ++++++++++---------
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 8dc91fd3396a..a6ea9811a05b 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -403,10 +403,10 @@ static int cxl_map_regs(struct cxl_dev_state *cxlds, struct cxl_register_map *ma
 static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
 				struct cxl_register_map *map)
 {
-	map->block_offset =
-		((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
-	map->barno = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
-	map->reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
+	map->block_offset = ((u64)reg_hi << 32) |
+			    (reg_lo & CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK);
+	map->barno = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BIR_MASK, reg_lo);
+	map->reg_type = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK, reg_lo);
 }
 
 /**
@@ -427,15 +427,15 @@ static int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
 	int regloc, i;
 
 	regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
-					   PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID);
+					   CXL_DVSEC_REG_LOCATOR);
 	if (!regloc)
 		return -ENXIO;
 
 	pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, &regloc_size);
 	regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);
 
-	regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
-	regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
+	regloc += CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET;
+	regblocks = (regloc_size - CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET) / 8;
 
 	for (i = 0; i < regblocks; i++, regloc += 8) {
 		u32 reg_lo, reg_hi;
diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index 7d3e4bf06b45..29b8eaef3a0a 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -7,17 +7,21 @@
 
 /*
  * See section 8.1 Configuration Space Registers in the CXL 2.0
- * Specification
+ * Specification. Names are taken straight from the specification with "CXL" and
+ * "DVSEC" redundancies removed. When obvious, abbreviations may be used.
  */
 #define PCI_DVSEC_HEADER1_LENGTH_MASK	GENMASK(31, 20)
 #define PCI_DVSEC_VENDOR_ID_CXL		0x1E98
-#define PCI_DVSEC_ID_CXL		0x0
 
-#define PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID	0x8
-#define PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET	0xC
+/* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */
+#define CXL_DVSEC_PCIE_DEVICE					0
 
-/* BAR Indicator Register (BIR) */
-#define CXL_REGLOC_BIR_MASK GENMASK(2, 0)
+/* CXL 2.0 8.1.9: Register Locator DVSEC */
+#define CXL_DVSEC_REG_LOCATOR					8
+#define   CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET			0xC
+#define     CXL_DVSEC_REG_LOCATOR_BIR_MASK			GENMASK(2, 0)
+#define	    CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK			GENMASK(15, 8)
+#define     CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK		GENMASK(31, 16)
 
 /* Register Block Identifier (RBI) */
 enum cxl_regloc_type {
@@ -28,7 +32,4 @@ enum cxl_regloc_type {
 	CXL_REGLOC_RBI_TYPES
 };
 
-#define CXL_REGLOC_RBI_MASK GENMASK(15, 8)
-#define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)
-
 #endif /* __CXL_PCI_H__ */
-- 
2.34.1

[PATCH 3/9] cxl/pci: Extract device status check

[Ben Widawsky]

The Memory Device Status register is inspected in the same way for at
least two flows in the CXL Type 3 Memory Device Software Guide
(Revision: 1.0): 2.13.9 Device discovery and mailbox ready sequence,
and 2.13.10 Media ready sequence. Extract this common functionality for
use by both.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/pci.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index a6ea9811a05b..6c8d09fb3a17 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -182,6 +182,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
 	return 0;
 }
 
+/*
+ * Implements roughly the bottom half of Figure 42 of the CXL Type 3 Memory
+ * Device Software Guide
+ */
+static int check_device_status(struct cxl_dev_state *cxlds)
+{
+	const u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
+
+	if (md_status & CXLMDEV_DEV_FATAL) {
+		dev_err(cxlds->dev, "Fatal: replace device\n");
+		return -EIO;
+	}
+
+	if (md_status & CXLMDEV_FW_HALT) {
+		dev_err(cxlds->dev, "FWHalt: reset or replace device\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
 /**
  * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox.
  * @cxlds: The device state to gain access to.
@@ -231,17 +252,13 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)
 	 * Hardware shouldn't allow a ready status but also have failure bits
 	 * set. Spit out an error, this should be a bug report
 	 */
-	rc = -EFAULT;
-	if (md_status & CXLMDEV_DEV_FATAL) {
-		dev_err(dev, "mbox: reported ready, but fatal\n");
+	rc = check_device_status(cxlds);
+	if (rc)
 		goto out;
-	}
-	if (md_status & CXLMDEV_FW_HALT) {
-		dev_err(dev, "mbox: reported ready, but halted\n");
-		goto out;
-	}
+
 	if (CXLMDEV_RESET_NEEDED(md_status)) {
 		dev_err(dev, "mbox: reported ready, but reset needed\n");
+		rc = -EFAULT;
 		goto out;
 	}
 
-- 
2.34.1

Re: [PATCH 3/9] cxl/pci: Extract device status check

[Dan Williams]

On Mon, Nov 29, 2021 at 1:47 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> The Memory Device Status register is inspected in the same way for at
> least two flows in the CXL Type 3 Memory Device Software Guide
> (Revision: 1.0): 2.13.9 Device discovery and mailbox ready sequence,
> and 2.13.10 Media ready sequence. Extract this common functionality for
> use by both.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/pci.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index a6ea9811a05b..6c8d09fb3a17 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -182,6 +182,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>         return 0;
>  }
>
> +/*
> + * Implements roughly the bottom half of Figure 42 of the CXL Type 3 Memory
> + * Device Software Guide
> + */

This version did not address the feedback about referencing the CXL
specification instead of the software guide.

> +static int check_device_status(struct cxl_dev_state *cxlds)
> +{
> +       const u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> +
> +       if (md_status & CXLMDEV_DEV_FATAL) {
> +               dev_err(cxlds->dev, "Fatal: replace device\n");
> +               return -EIO;
> +       }
> +
> +       if (md_status & CXLMDEV_FW_HALT) {
> +               dev_err(cxlds->dev, "FWHalt: reset or replace device\n");
> +               return -EBUSY;
> +       }
> +
> +       return 0;
> +}
> +
>  /**
>   * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox.
>   * @cxlds: The device state to gain access to.
> @@ -231,17 +252,13 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)
>          * Hardware shouldn't allow a ready status but also have failure bits
>          * set. Spit out an error, this should be a bug report
>          */
> -       rc = -EFAULT;
> -       if (md_status & CXLMDEV_DEV_FATAL) {
> -               dev_err(dev, "mbox: reported ready, but fatal\n");
> +       rc = check_device_status(cxlds);
> +       if (rc)
>                 goto out;
> -       }
> -       if (md_status & CXLMDEV_FW_HALT) {
> -               dev_err(dev, "mbox: reported ready, but halted\n");
> -               goto out;
> -       }
> +
>         if (CXLMDEV_RESET_NEEDED(md_status)) {
>                 dev_err(dev, "mbox: reported ready, but reset needed\n");
> +               rc = -EFAULT;
>                 goto out;

It also did not address the feedback that the reset needed check can
be dropped because per the spec* it is redundant with the other checks
above. The driver need not give up on mailbox commands just because
the media is not ready. So this wants a lead-in patch to clean that up
first.

"This field returns non-zero value if FW Halt is set or Media Status
is not in the ready state."

Going further I do not think the driver should preemptively abort
commands due to device-fatal, or even firmware halt for that matter.
The true sign of failure and dead firmware is timed-out responses to
incoming commands. Just in case there are some commands still
functional in a device-fatal condition, or the firmware-halt indicator
was a false positive, let the command through, and then if it fails
report these forensic statuses.

"Mailbox Interfaces Ready" is the only status that needs to be checked
preemptively, and only once at the beginning of time until the next
command timeout where the driver collects it to find out what
happened.

I'll take a shot at a lead-in patch to that effect.

Re: [PATCH 3/9] cxl/pci: Extract device status check

[Ben Widawsky]

On 21-12-02 09:09:44, Dan Williams wrote:
> On Mon, Nov 29, 2021 at 1:47 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > The Memory Device Status register is inspected in the same way for at
> > least two flows in the CXL Type 3 Memory Device Software Guide
> > (Revision: 1.0): 2.13.9 Device discovery and mailbox ready sequence,
> > and 2.13.10 Media ready sequence. Extract this common functionality for
> > use by both.
> >
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >  drivers/cxl/pci.c | 33 +++++++++++++++++++++++++--------
> >  1 file changed, 25 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index a6ea9811a05b..6c8d09fb3a17 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -182,6 +182,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
> >         return 0;
> >  }
> >
> > +/*
> > + * Implements roughly the bottom half of Figure 42 of the CXL Type 3 Memory
> > + * Device Software Guide
> > + */
> 
> This version did not address the feedback about referencing the CXL
> specification instead of the software guide.
> 

Apologies, I missed that request. I apparently missed your email entirely as I
was going through the revision.

> > +static int check_device_status(struct cxl_dev_state *cxlds)
> > +{
> > +       const u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> > +
> > +       if (md_status & CXLMDEV_DEV_FATAL) {
> > +               dev_err(cxlds->dev, "Fatal: replace device\n");
> > +               return -EIO;
> > +       }
> > +
> > +       if (md_status & CXLMDEV_FW_HALT) {
> > +               dev_err(cxlds->dev, "FWHalt: reset or replace device\n");
> > +               return -EBUSY;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  /**
> >   * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox.
> >   * @cxlds: The device state to gain access to.
> > @@ -231,17 +252,13 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)
> >          * Hardware shouldn't allow a ready status but also have failure bits
> >          * set. Spit out an error, this should be a bug report
> >          */
> > -       rc = -EFAULT;
> > -       if (md_status & CXLMDEV_DEV_FATAL) {
> > -               dev_err(dev, "mbox: reported ready, but fatal\n");
> > +       rc = check_device_status(cxlds);
> > +       if (rc)
> >                 goto out;
> > -       }
> > -       if (md_status & CXLMDEV_FW_HALT) {
> > -               dev_err(dev, "mbox: reported ready, but halted\n");
> > -               goto out;
> > -       }
> > +
> >         if (CXLMDEV_RESET_NEEDED(md_status)) {
> >                 dev_err(dev, "mbox: reported ready, but reset needed\n");
> > +               rc = -EFAULT;
> >                 goto out;
> 
> It also did not address the feedback that the reset needed check can
> be dropped because per the spec* it is redundant with the other checks
> above. The driver need not give up on mailbox commands just because
> the media is not ready. So this wants a lead-in patch to clean that up
> first.

Media ready droppage comes in the next patch. I can reorder them if you'd like,
or I suppose you could do it.

> 
> "This field returns non-zero value if FW Halt is set or Media Status
> is not in the ready state."
> 
> Going further I do not think the driver should preemptively abort
> commands due to device-fatal, or even firmware halt for that matter.
> The true sign of failure and dead firmware is timed-out responses to
> incoming commands. Just in case there are some commands still
> functional in a device-fatal condition, or the firmware-halt indicator
> was a false positive, let the command through, and then if it fails
> report these forensic statuses.

I admit I'm not looking for spec reference at this exact moment. I think trying
to throw commands at the device that has already told us is fatal, is a bad
idea.

> 
> "Mailbox Interfaces Ready" is the only status that needs to be checked
> preemptively, and only once at the beginning of time until the next
> command timeout where the driver collects it to find out what
> happened.

I think this happens in a later patch too, if I catch your meaning.

> 
> I'll take a shot at a lead-in patch to that effect.

Re: [PATCH 3/9] cxl/pci: Extract device status check

[Dan Williams]

On Thu, Dec 2, 2021 at 9:24 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-12-02 09:09:44, Dan Williams wrote:
> > On Mon, Nov 29, 2021 at 1:47 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > The Memory Device Status register is inspected in the same way for at
> > > least two flows in the CXL Type 3 Memory Device Software Guide
> > > (Revision: 1.0): 2.13.9 Device discovery and mailbox ready sequence,
> > > and 2.13.10 Media ready sequence. Extract this common functionality for
> > > use by both.
> > >
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > ---
> > >  drivers/cxl/pci.c | 33 +++++++++++++++++++++++++--------
> > >  1 file changed, 25 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > index a6ea9811a05b..6c8d09fb3a17 100644
> > > --- a/drivers/cxl/pci.c
> > > +++ b/drivers/cxl/pci.c
> > > @@ -182,6 +182,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
> > >         return 0;
> > >  }
> > >
> > > +/*
> > > + * Implements roughly the bottom half of Figure 42 of the CXL Type 3 Memory
> > > + * Device Software Guide
> > > + */
> >
> > This version did not address the feedback about referencing the CXL
> > specification instead of the software guide.
> >
>
> Apologies, I missed that request. I apparently missed your email entirely as I
> was going through the revision.
>
> > > +static int check_device_status(struct cxl_dev_state *cxlds)
> > > +{
> > > +       const u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> > > +
> > > +       if (md_status & CXLMDEV_DEV_FATAL) {
> > > +               dev_err(cxlds->dev, "Fatal: replace device\n");
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       if (md_status & CXLMDEV_FW_HALT) {
> > > +               dev_err(cxlds->dev, "FWHalt: reset or replace device\n");
> > > +               return -EBUSY;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  /**
> > >   * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox.
> > >   * @cxlds: The device state to gain access to.
> > > @@ -231,17 +252,13 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)
> > >          * Hardware shouldn't allow a ready status but also have failure bits
> > >          * set. Spit out an error, this should be a bug report
> > >          */
> > > -       rc = -EFAULT;
> > > -       if (md_status & CXLMDEV_DEV_FATAL) {
> > > -               dev_err(dev, "mbox: reported ready, but fatal\n");
> > > +       rc = check_device_status(cxlds);
> > > +       if (rc)
> > >                 goto out;
> > > -       }
> > > -       if (md_status & CXLMDEV_FW_HALT) {
> > > -               dev_err(dev, "mbox: reported ready, but halted\n");
> > > -               goto out;
> > > -       }
> > > +
> > >         if (CXLMDEV_RESET_NEEDED(md_status)) {
> > >                 dev_err(dev, "mbox: reported ready, but reset needed\n");
> > > +               rc = -EFAULT;
> > >                 goto out;
> >
> > It also did not address the feedback that the reset needed check can
> > be dropped because per the spec* it is redundant with the other checks
> > above. The driver need not give up on mailbox commands just because
> > the media is not ready. So this wants a lead-in patch to clean that up
> > first.
>
> Media ready droppage comes in the next patch. I can reorder them if you'd like,
> or I suppose you could do it.

Sure, I'll reorder.

>
> >
> > "This field returns non-zero value if FW Halt is set or Media Status
> > is not in the ready state."
> >
> > Going further I do not think the driver should preemptively abort
> > commands due to device-fatal, or even firmware halt for that matter.
> > The true sign of failure and dead firmware is timed-out responses to
> > incoming commands. Just in case there are some commands still
> > functional in a device-fatal condition, or the firmware-halt indicator
> > was a false positive, let the command through, and then if it fails
> > report these forensic statuses.
>
> I admit I'm not looking for spec reference at this exact moment. I think trying
> to throw commands at the device that has already told us is fatal, is a bad
> idea.

It is always going to be a race condition. The driver can check
preemptively all it wants, but inevitably there will be cases where
the device goes fatal immediately after checking. So, executing
commands against a device with an error status set cannot be
prevented. I am asking to just delete the soft hopeful code and use
the hard timeout fallback exclusively.

> > "Mailbox Interfaces Ready" is the only status that needs to be checked
> > preemptively, and only once at the beginning of time until the next
> > command timeout where the driver collects it to find out what
> > happened.
>
> I think this happens in a later patch too, if I catch your meaning.

Yeah, looks like that patch needs to come first and then a patch to
switch to timeout-only error handling.

Re: [PATCH 3/9] cxl/pci: Extract device status check

[Ben Widawsky]

On 21-12-02 09:32:59, Dan Williams wrote:
> On Thu, Dec 2, 2021 at 9:24 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > On 21-12-02 09:09:44, Dan Williams wrote:
> > > On Mon, Nov 29, 2021 at 1:47 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > >
> > > > The Memory Device Status register is inspected in the same way for at
> > > > least two flows in the CXL Type 3 Memory Device Software Guide
> > > > (Revision: 1.0): 2.13.9 Device discovery and mailbox ready sequence,
> > > > and 2.13.10 Media ready sequence. Extract this common functionality for
> > > > use by both.
> > > >
> > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > > ---
> > > >  drivers/cxl/pci.c | 33 +++++++++++++++++++++++++--------
> > > >  1 file changed, 25 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > > index a6ea9811a05b..6c8d09fb3a17 100644
> > > > --- a/drivers/cxl/pci.c
> > > > +++ b/drivers/cxl/pci.c
> > > > @@ -182,6 +182,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
> > > >         return 0;
> > > >  }
> > > >
> > > > +/*
> > > > + * Implements roughly the bottom half of Figure 42 of the CXL Type 3 Memory
> > > > + * Device Software Guide
> > > > + */
> > >
> > > This version did not address the feedback about referencing the CXL
> > > specification instead of the software guide.
> > >
> >
> > Apologies, I missed that request. I apparently missed your email entirely as I
> > was going through the revision.
> >
> > > > +static int check_device_status(struct cxl_dev_state *cxlds)
> > > > +{
> > > > +       const u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> > > > +
> > > > +       if (md_status & CXLMDEV_DEV_FATAL) {
> > > > +               dev_err(cxlds->dev, "Fatal: replace device\n");
> > > > +               return -EIO;
> > > > +       }
> > > > +
> > > > +       if (md_status & CXLMDEV_FW_HALT) {
> > > > +               dev_err(cxlds->dev, "FWHalt: reset or replace device\n");
> > > > +               return -EBUSY;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > >  /**
> > > >   * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox.
> > > >   * @cxlds: The device state to gain access to.
> > > > @@ -231,17 +252,13 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)
> > > >          * Hardware shouldn't allow a ready status but also have failure bits
> > > >          * set. Spit out an error, this should be a bug report
> > > >          */
> > > > -       rc = -EFAULT;
> > > > -       if (md_status & CXLMDEV_DEV_FATAL) {
> > > > -               dev_err(dev, "mbox: reported ready, but fatal\n");
> > > > +       rc = check_device_status(cxlds);
> > > > +       if (rc)
> > > >                 goto out;
> > > > -       }
> > > > -       if (md_status & CXLMDEV_FW_HALT) {
> > > > -               dev_err(dev, "mbox: reported ready, but halted\n");
> > > > -               goto out;
> > > > -       }
> > > > +
> > > >         if (CXLMDEV_RESET_NEEDED(md_status)) {
> > > >                 dev_err(dev, "mbox: reported ready, but reset needed\n");
> > > > +               rc = -EFAULT;
> > > >                 goto out;
> > >
> > > It also did not address the feedback that the reset needed check can
> > > be dropped because per the spec* it is redundant with the other checks
> > > above. The driver need not give up on mailbox commands just because
> > > the media is not ready. So this wants a lead-in patch to clean that up
> > > first.
> >
> > Media ready droppage comes in the next patch. I can reorder them if you'd like,
> > or I suppose you could do it.
> 
> Sure, I'll reorder.
> 
> >
> > >
> > > "This field returns non-zero value if FW Halt is set or Media Status
> > > is not in the ready state."
> > >
> > > Going further I do not think the driver should preemptively abort
> > > commands due to device-fatal, or even firmware halt for that matter.
> > > The true sign of failure and dead firmware is timed-out responses to
> > > incoming commands. Just in case there are some commands still
> > > functional in a device-fatal condition, or the firmware-halt indicator
> > > was a false positive, let the command through, and then if it fails
> > > report these forensic statuses.
> >
> > I admit I'm not looking for spec reference at this exact moment. I think trying
> > to throw commands at the device that has already told us is fatal, is a bad
> > idea.
> 
> It is always going to be a race condition. The driver can check
> preemptively all it wants, but inevitably there will be cases where
> the device goes fatal immediately after checking. So, executing
> commands against a device with an error status set cannot be
> prevented. I am asking to just delete the soft hopeful code and use
> the hard timeout fallback exclusively.
> 

It doesn't bother me much to do so, but it's not my preference. If you'd like to
change it, go ahead and I will review it. When I wrote the original
implementation, my idea was that command timeout and error conditions driven by
interrupts (media issues, firmware issues, whatever) would be disparate paths.
My belief is the spec allows so much latitude, extra paranoia makes sense.

> > > "Mailbox Interfaces Ready" is the only status that needs to be checked
> > > preemptively, and only once at the beginning of time until the next
> > > command timeout where the driver collects it to find out what
> > > happened.
> >
> > I think this happens in a later patch too, if I catch your meaning.
> 
> Yeah, looks like that patch needs to come first and then a patch to
> switch to timeout-only error handling.

Re: [PATCH 3/9] cxl/pci: Extract device status check

[Dan Williams]

On Fri, Dec 3, 2021 at 5:18 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
[..]
> >
>
> It doesn't bother me much to do so, but it's not my preference. If you'd like to
> change it, go ahead and I will review it. When I wrote the original
> implementation, my idea was that command timeout and error conditions driven by
> interrupts (media issues, firmware issues, whatever) would be disparate paths.
> My belief is the spec allows so much latitude, extra paranoia makes sense.

I think device drivers are where the "only the paranoid survive"
mantra falls apart. Paranoid drivers fall over earlier than necessary.
I just picture Dave trying to disable HAL 9000, but can't even attempt
to send the mailbox command because the CXL device's media disabled
status is set.

[PATCH 4/9] cxl/pci: Implement Interface Ready Timeout

[Ben Widawsky]

The original driver implementation used the doorbell timeout for the
Mailbox Interface Ready bit to piggy back off of, since the latter
doesn't have a defined timeout. This functionality, introduced in commit
8adaf747c9f0 ("cxl/mem: Find device capabilities"), can now be improved
since a timeout has been defined with an ECN to the 2.0 spec.

While devices implemented prior to the ECN could have an arbitrarily
long wait and still be within spec, the max ECN value (256s) is chosen
as the default for all devices. All vendors in the consortium agreed to
this amount and so it is reasonable to assume no devices made will
exceed this amount.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

---
Changes since v1:
- Use 60 seconds for timeout instead of 256 (Dan)
---
 drivers/cxl/pci.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 6c8d09fb3a17..b28c220d48ea 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -2,6 +2,7 @@
 /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/module.h>
+#include <linux/delay.h>
 #include <linux/sizes.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
@@ -298,6 +299,38 @@ static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *c
 static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
 {
 	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
+	unsigned long timeout;
+	u64 md_status;
+	int rc;
+
+	/*
+	 * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
+	 * dictate how long to wait for the mailbox to become ready. The new
+	 * field allows the device to tell software the amount of time to wait
+	 * before mailbox ready. This field allows for up to 255 seconds. 255
+	 * seconds is unreasonable long, and longer than other default timeouts
+	 * in the OS. Use the more sane, 60 seconds instead.
+	 *
+	 * 100ms is chosen as the specified pause as it is the value used in the
+	 * CXL Type 3 Memory Device Software Guide.
+	 */
+	timeout = jiffies + 60 * HZ;
+
+	rc = check_device_status(cxlds);
+	if (rc)
+		return rc;
+
+	do {
+		md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
+		if (md_status & CXLMDEV_MBOX_IF_READY)
+			break;
+		if (msleep_interruptible(100))
+			break;
+	} while (!time_after(jiffies, timeout));
+
+	/* It's assumed that once the interface is ready, it will remain ready. */
+	if (!(md_status & CXLMDEV_MBOX_IF_READY))
+		return -EIO;
 
 	cxlds->mbox_send = cxl_pci_mbox_send;
 	cxlds->payload_size =
-- 
2.34.1

Re: [PATCH 4/9] cxl/pci: Implement Interface Ready Timeout

[Jonathan Cameron]

On Mon, 29 Nov 2021 13:47:16 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> The original driver implementation used the doorbell timeout for the
> Mailbox Interface Ready bit to piggy back off of, since the latter
> doesn't have a defined timeout. This functionality, introduced in commit
> 8adaf747c9f0 ("cxl/mem: Find device capabilities"), can now be improved
> since a timeout has been defined with an ECN to the 2.0 spec.
> 
> While devices implemented prior to the ECN could have an arbitrarily
> long wait and still be within spec, the max ECN value (256s) is chosen
> as the default for all devices. All vendors in the consortium agreed to
> this amount and so it is reasonable to assume no devices made will
> exceed this amount.

Update the description to talk about the 60 seconds choice.

I'm fine with 60 seconds, but I can see it being controversial...

> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> 
> ---
> Changes since v1:
> - Use 60 seconds for timeout instead of 256 (Dan)
> ---
>  drivers/cxl/pci.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 6c8d09fb3a17..b28c220d48ea 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -2,6 +2,7 @@
>  /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/module.h>
> +#include <linux/delay.h>
>  #include <linux/sizes.h>
>  #include <linux/mutex.h>
>  #include <linux/list.h>
> @@ -298,6 +299,38 @@ static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *c
>  static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
>  {
>  	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
> +	unsigned long timeout;
> +	u64 md_status;
> +	int rc;
> +
> +	/*
> +	 * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
> +	 * dictate how long to wait for the mailbox to become ready. The new
> +	 * field allows the device to tell software the amount of time to wait
> +	 * before mailbox ready. This field allows for up to 255 seconds. 255
> +	 * seconds is unreasonable long, and longer than other default timeouts
> +	 * in the OS. Use the more sane, 60 seconds instead.
> +	 *
> +	 * 100ms is chosen as the specified pause as it is the value used in the
> +	 * CXL Type 3 Memory Device Software Guide.
> +	 */
> +	timeout = jiffies + 60 * HZ;
> +
> +	rc = check_device_status(cxlds);
> +	if (rc)
> +		return rc;
> +
> +	do {
> +		md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> +		if (md_status & CXLMDEV_MBOX_IF_READY)
> +			break;
> +		if (msleep_interruptible(100))
> +			break;
> +	} while (!time_after(jiffies, timeout));
> +
> +	/* It's assumed that once the interface is ready, it will remain ready. */
> +	if (!(md_status & CXLMDEV_MBOX_IF_READY))
> +		return -EIO;
>  
>  	cxlds->mbox_send = cxl_pci_mbox_send;
>  	cxlds->payload_size =

[PATCH v2 4/9] cxl/pci: Implement Interface Ready Timeout

[Ben Widawsky]

The original driver implementation used the doorbell timeout for the
Mailbox Interface Ready bit to piggy back off of, since the latter
doesn't have a defined timeout. This functionality, introduced in commit
8adaf747c9f0 ("cxl/mem: Find device capabilities"), can now be improved
since a timeout has been defined with an ECN to the 2.0 spec.

While devices implemented prior to the ECN could have an arbitrarily
long wait (256) and still be within spec, 60s is chosen as the default
for all devices. This value corresponds with important timeout values
already present in the kernel.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

---
Changes since v1:
- Use 60 seconds for timeout instead of 256 (Dan)
- Update commit message (Jonathan)
---
 drivers/cxl/pci.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 6c8d09fb3a17..b28c220d48ea 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -2,6 +2,7 @@
 /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/module.h>
+#include <linux/delay.h>
 #include <linux/sizes.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
@@ -298,6 +299,38 @@ static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *c
 static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
 {
 	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
+	unsigned long timeout;
+	u64 md_status;
+	int rc;
+
+	/*
+	 * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
+	 * dictate how long to wait for the mailbox to become ready. The new
+	 * field allows the device to tell software the amount of time to wait
+	 * before mailbox ready. This field allows for up to 255 seconds. 255
+	 * seconds is unreasonable long, and longer than other default timeouts
+	 * in the OS. Use the more sane, 60 seconds instead.
+	 *
+	 * 100ms is chosen as the specified pause as it is the value used in the
+	 * CXL Type 3 Memory Device Software Guide.
+	 */
+	timeout = jiffies + 60 * HZ;
+
+	rc = check_device_status(cxlds);
+	if (rc)
+		return rc;
+
+	do {
+		md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
+		if (md_status & CXLMDEV_MBOX_IF_READY)
+			break;
+		if (msleep_interruptible(100))
+			break;
+	} while (!time_after(jiffies, timeout));
+
+	/* It's assumed that once the interface is ready, it will remain ready. */
+	if (!(md_status & CXLMDEV_MBOX_IF_READY))
+		return -EIO;
 
 	cxlds->mbox_send = cxl_pci_mbox_send;
 	cxlds->payload_size =
-- 
2.34.1

Re: [PATCH v2 4/9] cxl/pci: Implement Interface Ready Timeout

[Jonathan Cameron]

On Wed, 1 Dec 2021 20:45:04 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> The original driver implementation used the doorbell timeout for the
> Mailbox Interface Ready bit to piggy back off of, since the latter
> doesn't have a defined timeout. This functionality, introduced in commit
> 8adaf747c9f0 ("cxl/mem: Find device capabilities"), can now be improved
> since a timeout has been defined with an ECN to the 2.0 spec.
> 
> While devices implemented prior to the ECN could have an arbitrarily
> long wait (256) and still be within spec, 60s is chosen as the default
> for all devices. This value corresponds with important timeout values
> already present in the kernel.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> ---
> Changes since v1:
> - Use 60 seconds for timeout instead of 256 (Dan)
> - Update commit message (Jonathan)
> ---
>  drivers/cxl/pci.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 6c8d09fb3a17..b28c220d48ea 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -2,6 +2,7 @@
>  /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/module.h>
> +#include <linux/delay.h>
>  #include <linux/sizes.h>
>  #include <linux/mutex.h>
>  #include <linux/list.h>
> @@ -298,6 +299,38 @@ static int cxl_pci_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *c
>  static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
>  {
>  	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
> +	unsigned long timeout;
> +	u64 md_status;
> +	int rc;
> +
> +	/*
> +	 * CXL 2.0 ECN "Add Mailbox Ready Time" defines a capability field to
> +	 * dictate how long to wait for the mailbox to become ready. The new
> +	 * field allows the device to tell software the amount of time to wait
> +	 * before mailbox ready. This field allows for up to 255 seconds. 255
> +	 * seconds is unreasonable long, and longer than other default timeouts
> +	 * in the OS. Use the more sane, 60 seconds instead.
> +	 *
> +	 * 100ms is chosen as the specified pause as it is the value used in the
> +	 * CXL Type 3 Memory Device Software Guide.
> +	 */
> +	timeout = jiffies + 60 * HZ;
> +
> +	rc = check_device_status(cxlds);
> +	if (rc)
> +		return rc;
> +
> +	do {
> +		md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> +		if (md_status & CXLMDEV_MBOX_IF_READY)
> +			break;
> +		if (msleep_interruptible(100))
> +			break;
> +	} while (!time_after(jiffies, timeout));
> +
> +	/* It's assumed that once the interface is ready, it will remain ready. */
> +	if (!(md_status & CXLMDEV_MBOX_IF_READY))
> +		return -EIO;
>  
>  	cxlds->mbox_send = cxl_pci_mbox_send;
>  	cxlds->payload_size =

[PATCH 5/9] cxl/pci: Don't poll doorbell for mailbox access

[Ben Widawsky]

The expectation is that the mailbox interface ready bit will be set at
the start of any access through the mailbox interface. Therefore,
waiting for the doorbell busy bit to be clear would imply that the
mailbox interface is ready. The original driver implementation used the
doorbell timeout for the Mailbox Interface Ready bit to piggyback off
of, since the latter doesn't have a defined timeout (introduced in
commit 8adaf747c9f0 ("cxl/mem: Find device capabilities"), a timeout has
since been defined with an ECN to the 2.0 spec). With the current driver
waiting for mailbox interface ready as a part of probe() it's no longer
necessary to use the piggyback.

With the piggybacking no longer necessary it doesn't make sense to check
doorbell status when acquiring the mailbox. It will be checked during
the normal mailbox exchange protocol.

While here, make minor whitespace fix in the comment describing the
behavior.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> (v1)
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
Changes since v1:
Reword commit message (Jonathan)
Drop comment block altogether (Dan)
Don't check IF ready in mbox_get() (Dan)
---
 drivers/cxl/pci.c | 36 +-----------------------------------
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b28c220d48ea..ada0d67c8194 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -219,46 +219,12 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds)
 
 	mutex_lock_io(&cxlds->mbox_mutex);
 
-	/*
-	 * XXX: There is some amount of ambiguity in the 2.0 version of the spec
-	 * around the mailbox interface ready (8.2.8.5.1.1).  The purpose of the
-	 * bit is to allow firmware running on the device to notify the driver
-	 * that it's ready to receive commands. It is unclear if the bit needs
-	 * to be read for each transaction mailbox, ie. the firmware can switch
-	 * it on and off as needed. Second, there is no defined timeout for
-	 * mailbox ready, like there is for the doorbell interface.
-	 *
-	 * Assumptions:
-	 * 1. The firmware might toggle the Mailbox Interface Ready bit, check
-	 *    it for every command.
-	 *
-	 * 2. If the doorbell is clear, the firmware should have first set the
-	 *    Mailbox Interface Ready bit. Therefore, waiting for the doorbell
-	 *    to be ready is sufficient.
-	 */
-	rc = cxl_pci_mbox_wait_for_doorbell(cxlds);
-	if (rc) {
-		dev_warn(dev, "Mailbox interface not ready\n");
-		goto out;
-	}
-
-	md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
-	if (!(md_status & CXLMDEV_MBOX_IF_READY && CXLMDEV_READY(md_status))) {
-		dev_err(dev, "mbox: reported doorbell ready, but not mbox ready\n");
-		rc = -EBUSY;
-		goto out;
-	}
-
-	/*
-	 * Hardware shouldn't allow a ready status but also have failure bits
-	 * set. Spit out an error, this should be a bug report
-	 */
 	rc = check_device_status(cxlds);
 	if (rc)
 		goto out;
 
 	if (CXLMDEV_RESET_NEEDED(md_status)) {
-		dev_err(dev, "mbox: reported ready, but reset needed\n");
+		dev_err(dev, "mbox: reset needed\n");
 		rc = -EFAULT;
 		goto out;
 	}
-- 
2.34.1

[PATCH 6/9] cxl/pci: Add new DVSEC definitions

[Ben Widawsky]

In preparation for properly supporting memory active timeout, and later
on, other attributes obtained from DVSEC fields, add the full list of
DVSEC identifiers from the CXL 2.0 specification.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com> (v1)
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---

Changes since v1:
- Reword commit message (Dan)
---
 drivers/cxl/pci.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index 29b8eaef3a0a..8ae2b4adc59d 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -16,6 +16,21 @@
 /* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */
 #define CXL_DVSEC_PCIE_DEVICE					0
 
+/* CXL 2.0 8.1.4: Non-CXL Function Map DVSEC */
+#define CXL_DVSEC_FUNCTION_MAP					2
+
+/* CXL 2.0 8.1.5: CXL 2.0 Extensions DVSEC for Ports */
+#define CXL_DVSEC_PORT_EXTENSIONS				3
+
+/* CXL 2.0 8.1.6: GPF DVSEC for CXL Port */
+#define CXL_DVSEC_PORT_GPF					4
+
+/* CXL 2.0 8.1.7: GPF DVSEC for CXL Device */
+#define CXL_DVSEC_DEVICE_GPF					5
+
+/* CXL 2.0 8.1.8: PCIe DVSEC for Flex Bus Port */
+#define CXL_DVSEC_PCIE_FLEXBUS_PORT				7
+
 /* CXL 2.0 8.1.9: Register Locator DVSEC */
 #define CXL_DVSEC_REG_LOCATOR					8
 #define   CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET			0xC
-- 
2.34.1

[PATCH 7/9] cxl/acpi: Map component registers for Root Ports

[Ben Widawsky]

This implements the TODO in cxl_acpi for mapping component registers.
cxl_acpi becomes the second consumer of CXL register block enumeration
(cxl_pci being the first). Moving the functionality to cxl_core allows
both of these drivers to use the functionality. Equally importantly it
allows cxl_core to use the functionality in the future.

CXL 2.0 root ports are similar to CXL 2.0 Downstream Ports with the main
distinction being they're a part of the CXL 2.0 host bridge. While
mapping their component registers is not immediately useful for the CXL
drivers, the movement of register block enumeration into core is a vital
step towards HDM decoder programming.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
Changes since v1:
- Add comment on why component register enumeration for root ports is
  optional (Jonathan)
- Fix kdoc for cxl_find_regblock (Jonathan)
- Convert cxl_reg_block macro to static inline (Dan)
- Rename cxl_reg_block cxl_reg_block to cxl_regmap_to_base (Dan)
- Make cxl_regmap_to_base return CXL_RESOURCE_NONE on failure (Dan)
---
 drivers/cxl/acpi.c      | 13 ++++++++--
 drivers/cxl/core/regs.c | 54 +++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h       |  4 +++
 drivers/cxl/pci.c       | 52 ---------------------------------------
 drivers/cxl/pci.h       |  9 +++++++
 5 files changed, 78 insertions(+), 54 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 3163167ecc3a..c656a49a11a9 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -7,6 +7,7 @@
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include "cxl.h"
+#include "pci.h"
 
 /* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
 #define CFMWS_INTERLEAVE_WAYS(x)	(1 << (x)->interleave_ways)
@@ -134,11 +135,13 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 
 __mock int match_add_root_ports(struct pci_dev *pdev, void *data)
 {
+	resource_size_t creg = CXL_RESOURCE_NONE;
 	struct cxl_walk_context *ctx = data;
 	struct pci_bus *root_bus = ctx->root;
 	struct cxl_port *port = ctx->port;
 	int type = pci_pcie_type(pdev);
 	struct device *dev = ctx->dev;
+	struct cxl_register_map map;
 	u32 lnkcap, port_num;
 	int rc;
 
@@ -152,9 +155,15 @@ __mock int match_add_root_ports(struct pci_dev *pdev, void *data)
 				  &lnkcap) != PCIBIOS_SUCCESSFUL)
 		return 0;
 
-	/* TODO walk DVSEC to find component register base */
+	/* The driver doesn't rely on component registers for Root Ports yet. */
+	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
+	if (!rc)
+		dev_info(&pdev->dev, "No component register block found\n");
+
+	creg = cxl_regmap_to_base(pdev, &map);
+
 	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
-	rc = cxl_add_dport(port, &pdev->dev, port_num, CXL_RESOURCE_NONE);
+	rc = cxl_add_dport(port, &pdev->dev, port_num, creg);
 	if (rc) {
 		ctx->error = rc;
 		return rc;
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index e37e23bf4355..7a1c4290f0a3 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -5,6 +5,7 @@
 #include <linux/slab.h>
 #include <linux/pci.h>
 #include <cxlmem.h>
+#include <pci.h>
 
 /**
  * DOC: cxl registers
@@ -247,3 +248,56 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_map_device_regs, CXL);
+
+static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
+				struct cxl_register_map *map)
+{
+	map->block_offset = ((u64)reg_hi << 32) |
+			    (reg_lo & CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK);
+	map->barno = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BIR_MASK, reg_lo);
+	map->reg_type = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK, reg_lo);
+}
+
+/**
+ * cxl_find_regblock() - Locate register blocks by type
+ * @pdev: The CXL PCI device to enumerate.
+ * @type: Register Block Indicator id
+ * @map: Enumeration output, clobbered on error
+ *
+ * Return: 0 if register block enumerated, negative error code otherwise
+ *
+ * A CXL DVSEC may point to one or more register blocks, search for them
+ * by @type.
+ */
+int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
+		      struct cxl_register_map *map)
+{
+	u32 regloc_size, regblocks;
+	int regloc, i;
+
+	regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
+					   CXL_DVSEC_REG_LOCATOR);
+	if (!regloc)
+		return -ENXIO;
+
+	pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, &regloc_size);
+	regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);
+
+	regloc += CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET;
+	regblocks = (regloc_size - CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET) / 8;
+
+	for (i = 0; i < regblocks; i++, regloc += 8) {
+		u32 reg_lo, reg_hi;
+
+		pci_read_config_dword(pdev, regloc, &reg_lo);
+		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
+
+		cxl_decode_regblock(reg_lo, reg_hi, map);
+
+		if (map->reg_type == type)
+			return 0;
+	}
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index ab4596f0b751..7150a9694f66 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -145,6 +145,10 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 			struct cxl_device_regs *regs,
 			struct cxl_register_map *map);
 
+enum cxl_regloc_type;
+int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
+		      struct cxl_register_map *map);
+
 #define CXL_RESOURCE_NONE ((resource_size_t) -1)
 #define CXL_TARGET_STRLEN 20
 
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index ada0d67c8194..6aa3dd4b29a1 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -416,58 +416,6 @@ static int cxl_map_regs(struct cxl_dev_state *cxlds, struct cxl_register_map *ma
 	return 0;
 }
 
-static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
-				struct cxl_register_map *map)
-{
-	map->block_offset = ((u64)reg_hi << 32) |
-			    (reg_lo & CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK);
-	map->barno = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BIR_MASK, reg_lo);
-	map->reg_type = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK, reg_lo);
-}
-
-/**
- * cxl_find_regblock() - Locate register blocks by type
- * @pdev: The CXL PCI device to enumerate.
- * @type: Register Block Indicator id
- * @map: Enumeration output, clobbered on error
- *
- * Return: 0 if register block enumerated, negative error code otherwise
- *
- * A CXL DVSEC may point to one or more register blocks, search for them
- * by @type.
- */
-static int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
-			     struct cxl_register_map *map)
-{
-	u32 regloc_size, regblocks;
-	int regloc, i;
-
-	regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
-					   CXL_DVSEC_REG_LOCATOR);
-	if (!regloc)
-		return -ENXIO;
-
-	pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, &regloc_size);
-	regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);
-
-	regloc += CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET;
-	regblocks = (regloc_size - CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET) / 8;
-
-	for (i = 0; i < regblocks; i++, regloc += 8) {
-		u32 reg_lo, reg_hi;
-
-		pci_read_config_dword(pdev, regloc, &reg_lo);
-		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
-
-		cxl_decode_regblock(reg_lo, reg_hi, map);
-
-		if (map->reg_type == type)
-			return 0;
-	}
-
-	return -ENODEV;
-}
-
 static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
 			  struct cxl_register_map *map)
 {
diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index 8ae2b4adc59d..f418272dbe38 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -47,4 +47,13 @@ enum cxl_regloc_type {
 	CXL_REGLOC_RBI_TYPES
 };
 
+static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
+						 struct cxl_register_map *map)
+{
+	if (!map)
+		return CXL_RESOURCE_NONE;
+
+	return pci_resource_start(pdev, map->barno) + map->block_offset;
+}
+
 #endif /* __CXL_PCI_H__ */
-- 
2.34.1

Re: [PATCH 7/9] cxl/acpi: Map component registers for Root Ports

[Jonathan Cameron]

On Mon, 29 Nov 2021 13:47:19 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> This implements the TODO in cxl_acpi for mapping component registers.
> cxl_acpi becomes the second consumer of CXL register block enumeration
> (cxl_pci being the first). Moving the functionality to cxl_core allows
> both of these drivers to use the functionality. Equally importantly it
> allows cxl_core to use the functionality in the future.
> 
> CXL 2.0 root ports are similar to CXL 2.0 Downstream Ports with the main
> distinction being they're a part of the CXL 2.0 host bridge. While
> mapping their component registers is not immediately useful for the CXL
> drivers, the movement of register block enumeration into core is a vital
> step towards HDM decoder programming.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 7/9] cxl/acpi: Map component registers for Root Ports

[Dan Williams]

On Mon, Nov 29, 2021 at 1:47 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> This implements the TODO in cxl_acpi for mapping component registers.
> cxl_acpi becomes the second consumer of CXL register block enumeration
> (cxl_pci being the first). Moving the functionality to cxl_core allows
> both of these drivers to use the functionality. Equally importantly it
> allows cxl_core to use the functionality in the future.
>
> CXL 2.0 root ports are similar to CXL 2.0 Downstream Ports with the main
> distinction being they're a part of the CXL 2.0 host bridge. While
> mapping their component registers is not immediately useful for the CXL
> drivers, the movement of register block enumeration into core is a vital
> step towards HDM decoder programming.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
> Changes since v1:
> - Add comment on why component register enumeration for root ports is
>   optional (Jonathan)
> - Fix kdoc for cxl_find_regblock (Jonathan)
> - Convert cxl_reg_block macro to static inline (Dan)
> - Rename cxl_reg_block cxl_reg_block to cxl_regmap_to_base (Dan)
> - Make cxl_regmap_to_base return CXL_RESOURCE_NONE on failure (Dan)

Hmm, it's not doing that...

> ---
>  drivers/cxl/acpi.c      | 13 ++++++++--
>  drivers/cxl/core/regs.c | 54 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h       |  4 +++
>  drivers/cxl/pci.c       | 52 ---------------------------------------
>  drivers/cxl/pci.h       |  9 +++++++
>  5 files changed, 78 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 3163167ecc3a..c656a49a11a9 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -7,6 +7,7 @@
>  #include <linux/acpi.h>
>  #include <linux/pci.h>
>  #include "cxl.h"
> +#include "pci.h"
>
>  /* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
>  #define CFMWS_INTERLEAVE_WAYS(x)       (1 << (x)->interleave_ways)
> @@ -134,11 +135,13 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
>
>  __mock int match_add_root_ports(struct pci_dev *pdev, void *data)
>  {
> +       resource_size_t creg = CXL_RESOURCE_NONE;
>         struct cxl_walk_context *ctx = data;
>         struct pci_bus *root_bus = ctx->root;
>         struct cxl_port *port = ctx->port;
>         int type = pci_pcie_type(pdev);
>         struct device *dev = ctx->dev;
> +       struct cxl_register_map map;
>         u32 lnkcap, port_num;
>         int rc;
>
> @@ -152,9 +155,15 @@ __mock int match_add_root_ports(struct pci_dev *pdev, void *data)
>                                   &lnkcap) != PCIBIOS_SUCCESSFUL)
>                 return 0;
>
> -       /* TODO walk DVSEC to find component register base */
> +       /* The driver doesn't rely on component registers for Root Ports yet. */
> +       rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> +       if (!rc)
> +               dev_info(&pdev->dev, "No component register block found\n");
> +
> +       creg = cxl_regmap_to_base(pdev, &map);
> +
>         port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
> -       rc = cxl_add_dport(port, &pdev->dev, port_num, CXL_RESOURCE_NONE);
> +       rc = cxl_add_dport(port, &pdev->dev, port_num, creg);
>         if (rc) {
>                 ctx->error = rc;
>                 return rc;
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index e37e23bf4355..7a1c4290f0a3 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -5,6 +5,7 @@
>  #include <linux/slab.h>
>  #include <linux/pci.h>
>  #include <cxlmem.h>
> +#include <pci.h>
>
>  /**
>   * DOC: cxl registers
> @@ -247,3 +248,56 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>         return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_map_device_regs, CXL);
> +
> +static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
> +                               struct cxl_register_map *map)
> +{
> +       map->block_offset = ((u64)reg_hi << 32) |
> +                           (reg_lo & CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK);
> +       map->barno = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BIR_MASK, reg_lo);
> +       map->reg_type = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK, reg_lo);
> +}
> +
> +/**
> + * cxl_find_regblock() - Locate register blocks by type
> + * @pdev: The CXL PCI device to enumerate.
> + * @type: Register Block Indicator id
> + * @map: Enumeration output, clobbered on error
> + *
> + * Return: 0 if register block enumerated, negative error code otherwise
> + *
> + * A CXL DVSEC may point to one or more register blocks, search for them
> + * by @type.
> + */
> +int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> +                     struct cxl_register_map *map)
> +{
> +       u32 regloc_size, regblocks;
> +       int regloc, i;
> +
> +       regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> +                                          CXL_DVSEC_REG_LOCATOR);
> +       if (!regloc)
> +               return -ENXIO;
> +
> +       pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, &regloc_size);
> +       regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);
> +
> +       regloc += CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET;
> +       regblocks = (regloc_size - CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET) / 8;
> +
> +       for (i = 0; i < regblocks; i++, regloc += 8) {
> +               u32 reg_lo, reg_hi;
> +
> +               pci_read_config_dword(pdev, regloc, &reg_lo);
> +               pci_read_config_dword(pdev, regloc + 4, &reg_hi);
> +
> +               cxl_decode_regblock(reg_lo, reg_hi, map);
> +
> +               if (map->reg_type == type)
> +                       return 0;
> +       }
> +
> +       return -ENODEV;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index ab4596f0b751..7150a9694f66 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -145,6 +145,10 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>                         struct cxl_device_regs *regs,
>                         struct cxl_register_map *map);
>
> +enum cxl_regloc_type;
> +int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> +                     struct cxl_register_map *map);
> +
>  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>  #define CXL_TARGET_STRLEN 20
>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index ada0d67c8194..6aa3dd4b29a1 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -416,58 +416,6 @@ static int cxl_map_regs(struct cxl_dev_state *cxlds, struct cxl_register_map *ma
>         return 0;
>  }
>
> -static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
> -                               struct cxl_register_map *map)
> -{
> -       map->block_offset = ((u64)reg_hi << 32) |
> -                           (reg_lo & CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK);
> -       map->barno = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BIR_MASK, reg_lo);
> -       map->reg_type = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK, reg_lo);
> -}
> -
> -/**
> - * cxl_find_regblock() - Locate register blocks by type
> - * @pdev: The CXL PCI device to enumerate.
> - * @type: Register Block Indicator id
> - * @map: Enumeration output, clobbered on error
> - *
> - * Return: 0 if register block enumerated, negative error code otherwise
> - *
> - * A CXL DVSEC may point to one or more register blocks, search for them
> - * by @type.
> - */
> -static int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> -                            struct cxl_register_map *map)
> -{
> -       u32 regloc_size, regblocks;
> -       int regloc, i;
> -
> -       regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> -                                          CXL_DVSEC_REG_LOCATOR);
> -       if (!regloc)
> -               return -ENXIO;
> -
> -       pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, &regloc_size);
> -       regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);
> -
> -       regloc += CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET;
> -       regblocks = (regloc_size - CXL_DVSEC_REG_LOCATOR_BLOCK1_OFFSET) / 8;
> -
> -       for (i = 0; i < regblocks; i++, regloc += 8) {
> -               u32 reg_lo, reg_hi;
> -
> -               pci_read_config_dword(pdev, regloc, &reg_lo);
> -               pci_read_config_dword(pdev, regloc + 4, &reg_hi);
> -
> -               cxl_decode_regblock(reg_lo, reg_hi, map);
> -
> -               if (map->reg_type == type)
> -                       return 0;
> -       }
> -
> -       return -ENODEV;
> -}
> -
>  static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>                           struct cxl_register_map *map)
>  {
> diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
> index 8ae2b4adc59d..f418272dbe38 100644
> --- a/drivers/cxl/pci.h
> +++ b/drivers/cxl/pci.h
> @@ -47,4 +47,13 @@ enum cxl_regloc_type {
>         CXL_REGLOC_RBI_TYPES
>  };
>
> +static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
> +                                                struct cxl_register_map *map)
> +{
> +       if (!map)
> +               return CXL_RESOURCE_NONE;

All the callers are passing in a non-NULL map. I was more thinking on
cxl_find_regblock() failure. It sets a signature in the map so that
cxl_regmap_to_base() returns CXL_RESOURCE_NONE.

Something like:

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 271e0ca5c8d9..20449237ec36 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -275,6 +275,7 @@ int cxl_find_regblock(struct pci_dev *pdev, enum
cxl_regloc_type type,
        u32 regloc_size, regblocks;
        int regloc, i;

+       map->block_offset = U64_MAX;
        regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
                                           CXL_DVSEC_REG_LOCATOR);
        if (!regloc)
@@ -298,6 +299,7 @@ int cxl_find_regblock(struct pci_dev *pdev, enum
cxl_regloc_type type,
                        return 0;
        }

+       map->block_offset = U64_MAX;
        return -ENODEV;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index bf527399a5de..3f3a1121eb93 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -63,7 +63,7 @@ enum cxl_regloc_type {
 static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
                                                 struct cxl_register_map *map)
 {
-       if (!map)
+       if (map->block_offset == U64_MAX)
                return CXL_RESOURCE_NONE;

        return pci_resource_start(pdev, map->barno) + map->block_offset;

Re: [PATCH 7/9] cxl/acpi: Map component registers for Root Ports

[Ben Widawsky]

On 21-12-03 20:37:26, Dan Williams wrote:
> On Mon, Nov 29, 2021 at 1:47 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > This implements the TODO in cxl_acpi for mapping component registers.
> > cxl_acpi becomes the second consumer of CXL register block enumeration
> > (cxl_pci being the first). Moving the functionality to cxl_core allows
> > both of these drivers to use the functionality. Equally importantly it
> > allows cxl_core to use the functionality in the future.
> >
> > CXL 2.0 root ports are similar to CXL 2.0 Downstream Ports with the main
> > distinction being they're a part of the CXL 2.0 host bridge. While
> > mapping their component registers is not immediately useful for the CXL
> > drivers, the movement of register block enumeration into core is a vital
> > step towards HDM decoder programming.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> > Changes since v1:
> > - Add comment on why component register enumeration for root ports is
> >   optional (Jonathan)
> > - Fix kdoc for cxl_find_regblock (Jonathan)
> > - Convert cxl_reg_block macro to static inline (Dan)
> > - Rename cxl_reg_block cxl_reg_block to cxl_regmap_to_base (Dan)
> > - Make cxl_regmap_to_base return CXL_RESOURCE_NONE on failure (Dan)
> 
> Hmm, it's not doing that...
> 

Oops. I'll fix that. I suppose the best check is to check @base? I don't see any
other obvious way.

Re: [PATCH 7/9] cxl/acpi: Map component registers for Root Ports

[Jonathan Cameron]

On Mon, 29 Nov 2021 13:47:19 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> This implements the TODO in cxl_acpi for mapping component registers.
> cxl_acpi becomes the second consumer of CXL register block enumeration
> (cxl_pci being the first). Moving the functionality to cxl_core allows
> both of these drivers to use the functionality. Equally importantly it
> allows cxl_core to use the functionality in the future.
> 
> CXL 2.0 root ports are similar to CXL 2.0 Downstream Ports with the main
> distinction being they're a part of the CXL 2.0 host bridge. While
> mapping their component registers is not immediately useful for the CXL
> drivers, the movement of register block enumeration into core is a vital
> step towards HDM decoder programming.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
> Changes since v1:
> - Add comment on why component register enumeration for root ports is
>   optional (Jonathan)
> - Fix kdoc for cxl_find_regblock (Jonathan)
> - Convert cxl_reg_block macro to static inline (Dan)
> - Rename cxl_reg_block cxl_reg_block to cxl_regmap_to_base (Dan)
> - Make cxl_regmap_to_base return CXL_RESOURCE_NONE on failure (Dan)
> ---
>  drivers/cxl/acpi.c      | 13 ++++++++--
>  drivers/cxl/core/regs.c | 54 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h       |  4 +++
>  drivers/cxl/pci.c       | 52 ---------------------------------------
>  drivers/cxl/pci.h       |  9 +++++++
>  5 files changed, 78 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 3163167ecc3a..c656a49a11a9 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -7,6 +7,7 @@
>  #include <linux/acpi.h>
>  #include <linux/pci.h>
>  #include "cxl.h"
> +#include "pci.h"
>  
>  /* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
>  #define CFMWS_INTERLEAVE_WAYS(x)	(1 << (x)->interleave_ways)
> @@ -134,11 +135,13 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
>  
>  __mock int match_add_root_ports(struct pci_dev *pdev, void *data)
>  {
> +	resource_size_t creg = CXL_RESOURCE_NONE;
>  	struct cxl_walk_context *ctx = data;
>  	struct pci_bus *root_bus = ctx->root;
>  	struct cxl_port *port = ctx->port;
>  	int type = pci_pcie_type(pdev);
>  	struct device *dev = ctx->dev;
> +	struct cxl_register_map map;
>  	u32 lnkcap, port_num;
>  	int rc;
>  
> @@ -152,9 +155,15 @@ __mock int match_add_root_ports(struct pci_dev *pdev, void *data)
>  				  &lnkcap) != PCIBIOS_SUCCESSFUL)
>  		return 0;
>  
> -	/* TODO walk DVSEC to find component register base */
> +	/* The driver doesn't rely on component registers for Root Ports yet. */
> +	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> +	if (!rc)
Check inverted.  cxl_find_regblock() returns 0 if it succeeded.
(or I've managed to miss a precursor patch).

> +		dev_info(&pdev->dev, "No component register block found\n");

[PATCH 8/9] cxl: Introduce module_cxl_driver

[Ben Widawsky]

Many CXL drivers simply want to register and unregister themselves.
module_driver already supported this. A simple wrapper around that
reduces a decent amount of boilerplate in upcoming patches.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/cxl.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 7150a9694f66..d39d45f4a770 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -308,6 +308,9 @@ int __cxl_driver_register(struct cxl_driver *cxl_drv, struct module *owner,
 #define cxl_driver_register(x) __cxl_driver_register(x, THIS_MODULE, KBUILD_MODNAME)
 void cxl_driver_unregister(struct cxl_driver *cxl_drv);
 
+#define module_cxl_driver(__cxl_driver) \
+	module_driver(__cxl_driver, cxl_driver_register, cxl_driver_unregister)
+
 #define CXL_DEVICE_NVDIMM_BRIDGE	1
 #define CXL_DEVICE_NVDIMM		2
 
-- 
2.34.1

Re: [PATCH 8/9] cxl: Introduce module_cxl_driver

[Jonathan Cameron]

On Mon, 29 Nov 2021 13:47:20 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Many CXL drivers simply want to register and unregister themselves.
> module_driver already supported this. A simple wrapper around that
> reduces a decent amount of boilerplate in upcoming patches.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
FWIW this is obviously useful.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/cxl.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 7150a9694f66..d39d45f4a770 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -308,6 +308,9 @@ int __cxl_driver_register(struct cxl_driver *cxl_drv, struct module *owner,
>  #define cxl_driver_register(x) __cxl_driver_register(x, THIS_MODULE, KBUILD_MODNAME)
>  void cxl_driver_unregister(struct cxl_driver *cxl_drv);
>  
> +#define module_cxl_driver(__cxl_driver) \
> +	module_driver(__cxl_driver, cxl_driver_register, cxl_driver_unregister)
> +
>  #define CXL_DEVICE_NVDIMM_BRIDGE	1
>  #define CXL_DEVICE_NVDIMM		2
>  

[PATCH 9/9] cxl/core: Convert decoder range to resource

[Ben Widawsky]

CXL decoders manage address ranges in a hierarchical fashion whereby a
leaf is a unique subregion of its parent decoder (midlevel or root). It
therefore makes sense to use the resource API for handling this.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> (v1)
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
Changes since v1:
- Use %pr for resource formatting (Dan)
- Drop unnecessary zero initialization (Dan)
- Add comment about root decoder resource naming (Dan)
---
 drivers/cxl/acpi.c     | 22 ++++++++--------------
 drivers/cxl/core/bus.c | 23 +++++++++++++++++++++--
 drivers/cxl/cxl.h      |  8 ++++++--
 3 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index c656a49a11a9..da70f1836db6 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -108,10 +108,8 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 
 	cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions);
 	cxld->target_type = CXL_DECODER_EXPANDER;
-	cxld->range = (struct range){
-		.start = cfmws->base_hpa,
-		.end = cfmws->base_hpa + cfmws->window_size - 1,
-	};
+	cxld->platform_res = (struct resource)DEFINE_RES_MEM(cfmws->base_hpa,
+							     cfmws->window_size);
 	cxld->interleave_ways = CFMWS_INTERLEAVE_WAYS(cfmws);
 	cxld->interleave_granularity = CFMWS_INTERLEAVE_GRANULARITY(cfmws);
 
@@ -121,14 +119,13 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 	else
 		rc = cxl_decoder_autoremove(dev, cxld);
 	if (rc) {
-		dev_err(dev, "Failed to add decoder for %#llx-%#llx\n",
-			cfmws->base_hpa,
-			cfmws->base_hpa + cfmws->window_size - 1);
+		dev_err(dev, "Failed to add decoder for %pr\n",
+			&cxld->platform_res);
 		return 0;
 	}
-	dev_dbg(dev, "add: %s node: %d range %#llx-%#llx\n",
-		dev_name(&cxld->dev), phys_to_target_node(cxld->range.start),
-		cfmws->base_hpa, cfmws->base_hpa + cfmws->window_size - 1);
+	dev_dbg(dev, "add: %s node: %d range %pr\n", dev_name(&cxld->dev),
+		phys_to_target_node(cxld->platform_res.start),
+		&cxld->platform_res);
 
 	return 0;
 }
@@ -270,10 +267,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 	cxld->interleave_ways = 1;
 	cxld->interleave_granularity = PAGE_SIZE;
 	cxld->target_type = CXL_DECODER_EXPANDER;
-	cxld->range = (struct range) {
-		.start = 0,
-		.end = -1,
-	};
+	cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
 
 	device_lock(&port->dev);
 	dport = list_first_entry(&port->dports, typeof(*dport), list);
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
index 17a4fff029f8..ab756a53a983 100644
--- a/drivers/cxl/core/bus.c
+++ b/drivers/cxl/core/bus.c
@@ -46,8 +46,14 @@ static ssize_t start_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	u64 start;
 
-	return sysfs_emit(buf, "%#llx\n", cxld->range.start);
+	if (is_root_decoder(dev))
+		start = cxld->platform_res.start;
+	else
+		start = cxld->decoder_range.start;
+
+	return sysfs_emit(buf, "%#llx\n", start);
 }
 static DEVICE_ATTR_RO(start);
 
@@ -55,8 +61,14 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	u64 size;
 
-	return sysfs_emit(buf, "%#llx\n", range_len(&cxld->range));
+	if (is_root_decoder(dev))
+		size = resource_size(&cxld->platform_res);
+	else
+		size = range_len(&cxld->decoder_range);
+
+	return sysfs_emit(buf, "%#llx\n", size);
 }
 static DEVICE_ATTR_RO(size);
 
@@ -548,6 +560,13 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
 	if (rc)
 		return rc;
 
+	/*
+	 * Platform decoder resources should show up with a reasonable name. All
+	 * other resources are just sub ranges within the main decoder resource.
+	 */
+	if (is_root_decoder(dev))
+		cxld->platform_res.name = dev_name(dev);
+
 	return device_add(dev);
 }
 EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, CXL);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index d39d45f4a770..ad816fb5bdcc 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -179,7 +179,8 @@ enum cxl_decoder_type {
  * struct cxl_decoder - CXL address range decode configuration
  * @dev: this decoder's device
  * @id: kernel device name id
- * @range: address range considered by this decoder
+ * @platform_res: address space resources considered by root decoder
+ * @decoder_range: address space resources considered by midlevel decoder
  * @interleave_ways: number of cxl_dports in this decode
  * @interleave_granularity: data stride per dport
  * @target_type: accelerator vs expander (type2 vs type3) selector
@@ -190,7 +191,10 @@ enum cxl_decoder_type {
 struct cxl_decoder {
 	struct device dev;
 	int id;
-	struct range range;
+	union {
+		struct resource platform_res;
+		struct range decoder_range;
+	};
 	int interleave_ways;
 	int interleave_granularity;
 	enum cxl_decoder_type target_type;
-- 
2.34.1