[PATCH 0/8] cxl/pci: Add fundamental error handling

[PATCH 0/8] cxl/pci: Add fundamental error handling

[Dan Williams]

Add a 'struct pci_error_handlers' instance for the cxl_pci driver.
Section 8.2.5.9 "CXL RAS Capability Structure" of the CXL 2.0
specification defines the error sources considered in this
implementation. The RAS Capability Structure defines protocol, link and
internal errors which are distinct from memory poison errors that are
conveyed via direct consumption and/or media scanning.

The errors reported by the RAS registers are categorized into
correctable and uncorrectable errors, where the uncorrectable errors are
optionally steered to either fatal or non-fatal AER events. Table 224
"Device Specific Error Reporting and Nomenclature Guidelines" in the CXL
2.0 specification outlines that the remediation for uncorrectable errors
is a reset to recover. This matches how the Linux PCIe AER core treats
uncorrectable errors as occasions to reset the device to recover
operation.

While the specification notes "CXL Reset" or "Secondary Bus Reset" as
theoretical recovery options, they are not feasible in practice since
in-flight CXL.mem operations may not terminate and cause knock-on system
fatal events. Reset is only reliable for recovering CXL.io, it is not
reliable for recovering CXL.mem. Assuming the system survives, a reset
causes CXL.mem operation to restart from scratch.

The "ECN: Error Isolation on CXL.mem and CXL.cache" [1] document
recognizes the CXL Reset vs CXL.mem operational conflict and helps to at
least provide a mechanism for the Root Port to terminate in flight
CXL.mem operations with completions. That still poses problems in
practice if the kernel is running out of "System RAM" backed by the CXL
device and poison is used to convey the data lost to the protocol error.

Regardless of whether the reset and restart of CXL.mem operations is
feasible / successful, the logging is still useful. So, the
implementation reads, reports, and clears the status in the RAS
Capability Structure registers, and it notifies the 'struct cxl_memdev'
associated with the given PCIe endpoint to reattach to its driver over
the reset so that the HDM decoder configuration can be reconstructed.

The first half of the series reworks component register mapping so that
the cxl_pci driver can own the RAS Capability while the cxl_port driver
continues to own the HDM Decoder Capability. The last half implements
the RAS Capability Structure mapping and reporting via 'struct
pci_error_handlers'.

[1]: https://www.computeexpresslink.org/spec-landing

---


Dan Williams (8):
      cxl/pci: Cleanup repeated code in cxl_probe_regs() helpers
      cxl/pci: Cleanup cxl_map_device_regs()
      cxl/pci: Kill cxl_map_regs()
      cxl/core/regs: Make cxl_map_{component,device}_regs() device generic
      cxl/port: Limit the port driver to just the HDM Decoder Capability
      cxl/pci: Prepare for mapping RAS Capability Structure
      cxl/pci: Find and map the RAS Capability Structure
      cxl/pci: Add (hopeful) error handling support


 drivers/cxl/core/hdm.c    |   33 +++++----
 drivers/cxl/core/memdev.c |    1 
 drivers/cxl/core/pci.c    |    3 -
 drivers/cxl/core/port.c   |    2 -
 drivers/cxl/core/regs.c   |  172 ++++++++++++++++++++++++++-------------------
 drivers/cxl/cxl.h         |   36 +++++++--
 drivers/cxl/cxlmem.h      |    2 +
 drivers/cxl/cxlpci.h      |    9 --
 drivers/cxl/pci.c         |  163 ++++++++++++++++++++++++++++++++-----------
 9 files changed, 273 insertions(+), 148 deletions(-)

base-commit: 74be98774dfbc5b8b795db726bd772e735d2edd4

[PATCH 1/8] cxl/pci: Cleanup repeated code in cxl_probe_regs() helpers

[Dan Williams]

Rather then duplicating the setting of valid, length, and offset for
each type, just convey a pointer to the register map to common code.

Yes, the equivalent change in cxl_probe_component_regs() does not save
any lines of code, but it is preparation for adding another component
register type to map (RAS Capability Strucutre).

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/regs.c |   46 ++++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 39a129c57d40..bd6ae14b679e 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -59,36 +59,41 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
 
 	for (cap = 1; cap <= cap_count; cap++) {
 		void __iomem *register_block;
-		u32 hdr;
-		int decoder_cnt;
+		struct cxl_reg_map *rmap;
 		u16 cap_id, offset;
-		u32 length;
+		u32 length, hdr;
 
 		hdr = readl(base + cap * 0x4);
 
 		cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr);
 		offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr);
 		register_block = base + offset;
+		hdr = readl(register_block);
 
+		rmap = NULL;
 		switch (cap_id) {
-		case CXL_CM_CAP_CAP_ID_HDM:
+		case CXL_CM_CAP_CAP_ID_HDM: {
+			int decoder_cnt;
+
 			dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
 				offset);
 
-			hdr = readl(register_block);
-
 			decoder_cnt = cxl_hdm_decoder_count(hdr);
 			length = 0x20 * decoder_cnt + 0x10;
-
-			map->hdm_decoder.valid = true;
-			map->hdm_decoder.offset = CXL_CM_OFFSET + offset;
-			map->hdm_decoder.size = length;
+			rmap = &map->hdm_decoder;
 			break;
+		}
 		default:
 			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
 				offset);
 			break;
 		}
+
+		if (!rmap)
+			continue;
+		rmap->valid = true;
+		rmap->offset = CXL_CM_OFFSET + offset;
+		rmap->size = length;
 	}
 }
 EXPORT_SYMBOL_NS_GPL(cxl_probe_component_regs, CXL);
@@ -117,6 +122,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 	cap_count = FIELD_GET(CXLDEV_CAP_ARRAY_COUNT_MASK, cap_array);
 
 	for (cap = 1; cap <= cap_count; cap++) {
+		struct cxl_reg_map *rmap;
 		u32 offset, length;
 		u16 cap_id;
 
@@ -125,28 +131,22 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 		offset = readl(base + cap * 0x10 + 0x4);
 		length = readl(base + cap * 0x10 + 0x8);
 
+		rmap = NULL;
 		switch (cap_id) {
 		case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
 			dev_dbg(dev, "found Status capability (0x%x)\n", offset);
-
-			map->status.valid = true;
-			map->status.offset = offset;
-			map->status.size = length;
+			rmap = &map->status;
 			break;
 		case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
 			dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
-			map->mbox.valid = true;
-			map->mbox.offset = offset;
-			map->mbox.size = length;
+			rmap = &map->mbox;
 			break;
 		case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
 			dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
 			break;
 		case CXLDEV_CAP_CAP_ID_MEMDEV:
 			dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
-			map->memdev.valid = true;
-			map->memdev.offset = offset;
-			map->memdev.size = length;
+			rmap = &map->memdev;
 			break;
 		default:
 			if (cap_id >= 0x8000)
@@ -155,6 +155,12 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 				dev_dbg(dev, "Unknown cap ID: %#x offset: %#x\n", cap_id, offset);
 			break;
 		}
+
+		if (!rmap)
+			continue;
+		rmap->valid = true;
+		rmap->offset = offset;
+		rmap->size = length;
 	}
 }
 EXPORT_SYMBOL_NS_GPL(cxl_probe_device_regs, CXL);

Re: [PATCH 1/8] cxl/pci: Cleanup repeated code in cxl_probe_regs() helpers

[Jonathan Cameron]

On Tue, 15 Mar 2022 21:13:47 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Rather then duplicating the setting of valid, length, and offset for

than

> each type, just convey a pointer to the register map to common code.
> 
> Yes, the equivalent change in cxl_probe_component_regs() does not save

Why "equivalent"?  I'd just drop that word as not clear what it's equivalent to
and sentence makes sense without it.

> any lines of code, but it is preparation for adding another component
> register type to map (RAS Capability Strucutre).

Structure

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

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

> ---
>  drivers/cxl/core/regs.c |   46 ++++++++++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 39a129c57d40..bd6ae14b679e 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -59,36 +59,41 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  
>  	for (cap = 1; cap <= cap_count; cap++) {
>  		void __iomem *register_block;
> -		u32 hdr;
> -		int decoder_cnt;
> +		struct cxl_reg_map *rmap;
>  		u16 cap_id, offset;
> -		u32 length;
> +		u32 length, hdr;

Unrelated change, but meh, it makes sense and doesn't effect readability of overall
patch much.

>  
>  		hdr = readl(base + cap * 0x4);
>  
>  		cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr);
>  		offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr);
>  		register_block = base + offset;
> +		hdr = readl(register_block);
>  
> +		rmap = NULL;
>  		switch (cap_id) {
> -		case CXL_CM_CAP_CAP_ID_HDM:
> +		case CXL_CM_CAP_CAP_ID_HDM: {
> +			int decoder_cnt;
> +
>  			dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
>  				offset);
>  
> -			hdr = readl(register_block);
> -
>  			decoder_cnt = cxl_hdm_decoder_count(hdr);
>  			length = 0x20 * decoder_cnt + 0x10;
> -
> -			map->hdm_decoder.valid = true;
> -			map->hdm_decoder.offset = CXL_CM_OFFSET + offset;
> -			map->hdm_decoder.size = length;
> +			rmap = &map->hdm_decoder;
>  			break;
> +		}
>  		default:
>  			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
>  				offset);
>  			break;
>  		}
> +
> +		if (!rmap)
> +			continue;
> +		rmap->valid = true;
> +		rmap->offset = CXL_CM_OFFSET + offset;
> +		rmap->size = length;
>  	}
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_probe_component_regs, CXL);
> @@ -117,6 +122,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  	cap_count = FIELD_GET(CXLDEV_CAP_ARRAY_COUNT_MASK, cap_array);
>  
>  	for (cap = 1; cap <= cap_count; cap++) {
> +		struct cxl_reg_map *rmap;
>  		u32 offset, length;
>  		u16 cap_id;
>  
> @@ -125,28 +131,22 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  		offset = readl(base + cap * 0x10 + 0x4);
>  		length = readl(base + cap * 0x10 + 0x8);
>  
> +		rmap = NULL;
>  		switch (cap_id) {
>  		case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
>  			dev_dbg(dev, "found Status capability (0x%x)\n", offset);
> -
> -			map->status.valid = true;
> -			map->status.offset = offset;
> -			map->status.size = length;
> +			rmap = &map->status;
>  			break;
>  		case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
>  			dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
> -			map->mbox.valid = true;
> -			map->mbox.offset = offset;
> -			map->mbox.size = length;
> +			rmap = &map->mbox;
>  			break;
>  		case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
>  			dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
>  			break;
>  		case CXLDEV_CAP_CAP_ID_MEMDEV:
>  			dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
> -			map->memdev.valid = true;
> -			map->memdev.offset = offset;
> -			map->memdev.size = length;
> +			rmap = &map->memdev;
>  			break;
>  		default:
>  			if (cap_id >= 0x8000)
> @@ -155,6 +155,12 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  				dev_dbg(dev, "Unknown cap ID: %#x offset: %#x\n", cap_id, offset);
>  			break;
>  		}
> +
> +		if (!rmap)
> +			continue;
> +		rmap->valid = true;
> +		rmap->offset = offset;
> +		rmap->size = length;
>  	}
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_probe_device_regs, CXL);
> 

[PATCH 2/8] cxl/pci: Cleanup cxl_map_device_regs()

[Dan Williams]

Use a loop to reduce the duplicated code in cxl_map_device_regs(). This
is in preparation for deleting cxl_map_regs().

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/regs.c |   51 ++++++++++++++++++-----------------------------
 1 file changed, 20 insertions(+), 31 deletions(-)

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index bd6ae14b679e..bd766e461f7d 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -211,42 +211,31 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 			struct cxl_device_regs *regs,
 			struct cxl_register_map *map)
 {
+	resource_size_t phys_addr =
+		pci_resource_start(pdev, map->barno) + map->block_offset;
 	struct device *dev = &pdev->dev;
-	resource_size_t phys_addr;
-
-	phys_addr = pci_resource_start(pdev, map->barno);
-	phys_addr += map->block_offset;
-
-	if (map->device_map.status.valid) {
-		resource_size_t addr;
+	struct mapinfo {
+		struct cxl_reg_map *rmap;
+		void __iomem **addr;
+	} mapinfo[] = {
+		{ .rmap = &map->device_map.status, &regs->status, },
+		{ .rmap = &map->device_map.mbox, &regs->mbox, },
+		{ .rmap = &map->device_map.memdev, &regs->memdev, },
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(mapinfo); i++) {
+		struct mapinfo *mi = &mapinfo[i];
 		resource_size_t length;
-
-		addr = phys_addr + map->device_map.status.offset;
-		length = map->device_map.status.size;
-		regs->status = devm_cxl_iomap_block(dev, addr, length);
-		if (!regs->status)
-			return -ENOMEM;
-	}
-
-	if (map->device_map.mbox.valid) {
 		resource_size_t addr;
-		resource_size_t length;
 
-		addr = phys_addr + map->device_map.mbox.offset;
-		length = map->device_map.mbox.size;
-		regs->mbox = devm_cxl_iomap_block(dev, addr, length);
-		if (!regs->mbox)
-			return -ENOMEM;
-	}
-
-	if (map->device_map.memdev.valid) {
-		resource_size_t addr;
-		resource_size_t length;
+		if (!mi->rmap->valid)
+			continue;
 
-		addr = phys_addr + map->device_map.memdev.offset;
-		length = map->device_map.memdev.size;
-		regs->memdev = devm_cxl_iomap_block(dev, addr, length);
-		if (!regs->memdev)
+		addr = phys_addr + mi->rmap->offset;
+		length = mi->rmap->size;
+		*(mi->addr) = devm_cxl_iomap_block(dev, addr, length);
+		if (!*(mi->addr))
 			return -ENOMEM;
 	}
 

Re: [PATCH 2/8] cxl/pci: Cleanup cxl_map_device_regs()

[Jonathan Cameron]

On Tue, 15 Mar 2022 21:13:52 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Use a loop to reduce the duplicated code in cxl_map_device_regs(). This
> is in preparation for deleting cxl_map_regs().
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Trivial style comments inline.  Otherwise LGTM

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

> ---
>  drivers/cxl/core/regs.c |   51 ++++++++++++++++++-----------------------------
>  1 file changed, 20 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index bd6ae14b679e..bd766e461f7d 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -211,42 +211,31 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map)
>  {
> +	resource_size_t phys_addr =
> +		pci_resource_start(pdev, map->barno) + map->block_offset;

I'm not totally convinced by this refactoring as it's ugly either
way...  Still your code, and I don't care that strongly ;)

>  	struct device *dev = &pdev->dev;
> -	resource_size_t phys_addr;
> -
> -	phys_addr = pci_resource_start(pdev, map->barno);
> -	phys_addr += map->block_offset;
> -
> -	if (map->device_map.status.valid) {
> -		resource_size_t addr;
> +	struct mapinfo {
> +		struct cxl_reg_map *rmap;
> +		void __iomem **addr;
> +	} mapinfo[] = {
> +		{ .rmap = &map->device_map.status, &regs->status, },

Combining c99 style .rmap for first parameter and then not doing it
for the second is a bit odd looking.  Was there a strong reason for
doing this?  I'd just drop the ".rmap =" as it's not as though
we need to look far to see what it's setting.

> +		{ .rmap = &map->device_map.mbox, &regs->mbox, },
> +		{ .rmap = &map->device_map.memdev, &regs->memdev, },
> +	};
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mapinfo); i++) {
> +		struct mapinfo *mi = &mapinfo[i];
>  		resource_size_t length;
> -
> -		addr = phys_addr + map->device_map.status.offset;
> -		length = map->device_map.status.size;
> -		regs->status = devm_cxl_iomap_block(dev, addr, length);
> -		if (!regs->status)
> -			return -ENOMEM;
> -	}
> -
> -	if (map->device_map.mbox.valid) {
>  		resource_size_t addr;
> -		resource_size_t length;
>  
> -		addr = phys_addr + map->device_map.mbox.offset;
> -		length = map->device_map.mbox.size;
> -		regs->mbox = devm_cxl_iomap_block(dev, addr, length);
> -		if (!regs->mbox)
> -			return -ENOMEM;
> -	}
> -
> -	if (map->device_map.memdev.valid) {
> -		resource_size_t addr;
> -		resource_size_t length;
> +		if (!mi->rmap->valid)
> +			continue;
>  
> -		addr = phys_addr + map->device_map.memdev.offset;
> -		length = map->device_map.memdev.size;
> -		regs->memdev = devm_cxl_iomap_block(dev, addr, length);
> -		if (!regs->memdev)
> +		addr = phys_addr + mi->rmap->offset;
> +		length = mi->rmap->size;
> +		*(mi->addr) = devm_cxl_iomap_block(dev, addr, length);
> +		if (!*(mi->addr))
>  			return -ENOMEM;
>  	}
>  
> 

Re: [PATCH 2/8] cxl/pci: Cleanup cxl_map_device_regs()

[Dan Williams]

On Thu, Mar 17, 2022 at 3:08 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 15 Mar 2022 21:13:52 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > Use a loop to reduce the duplicated code in cxl_map_device_regs(). This
> > is in preparation for deleting cxl_map_regs().
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Trivial style comments inline.  Otherwise LGTM
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> > ---
> >  drivers/cxl/core/regs.c |   51 ++++++++++++++++++-----------------------------
> >  1 file changed, 20 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> > index bd6ae14b679e..bd766e461f7d 100644
> > --- a/drivers/cxl/core/regs.c
> > +++ b/drivers/cxl/core/regs.c
> > @@ -211,42 +211,31 @@ int cxl_map_device_regs(struct pci_dev *pdev,
> >                       struct cxl_device_regs *regs,
> >                       struct cxl_register_map *map)
> >  {
> > +     resource_size_t phys_addr =
> > +             pci_resource_start(pdev, map->barno) + map->block_offset;
>
> I'm not totally convinced by this refactoring as it's ugly either
> way...  Still your code, and I don't care that strongly ;)

Fair enough, but isn't there intrinsic beauty in a diff that deletes
more code than it adds?

The cleaner aspect to me is that the RAS Capability Structure support
can be added with a one line change rather than a new if block in
cxl_map_component_regs().

>
> >       struct device *dev = &pdev->dev;
> > -     resource_size_t phys_addr;
> > -
> > -     phys_addr = pci_resource_start(pdev, map->barno);
> > -     phys_addr += map->block_offset;
> > -
> > -     if (map->device_map.status.valid) {
> > -             resource_size_t addr;
> > +     struct mapinfo {
> > +             struct cxl_reg_map *rmap;
> > +             void __iomem **addr;
> > +     } mapinfo[] = {
> > +             { .rmap = &map->device_map.status, &regs->status, },
>
> Combining c99 style .rmap for first parameter and then not doing it
> for the second is a bit odd looking.  Was there a strong reason for
> doing this?  I'd just drop the ".rmap =" as it's not as though
> we need to look far to see what it's setting.

Good catch, yeah, not sure why I typed it that way, will fix.

[PATCH 3/8] cxl/pci: Kill cxl_map_regs()

[Dan Williams]

The component registers are currently unused by the cxl_pci driver.
Only the physical address base of the component registers is conveyed to
the cxl_mem driver. Just call cxl_map_device_registers() directly.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |   23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 994c79bf6afd..0efbb356cce0 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -346,27 +346,6 @@ static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
 	return 0;
 }
 
-static int cxl_map_regs(struct cxl_dev_state *cxlds, struct cxl_register_map *map)
-{
-	struct device *dev = cxlds->dev;
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	switch (map->reg_type) {
-	case CXL_REGLOC_RBI_COMPONENT:
-		cxl_map_component_regs(pdev, &cxlds->regs.component, map);
-		dev_dbg(dev, "Mapping component registers...\n");
-		break;
-	case CXL_REGLOC_RBI_MEMDEV:
-		cxl_map_device_regs(pdev, &cxlds->regs.device_regs, map);
-		dev_dbg(dev, "Probing device registers...\n");
-		break;
-	default:
-		break;
-	}
-
-	return 0;
-}
-
 static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
 			  struct cxl_register_map *map)
 {
@@ -599,7 +578,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
-	rc = cxl_map_regs(cxlds, &map);
+	rc = cxl_map_device_regs(pdev, &cxlds->regs.device_regs, &map);
 	if (rc)
 		return rc;
 

Re: [PATCH 3/8] cxl/pci: Kill cxl_map_regs()

[Jonathan Cameron]

On Tue, 15 Mar 2022 21:13:58 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The component registers are currently unused by the cxl_pci driver.
> Only the physical address base of the component registers is conveyed to
> the cxl_mem driver. Just call cxl_map_device_registers() directly.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Makes sense.   Not sure how we ended up with the unused component register
handling.  I guess code evolution...

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

> ---
>  drivers/cxl/pci.c |   23 +----------------------
>  1 file changed, 1 insertion(+), 22 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 994c79bf6afd..0efbb356cce0 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -346,27 +346,6 @@ static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
>  	return 0;
>  }
>  
> -static int cxl_map_regs(struct cxl_dev_state *cxlds, struct cxl_register_map *map)
> -{
> -	struct device *dev = cxlds->dev;
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -
> -	switch (map->reg_type) {
> -	case CXL_REGLOC_RBI_COMPONENT:
> -		cxl_map_component_regs(pdev, &cxlds->regs.component, map);
> -		dev_dbg(dev, "Mapping component registers...\n");
> -		break;
> -	case CXL_REGLOC_RBI_MEMDEV:
> -		cxl_map_device_regs(pdev, &cxlds->regs.device_regs, map);
> -		dev_dbg(dev, "Probing device registers...\n");
> -		break;
> -	default:
> -		break;
> -	}
> -
> -	return 0;
> -}
> -
>  static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>  			  struct cxl_register_map *map)
>  {
> @@ -599,7 +578,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> -	rc = cxl_map_regs(cxlds, &map);
> +	rc = cxl_map_device_regs(pdev, &cxlds->regs.device_regs, &map);
>  	if (rc)
>  		return rc;
>  
> 
> 

Re: [PATCH 3/8] cxl/pci: Kill cxl_map_regs()

[Dan Williams]

On Thu, Mar 17, 2022 at 3:10 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 15 Mar 2022 21:13:58 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > The component registers are currently unused by the cxl_pci driver.
> > Only the physical address base of the component registers is conveyed to
> > the cxl_mem driver. Just call cxl_map_device_registers() directly.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Makes sense.   Not sure how we ended up with the unused component register
> handling.  I guess code evolution...

Yeah, this happened when the cxl_port and cxl_mem drivers split the
responsibility of those blocks to downstream drivers.

>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> > ---
> >  drivers/cxl/pci.c |   23 +----------------------
> >  1 file changed, 1 insertion(+), 22 deletions(-)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 994c79bf6afd..0efbb356cce0 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -346,27 +346,6 @@ static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
> >       return 0;
> >  }
> >
> > -static int cxl_map_regs(struct cxl_dev_state *cxlds, struct cxl_register_map *map)
> > -{
> > -     struct device *dev = cxlds->dev;
> > -     struct pci_dev *pdev = to_pci_dev(dev);
> > -
> > -     switch (map->reg_type) {
> > -     case CXL_REGLOC_RBI_COMPONENT:
> > -             cxl_map_component_regs(pdev, &cxlds->regs.component, map);
> > -             dev_dbg(dev, "Mapping component registers...\n");
> > -             break;
> > -     case CXL_REGLOC_RBI_MEMDEV:
> > -             cxl_map_device_regs(pdev, &cxlds->regs.device_regs, map);
> > -             dev_dbg(dev, "Probing device registers...\n");
> > -             break;
> > -     default:
> > -             break;
> > -     }
> > -
> > -     return 0;
> > -}
> > -
> >  static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> >                         struct cxl_register_map *map)
> >  {
> > @@ -599,7 +578,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >       if (rc)
> >               return rc;
> >
> > -     rc = cxl_map_regs(cxlds, &map);
> > +     rc = cxl_map_device_regs(pdev, &cxlds->regs.device_regs, &map);
> >       if (rc)
> >               return rc;
> >
> >
> >
>

[PATCH 4/8] cxl/core/regs: Make cxl_map_{component, device}_regs() device generic

[Dan Williams]

There is no need to carry the barno and the block offset through the
stack, just convert them to a resource base immediately.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c  |    3 +--
 drivers/cxl/core/port.c |    2 +-
 drivers/cxl/core/regs.c |   40 +++++++++++++++++++++++-----------------
 drivers/cxl/cxl.h       |   12 ++++++------
 drivers/cxl/cxlpci.h    |    9 ---------
 drivers/cxl/pci.c       |   25 ++++++-------------------
 6 files changed, 37 insertions(+), 54 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index c9a494d6976a..6e9ad9933dd4 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -46,8 +46,7 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
 		dev_dbg(&port->dev, "failed to find component registers\n");
 
 	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
-	dport = devm_cxl_add_dport(port, &pdev->dev, port_num,
-				   cxl_regmap_to_base(pdev, &map));
+	dport = devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
 	if (IS_ERR(dport)) {
 		ctx->error = PTR_ERR(dport);
 		return PTR_ERR(dport);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index c1681248f322..2cf27e71d8af 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -972,7 +972,7 @@ static resource_size_t find_component_registers(struct device *dev)
 	pdev = to_pci_dev(dev);
 
 	cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
-	return cxl_regmap_to_base(pdev, &map);
+	return map.resource;
 }
 
 static int add_port_attach_ep(struct cxl_memdev *cxlmd,
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index bd766e461f7d..219c7d0e43e2 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -186,17 +186,13 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
 	return ret_val;
 }
 
-int cxl_map_component_regs(struct pci_dev *pdev,
-			   struct cxl_component_regs *regs,
+int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
 			   struct cxl_register_map *map)
 {
-	struct device *dev = &pdev->dev;
 	resource_size_t phys_addr;
 	resource_size_t length;
 
-	phys_addr = pci_resource_start(pdev, map->barno);
-	phys_addr += map->block_offset;
-
+	phys_addr = map->resource;
 	phys_addr += map->component_map.hdm_decoder.offset;
 	length = map->component_map.hdm_decoder.size;
 	regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length);
@@ -207,13 +203,11 @@ int cxl_map_component_regs(struct pci_dev *pdev,
 }
 EXPORT_SYMBOL_NS_GPL(cxl_map_component_regs, CXL);
 
-int cxl_map_device_regs(struct pci_dev *pdev,
+int cxl_map_device_regs(struct device *dev,
 			struct cxl_device_regs *regs,
 			struct cxl_register_map *map)
 {
-	resource_size_t phys_addr =
-		pci_resource_start(pdev, map->barno) + map->block_offset;
-	struct device *dev = &pdev->dev;
+	resource_size_t phys_addr = map->resource;
 	struct mapinfo {
 		struct cxl_reg_map *rmap;
 		void __iomem **addr;
@@ -243,13 +237,24 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 }
 EXPORT_SYMBOL_NS_GPL(cxl_map_device_regs, CXL);
 
-static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
+static bool cxl_decode_regblock(struct pci_dev *pdev, 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);
+	int bar = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BIR_MASK, reg_lo);
+	u64 offset = ((u64)reg_hi << 32) |
+		     (reg_lo & CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK);
+
+	if (offset > pci_resource_len(pdev, bar)) {
+		dev_warn(&pdev->dev,
+			 "BAR%d: %pr: too small (offset: %pa, type: %d)\n", bar,
+			 &pdev->resource[bar], &offset, map->reg_type);
+		return false;
+	}
+
 	map->reg_type = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK, reg_lo);
+	map->resource = pci_resource_start(pdev, bar) + offset;
+	map->max_size = pci_resource_len(pdev, bar) - offset;
+	return true;
 }
 
 /**
@@ -269,7 +274,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;
+	map->resource = CXL_RESOURCE_NONE;
 	regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
 					   CXL_DVSEC_REG_LOCATOR);
 	if (!regloc)
@@ -287,13 +292,14 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
 		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 (!cxl_decode_regblock(pdev, reg_lo, reg_hi, map))
+			continue;
 
 		if (map->reg_type == type)
 			return 0;
 	}
 
-	map->block_offset = U64_MAX;
+	map->resource = CXL_RESOURCE_NONE;
 	return -ENODEV;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 990b6670222e..2080a75c61fe 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -132,17 +132,17 @@ struct cxl_device_reg_map {
 /**
  * struct cxl_register_map - DVSEC harvested register block mapping parameters
  * @base: virtual base of the register-block-BAR + @block_offset
- * @block_offset: offset to start of register block in @barno
+ * @resource: physical resource base of the register block
+ * @max_size: maximum mapping size to perform register search
  * @reg_type: see enum cxl_regloc_type
- * @barno: PCI BAR number containing the register block
  * @component_map: cxl_reg_map for component registers
  * @device_map: cxl_reg_maps for device registers
  */
 struct cxl_register_map {
 	void __iomem *base;
-	u64 block_offset;
+	resource_size_t resource;
+	resource_size_t max_size;
 	u8 reg_type;
-	u8 barno;
 	union {
 		struct cxl_component_reg_map component_map;
 		struct cxl_device_reg_map device_map;
@@ -153,10 +153,10 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
 			      struct cxl_component_reg_map *map);
 void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 			   struct cxl_device_reg_map *map);
-int cxl_map_component_regs(struct pci_dev *pdev,
+int cxl_map_component_regs(struct device *dev,
 			   struct cxl_component_regs *regs,
 			   struct cxl_register_map *map);
-int cxl_map_device_regs(struct pci_dev *pdev,
+int cxl_map_device_regs(struct device *dev,
 			struct cxl_device_regs *regs,
 			struct cxl_register_map *map);
 
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 329e7ea3f36a..3c5be664ab22 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -62,14 +62,5 @@ 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->block_offset == U64_MAX)
-		return CXL_RESOURCE_NONE;
-
-	return pci_resource_start(pdev, map->barno) + map->block_offset;
-}
-
 int devm_cxl_port_enumerate_dports(struct cxl_port *port);
 #endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 0efbb356cce0..d8361331a013 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -275,35 +275,22 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
 
 static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
 {
-	void __iomem *addr;
-	int bar = map->barno;
 	struct device *dev = &pdev->dev;
-	resource_size_t offset = map->block_offset;
 
-	/* Basic sanity check that BAR is big enough */
-	if (pci_resource_len(pdev, bar) < offset) {
-		dev_err(dev, "BAR%d: %pr: too small (offset: %pa)\n", bar,
-			&pdev->resource[bar], &offset);
-		return -ENXIO;
-	}
-
-	addr = pci_iomap(pdev, bar, 0);
-	if (!addr) {
+	map->base = ioremap(map->resource, map->max_size);
+	if (!map->base) {
 		dev_err(dev, "failed to map registers\n");
 		return -ENOMEM;
 	}
 
-	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %pa\n",
-		bar, &offset);
-
-	map->base = addr + map->block_offset;
+	dev_dbg(dev, "Mapped CXL Memory Device resource %pa\n", &map->resource);
 	return 0;
 }
 
 static void cxl_unmap_regblock(struct pci_dev *pdev,
 			       struct cxl_register_map *map)
 {
-	pci_iounmap(pdev, map->base - map->block_offset);
+	iounmap(map->base);
 	map->base = NULL;
 }
 
@@ -578,7 +565,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
-	rc = cxl_map_device_regs(pdev, &cxlds->regs.device_regs, &map);
+	rc = cxl_map_device_regs(&pdev->dev, &cxlds->regs.device_regs, &map);
 	if (rc)
 		return rc;
 
@@ -591,7 +578,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
 
-	cxlds->component_reg_phys = cxl_regmap_to_base(pdev, &map);
+	cxlds->component_reg_phys = map.resource;
 
 	rc = cxl_pci_setup_mailbox(cxlds);
 	if (rc)

Re: [PATCH 4/8] cxl/core/regs: Make cxl_map_{component, device}_regs() device generic

[Jonathan Cameron]

On Tue, 15 Mar 2022 21:14:03 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> There is no need to carry the barno and the block offset through the
> stack, just convert them to a resource base immediately.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Other that this involving paths where we try to carry on after a broken
device is detected which I'd be far from confident are correct or regularly
tested, the patch looks fine.
Particular case that bothers me is undersized bars. Before this patch
those were (I think) an error that would fail probe.  Are they still after
this patch?

Assuming that part is fine.

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

> ---
>  drivers/cxl/core/pci.c  |    3 +--
>  drivers/cxl/core/port.c |    2 +-
>  drivers/cxl/core/regs.c |   40 +++++++++++++++++++++++-----------------
>  drivers/cxl/cxl.h       |   12 ++++++------
>  drivers/cxl/cxlpci.h    |    9 ---------
>  drivers/cxl/pci.c       |   25 ++++++-------------------
>  6 files changed, 37 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index c9a494d6976a..6e9ad9933dd4 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -46,8 +46,7 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
>  		dev_dbg(&port->dev, "failed to find component registers\n");
>  
>  	port_num = FIELD_GET(PCI_EXP_LNKCAP_PN, lnkcap);
> -	dport = devm_cxl_add_dport(port, &pdev->dev, port_num,
> -				   cxl_regmap_to_base(pdev, &map));
> +	dport = devm_cxl_add_dport(port, &pdev->dev, port_num, map.resource);
>  	if (IS_ERR(dport)) {
>  		ctx->error = PTR_ERR(dport);
>  		return PTR_ERR(dport);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index c1681248f322..2cf27e71d8af 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -972,7 +972,7 @@ static resource_size_t find_component_registers(struct device *dev)
>  	pdev = to_pci_dev(dev);
>  
>  	cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> -	return cxl_regmap_to_base(pdev, &map);
> +	return map.resource;
>  }
>  
>  static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index bd766e461f7d..219c7d0e43e2 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -186,17 +186,13 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
>  	return ret_val;
>  }
>  
> -int cxl_map_component_regs(struct pci_dev *pdev,
> -			   struct cxl_component_regs *regs,
> +int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
>  			   struct cxl_register_map *map)
>  {
> -	struct device *dev = &pdev->dev;
>  	resource_size_t phys_addr;
>  	resource_size_t length;
>  
> -	phys_addr = pci_resource_start(pdev, map->barno);
> -	phys_addr += map->block_offset;
> -
> +	phys_addr = map->resource;
>  	phys_addr += map->component_map.hdm_decoder.offset;
>  	length = map->component_map.hdm_decoder.size;
>  	regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length);
> @@ -207,13 +203,11 @@ int cxl_map_component_regs(struct pci_dev *pdev,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_map_component_regs, CXL);
>  
> -int cxl_map_device_regs(struct pci_dev *pdev,
> +int cxl_map_device_regs(struct device *dev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map)
>  {
> -	resource_size_t phys_addr =
> -		pci_resource_start(pdev, map->barno) + map->block_offset;
> -	struct device *dev = &pdev->dev;
> +	resource_size_t phys_addr = map->resource;
>  	struct mapinfo {
>  		struct cxl_reg_map *rmap;
>  		void __iomem **addr;
> @@ -243,13 +237,24 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_map_device_regs, CXL);
>  
> -static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi,
> +static bool cxl_decode_regblock(struct pci_dev *pdev, 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);
> +	int bar = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BIR_MASK, reg_lo);
> +	u64 offset = ((u64)reg_hi << 32) |
> +		     (reg_lo & CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK);
> +
> +	if (offset > pci_resource_len(pdev, bar)) {
> +		dev_warn(&pdev->dev,
> +			 "BAR%d: %pr: too small (offset: %pa, type: %d)\n", bar,
> +			 &pdev->resource[bar], &offset, map->reg_type);
> +		return false;
> +	}
> +
>  	map->reg_type = FIELD_GET(CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK, reg_lo);
> +	map->resource = pci_resource_start(pdev, bar) + offset;
> +	map->max_size = pci_resource_len(pdev, bar) - offset;
> +	return true;
>  }
>  
>  /**
> @@ -269,7 +274,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;
> +	map->resource = CXL_RESOURCE_NONE;
>  	regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
>  					   CXL_DVSEC_REG_LOCATOR);
>  	if (!regloc)
> @@ -287,13 +292,14 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>  		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 (!cxl_decode_regblock(pdev, reg_lo, reg_hi, map))
> +			continue;
>  
>  		if (map->reg_type == type)
>  			return 0;
>  	}
>  
> -	map->block_offset = U64_MAX;
> +	map->resource = CXL_RESOURCE_NONE;
>  	return -ENODEV;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 990b6670222e..2080a75c61fe 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -132,17 +132,17 @@ struct cxl_device_reg_map {
>  /**
>   * struct cxl_register_map - DVSEC harvested register block mapping parameters
>   * @base: virtual base of the register-block-BAR + @block_offset
> - * @block_offset: offset to start of register block in @barno
> + * @resource: physical resource base of the register block
> + * @max_size: maximum mapping size to perform register search
>   * @reg_type: see enum cxl_regloc_type
> - * @barno: PCI BAR number containing the register block
>   * @component_map: cxl_reg_map for component registers
>   * @device_map: cxl_reg_maps for device registers
>   */
>  struct cxl_register_map {
>  	void __iomem *base;
> -	u64 block_offset;
> +	resource_size_t resource;
> +	resource_size_t max_size;
>  	u8 reg_type;
> -	u8 barno;
>  	union {
>  		struct cxl_component_reg_map component_map;
>  		struct cxl_device_reg_map device_map;
> @@ -153,10 +153,10 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  			      struct cxl_component_reg_map *map);
>  void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  			   struct cxl_device_reg_map *map);
> -int cxl_map_component_regs(struct pci_dev *pdev,
> +int cxl_map_component_regs(struct device *dev,
>  			   struct cxl_component_regs *regs,
>  			   struct cxl_register_map *map);
> -int cxl_map_device_regs(struct pci_dev *pdev,
> +int cxl_map_device_regs(struct device *dev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map);
>  
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 329e7ea3f36a..3c5be664ab22 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -62,14 +62,5 @@ 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->block_offset == U64_MAX)
> -		return CXL_RESOURCE_NONE;
> -
> -	return pci_resource_start(pdev, map->barno) + map->block_offset;
> -}
> -
>  int devm_cxl_port_enumerate_dports(struct cxl_port *port);
>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 0efbb356cce0..d8361331a013 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -275,35 +275,22 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
>  
>  static int cxl_map_regblock(struct pci_dev *pdev, struct cxl_register_map *map)
>  {
> -	void __iomem *addr;
> -	int bar = map->barno;
>  	struct device *dev = &pdev->dev;
> -	resource_size_t offset = map->block_offset;
>  
> -	/* Basic sanity check that BAR is big enough */
> -	if (pci_resource_len(pdev, bar) < offset) {
> -		dev_err(dev, "BAR%d: %pr: too small (offset: %pa)\n", bar,
> -			&pdev->resource[bar], &offset);
> -		return -ENXIO;
> -	}
> -
> -	addr = pci_iomap(pdev, bar, 0);
> -	if (!addr) {
> +	map->base = ioremap(map->resource, map->max_size);
> +	if (!map->base) {
>  		dev_err(dev, "failed to map registers\n");
>  		return -ENOMEM;
>  	}
>  
> -	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %pa\n",
> -		bar, &offset);
> -
> -	map->base = addr + map->block_offset;
> +	dev_dbg(dev, "Mapped CXL Memory Device resource %pa\n", &map->resource);
>  	return 0;
>  }
>  
>  static void cxl_unmap_regblock(struct pci_dev *pdev,
>  			       struct cxl_register_map *map)
>  {
> -	pci_iounmap(pdev, map->base - map->block_offset);
> +	iounmap(map->base);
>  	map->base = NULL;
>  }
>  
> @@ -578,7 +565,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> -	rc = cxl_map_device_regs(pdev, &cxlds->regs.device_regs, &map);
> +	rc = cxl_map_device_regs(&pdev->dev, &cxlds->regs.device_regs, &map);
>  	if (rc)
>  		return rc;
>  
> @@ -591,7 +578,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
>  
> -	cxlds->component_reg_phys = cxl_regmap_to_base(pdev, &map);
> +	cxlds->component_reg_phys = map.resource;
>  
>  	rc = cxl_pci_setup_mailbox(cxlds);
>  	if (rc)
> 

Re: [PATCH 4/8] cxl/core/regs: Make cxl_map_{component, device}_regs() device generic

[Dan Williams]

On Thu, Mar 17, 2022 at 3:25 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 15 Mar 2022 21:14:03 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > There is no need to carry the barno and the block offset through the
> > stack, just convert them to a resource base immediately.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Other that this involving paths where we try to carry on after a broken
> device is detected which I'd be far from confident are correct or regularly
> tested, the patch looks fine.

cxl_test covers some of these cases, but otherwise the failed state
should be equivalent to the state after "cxl disable-memdev $memX"

> Particular case that bothers me is undersized bars. Before this patch
> those were (I think) an error that would fail probe.  Are they still after
> this patch?

Yes, that check just moves to cxl_decode_regblock() directly.

>
> Assuming that part is fine.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks.

[PATCH 5/8] cxl/port: Limit the port driver to just the HDM Decoder Capability

[Dan Williams]

Update the port driver to use cxl_map_component_registers() so that the
component register block can be shared between the cxl_pci driver and
the cxl_port driver. I.e. stop the port driver from reserving the entire
component register block for itself via request_region() when it only
needs the HDM Decoder Capability subset.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c |   32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 0e89a7a932d4..09221afca309 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -77,18 +77,22 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
 		cxlhdm->interleave_mask |= GENMASK(14, 12);
 }
 
-static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
-					  void __iomem *crb)
+static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
+				struct cxl_component_regs *regs)
 {
-	struct cxl_component_reg_map map;
+	struct cxl_register_map map = {
+		.resource = port->component_reg_phys,
+		.base = crb,
+		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
+	};
 
-	cxl_probe_component_regs(&port->dev, crb, &map);
-	if (!map.hdm_decoder.valid) {
+	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
+	if (!map.component_map.hdm_decoder.valid) {
 		dev_err(&port->dev, "HDM decoder registers invalid\n");
-		return IOMEM_ERR_PTR(-ENXIO);
+		return -ENXIO;
 	}
 
-	return crb + map.hdm_decoder.offset;
+	return cxl_map_component_regs(&port->dev, regs, &map);
 }
 
 /**
@@ -98,25 +102,25 @@ static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
 struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port)
 {
 	struct device *dev = &port->dev;
-	void __iomem *crb, *hdm;
 	struct cxl_hdm *cxlhdm;
+	void __iomem *crb;
+	int rc;
 
 	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
 	if (!cxlhdm)
 		return ERR_PTR(-ENOMEM);
 
 	cxlhdm->port = port;
-	crb = devm_cxl_iomap_block(dev, port->component_reg_phys,
-				   CXL_COMPONENT_REG_BLOCK_SIZE);
+	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
 	if (!crb) {
 		dev_err(dev, "No component registers mapped\n");
 		return ERR_PTR(-ENXIO);
 	}
 
-	hdm = map_hdm_decoder_regs(port, crb);
-	if (IS_ERR(hdm))
-		return ERR_CAST(hdm);
-	cxlhdm->regs.hdm_decoder = hdm;
+	rc = map_hdm_decoder_regs(port, crb, &cxlhdm->regs);
+	iounmap(crb);
+	if (rc)
+		return ERR_PTR(rc);
 
 	parse_hdm_decoder_caps(cxlhdm);
 	if (cxlhdm->decoder_count == 0) {

Re: [PATCH 5/8] cxl/port: Limit the port driver to just the HDM Decoder Capability

[Jonathan Cameron]

On Tue, 15 Mar 2022 21:14:08 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Update the port driver to use cxl_map_component_registers() so that the
> component register block can be shared between the cxl_pci driver and
> the cxl_port driver. I.e. stop the port driver from reserving the entire
> component register block for itself via request_region() when it only
> needs the HDM Decoder Capability subset.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

We go through a dance in the other callers to cxl_probe_component_regs
cxl_map_component_regs that means we don't have block mapped at tim
of calling cxl_map_component_regs().

I'd gotten it into my head that we had that separation for a reason,
and it doesn't exist here as both are called with the block mapped.
Looking again I think that was needed because of use of pci_iomap
for the bar so I guess the same constraint doesn't apply here?

Thanks,

Jonathan



> ---
>  drivers/cxl/core/hdm.c |   32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0e89a7a932d4..09221afca309 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -77,18 +77,22 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  		cxlhdm->interleave_mask |= GENMASK(14, 12);
>  }
>  
> -static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
> -					  void __iomem *crb)
> +static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> +				struct cxl_component_regs *regs)
>  {
> -	struct cxl_component_reg_map map;
> +	struct cxl_register_map map = {
> +		.resource = port->component_reg_phys,
> +		.base = crb,
> +		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> +	};
>  
> -	cxl_probe_component_regs(&port->dev, crb, &map);
> -	if (!map.hdm_decoder.valid) {
> +	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> +	if (!map.component_map.hdm_decoder.valid) {
>  		dev_err(&port->dev, "HDM decoder registers invalid\n");
> -		return IOMEM_ERR_PTR(-ENXIO);
> +		return -ENXIO;
>  	}
>  
> -	return crb + map.hdm_decoder.offset;
> +	return cxl_map_component_regs(&port->dev, regs, &map);
>  }
>  
>  /**
> @@ -98,25 +102,25 @@ static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
>  struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port)
>  {
>  	struct device *dev = &port->dev;
> -	void __iomem *crb, *hdm;
>  	struct cxl_hdm *cxlhdm;
> +	void __iomem *crb;
> +	int rc;
>  
>  	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
>  	if (!cxlhdm)
>  		return ERR_PTR(-ENOMEM);
>  
>  	cxlhdm->port = port;
> -	crb = devm_cxl_iomap_block(dev, port->component_reg_phys,
> -				   CXL_COMPONENT_REG_BLOCK_SIZE);
> +	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
>  	if (!crb) {
>  		dev_err(dev, "No component registers mapped\n");
>  		return ERR_PTR(-ENXIO);
>  	}
>  
> -	hdm = map_hdm_decoder_regs(port, crb);
> -	if (IS_ERR(hdm))
> -		return ERR_CAST(hdm);
> -	cxlhdm->regs.hdm_decoder = hdm;
> +	rc = map_hdm_decoder_regs(port, crb, &cxlhdm->regs);
> +	iounmap(crb);
> +	if (rc)
> +		return ERR_PTR(rc);
>  
>  	parse_hdm_decoder_caps(cxlhdm);
>  	if (cxlhdm->decoder_count == 0) {
> 

[PATCH 6/8] cxl/pci: Prepare for mapping RAS Capability Structure

[Dan Williams]

The RAS Capabilitiy Structure is a CXL Component register capability
block. Unlike the HDM Decoder Capability, it will be referenced by the
cxl_pci driver in response to PCIe AER events. Due to this it is no
longer the case that cxl_map_component_regs() can assume that it should
map all component registers. Plumb a bitmask of capability ids to map
through cxl_map_component_regs().

For symmetry cxl_probe_device_regs() is updated to populate @id in
'struct cxl_reg_map' even though cxl_map_device_regs() does not have a
need to map a subset of the device registers per caller.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c  |    3 ++-
 drivers/cxl/core/regs.c |   36 ++++++++++++++++++++++++++----------
 drivers/cxl/cxl.h       |    7 ++++---
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 09221afca309..b348217ab704 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -92,7 +92,8 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
 		return -ENXIO;
 	}
 
-	return cxl_map_component_regs(&port->dev, regs, &map);
+	return cxl_map_component_regs(&port->dev, regs, &map,
+				      BIT(CXL_CM_CAP_CAP_ID_HDM));
 }
 
 /**
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 219c7d0e43e2..c022c8937dfc 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -92,6 +92,7 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
 		if (!rmap)
 			continue;
 		rmap->valid = true;
+		rmap->id = cap_id;
 		rmap->offset = CXL_CM_OFFSET + offset;
 		rmap->size = length;
 	}
@@ -159,6 +160,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 		if (!rmap)
 			continue;
 		rmap->valid = true;
+		rmap->id = cap_id;
 		rmap->offset = offset;
 		rmap->size = length;
 	}
@@ -187,17 +189,31 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
 }
 
 int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
-			   struct cxl_register_map *map)
+			   struct cxl_register_map *map, unsigned long map_mask)
 {
-	resource_size_t phys_addr;
-	resource_size_t length;
-
-	phys_addr = map->resource;
-	phys_addr += map->component_map.hdm_decoder.offset;
-	length = map->component_map.hdm_decoder.size;
-	regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length);
-	if (!regs->hdm_decoder)
-		return -ENOMEM;
+	struct mapinfo {
+		struct cxl_reg_map *rmap;
+		void __iomem **addr;
+	} mapinfo[] = {
+		{ .rmap = &map->component_map.hdm_decoder, &regs->hdm_decoder },
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(mapinfo); i++) {
+		struct mapinfo *mi = &mapinfo[i];
+		resource_size_t phys_addr;
+		resource_size_t length;
+
+		if (!mi->rmap->valid)
+			continue;
+		if (!test_bit(mi->rmap->id, &map_mask))
+			continue;
+		phys_addr = map->resource + mi->rmap->offset;
+		length = mi->rmap->size;
+		*(mi->addr) = devm_cxl_iomap_block(dev, phys_addr, length);
+		if (!*(mi->addr))
+			return -ENOMEM;
+	}
 
 	return 0;
 }
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 2080a75c61fe..52bd77d8e22a 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -115,6 +115,7 @@ struct cxl_regs {
 
 struct cxl_reg_map {
 	bool valid;
+	int id;
 	unsigned long offset;
 	unsigned long size;
 };
@@ -153,9 +154,9 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
 			      struct cxl_component_reg_map *map);
 void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 			   struct cxl_device_reg_map *map);
-int cxl_map_component_regs(struct device *dev,
-			   struct cxl_component_regs *regs,
-			   struct cxl_register_map *map);
+int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
+			   struct cxl_register_map *map,
+			   unsigned long map_mask);
 int cxl_map_device_regs(struct device *dev,
 			struct cxl_device_regs *regs,
 			struct cxl_register_map *map);

Re: [PATCH 6/8] cxl/pci: Prepare for mapping RAS Capability Structure

[Jonathan Cameron]

On Tue, 15 Mar 2022 21:14:14 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The RAS Capabilitiy Structure is a CXL Component register capability
> block. Unlike the HDM Decoder Capability, it will be referenced by the
> cxl_pci driver in response to PCIe AER events. Due to this it is no
> longer the case that cxl_map_component_regs() can assume that it should
> map all component registers. Plumb a bitmask of capability ids to map
> through cxl_map_component_regs().
> 
> For symmetry cxl_probe_device_regs() is updated to populate @id in
> 'struct cxl_reg_map' even though cxl_map_device_regs() does not have a
> need to map a subset of the device registers per caller.

I guess it doesn't hurt to add that, though without the mask being
passed into appropriate functions it's a bit pointless.  What we have
is now 'half symmetric' perhaps.

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
A few trivial comments inline, but basically looks good to me.

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

> ---
>  drivers/cxl/core/hdm.c  |    3 ++-
>  drivers/cxl/core/regs.c |   36 ++++++++++++++++++++++++++----------
>  drivers/cxl/cxl.h       |    7 ++++---
>  3 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 09221afca309..b348217ab704 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -92,7 +92,8 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
>  		return -ENXIO;
>  	}
>  
> -	return cxl_map_component_regs(&port->dev, regs, &map);
> +	return cxl_map_component_regs(&port->dev, regs, &map,
> +				      BIT(CXL_CM_CAP_CAP_ID_HDM));
>  }
>  
>  /**
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 219c7d0e43e2..c022c8937dfc 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -92,6 +92,7 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  		if (!rmap)
>  			continue;
>  		rmap->valid = true;
> +		rmap->id = cap_id;
>  		rmap->offset = CXL_CM_OFFSET + offset;
>  		rmap->size = length;
>  	}
> @@ -159,6 +160,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  		if (!rmap)
>  			continue;
>  		rmap->valid = true;
> +		rmap->id = cap_id;
>  		rmap->offset = offset;
>  		rmap->size = length;
>  	}
> @@ -187,17 +189,31 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
>  }
>  
>  int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
> -			   struct cxl_register_map *map)
> +			   struct cxl_register_map *map, unsigned long map_mask)

Maybe pass an unsigned long *map_mask from the start for the inevitable
capability IDs passing the minimum length of a long?
Disadvantage being you'll need to roll a local BITMAP to pass in at all callsites.

>  {
> -	resource_size_t phys_addr;
> -	resource_size_t length;
> -
> -	phys_addr = map->resource;
> -	phys_addr += map->component_map.hdm_decoder.offset;
> -	length = map->component_map.hdm_decoder.size;
> -	regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length);
> -	if (!regs->hdm_decoder)
> -		return -ENOMEM;
> +	struct mapinfo {
> +		struct cxl_reg_map *rmap;
> +		void __iomem **addr;
> +	} mapinfo[] = {
> +		{ .rmap = &map->component_map.hdm_decoder, &regs->hdm_decoder },

As in previous instance, mixing two styles of assignment seems odd.

> +	};
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mapinfo); i++) {
> +		struct mapinfo *mi = &mapinfo[i];
> +		resource_size_t phys_addr;
> +		resource_size_t length;
> +
> +		if (!mi->rmap->valid)
> +			continue;
> +		if (!test_bit(mi->rmap->id, &map_mask))
> +			continue;
> +		phys_addr = map->resource + mi->rmap->offset;
> +		length = mi->rmap->size;
> +		*(mi->addr) = devm_cxl_iomap_block(dev, phys_addr, length);
> +		if (!*(mi->addr))
> +			return -ENOMEM;
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 2080a75c61fe..52bd77d8e22a 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -115,6 +115,7 @@ struct cxl_regs {
>  
>  struct cxl_reg_map {
>  	bool valid;
> +	int id;
>  	unsigned long offset;
>  	unsigned long size;
>  };
> @@ -153,9 +154,9 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  			      struct cxl_component_reg_map *map);
>  void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  			   struct cxl_device_reg_map *map);
> -int cxl_map_component_regs(struct device *dev,
> -			   struct cxl_component_regs *regs,
> -			   struct cxl_register_map *map);
> +int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,

Worth the rewrapping and extra diff as a result?  (I think it's precisely 80 chars)

> +			   struct cxl_register_map *map,
> +			   unsigned long map_mask);
>  int cxl_map_device_regs(struct device *dev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map);
> 

Re: [PATCH 6/8] cxl/pci: Prepare for mapping RAS Capability Structure

[Dan Williams]

On Thu, Mar 17, 2022 at 3:57 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 15 Mar 2022 21:14:14 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > The RAS Capabilitiy Structure is a CXL Component register capability
> > block. Unlike the HDM Decoder Capability, it will be referenced by the
> > cxl_pci driver in response to PCIe AER events. Due to this it is no
> > longer the case that cxl_map_component_regs() can assume that it should
> > map all component registers. Plumb a bitmask of capability ids to map
> > through cxl_map_component_regs().
> >
> > For symmetry cxl_probe_device_regs() is updated to populate @id in
> > 'struct cxl_reg_map' even though cxl_map_device_regs() does not have a
> > need to map a subset of the device registers per caller.
>
> I guess it doesn't hurt to add that, though without the mask being
> passed into appropriate functions it's a bit pointless.  What we have
> is now 'half symmetric' perhaps.

True, I was mainly worried about someone dumping the rmap->id in a
debug session and wondering why all the ids are zero for
device-register instances, but no need to add the per-id allocation
since there's only one cxl_map_device_regs() caller today.

> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> A few trivial comments inline, but basically looks good to me.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> > ---
> >  drivers/cxl/core/hdm.c  |    3 ++-
> >  drivers/cxl/core/regs.c |   36 ++++++++++++++++++++++++++----------
> >  drivers/cxl/cxl.h       |    7 ++++---
> >  3 files changed, 32 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 09221afca309..b348217ab704 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -92,7 +92,8 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> >               return -ENXIO;
> >       }
> >
> > -     return cxl_map_component_regs(&port->dev, regs, &map);
> > +     return cxl_map_component_regs(&port->dev, regs, &map,
> > +                                   BIT(CXL_CM_CAP_CAP_ID_HDM));
> >  }
> >
> >  /**
> > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> > index 219c7d0e43e2..c022c8937dfc 100644
> > --- a/drivers/cxl/core/regs.c
> > +++ b/drivers/cxl/core/regs.c
> > @@ -92,6 +92,7 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
> >               if (!rmap)
> >                       continue;
> >               rmap->valid = true;
> > +             rmap->id = cap_id;
> >               rmap->offset = CXL_CM_OFFSET + offset;
> >               rmap->size = length;
> >       }
> > @@ -159,6 +160,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
> >               if (!rmap)
> >                       continue;
> >               rmap->valid = true;
> > +             rmap->id = cap_id;
> >               rmap->offset = offset;
> >               rmap->size = length;
> >       }
> > @@ -187,17 +189,31 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> >  }
> >
> >  int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
> > -                        struct cxl_register_map *map)
> > +                        struct cxl_register_map *map, unsigned long map_mask)
>
> Maybe pass an unsigned long *map_mask from the start for the inevitable
> capability IDs passing the minimum length of a long?
> Disadvantage being you'll need to roll a local BITMAP to pass in at all callsites.

I'm ok to leave that to future folks since we're only up to 8 ids in
today's spec, and the compiler will flag "error: left shift count >=
width of type" if someone tries to add support for that high numbered
capability id.

>
> >  {
> > -     resource_size_t phys_addr;
> > -     resource_size_t length;
> > -
> > -     phys_addr = map->resource;
> > -     phys_addr += map->component_map.hdm_decoder.offset;
> > -     length = map->component_map.hdm_decoder.size;
> > -     regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length);
> > -     if (!regs->hdm_decoder)
> > -             return -ENOMEM;
> > +     struct mapinfo {
> > +             struct cxl_reg_map *rmap;
> > +             void __iomem **addr;
> > +     } mapinfo[] = {
> > +             { .rmap = &map->component_map.hdm_decoder, &regs->hdm_decoder },
>
> As in previous instance, mixing two styles of assignment seems odd.

Yup, will fix.

>
> > +     };
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(mapinfo); i++) {
> > +             struct mapinfo *mi = &mapinfo[i];
> > +             resource_size_t phys_addr;
> > +             resource_size_t length;
> > +
> > +             if (!mi->rmap->valid)
> > +                     continue;
> > +             if (!test_bit(mi->rmap->id, &map_mask))
> > +                     continue;
> > +             phys_addr = map->resource + mi->rmap->offset;
> > +             length = mi->rmap->size;
> > +             *(mi->addr) = devm_cxl_iomap_block(dev, phys_addr, length);
> > +             if (!*(mi->addr))
> > +                     return -ENOMEM;
> > +     }
> >
> >       return 0;
> >  }
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 2080a75c61fe..52bd77d8e22a 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -115,6 +115,7 @@ struct cxl_regs {
> >
> >  struct cxl_reg_map {
> >       bool valid;
> > +     int id;
> >       unsigned long offset;
> >       unsigned long size;
> >  };
> > @@ -153,9 +154,9 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
> >                             struct cxl_component_reg_map *map);
> >  void cxl_probe_device_regs(struct device *dev, void __iomem *base,
> >                          struct cxl_device_reg_map *map);
> > -int cxl_map_component_regs(struct device *dev,
> > -                        struct cxl_component_regs *regs,
> > -                        struct cxl_register_map *map);
> > +int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
>
> Worth the rewrapping and extra diff as a result?  (I think it's precisely 80 chars)

clang-format says yes. If I ask it to flow just the new @map_mask
argument it reflows the whole declaration. However, this formatting
change should be pushed back to "[PATCH 4/8] cxl/core/regs: Make
cxl_map_{component, device}_regs() device generic", because that's the
patch that reduced the first argument from "struct pci_dev *pdev" to
the shorter "struct device *dev" allowing the second arg to move onto
the same line.

>
> > +                        struct cxl_register_map *map,
> > +                        unsigned long map_mask);
> >  int cxl_map_device_regs(struct device *dev,
> >                       struct cxl_device_regs *regs,
> >                       struct cxl_register_map *map);
> >
>

Re: [PATCH 6/8] cxl/pci: Prepare for mapping RAS Capability Structure

[Ben Widawsky]

On 22-03-15 21:14:14, Dan Williams wrote:
> The RAS Capabilitiy Structure is a CXL Component register capability
> block. Unlike the HDM Decoder Capability, it will be referenced by the
> cxl_pci driver in response to PCIe AER events. Due to this it is no
> longer the case that cxl_map_component_regs() can assume that it should
> map all component registers. Plumb a bitmask of capability ids to map
> through cxl_map_component_regs().
> 
> For symmetry cxl_probe_device_regs() is updated to populate @id in
> 'struct cxl_reg_map' even though cxl_map_device_regs() does not have a
> need to map a subset of the device registers per caller.

This seems weird to me. You spent the first 4 or so patches consolidating the
mapping into a nice loop only to break out an ID to do individual mappings
again. Are you sure this is such a win over having discrete mapping functions?

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/hdm.c  |    3 ++-
>  drivers/cxl/core/regs.c |   36 ++++++++++++++++++++++++++----------
>  drivers/cxl/cxl.h       |    7 ++++---
>  3 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 09221afca309..b348217ab704 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -92,7 +92,8 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
>  		return -ENXIO;
>  	}
>  
> -	return cxl_map_component_regs(&port->dev, regs, &map);
> +	return cxl_map_component_regs(&port->dev, regs, &map,
> +				      BIT(CXL_CM_CAP_CAP_ID_HDM));
>  }
>  
>  /**
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 219c7d0e43e2..c022c8937dfc 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -92,6 +92,7 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  		if (!rmap)
>  			continue;
>  		rmap->valid = true;
> +		rmap->id = cap_id;
>  		rmap->offset = CXL_CM_OFFSET + offset;
>  		rmap->size = length;
>  	}
> @@ -159,6 +160,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  		if (!rmap)
>  			continue;
>  		rmap->valid = true;
> +		rmap->id = cap_id;
>  		rmap->offset = offset;
>  		rmap->size = length;
>  	}
> @@ -187,17 +189,31 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
>  }
>  
>  int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
> -			   struct cxl_register_map *map)
> +			   struct cxl_register_map *map, unsigned long map_mask)
>  {
> -	resource_size_t phys_addr;
> -	resource_size_t length;
> -
> -	phys_addr = map->resource;
> -	phys_addr += map->component_map.hdm_decoder.offset;
> -	length = map->component_map.hdm_decoder.size;
> -	regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length);
> -	if (!regs->hdm_decoder)
> -		return -ENOMEM;
> +	struct mapinfo {
> +		struct cxl_reg_map *rmap;
> +		void __iomem **addr;
> +	} mapinfo[] = {
> +		{ .rmap = &map->component_map.hdm_decoder, &regs->hdm_decoder },
> +	};
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mapinfo); i++) {
> +		struct mapinfo *mi = &mapinfo[i];
> +		resource_size_t phys_addr;
> +		resource_size_t length;
> +
> +		if (!mi->rmap->valid)
> +			continue;
> +		if (!test_bit(mi->rmap->id, &map_mask))
> +			continue;
> +		phys_addr = map->resource + mi->rmap->offset;
> +		length = mi->rmap->size;
> +		*(mi->addr) = devm_cxl_iomap_block(dev, phys_addr, length);
> +		if (!*(mi->addr))
> +			return -ENOMEM;
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 2080a75c61fe..52bd77d8e22a 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -115,6 +115,7 @@ struct cxl_regs {
>  
>  struct cxl_reg_map {
>  	bool valid;
> +	int id;
>  	unsigned long offset;
>  	unsigned long size;
>  };
> @@ -153,9 +154,9 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  			      struct cxl_component_reg_map *map);
>  void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  			   struct cxl_device_reg_map *map);
> -int cxl_map_component_regs(struct device *dev,
> -			   struct cxl_component_regs *regs,
> -			   struct cxl_register_map *map);
> +int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
> +			   struct cxl_register_map *map,
> +			   unsigned long map_mask);
>  int cxl_map_device_regs(struct device *dev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map);
> 

Re: [PATCH 6/8] cxl/pci: Prepare for mapping RAS Capability Structure

[Dan Williams]

On Thu, Mar 17, 2022 at 10:32 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 22-03-15 21:14:14, Dan Williams wrote:
> > The RAS Capabilitiy Structure is a CXL Component register capability
> > block. Unlike the HDM Decoder Capability, it will be referenced by the
> > cxl_pci driver in response to PCIe AER events. Due to this it is no
> > longer the case that cxl_map_component_regs() can assume that it should
> > map all component registers. Plumb a bitmask of capability ids to map
> > through cxl_map_component_regs().
> >
> > For symmetry cxl_probe_device_regs() is updated to populate @id in
> > 'struct cxl_reg_map' even though cxl_map_device_regs() does not have a
> > need to map a subset of the device registers per caller.
>
> This seems weird to me. You spent the first 4 or so patches consolidating the
> mapping into a nice loop only to break out an ID to do individual mappings
> again. Are you sure this is such a win over having discrete mapping functions?

The loop is still there. This allows cxl_port and cxl_pci to share all
the same logic save for a bitmap to select the block. You're angling
for a:

cxl_map_hdm_regs(&port->dev, regs, &map);

...? Internally that cxl_map_hdm_regs() should be sharing code with
cxl_map_ras_regs(), so as far as I can see "discrete mapping
functions" is just asking for the below, and I'd rather skip the extra
wrapper:

int cxl_map_hdm_regs(struct pci_dev *pdev,
                           struct cxl_component_regs *regs,
                           struct cxl_register_map *map)
{
    return cxl_map_component_regs(&port->dev, regs, &map,
BIT(CXL_CM_CAP_CAP_ID_HDM));
}

[PATCH 7/8] cxl/pci: Find and map the RAS Capability Structure

[Dan Williams]

The RAS Capability Structure has some ancillary information that may be
relevant with respect to AER events, link and protcol error status
registers. Map the RAS Capability Registers in support of defining a
'struct pci_error_handlers' instance for the cxl_pci driver.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/regs.c |    7 +++++++
 drivers/cxl/cxl.h       |   19 +++++++++++++++++++
 drivers/cxl/pci.c       |    8 ++++++++
 3 files changed, 34 insertions(+)

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index c022c8937dfc..53aac68b9ce4 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -83,6 +83,12 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
 			rmap = &map->hdm_decoder;
 			break;
 		}
+		case CXL_CM_CAP_CAP_ID_RAS:
+			dev_dbg(dev, "found RAS capability (0x%x)\n",
+				offset);
+			length = CXL_RAS_CAPABILITY_LENGTH;
+			rmap = &map->ras;
+			break;
 		default:
 			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
 				offset);
@@ -196,6 +202,7 @@ int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
 		void __iomem **addr;
 	} mapinfo[] = {
 		{ .rmap = &map->component_map.hdm_decoder, &regs->hdm_decoder },
+		{ .rmap = &map->component_map.ras, &regs->ras },
 	};
 	int i;
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 52bd77d8e22a..cf3d8d0aaf22 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -32,6 +32,7 @@
 #define   CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24)
 #define CXL_CM_CAP_PTR_MASK GENMASK(31, 20)
 
+#define   CXL_CM_CAP_CAP_ID_RAS 0x2
 #define   CXL_CM_CAP_CAP_ID_HDM 0x5
 #define   CXL_CM_CAP_CAP_HDM_VERSION 1
 
@@ -64,6 +65,21 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr)
 	return val ? val * 2 : 1;
 }
 
+/* RAS Registers CXL 2.0 8.2.5.9 CXL RAS Capability Structure */
+#define CXL_RAS_UNCORRECTABLE_STATUS_OFFSET 0x0
+#define   CXL_RAS_UNCORRECTABLE_STATUS_MASK (GENMASK(16, 14) | GENMASK(11, 0))
+#define CXL_RAS_UNCORRECTABLE_MASK_OFFSET 0x4
+#define   CXL_RAS_UNCORRECTABLE_MASK_MASK (GENMASK(16, 14) | GENMASK(11, 0))
+#define CXL_RAS_UNCORRECTABLE_SEVERITY_OFFSET 0x8
+#define   CXL_RAS_UNCORRECTABLE_SEVERITY_MASK (GENMASK(16, 14) | GENMASK(11, 0))
+#define CXL_RAS_CORRECTABLE_STATUS_OFFSET 0xC
+#define   CXL_RAS_CORRECTABLE_STATUS_MASK GENMASK(6, 0)
+#define CXL_RAS_CORRECTABLE_MASK_OFFSET 0x10
+#define   CXL_RAS_CORRECTABLE_MASK_MASK GENMASK(6, 0)
+#define CXL_RAS_CAP_CONTROL_OFFSET 0x14
+#define CXL_RAS_HEADER_LOG_OFFSET 0x18
+#define CXL_RAS_CAPABILITY_LENGTH 0x58
+
 /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
 #define CXLDEV_CAP_ARRAY_OFFSET 0x0
 #define   CXLDEV_CAP_ARRAY_CAP_ID 0
@@ -98,9 +114,11 @@ struct cxl_regs {
 	/*
 	 * Common set of CXL Component register block base pointers
 	 * @hdm_decoder: CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure
+	 * @ras: CXL 2.0 8.2.5.9 CXL RAS Capability Structure
 	 */
 	struct_group_tagged(cxl_component_regs, component,
 		void __iomem *hdm_decoder;
+		void __iomem *ras;
 	);
 	/*
 	 * Common set of CXL Device register block base pointers
@@ -122,6 +140,7 @@ struct cxl_reg_map {
 
 struct cxl_component_reg_map {
 	struct cxl_reg_map hdm_decoder;
+	struct cxl_reg_map ras;
 };
 
 struct cxl_device_reg_map {
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index d8361331a013..bde8929450f0 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -310,6 +310,9 @@ static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
 			return -ENXIO;
 		}
 
+		if (!comp_map->ras.valid)
+			dev_dbg(dev, "RAS registers not found\n");
+
 		dev_dbg(dev, "Set up component registers\n");
 		break;
 	case CXL_REGLOC_RBI_MEMDEV:
@@ -580,6 +583,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	cxlds->component_reg_phys = map.resource;
 
+	rc = cxl_map_component_regs(&pdev->dev, &cxlds->regs.component,
+				    &map, BIT(CXL_CM_CAP_CAP_ID_RAS));
+	if (rc)
+		dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
+
 	rc = cxl_pci_setup_mailbox(cxlds);
 	if (rc)
 		return rc;

Re: [PATCH 7/8] cxl/pci: Find and map the RAS Capability Structure

[Jonathan Cameron]

On Tue, 15 Mar 2022 21:14:19 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The RAS Capability Structure has some ancillary information that may be
> relevant with respect to AER events, link and protcol error status
> registers. Map the RAS Capability Registers in support of defining a
> 'struct pci_error_handlers' instance for the cxl_pci driver.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Looks right to me.

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

> ---
>  drivers/cxl/core/regs.c |    7 +++++++
>  drivers/cxl/cxl.h       |   19 +++++++++++++++++++
>  drivers/cxl/pci.c       |    8 ++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index c022c8937dfc..53aac68b9ce4 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -83,6 +83,12 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  			rmap = &map->hdm_decoder;
>  			break;
>  		}
> +		case CXL_CM_CAP_CAP_ID_RAS:
> +			dev_dbg(dev, "found RAS capability (0x%x)\n",
> +				offset);
> +			length = CXL_RAS_CAPABILITY_LENGTH;
> +			rmap = &map->ras;
> +			break;
>  		default:
>  			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
>  				offset);
> @@ -196,6 +202,7 @@ int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
>  		void __iomem **addr;
>  	} mapinfo[] = {
>  		{ .rmap = &map->component_map.hdm_decoder, &regs->hdm_decoder },
> +		{ .rmap = &map->component_map.ras, &regs->ras },
>  	};
>  	int i;
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 52bd77d8e22a..cf3d8d0aaf22 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -32,6 +32,7 @@
>  #define   CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24)
>  #define CXL_CM_CAP_PTR_MASK GENMASK(31, 20)
>  
> +#define   CXL_CM_CAP_CAP_ID_RAS 0x2
>  #define   CXL_CM_CAP_CAP_ID_HDM 0x5
>  #define   CXL_CM_CAP_CAP_HDM_VERSION 1
>  
> @@ -64,6 +65,21 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr)
>  	return val ? val * 2 : 1;
>  }
>  
> +/* RAS Registers CXL 2.0 8.2.5.9 CXL RAS Capability Structure */
> +#define CXL_RAS_UNCORRECTABLE_STATUS_OFFSET 0x0
> +#define   CXL_RAS_UNCORRECTABLE_STATUS_MASK (GENMASK(16, 14) | GENMASK(11, 0))
> +#define CXL_RAS_UNCORRECTABLE_MASK_OFFSET 0x4
> +#define   CXL_RAS_UNCORRECTABLE_MASK_MASK (GENMASK(16, 14) | GENMASK(11, 0))
> +#define CXL_RAS_UNCORRECTABLE_SEVERITY_OFFSET 0x8
> +#define   CXL_RAS_UNCORRECTABLE_SEVERITY_MASK (GENMASK(16, 14) | GENMASK(11, 0))
> +#define CXL_RAS_CORRECTABLE_STATUS_OFFSET 0xC
> +#define   CXL_RAS_CORRECTABLE_STATUS_MASK GENMASK(6, 0)
> +#define CXL_RAS_CORRECTABLE_MASK_OFFSET 0x10
> +#define   CXL_RAS_CORRECTABLE_MASK_MASK GENMASK(6, 0)
> +#define CXL_RAS_CAP_CONTROL_OFFSET 0x14
> +#define CXL_RAS_HEADER_LOG_OFFSET 0x18
> +#define CXL_RAS_CAPABILITY_LENGTH 0x58
> +
>  /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
>  #define CXLDEV_CAP_ARRAY_OFFSET 0x0
>  #define   CXLDEV_CAP_ARRAY_CAP_ID 0
> @@ -98,9 +114,11 @@ struct cxl_regs {
>  	/*
>  	 * Common set of CXL Component register block base pointers
>  	 * @hdm_decoder: CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure
> +	 * @ras: CXL 2.0 8.2.5.9 CXL RAS Capability Structure
>  	 */
>  	struct_group_tagged(cxl_component_regs, component,
>  		void __iomem *hdm_decoder;
> +		void __iomem *ras;
>  	);
>  	/*
>  	 * Common set of CXL Device register block base pointers
> @@ -122,6 +140,7 @@ struct cxl_reg_map {
>  
>  struct cxl_component_reg_map {
>  	struct cxl_reg_map hdm_decoder;
> +	struct cxl_reg_map ras;
>  };
>  
>  struct cxl_device_reg_map {
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index d8361331a013..bde8929450f0 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -310,6 +310,9 @@ static int cxl_probe_regs(struct pci_dev *pdev, struct cxl_register_map *map)
>  			return -ENXIO;
>  		}
>  
> +		if (!comp_map->ras.valid)
> +			dev_dbg(dev, "RAS registers not found\n");
> +
>  		dev_dbg(dev, "Set up component registers\n");
>  		break;
>  	case CXL_REGLOC_RBI_MEMDEV:
> @@ -580,6 +583,11 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	cxlds->component_reg_phys = map.resource;
>  
> +	rc = cxl_map_component_regs(&pdev->dev, &cxlds->regs.component,
> +				    &map, BIT(CXL_CM_CAP_CAP_ID_RAS));
> +	if (rc)
> +		dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
> +
>  	rc = cxl_pci_setup_mailbox(cxlds);
>  	if (rc)
>  		return rc;
> 

[PATCH 8/8] cxl/pci: Add (hopeful) error handling support

[Dan Williams]

Add nominal error handling that tears down CXL.mem in response to error
notifications that imply a device reset. Given some CXL.mem may be
operating as System RAM, there is a high likelihood that these error
events are fatal. However, if the system survives the notification the
expectation is that the driver behavior is equivalent to a hot-unplug
and re-plug of an endpoint.

Note that this does not change the mask values from the default. That
awaits CXL _OSC support to determine whether platform firmware is in
control of the mask registers.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/memdev.c |    1 
 drivers/cxl/cxlmem.h      |    2 +
 drivers/cxl/pci.c         |  109 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 112 insertions(+)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 1f76b28f9826..223d512790e1 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -341,6 +341,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
 	 * needed as this is ordered with cdev_add() publishing the device.
 	 */
 	cxlmd->cxlds = cxlds;
+	cxlds->cxlmd = cxlmd;
 
 	cdev = &cxlmd->cdev;
 	rc = cdev_device_add(cdev, dev);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 5d33ce24fe09..f58e16951414 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -117,6 +117,7 @@ struct cxl_endpoint_dvsec_info {
  * Currently only memory devices are represented.
  *
  * @dev: The device associated with this CXL state
+ * @cxlmd: The device representing the CXL.mem capabilities of @dev
  * @regs: Parsed register blocks
  * @cxl_dvsec: Offset to the PCIe device DVSEC
  * @payload_size: Size of space for payload
@@ -148,6 +149,7 @@ struct cxl_endpoint_dvsec_info {
  */
 struct cxl_dev_state {
 	struct device *dev;
+	struct cxl_memdev *cxlmd;
 
 	struct cxl_regs regs;
 	int cxl_dvsec;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index bde8929450f0..823cbfa093fa 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -8,6 +8,7 @@
 #include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/pci.h>
+#include <linux/aer.h>
 #include <linux/io.h>
 #include "cxlmem.h"
 #include "cxlpci.h"
@@ -533,6 +534,11 @@ static void cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
 	info->ranges = __cxl_dvsec_ranges(cxlds, info);
 }
 
+static void disable_aer(void *pdev)
+{
+	pci_disable_pcie_error_reporting(pdev);
+}
+
 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct cxl_register_map map;
@@ -554,6 +560,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	cxlds = cxl_dev_state_create(&pdev->dev);
 	if (IS_ERR(cxlds))
 		return PTR_ERR(cxlds);
+	pci_set_drvdata(pdev, cxlds);
 
 	cxlds->serial = pci_get_dsn(pdev);
 	cxlds->cxl_dvsec = pci_find_dvsec_capability(
@@ -610,6 +617,14 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
 
+	if (cxlds->regs.ras) {
+		pci_enable_pcie_error_reporting(pdev);
+		rc = devm_add_action_or_reset(&pdev->dev, disable_aer, pdev);
+		if (rc)
+			return rc;
+	}
+	pci_save_state(pdev);
+
 	if (range_len(&cxlds->pmem_range) && IS_ENABLED(CONFIG_CXL_PMEM))
 		rc = devm_cxl_add_nvdimm(&pdev->dev, cxlmd);
 
@@ -623,10 +638,104 @@ static const struct pci_device_id cxl_mem_pci_tbl[] = {
 };
 MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl);
 
+/*
+ * Log the state of the RAS status registers and prepare them to log the
+ * next error status.
+ */
+static void cxl_report_and_clear(struct cxl_dev_state *cxlds)
+{
+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
+	struct device *dev = &cxlmd->dev;
+	void __iomem *addr;
+	u32 status;
+
+	if (!cxlds->regs.ras)
+		return;
+
+	addr = cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_STATUS_OFFSET;
+	status = readl(addr);
+	if (status & CXL_RAS_UNCORRECTABLE_STATUS_MASK) {
+		dev_warn(cxlds->dev, "%s: uncorrectable status: %#08x\n",
+			 dev_name(dev), status);
+		writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
+	}
+
+	addr = cxlds->regs.ras + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
+	status = readl(addr);
+	if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
+		dev_warn(cxlds->dev, "%s: correctable status: %#08x\n",
+			 dev_name(dev), status);
+		writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
+	}
+}
+
+static pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
+					   pci_channel_state_t state)
+{
+	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
+	struct device *dev = &cxlmd->dev;
+
+	/*
+	 * A frozen channel indicates an impending reset which is fatal to
+	 * CXL.mem operation, and will likely crash the system. On the off
+	 * chance the situation is recoverable dump the status of the RAS
+	 * capability registers and bounce the active state of the memdev.
+	 */
+	cxl_report_and_clear(cxlds);
+
+	switch (state) {
+	case pci_channel_io_normal:
+		return PCI_ERS_RESULT_CAN_RECOVER;
+	case pci_channel_io_frozen:
+		dev_warn(&pdev->dev,
+			 "%s: frozen state error detected, disable CXL.mem\n",
+			 dev_name(dev));
+		device_release_driver(dev);
+		return PCI_ERS_RESULT_NEED_RESET;
+	case pci_channel_io_perm_failure:
+		dev_warn(&pdev->dev,
+			 "failure state error detected, request disconnect\n");
+		return PCI_ERS_RESULT_DISCONNECT;
+	}
+	return PCI_ERS_RESULT_NEED_RESET;
+}
+
+static pci_ers_result_t cxl_slot_reset(struct pci_dev *pdev)
+{
+	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
+	struct device *dev = &cxlmd->dev;
+
+	dev_info(&pdev->dev, "%s: restart CXL.mem after slot reset\n",
+		 dev_name(dev));
+	pci_restore_state(pdev);
+	if (device_attach(dev) <= 0)
+		return PCI_ERS_RESULT_DISCONNECT;
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void cxl_error_resume(struct pci_dev *pdev)
+{
+	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
+	struct device *dev = &cxlmd->dev;
+
+	dev_info(&pdev->dev, "%s: error resume %s\n", dev_name(dev),
+		 dev->driver ? "successful" : "failed");
+}
+
+static const struct pci_error_handlers cxl_error_handlers = {
+	.error_detected	= cxl_error_detected,
+	.slot_reset	= cxl_slot_reset,
+	.resume		= cxl_error_resume,
+};
+
 static struct pci_driver cxl_pci_driver = {
 	.name			= KBUILD_MODNAME,
 	.id_table		= cxl_mem_pci_tbl,
 	.probe			= cxl_pci_probe,
+	.err_handler		= &cxl_error_handlers,
 	.driver	= {
 		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
 	},

Re: [PATCH 8/8] cxl/pci: Add (hopeful) error handling support

[Jonathan Cameron]

On Tue, 15 Mar 2022 21:14:24 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Add nominal error handling that tears down CXL.mem in response to error
> notifications that imply a device reset. Given some CXL.mem may be
> operating as System RAM, there is a high likelihood that these error
> events are fatal. However, if the system survives the notification the
> expectation is that the driver behavior is equivalent to a hot-unplug
> and re-plug of an endpoint.
> 
> Note that this does not change the mask values from the default. That
> awaits CXL _OSC support to determine whether platform firmware is in
> control of the mask registers.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I'm far from an expert in PCI error handling...  I've asked one of our RAS
folk to take a quick look so he may well reply as well.

With that in mind...

> ---
>  drivers/cxl/core/memdev.c |    1 
>  drivers/cxl/cxlmem.h      |    2 +
>  drivers/cxl/pci.c         |  109 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 112 insertions(+)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 1f76b28f9826..223d512790e1 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -341,6 +341,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
>  	 * needed as this is ordered with cdev_add() publishing the device.
>  	 */
>  	cxlmd->cxlds = cxlds;
> +	cxlds->cxlmd = cxlmd;
>  
>  	cdev = &cxlmd->cdev;
>  	rc = cdev_device_add(cdev, dev);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 5d33ce24fe09..f58e16951414 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -117,6 +117,7 @@ struct cxl_endpoint_dvsec_info {
>   * Currently only memory devices are represented.
>   *
>   * @dev: The device associated with this CXL state
> + * @cxlmd: The device representing the CXL.mem capabilities of @dev
>   * @regs: Parsed register blocks
>   * @cxl_dvsec: Offset to the PCIe device DVSEC
>   * @payload_size: Size of space for payload
> @@ -148,6 +149,7 @@ struct cxl_endpoint_dvsec_info {
>   */
>  struct cxl_dev_state {
>  	struct device *dev;
> +	struct cxl_memdev *cxlmd;

This is only used I think to access the cxlmd->dev. Perhaps better to pass
the dev and avoid having a reference to the memdev in here?

>  
>  	struct cxl_regs regs;
>  	int cxl_dvsec;
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index bde8929450f0..823cbfa093fa 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -8,6 +8,7 @@
>  #include <linux/mutex.h>
>  #include <linux/list.h>
>  #include <linux/pci.h>
> +#include <linux/aer.h>
>  #include <linux/io.h>
>  #include "cxlmem.h"
>  #include "cxlpci.h"
> @@ -533,6 +534,11 @@ static void cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
>  	info->ranges = __cxl_dvsec_ranges(cxlds, info);
>  }
>  
> +static void disable_aer(void *pdev)
> +{
> +	pci_disable_pcie_error_reporting(pdev);
> +}
> +
>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct cxl_register_map map;
> @@ -554,6 +560,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	cxlds = cxl_dev_state_create(&pdev->dev);
>  	if (IS_ERR(cxlds))
>  		return PTR_ERR(cxlds);
> +	pci_set_drvdata(pdev, cxlds);
>  
>  	cxlds->serial = pci_get_dsn(pdev);
>  	cxlds->cxl_dvsec = pci_find_dvsec_capability(
> @@ -610,6 +617,14 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (IS_ERR(cxlmd))
>  		return PTR_ERR(cxlmd);
>  
> +	if (cxlds->regs.ras) {
> +		pci_enable_pcie_error_reporting(pdev);
> +		rc = devm_add_action_or_reset(&pdev->dev, disable_aer, pdev);
> +		if (rc)
> +			return rc;
> +	}
> +	pci_save_state(pdev);
> +
>  	if (range_len(&cxlds->pmem_range) && IS_ENABLED(CONFIG_CXL_PMEM))
>  		rc = devm_cxl_add_nvdimm(&pdev->dev, cxlmd);
>  
> @@ -623,10 +638,104 @@ static const struct pci_device_id cxl_mem_pci_tbl[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl);
>  
> +/*
> + * Log the state of the RAS status registers and prepare them to log the
> + * next error status.
> + */
> +static void cxl_report_and_clear(struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_memdev *cxlmd = cxlds->cxlmd;
> +	struct device *dev = &cxlmd->dev;
> +	void __iomem *addr;
> +	u32 status;
> +
> +	if (!cxlds->regs.ras)
> +		return;
> +
> +	addr = cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_STATUS_OFFSET;
> +	status = readl(addr);
> +	if (status & CXL_RAS_UNCORRECTABLE_STATUS_MASK) {
> +		dev_warn(cxlds->dev, "%s: uncorrectable status: %#08x\n",
> +			 dev_name(dev), status);
> +		writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
> +	}
> +
> +	addr = cxlds->regs.ras + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
> +	status = readl(addr);
> +	if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
> +		dev_warn(cxlds->dev, "%s: correctable status: %#08x\n",
> +			 dev_name(dev), status);
> +		writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
> +	}
> +}
> +
> +static pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> +					   pci_channel_state_t state)
> +{
> +	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> +	struct cxl_memdev *cxlmd = cxlds->cxlmd;
> +	struct device *dev = &cxlmd->dev;

Perhaps a more informative name than dev given we have several devs
involved here?

> +
> +	/*
> +	 * A frozen channel indicates an impending reset which is fatal to
> +	 * CXL.mem operation, and will likely crash the system. On the off
> +	 * chance the situation is recoverable dump the status of the RAS
> +	 * capability registers and bounce the active state of the memdev.
> +	 */
> +	cxl_report_and_clear(cxlds);
> +
> +	switch (state) {
> +	case pci_channel_io_normal:
> +		return PCI_ERS_RESULT_CAN_RECOVER;
> +	case pci_channel_io_frozen:
> +		dev_warn(&pdev->dev,
> +			 "%s: frozen state error detected, disable CXL.mem\n",
> +			 dev_name(dev));
> +		device_release_driver(dev);
> +		return PCI_ERS_RESULT_NEED_RESET;
> +	case pci_channel_io_perm_failure:
> +		dev_warn(&pdev->dev,
> +			 "failure state error detected, request disconnect\n");
> +		return PCI_ERS_RESULT_DISCONNECT;
> +	}
> +	return PCI_ERS_RESULT_NEED_RESET;
> +}
> +
> +static pci_ers_result_t cxl_slot_reset(struct pci_dev *pdev)
> +{
> +	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> +	struct cxl_memdev *cxlmd = cxlds->cxlmd;
> +	struct device *dev = &cxlmd->dev;
> +
> +	dev_info(&pdev->dev, "%s: restart CXL.mem after slot reset\n",
> +		 dev_name(dev));
> +	pci_restore_state(pdev);
> +	if (device_attach(dev) <= 0)
> +		return PCI_ERS_RESULT_DISCONNECT;
> +	return PCI_ERS_RESULT_RECOVERED;
> +}
> +
> +static void cxl_error_resume(struct pci_dev *pdev)
> +{
> +	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> +	struct cxl_memdev *cxlmd = cxlds->cxlmd;
> +	struct device *dev = &cxlmd->dev;
> +
> +	dev_info(&pdev->dev, "%s: error resume %s\n", dev_name(dev),
> +		 dev->driver ? "successful" : "failed");
> +}
> +
> +static const struct pci_error_handlers cxl_error_handlers = {
> +	.error_detected	= cxl_error_detected,
> +	.slot_reset	= cxl_slot_reset,
> +	.resume		= cxl_error_resume,
> +};
> +
>  static struct pci_driver cxl_pci_driver = {
>  	.name			= KBUILD_MODNAME,
>  	.id_table		= cxl_mem_pci_tbl,
>  	.probe			= cxl_pci_probe,
> +	.err_handler		= &cxl_error_handlers,
>  	.driver	= {
>  		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
>  	},
> 

RE: [PATCH 8/8] cxl/pci: Add (hopeful) error handling support

[Shiju Jose]

Hi Dan,

>-----Original Message-----
>From: Dan Williams <dan.j.williams@intel.com>
>Sent: 16 March 2022 04:14
>To: linux-cxl@vger.kernel.org
>Cc: ben.widawsky@intel.com; vishal.l.verma@intel.com;
>alison.schofield@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; ira.weiny@intel.com; linux-
>pci@vger.kernel.org
>Subject: [PATCH 8/8] cxl/pci: Add (hopeful) error handling support
>
>Add nominal error handling that tears down CXL.mem in response to error
>notifications that imply a device reset. Given some CXL.mem may be
>operating as System RAM, there is a high likelihood that these error events
>are fatal. However, if the system survives the notification the expectation is
>that the driver behavior is equivalent to a hot-unplug and re-plug of an
>endpoint.
>
>Note that this does not change the mask values from the default. That awaits
>CXL _OSC support to determine whether platform firmware is in control of the
>mask registers.
>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>---
> drivers/cxl/core/memdev.c |    1
> drivers/cxl/cxlmem.h      |    2 +
> drivers/cxl/pci.c         |  109
>+++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 112 insertions(+)
>
>diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index
>1f76b28f9826..223d512790e1 100644
>--- a/drivers/cxl/core/memdev.c
>+++ b/drivers/cxl/core/memdev.c
>@@ -341,6 +341,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct
>cxl_dev_state *cxlds)
> 	 * needed as this is ordered with cdev_add() publishing the device.
> 	 */
> 	cxlmd->cxlds = cxlds;
>+	cxlds->cxlmd = cxlmd;
>
> 	cdev = &cxlmd->cdev;
> 	rc = cdev_device_add(cdev, dev);
>diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
>5d33ce24fe09..f58e16951414 100644
>--- a/drivers/cxl/cxlmem.h
>+++ b/drivers/cxl/cxlmem.h
>@@ -117,6 +117,7 @@ struct cxl_endpoint_dvsec_info {
>  * Currently only memory devices are represented.
>  *
>  * @dev: The device associated with this CXL state
>+ * @cxlmd: The device representing the CXL.mem capabilities of @dev
>  * @regs: Parsed register blocks
>  * @cxl_dvsec: Offset to the PCIe device DVSEC
>  * @payload_size: Size of space for payload @@ -148,6 +149,7 @@ struct
>cxl_endpoint_dvsec_info {
>  */
> struct cxl_dev_state {
> 	struct device *dev;
>+	struct cxl_memdev *cxlmd;
>
> 	struct cxl_regs regs;
> 	int cxl_dvsec;
>diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
>bde8929450f0..823cbfa093fa 100644
>--- a/drivers/cxl/pci.c
>+++ b/drivers/cxl/pci.c
>@@ -8,6 +8,7 @@
> #include <linux/mutex.h>
> #include <linux/list.h>
> #include <linux/pci.h>
>+#include <linux/aer.h>
> #include <linux/io.h>
> #include "cxlmem.h"
> #include "cxlpci.h"
>@@ -533,6 +534,11 @@ static void cxl_dvsec_ranges(struct cxl_dev_state
>*cxlds)
> 	info->ranges = __cxl_dvsec_ranges(cxlds, info);  }
>
>+static void disable_aer(void *pdev)
>+{
>+	pci_disable_pcie_error_reporting(pdev);
>+}
>+
> static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>{
> 	struct cxl_register_map map;
>@@ -554,6 +560,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const
>struct pci_device_id *id)
> 	cxlds = cxl_dev_state_create(&pdev->dev);
> 	if (IS_ERR(cxlds))
> 		return PTR_ERR(cxlds);
>+	pci_set_drvdata(pdev, cxlds);
>
> 	cxlds->serial = pci_get_dsn(pdev);
> 	cxlds->cxl_dvsec = pci_find_dvsec_capability( @@ -610,6 +617,14 @@
>static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> 	if (IS_ERR(cxlmd))
> 		return PTR_ERR(cxlmd);
>
>+	if (cxlds->regs.ras) {
>+		pci_enable_pcie_error_reporting(pdev);
>+		rc = devm_add_action_or_reset(&pdev->dev, disable_aer,
>pdev);
>+		if (rc)
>+			return rc;
>+	}
>+	pci_save_state(pdev);
>+
> 	if (range_len(&cxlds->pmem_range) &&
>IS_ENABLED(CONFIG_CXL_PMEM))
> 		rc = devm_cxl_add_nvdimm(&pdev->dev, cxlmd);
>
>@@ -623,10 +638,104 @@ static const struct pci_device_id cxl_mem_pci_tbl[]
>= {  };  MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl);
>
>+/*
>+ * Log the state of the RAS status registers and prepare them to log
>+the
>+ * next error status.
>+ */
>+static void cxl_report_and_clear(struct cxl_dev_state *cxlds) {
>+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
>+	struct device *dev = &cxlmd->dev;
>+	void __iomem *addr;
>+	u32 status;
>+
>+	if (!cxlds->regs.ras)
>+		return;
In the cxl_error_detected () may need to return PCI_ERS_RESULT_NONE
for the following cases, if exist,
1. if (!cxlds->regs.ras),
2. if any errors would be reported during the dev initialization. 

>+
>+	addr = cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_STATUS_OFFSET;
>+	status = readl(addr);
>+	if (status & CXL_RAS_UNCORRECTABLE_STATUS_MASK) {
>+		dev_warn(cxlds->dev, "%s: uncorrectable status: %#08x\n",
>+			 dev_name(dev), status);
>+		writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK,
>addr);
>+	}
For the uncorrectable non-fatal errors, if any, may need to return PCI_ERS_RESULT_NEED_RESET
to trigger the slot reset to prevent more serious issues later. For this case the state would be
"pci_channel_io_normal".
 
>+
>+	addr = cxlds->regs.ras + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
>+	status = readl(addr);
>+	if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
>+		dev_warn(cxlds->dev, "%s: correctable status: %#08x\n",
>+			 dev_name(dev), status);
>+		writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK,
>addr);
>+	}
>+}
>+
>+static pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>+					   pci_channel_state_t state)
>+{
>+	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
>+	struct device *dev = &cxlmd->dev;
>+
>+	/*
>+	 * A frozen channel indicates an impending reset which is fatal to
>+	 * CXL.mem operation, and will likely crash the system. On the off
>+	 * chance the situation is recoverable dump the status of the RAS
>+	 * capability registers and bounce the active state of the memdev.
>+	 */
>+	cxl_report_and_clear(cxlds);
>+
>+	switch (state) {
>+	case pci_channel_io_normal:
>+		return PCI_ERS_RESULT_CAN_RECOVER;
>+	case pci_channel_io_frozen:
>+		dev_warn(&pdev->dev,
>+			 "%s: frozen state error detected, disable
>CXL.mem\n",
>+			 dev_name(dev));
>+		device_release_driver(dev);
>+		return PCI_ERS_RESULT_NEED_RESET;
>+	case pci_channel_io_perm_failure:
>+		dev_warn(&pdev->dev,
>+			 "failure state error detected, request disconnect\n");
>+		return PCI_ERS_RESULT_DISCONNECT;
>+	}
>+	return PCI_ERS_RESULT_NEED_RESET;
>+}
>+
>+static pci_ers_result_t cxl_slot_reset(struct pci_dev *pdev) {
>+	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
>+	struct device *dev = &cxlmd->dev;
>+
>+	dev_info(&pdev->dev, "%s: restart CXL.mem after slot reset\n",
>+		 dev_name(dev));
>+	pci_restore_state(pdev);
1. Do we need to call pci_save_state(pdev) here after the reset? though pci_save_state(pdev) is being invoked in the  
cxl_pci_probe().

>+	if (device_attach(dev) <= 0)
>+		return PCI_ERS_RESULT_DISCONNECT;
My understanding is that pci_disable_pcie_error_reporting(pdev) would be called
in the disable_aer () in the reset, 
pci_enable_pcie_error_reporting(pdev) may need to call here after the reset?
 
>+	return PCI_ERS_RESULT_RECOVERED;
>+}
>+
>+static void cxl_error_resume(struct pci_dev *pdev) {
>+	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
>+	struct device *dev = &cxlmd->dev;
>+
>+	dev_info(&pdev->dev, "%s: error resume %s\n", dev_name(dev),
>+		 dev->driver ? "successful" : "failed"); }
>+
>+static const struct pci_error_handlers cxl_error_handlers = {
>+	.error_detected	= cxl_error_detected,
>+	.slot_reset	= cxl_slot_reset,
>+	.resume		= cxl_error_resume,
If the FLR (Function level reset) supported, please add the corresponding callback functions
reset_prepare(..) and reset_done(..).

>+};
>+
> static struct pci_driver cxl_pci_driver = {
> 	.name			= KBUILD_MODNAME,
> 	.id_table		= cxl_mem_pci_tbl,
> 	.probe			= cxl_pci_probe,
>+	.err_handler		= &cxl_error_handlers,
> 	.driver	= {
> 		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
> 	},


Thanks,
Shiju

Re: [PATCH 8/8] cxl/pci: Add (hopeful) error handling support

[Dan Williams]

On Fri, Mar 18, 2022 at 2:42 AM Shiju Jose <shiju.jose@huawei.com> wrote:
>
> Hi Dan,

Hi, thanks for taking a look at this.

>
> >-----Original Message-----
> >From: Dan Williams <dan.j.williams@intel.com>
> >Sent: 16 March 2022 04:14
> >To: linux-cxl@vger.kernel.org
> >Cc: ben.widawsky@intel.com; vishal.l.verma@intel.com;
> >alison.schofield@intel.com; Jonathan Cameron
> ><jonathan.cameron@huawei.com>; ira.weiny@intel.com; linux-
> >pci@vger.kernel.org
> >Subject: [PATCH 8/8] cxl/pci: Add (hopeful) error handling support
> >
> >Add nominal error handling that tears down CXL.mem in response to error
> >notifications that imply a device reset. Given some CXL.mem may be
> >operating as System RAM, there is a high likelihood that these error events
> >are fatal. However, if the system survives the notification the expectation is
> >that the driver behavior is equivalent to a hot-unplug and re-plug of an
> >endpoint.
> >
> >Note that this does not change the mask values from the default. That awaits
> >CXL _OSC support to determine whether platform firmware is in control of the
> >mask registers.
> >
> >Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >---
> > drivers/cxl/core/memdev.c |    1
> > drivers/cxl/cxlmem.h      |    2 +
> > drivers/cxl/pci.c         |  109
> >+++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 112 insertions(+)
> >
> >diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index
> >1f76b28f9826..223d512790e1 100644
> >--- a/drivers/cxl/core/memdev.c
> >+++ b/drivers/cxl/core/memdev.c
> >@@ -341,6 +341,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct
> >cxl_dev_state *cxlds)
> >        * needed as this is ordered with cdev_add() publishing the device.
> >        */
> >       cxlmd->cxlds = cxlds;
> >+      cxlds->cxlmd = cxlmd;
> >
> >       cdev = &cxlmd->cdev;
> >       rc = cdev_device_add(cdev, dev);
> >diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
> >5d33ce24fe09..f58e16951414 100644
> >--- a/drivers/cxl/cxlmem.h
> >+++ b/drivers/cxl/cxlmem.h
> >@@ -117,6 +117,7 @@ struct cxl_endpoint_dvsec_info {
> >  * Currently only memory devices are represented.
> >  *
> >  * @dev: The device associated with this CXL state
> >+ * @cxlmd: The device representing the CXL.mem capabilities of @dev
> >  * @regs: Parsed register blocks
> >  * @cxl_dvsec: Offset to the PCIe device DVSEC
> >  * @payload_size: Size of space for payload @@ -148,6 +149,7 @@ struct
> >cxl_endpoint_dvsec_info {
> >  */
> > struct cxl_dev_state {
> >       struct device *dev;
> >+      struct cxl_memdev *cxlmd;
> >
> >       struct cxl_regs regs;
> >       int cxl_dvsec;
> >diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
> >bde8929450f0..823cbfa093fa 100644
> >--- a/drivers/cxl/pci.c
> >+++ b/drivers/cxl/pci.c
> >@@ -8,6 +8,7 @@
> > #include <linux/mutex.h>
> > #include <linux/list.h>
> > #include <linux/pci.h>
> >+#include <linux/aer.h>
> > #include <linux/io.h>
> > #include "cxlmem.h"
> > #include "cxlpci.h"
> >@@ -533,6 +534,11 @@ static void cxl_dvsec_ranges(struct cxl_dev_state
> >*cxlds)
> >       info->ranges = __cxl_dvsec_ranges(cxlds, info);  }
> >
> >+static void disable_aer(void *pdev)
> >+{
> >+      pci_disable_pcie_error_reporting(pdev);
> >+}
> >+
> > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >{
> >       struct cxl_register_map map;
> >@@ -554,6 +560,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const
> >struct pci_device_id *id)
> >       cxlds = cxl_dev_state_create(&pdev->dev);
> >       if (IS_ERR(cxlds))
> >               return PTR_ERR(cxlds);
> >+      pci_set_drvdata(pdev, cxlds);
> >
> >       cxlds->serial = pci_get_dsn(pdev);
> >       cxlds->cxl_dvsec = pci_find_dvsec_capability( @@ -610,6 +617,14 @@
> >static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >       if (IS_ERR(cxlmd))
> >               return PTR_ERR(cxlmd);
> >
> >+      if (cxlds->regs.ras) {
> >+              pci_enable_pcie_error_reporting(pdev);
> >+              rc = devm_add_action_or_reset(&pdev->dev, disable_aer,
> >pdev);
> >+              if (rc)
> >+                      return rc;
> >+      }
> >+      pci_save_state(pdev);
> >+
> >       if (range_len(&cxlds->pmem_range) &&
> >IS_ENABLED(CONFIG_CXL_PMEM))
> >               rc = devm_cxl_add_nvdimm(&pdev->dev, cxlmd);
> >
> >@@ -623,10 +638,104 @@ static const struct pci_device_id cxl_mem_pci_tbl[]
> >= {  };  MODULE_DEVICE_TABLE(pci, cxl_mem_pci_tbl);
> >
> >+/*
> >+ * Log the state of the RAS status registers and prepare them to log
> >+the
> >+ * next error status.
> >+ */
> >+static void cxl_report_and_clear(struct cxl_dev_state *cxlds) {
> >+      struct cxl_memdev *cxlmd = cxlds->cxlmd;
> >+      struct device *dev = &cxlmd->dev;
> >+      void __iomem *addr;
> >+      u32 status;
> >+
> >+      if (!cxlds->regs.ras)
> >+              return;
> In the cxl_error_detected () may need to return PCI_ERS_RESULT_NONE
> for the following cases, if exist,
> 1. if (!cxlds->regs.ras),

Yes, but given that the RAS capability is mandatory for CXL devices
then I think the driver should just fail to register altogether if the
RAS register are not found / mapped.

> 2. if any errors would be reported during the dev initialization.

This can't happen. The err_handler callback process takes the
device_lock() which ensures that any initialization that has started
completes before the callback is invoked.

>
> >+
> >+      addr = cxlds->regs.ras + CXL_RAS_UNCORRECTABLE_STATUS_OFFSET;
> >+      status = readl(addr);
> >+      if (status & CXL_RAS_UNCORRECTABLE_STATUS_MASK) {
> >+              dev_warn(cxlds->dev, "%s: uncorrectable status: %#08x\n",
> >+                       dev_name(dev), status);
> >+              writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK,
> >addr);
> >+      }
> For the uncorrectable non-fatal errors, if any, may need to return PCI_ERS_RESULT_NEED_RESET
> to trigger the slot reset to prevent more serious issues later. For this case the state would be
> "pci_channel_io_normal".
>

Ah true, some pci_channel_io_normal recovery conditions still result
in reset, will fix.

> >+
> >+      addr = cxlds->regs.ras + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
> >+      status = readl(addr);
> >+      if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
> >+              dev_warn(cxlds->dev, "%s: correctable status: %#08x\n",
> >+                       dev_name(dev), status);
> >+              writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK,
> >addr);
> >+      }
> >+}
> >+
> >+static pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> >+                                         pci_channel_state_t state)
> >+{
> >+      struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> >+      struct cxl_memdev *cxlmd = cxlds->cxlmd;
> >+      struct device *dev = &cxlmd->dev;
> >+
> >+      /*
> >+       * A frozen channel indicates an impending reset which is fatal to
> >+       * CXL.mem operation, and will likely crash the system. On the off
> >+       * chance the situation is recoverable dump the status of the RAS
> >+       * capability registers and bounce the active state of the memdev.
> >+       */
> >+      cxl_report_and_clear(cxlds);
> >+
> >+      switch (state) {
> >+      case pci_channel_io_normal:
> >+              return PCI_ERS_RESULT_CAN_RECOVER;
> >+      case pci_channel_io_frozen:
> >+              dev_warn(&pdev->dev,
> >+                       "%s: frozen state error detected, disable
> >CXL.mem\n",
> >+                       dev_name(dev));
> >+              device_release_driver(dev);
> >+              return PCI_ERS_RESULT_NEED_RESET;
> >+      case pci_channel_io_perm_failure:
> >+              dev_warn(&pdev->dev,
> >+                       "failure state error detected, request disconnect\n");
> >+              return PCI_ERS_RESULT_DISCONNECT;
> >+      }
> >+      return PCI_ERS_RESULT_NEED_RESET;
> >+}
> >+
> >+static pci_ers_result_t cxl_slot_reset(struct pci_dev *pdev) {
> >+      struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> >+      struct cxl_memdev *cxlmd = cxlds->cxlmd;
> >+      struct device *dev = &cxlmd->dev;
> >+
> >+      dev_info(&pdev->dev, "%s: restart CXL.mem after slot reset\n",
> >+               dev_name(dev));
> >+      pci_restore_state(pdev);
> 1. Do we need to call pci_save_state(pdev) here after the reset? though pci_save_state(pdev) is being invoked in the
> cxl_pci_probe().

The save state after probe is sufficient, it does not need to be
snapshotted again as far as I can see.

>
> >+      if (device_attach(dev) <= 0)
> >+              return PCI_ERS_RESULT_DISCONNECT;
> My understanding is that pci_disable_pcie_error_reporting(pdev) would be called
> in the disable_aer () in the reset,
> pci_enable_pcie_error_reporting(pdev) may need to call here after the reset?

After the device is disconnected the driver needs to be reloaded to
recover AER operation.

>
> >+      return PCI_ERS_RESULT_RECOVERED;
> >+}
> >+
> >+static void cxl_error_resume(struct pci_dev *pdev) {
> >+      struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> >+      struct cxl_memdev *cxlmd = cxlds->cxlmd;
> >+      struct device *dev = &cxlmd->dev;
> >+
> >+      dev_info(&pdev->dev, "%s: error resume %s\n", dev_name(dev),
> >+               dev->driver ? "successful" : "failed"); }
> >+
> >+static const struct pci_error_handlers cxl_error_handlers = {
> >+      .error_detected = cxl_error_detected,
> >+      .slot_reset     = cxl_slot_reset,
> >+      .resume         = cxl_error_resume,
> If the FLR (Function level reset) supported, please add the corresponding callback functions
> reset_prepare(..) and reset_done(..).

No, FLR does not recover any of the errors reported via AER.

>
> >+};
> >+
> > static struct pci_driver cxl_pci_driver = {
> >       .name                   = KBUILD_MODNAME,
> >       .id_table               = cxl_mem_pci_tbl,
> >       .probe                  = cxl_pci_probe,
> >+      .err_handler            = &cxl_error_handlers,
> >       .driver = {
> >               .probe_type     = PROBE_PREFER_ASYNCHRONOUS,
> >       },
>
>
> Thanks,
> Shiju

Re: [PATCH 0/8] cxl/pci: Add fundamental error handling

[Dan Williams]

On Tue, Mar 15, 2022 at 9:14 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Add a 'struct pci_error_handlers' instance for the cxl_pci driver.
> Section 8.2.5.9 "CXL RAS Capability Structure" of the CXL 2.0
> specification defines the error sources considered in this
> implementation. The RAS Capability Structure defines protocol, link and
> internal errors which are distinct from memory poison errors that are
> conveyed via direct consumption and/or media scanning.
>
> The errors reported by the RAS registers are categorized into
> correctable and uncorrectable errors, where the uncorrectable errors are
> optionally steered to either fatal or non-fatal AER events. Table 224
> "Device Specific Error Reporting and Nomenclature Guidelines" in the CXL
> 2.0 specification outlines that the remediation for uncorrectable errors
> is a reset to recover. This matches how the Linux PCIe AER core treats
> uncorrectable errors as occasions to reset the device to recover
> operation.
>
> While the specification notes "CXL Reset" or "Secondary Bus Reset" as
> theoretical recovery options, they are not feasible in practice since
> in-flight CXL.mem operations may not terminate and cause knock-on system
> fatal events. Reset is only reliable for recovering CXL.io, it is not
> reliable for recovering CXL.mem. Assuming the system survives, a reset
> causes CXL.mem operation to restart from scratch.
>
> The "ECN: Error Isolation on CXL.mem and CXL.cache" [1] document
> recognizes the CXL Reset vs CXL.mem operational conflict and helps to at
> least provide a mechanism for the Root Port to terminate in flight
> CXL.mem operations with completions. That still poses problems in
> practice if the kernel is running out of "System RAM" backed by the CXL
> device and poison is used to convey the data lost to the protocol error.
>
> Regardless of whether the reset and restart of CXL.mem operations is
> feasible / successful, the logging is still useful. So, the
> implementation reads, reports, and clears the status in the RAS
> Capability Structure registers, and it notifies the 'struct cxl_memdev'
> associated with the given PCIe endpoint to reattach to its driver over
> the reset so that the HDM decoder configuration can be reconstructed.
>
> The first half of the series reworks component register mapping so that
> the cxl_pci driver can own the RAS Capability while the cxl_port driver
> continues to own the HDM Decoder Capability. The last half implements
> the RAS Capability Structure mapping and reporting via 'struct
> pci_error_handlers'.
>
> [1]: https://www.computeexpresslink.org/spec-landing
>
> ---
>
>
> Dan Williams (8):
>       cxl/pci: Cleanup repeated code in cxl_probe_regs() helpers
>       cxl/pci: Cleanup cxl_map_device_regs()
>       cxl/pci: Kill cxl_map_regs()
>       cxl/core/regs: Make cxl_map_{component,device}_regs() device generic
>       cxl/port: Limit the port driver to just the HDM Decoder Capability
>       cxl/pci: Prepare for mapping RAS Capability Structure
>       cxl/pci: Find and map the RAS Capability Structure
>       cxl/pci: Add (hopeful) error handling support
>
>
>  drivers/cxl/core/hdm.c    |   33 +++++----
>  drivers/cxl/core/memdev.c |    1
>  drivers/cxl/core/pci.c    |    3 -
>  drivers/cxl/core/port.c   |    2 -
>  drivers/cxl/core/regs.c   |  172 ++++++++++++++++++++++++++-------------------
>  drivers/cxl/cxl.h         |   36 +++++++--
>  drivers/cxl/cxlmem.h      |    2 +
>  drivers/cxl/cxlpci.h      |    9 --
>  drivers/cxl/pci.c         |  163 ++++++++++++++++++++++++++++++++-----------
>  9 files changed, 273 insertions(+), 148 deletions(-)
>
> base-commit: 74be98774dfbc5b8b795db726bd772e735d2edd4

Apologies, wrong base-commit, this series is based on that commit + this series:

https://lore.kernel.org/linux-cxl/164730733718.3806189.9721916820488234094.stgit@dwillia2-desk3.amr.corp.intel.com/