[PATCH 0/5] cxl/hdm: Decoder enumeration fixes

[PATCH 0/5] cxl/hdm: Decoder enumeration fixes

[Dan Williams]

While testing region autodetect with physical hardware a few fixes fell
out. The most interesting being evidence that a device is sensitive to
8-byte reads of 2 consecutive 4-byte registers. The other is a long
reported issue by Jonathan on how "passthrough" decoders are detected,
and having an example with physical hardware to reinforce the
observation from QEMU.

The rest are ancillary fixes and new debug messages.

---

Dan Williams (5):
      cxl/hdm: Fail upon detecting 0-sized decoders
      cxl/hdm: Use 4-byte reads to retrieve HDM decoder base+limit
      cxl/core: Drop unused io-64-nonatomic-lo-hi.h
      cxl/port: Scan single-target ports for decoders
      cxl/hdm: Add more HDM decoder debug messages at startup


 drivers/cxl/core/hdm.c  |   52 ++++++++++++++++++++++++++++++++++++-----------
 drivers/cxl/core/mbox.c |    1 -
 drivers/cxl/core/port.c |    1 -
 drivers/cxl/port.c      |   18 ++++++++++++----
 4 files changed, 53 insertions(+), 19 deletions(-)

base-commit: 24b18197184ac39bb8566fb82c0bf788bcd0d45b

[PATCH 1/5] cxl/hdm: Fail upon detecting 0-sized decoders

[Dan Williams]

Decoders committed with 0-size lead to later crashes on shutdown as
__cxl_dpa_release() assumes a 'struct resource' has been established in
the in 'cxlds->dpa_res'. Just fail the driver load in this instance
since there are deeper problems with the enumeration or the setup when
this happens.

Fixes: 9c57cde0dcbd ("cxl/hdm: Enumerate allocated DPA")
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 02cc2c38b44b..35b338b716fe 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -269,8 +269,11 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
 
 	lockdep_assert_held_write(&cxl_dpa_rwsem);
 
-	if (!len)
-		goto success;
+	if (!len) {
+		dev_warn(dev, "decoder%d.%d: empty reservation attempted\n",
+			 port->id, cxled->cxld.id);
+		return -EINVAL;
+	}
 
 	if (cxled->dpa_res) {
 		dev_dbg(dev, "decoder%d.%d: existing allocation %pr assigned\n",
@@ -323,7 +326,6 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
 		cxled->mode = CXL_DECODER_MIXED;
 	}
 
-success:
 	port->hdm_end++;
 	get_device(&cxled->cxld.dev);
 	return 0;
@@ -833,6 +835,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 				 port->id, cxld->id);
 			return -ENXIO;
 		}
+
+		if (size == 0) {
+			dev_warn(&port->dev,
+				 "decoder%d.%d: Committed with zero size\n",
+				 port->id, cxld->id);
+			return -ENXIO;
+		}
 		port->commit_end = cxld->id;
 	} else {
 		/* unless / until type-2 drivers arrive, assume type-3 */

[PATCH 2/5] cxl/hdm: Use 4-byte reads to retrieve HDM decoder base+limit

[Dan Williams]

The CXL specification mandates that 4-byte registers must be accessed
with 4-byte access cycles. CXL 3.0 8.2.3 "Component Register Layout and
Definition" states that the behavior is undefined if (2) 32-bit
registers are accessed as an 8-byte quantity. It turns out that at least
one hardware implementation is sensitive to this in practice. The @size
variable results in zero with:

    size = readq(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));

...and the correct size with:

    lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
    hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which));
    size = (hi << 32) + lo;

Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 35b338b716fe..6fdf7981ddc7 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
-#include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/seq_file.h>
 #include <linux/device.h>
 #include <linux/delay.h>
@@ -785,8 +784,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			    int *target_map, void __iomem *hdm, int which,
 			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
 {
+	u64 size, base, skip, dpa_size, lo, hi;
 	struct cxl_endpoint_decoder *cxled;
-	u64 size, base, skip, dpa_size;
 	bool committed;
 	u32 remainder;
 	int i, rc;
@@ -801,8 +800,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 							which, info);
 
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
-	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
-	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
+	lo = readl(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
+	hi = readl(hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(which));
+	base = (hi << 32) + lo;
+	lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
+	hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which));
+	size = (hi << 32) + lo;
 	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
 	cxld->commit = cxl_decoder_commit;
 	cxld->reset = cxl_decoder_reset;
@@ -865,8 +868,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		return rc;
 
 	if (!info) {
-		target_list.value =
-			ioread64_hi_lo(hdm + CXL_HDM_DECODER0_TL_LOW(which));
+		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
+		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
+		target_list.value = (hi << 32) + lo;
 		for (i = 0; i < cxld->interleave_ways; i++)
 			target_map[i] = target_list.target_id[i];
 
@@ -883,7 +887,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			port->id, cxld->id, size, cxld->interleave_ways);
 		return -ENXIO;
 	}
-	skip = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SKIP_LOW(which));
+	lo = readl(hdm + CXL_HDM_DECODER0_SKIP_LOW(which));
+	hi = readl(hdm + CXL_HDM_DECODER0_SKIP_HIGH(which));
+	skip = (hi << 32) + lo;
 	cxled = to_cxl_endpoint_decoder(&cxld->dev);
 	rc = devm_cxl_dpa_reserve(cxled, *dpa_base + skip, dpa_size, skip);
 	if (rc) {

[PATCH 4/5] cxl/port: Scan single-target ports for decoders

[Dan Williams]

Do not assume that a single-target port falls back to a passthrough
decoder configuration. Scan for decoders and only fallback after probing
that the HDM decoder capability is not present.

One user visible affect of this bug is the inability to enumerate
present CXL regions as the decoder settings for the present decoders are
skipped.

Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Link: http://lore.kernel.org/r/20230227153128.8164-1-Jonathan.Cameron@huawei.com
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c |    5 +++--
 drivers/cxl/port.c     |   18 +++++++++++++-----
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 6fdf7981ddc7..abe3877cfa63 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -92,8 +92,9 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
 
 	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 -ENXIO;
+		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
+		/* unique error code to indicate no HDM decoder capability */
+		return -ENODEV;
 	}
 
 	return cxl_map_component_regs(&port->dev, regs, &map,
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 22a7ab2bae7c..eb57324c4ad4 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -66,14 +66,22 @@ static int cxl_switch_port_probe(struct cxl_port *port)
 	if (rc < 0)
 		return rc;
 
-	if (rc == 1)
-		return devm_cxl_add_passthrough_decoder(port);
-
 	cxlhdm = devm_cxl_setup_hdm(port, NULL);
-	if (IS_ERR(cxlhdm))
+	if (!IS_ERR(cxlhdm))
+		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
+
+	if (PTR_ERR(cxlhdm) != -ENODEV) {
+		dev_err(&port->dev, "Failed to map HDM decoder capability\n");
 		return PTR_ERR(cxlhdm);
+	}
+
+	if (rc == 1) {
+		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
+		return devm_cxl_add_passthrough_decoder(port);
+	}
 
-	return devm_cxl_enumerate_decoders(cxlhdm, NULL);
+	dev_err(&port->dev, "HDM decoder capability not found\n");
+	return -ENXIO;
 }
 
 static int cxl_endpoint_port_probe(struct cxl_port *port)

Re: [PATCH 1/5] cxl/hdm: Fail upon detecting 0-sized decoders

[Alison Schofield]

On Fri, Apr 14, 2023 at 11:53:55AM -0700, Dan Williams wrote:
> Decoders committed with 0-size lead to later crashes on shutdown as
> __cxl_dpa_release() assumes a 'struct resource' has been established in
> the in 'cxlds->dpa_res'. Just fail the driver load in this instance
> since there are deeper problems with the enumeration or the setup when
> this happens.
> 
> Fixes: 9c57cde0dcbd ("cxl/hdm: Enumerate allocated DPA")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>


Reviewed-by: Alison Schofield <alison.schofield@intel.com>


> 
snip

Re: [PATCH 2/5] cxl/hdm: Use 4-byte reads to retrieve HDM decoder base+limit

[Alison Schofield]

On Fri, Apr 14, 2023 at 11:54:00AM -0700, Dan Williams wrote:
> The CXL specification mandates that 4-byte registers must be accessed
> with 4-byte access cycles. CXL 3.0 8.2.3 "Component Register Layout and
> Definition" states that the behavior is undefined if (2) 32-bit
> registers are accessed as an 8-byte quantity. It turns out that at least
> one hardware implementation is sensitive to this in practice. The @size
> variable results in zero with:
> 
>     size = readq(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> 
> ...and the correct size with:
> 
>     lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
>     hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which));
>     size = (hi << 32) + lo;
> 
> Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I see you got rid of ioread64_hi_lo(), so this can't be
happening anywhere else. Are all the other readl, writel
usages known to be OK, or do you need review help against
the spec?

Reviewed-by: Alison Schofield <alison.schofield@intel.com>


> ---
>  drivers/cxl/core/hdm.c |   20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 35b338b716fe..6fdf7981ddc7 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -1,6 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> -#include <linux/io-64-nonatomic-hi-lo.h>
>  #include <linux/seq_file.h>
>  #include <linux/device.h>
>  #include <linux/delay.h>
> @@ -785,8 +784,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  			    int *target_map, void __iomem *hdm, int which,
>  			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
>  {
> +	u64 size, base, skip, dpa_size, lo, hi;
>  	struct cxl_endpoint_decoder *cxled;
> -	u64 size, base, skip, dpa_size;
>  	bool committed;
>  	u32 remainder;
>  	int i, rc;
> @@ -801,8 +800,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  							which, info);
>  
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
> -	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> -	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> +	lo = readl(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> +	hi = readl(hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(which));
> +	base = (hi << 32) + lo;
> +	lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> +	hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which));
> +	size = (hi << 32) + lo;
>  	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
>  	cxld->commit = cxl_decoder_commit;
>  	cxld->reset = cxl_decoder_reset;
> @@ -865,8 +868,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		return rc;
>  
>  	if (!info) {
> -		target_list.value =
> -			ioread64_hi_lo(hdm + CXL_HDM_DECODER0_TL_LOW(which));
> +		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
> +		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
> +		target_list.value = (hi << 32) + lo;
>  		for (i = 0; i < cxld->interleave_ways; i++)
>  			target_map[i] = target_list.target_id[i];
>  
> @@ -883,7 +887,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  			port->id, cxld->id, size, cxld->interleave_ways);
>  		return -ENXIO;
>  	}
> -	skip = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SKIP_LOW(which));
> +	lo = readl(hdm + CXL_HDM_DECODER0_SKIP_LOW(which));
> +	hi = readl(hdm + CXL_HDM_DECODER0_SKIP_HIGH(which));
> +	skip = (hi << 32) + lo;
>  	cxled = to_cxl_endpoint_decoder(&cxld->dev);
>  	rc = devm_cxl_dpa_reserve(cxled, *dpa_base + skip, dpa_size, skip);
>  	if (rc) {
> 

Re: [PATCH 2/5] cxl/hdm: Use 4-byte reads to retrieve HDM decoder base+limit

[Dan Williams]

Alison Schofield wrote:
> On Fri, Apr 14, 2023 at 11:54:00AM -0700, Dan Williams wrote:
> > The CXL specification mandates that 4-byte registers must be accessed
> > with 4-byte access cycles. CXL 3.0 8.2.3 "Component Register Layout and
> > Definition" states that the behavior is undefined if (2) 32-bit
> > registers are accessed as an 8-byte quantity. It turns out that at least
> > one hardware implementation is sensitive to this in practice. The @size
> > variable results in zero with:
> > 
> >     size = readq(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> > 
> > ...and the correct size with:
> > 
> >     lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> >     hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which));
> >     size = (hi << 32) + lo;
> > 
> > Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> I see you got rid of ioread64_hi_lo(), so this can't be
> happening anywhere else. Are all the other readl, writel
> usages known to be OK, or do you need review help against
> the spec?

Good question. That's what I looked to answer in patch3. As far as I can
see all the other readq() usage in the driver is for registers defined
as 64-bit, so that patch ended up only being a deletion of unneeded
includes.

Re: [PATCH 4/5] cxl/port: Scan single-target ports for decoders

[Alison Schofield]

On Fri, Apr 14, 2023 at 11:54:11AM -0700, Dan Williams wrote:
> Do not assume that a single-target port falls back to a passthrough
> decoder configuration. Scan for decoders and only fallback after probing
> that the HDM decoder capability is not present.
> 
> One user visible affect of this bug is the inability to enumerate
> present CXL regions as the decoder settings for the present decoders are
> skipped.
> 
> Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Link: http://lore.kernel.org/r/20230227153128.8164-1-Jonathan.Cameron@huawei.com
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> ---
>  drivers/cxl/core/hdm.c |    5 +++--
>  drivers/cxl/port.c     |   18 +++++++++++++-----
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 6fdf7981ddc7..abe3877cfa63 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -92,8 +92,9 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
>  
>  	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 -ENXIO;
> +		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> +		/* unique error code to indicate no HDM decoder capability */
> +		return -ENODEV;
>  	}
>  
>  	return cxl_map_component_regs(&port->dev, regs, &map,
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 22a7ab2bae7c..eb57324c4ad4 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -66,14 +66,22 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  	if (rc < 0)
>  		return rc;
>  
> -	if (rc == 1)
> -		return devm_cxl_add_passthrough_decoder(port);
> -
>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
> -	if (IS_ERR(cxlhdm))
> +	if (!IS_ERR(cxlhdm))
> +		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> +
> +	if (PTR_ERR(cxlhdm) != -ENODEV) {
> +		dev_err(&port->dev, "Failed to map HDM decoder capability\n");
>  		return PTR_ERR(cxlhdm);
> +	}
> +
> +	if (rc == 1) {
> +		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
> +		return devm_cxl_add_passthrough_decoder(port);
> +	}
>  
> -	return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> +	dev_err(&port->dev, "HDM decoder capability not found\n");
> +	return -ENXIO;
>  }
>  
>  static int cxl_endpoint_port_probe(struct cxl_port *port)
> 

Re: [PATCH 1/5] cxl/hdm: Fail upon detecting 0-sized decoders

[Dave Jiang]

On 4/14/23 11:53 AM, Dan Williams wrote:
> Decoders committed with 0-size lead to later crashes on shutdown as
> __cxl_dpa_release() assumes a 'struct resource' has been established in
> the in 'cxlds->dpa_res'. Just fail the driver load in this instance
> since there are deeper problems with the enumeration or the setup when
> this happens.
> 
> Fixes: 9c57cde0dcbd ("cxl/hdm: Enumerate allocated DPA")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/core/hdm.c |   15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 02cc2c38b44b..35b338b716fe 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -269,8 +269,11 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>   
>   	lockdep_assert_held_write(&cxl_dpa_rwsem);
>   
> -	if (!len)
> -		goto success;
> +	if (!len) {
> +		dev_warn(dev, "decoder%d.%d: empty reservation attempted\n",
> +			 port->id, cxled->cxld.id);
> +		return -EINVAL;
> +	}
>   
>   	if (cxled->dpa_res) {
>   		dev_dbg(dev, "decoder%d.%d: existing allocation %pr assigned\n",
> @@ -323,7 +326,6 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>   		cxled->mode = CXL_DECODER_MIXED;
>   	}
>   
> -success:
>   	port->hdm_end++;
>   	get_device(&cxled->cxld.dev);
>   	return 0;
> @@ -833,6 +835,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   				 port->id, cxld->id);
>   			return -ENXIO;
>   		}
> +
> +		if (size == 0) {
> +			dev_warn(&port->dev,
> +				 "decoder%d.%d: Committed with zero size\n",
> +				 port->id, cxld->id);
> +			return -ENXIO;
> +		}
>   		port->commit_end = cxld->id;
>   	} else {
>   		/* unless / until type-2 drivers arrive, assume type-3 */
> 

Re: [PATCH 2/5] cxl/hdm: Use 4-byte reads to retrieve HDM decoder base+limit

[Dave Jiang]

On 4/14/23 11:54 AM, Dan Williams wrote:
> The CXL specification mandates that 4-byte registers must be accessed
> with 4-byte access cycles. CXL 3.0 8.2.3 "Component Register Layout and
> Definition" states that the behavior is undefined if (2) 32-bit
> registers are accessed as an 8-byte quantity. It turns out that at least
> one hardware implementation is sensitive to this in practice. The @size
> variable results in zero with:
> 
>      size = readq(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> 
> ...and the correct size with:
> 
>      lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
>      hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which));
>      size = (hi << 32) + lo;
> 
> Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/core/hdm.c |   20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 35b338b716fe..6fdf7981ddc7 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -1,6 +1,5 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> -#include <linux/io-64-nonatomic-hi-lo.h>
>   #include <linux/seq_file.h>
>   #include <linux/device.h>
>   #include <linux/delay.h>
> @@ -785,8 +784,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   			    int *target_map, void __iomem *hdm, int which,
>   			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
>   {
> +	u64 size, base, skip, dpa_size, lo, hi;
>   	struct cxl_endpoint_decoder *cxled;
> -	u64 size, base, skip, dpa_size;
>   	bool committed;
>   	u32 remainder;
>   	int i, rc;
> @@ -801,8 +800,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   							which, info);
>   
>   	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
> -	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> -	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> +	lo = readl(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> +	hi = readl(hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(which));
> +	base = (hi << 32) + lo;
> +	lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> +	hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which));
> +	size = (hi << 32) + lo;
>   	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
>   	cxld->commit = cxl_decoder_commit;
>   	cxld->reset = cxl_decoder_reset;
> @@ -865,8 +868,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   		return rc;
>   
>   	if (!info) {
> -		target_list.value =
> -			ioread64_hi_lo(hdm + CXL_HDM_DECODER0_TL_LOW(which));
> +		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
> +		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
> +		target_list.value = (hi << 32) + lo;
>   		for (i = 0; i < cxld->interleave_ways; i++)
>   			target_map[i] = target_list.target_id[i];
>   
> @@ -883,7 +887,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   			port->id, cxld->id, size, cxld->interleave_ways);
>   		return -ENXIO;
>   	}
> -	skip = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SKIP_LOW(which));
> +	lo = readl(hdm + CXL_HDM_DECODER0_SKIP_LOW(which));
> +	hi = readl(hdm + CXL_HDM_DECODER0_SKIP_HIGH(which));
> +	skip = (hi << 32) + lo;
>   	cxled = to_cxl_endpoint_decoder(&cxld->dev);
>   	rc = devm_cxl_dpa_reserve(cxled, *dpa_base + skip, dpa_size, skip);
>   	if (rc) {
> 

Re: [PATCH 4/5] cxl/port: Scan single-target ports for decoders

[Dave Jiang]

On 4/14/23 11:54 AM, Dan Williams wrote:
> Do not assume that a single-target port falls back to a passthrough
> decoder configuration. Scan for decoders and only fallback after probing
> that the HDM decoder capability is not present.
> 
> One user visible affect of this bug is the inability to enumerate
> present CXL regions as the decoder settings for the present decoders are
> skipped.
> 
> Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Link: http://lore.kernel.org/r/20230227153128.8164-1-Jonathan.Cameron@huawei.com
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/core/hdm.c |    5 +++--
>   drivers/cxl/port.c     |   18 +++++++++++++-----
>   2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 6fdf7981ddc7..abe3877cfa63 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -92,8 +92,9 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
>   
>   	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 -ENXIO;
> +		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> +		/* unique error code to indicate no HDM decoder capability */
> +		return -ENODEV;
>   	}
>   
>   	return cxl_map_component_regs(&port->dev, regs, &map,
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 22a7ab2bae7c..eb57324c4ad4 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -66,14 +66,22 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>   	if (rc < 0)
>   		return rc;
>   
> -	if (rc == 1)
> -		return devm_cxl_add_passthrough_decoder(port);
> -
>   	cxlhdm = devm_cxl_setup_hdm(port, NULL);
> -	if (IS_ERR(cxlhdm))
> +	if (!IS_ERR(cxlhdm))
> +		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> +
> +	if (PTR_ERR(cxlhdm) != -ENODEV) {
> +		dev_err(&port->dev, "Failed to map HDM decoder capability\n");
>   		return PTR_ERR(cxlhdm);
> +	}
> +
> +	if (rc == 1) {
> +		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
> +		return devm_cxl_add_passthrough_decoder(port);
> +	}
>   
> -	return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> +	dev_err(&port->dev, "HDM decoder capability not found\n");
> +	return -ENXIO;
>   }
>   
>   static int cxl_endpoint_port_probe(struct cxl_port *port)
> 

Re: [PATCH 4/5] cxl/port: Scan single-target ports for decoders

[Jonathan Cameron]

On Fri, 14 Apr 2023 11:54:11 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Do not assume that a single-target port falls back to a passthrough
> decoder configuration. Scan for decoders and only fallback after probing
> that the HDM decoder capability is not present.
> 
> One user visible affect of this bug is the inability to enumerate
> present CXL regions as the decoder settings for the present decoders are
> skipped.
> 
> Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Link: http://lore.kernel.org/r/20230227153128.8164-1-Jonathan.Cameron@huawei.com
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

*Happy face*

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


> ---
>  drivers/cxl/core/hdm.c |    5 +++--
>  drivers/cxl/port.c     |   18 +++++++++++++-----
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 6fdf7981ddc7..abe3877cfa63 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -92,8 +92,9 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
>  
>  	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 -ENXIO;
> +		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> +		/* unique error code to indicate no HDM decoder capability */
> +		return -ENODEV;
>  	}
>  
>  	return cxl_map_component_regs(&port->dev, regs, &map,
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 22a7ab2bae7c..eb57324c4ad4 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -66,14 +66,22 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  	if (rc < 0)
>  		return rc;
>  
> -	if (rc == 1)
> -		return devm_cxl_add_passthrough_decoder(port);
> -
>  	cxlhdm = devm_cxl_setup_hdm(port, NULL);
> -	if (IS_ERR(cxlhdm))
> +	if (!IS_ERR(cxlhdm))
> +		return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> +
> +	if (PTR_ERR(cxlhdm) != -ENODEV) {
> +		dev_err(&port->dev, "Failed to map HDM decoder capability\n");
>  		return PTR_ERR(cxlhdm);
> +	}
> +
> +	if (rc == 1) {
> +		dev_dbg(&port->dev, "Fallback to passthrough decoder\n");
> +		return devm_cxl_add_passthrough_decoder(port);
> +	}
>  
> -	return devm_cxl_enumerate_decoders(cxlhdm, NULL);
> +	dev_err(&port->dev, "HDM decoder capability not found\n");
> +	return -ENXIO;
>  }
>  
>  static int cxl_endpoint_port_probe(struct cxl_port *port)
> 
> 

Re: [PATCH 1/5] cxl/hdm: Fail upon detecting 0-sized decoders

[Jonathan Cameron]

On Fri, 14 Apr 2023 11:53:55 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Decoders committed with 0-size lead to later crashes on shutdown as
> __cxl_dpa_release() assumes a 'struct resource' has been established in
> the in 'cxlds->dpa_res'. Just fail the driver load in this instance
> since there are deeper problems with the enumeration or the setup when
> this happens.
> 
> Fixes: 9c57cde0dcbd ("cxl/hdm: Enumerate allocated DPA")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

What happened to these?  Seem not to have gone upstream yet.

This seems reasonable to me as well

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

> ---
>  drivers/cxl/core/hdm.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 02cc2c38b44b..35b338b716fe 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -269,8 +269,11 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  
>  	lockdep_assert_held_write(&cxl_dpa_rwsem);
>  
> -	if (!len)
> -		goto success;
> +	if (!len) {
> +		dev_warn(dev, "decoder%d.%d: empty reservation attempted\n",
> +			 port->id, cxled->cxld.id);
> +		return -EINVAL;
> +	}
>  
>  	if (cxled->dpa_res) {
>  		dev_dbg(dev, "decoder%d.%d: existing allocation %pr assigned\n",
> @@ -323,7 +326,6 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  		cxled->mode = CXL_DECODER_MIXED;
>  	}
>  
> -success:
>  	port->hdm_end++;
>  	get_device(&cxled->cxld.dev);
>  	return 0;
> @@ -833,6 +835,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  				 port->id, cxld->id);
>  			return -ENXIO;
>  		}
> +
> +		if (size == 0) {
> +			dev_warn(&port->dev,
> +				 "decoder%d.%d: Committed with zero size\n",
> +				 port->id, cxld->id);
> +			return -ENXIO;
> +		}
>  		port->commit_end = cxld->id;
>  	} else {
>  		/* unless / until type-2 drivers arrive, assume type-3 */
> 

Re: [PATCH 2/5] cxl/hdm: Use 4-byte reads to retrieve HDM decoder base+limit

[Jonathan Cameron]

On Fri, 14 Apr 2023 11:54:00 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The CXL specification mandates that 4-byte registers must be accessed
> with 4-byte access cycles. CXL 3.0 8.2.3 "Component Register Layout and
> Definition" states that the behavior is undefined if (2) 32-bit
> registers are accessed as an 8-byte quantity. It turns out that at least
> one hardware implementation is sensitive to this in practice. The @size
> variable results in zero with:
> 
>     size = readq(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> 
> ...and the correct size with:
> 
>     lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
>     hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which));
>     size = (hi << 32) + lo;

Hmm. Annoying that there isn't an always present split version of the
ioread64_hi_lo like there effectively is for hi_low_readq()

Mind you, why was this using the ioread64_hi_lo() variant rather
than hi_lo_readq()?  Far as I can tell that wouldn't have suffered
from this problem in the first place.

There is at least one other direct user of that function, so maybe
we should just use it here as well?

Jonathan

> 
> Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/hdm.c |   20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 35b338b716fe..6fdf7981ddc7 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -1,6 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> -#include <linux/io-64-nonatomic-hi-lo.h>
>  #include <linux/seq_file.h>
>  #include <linux/device.h>
>  #include <linux/delay.h>
> @@ -785,8 +784,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  			    int *target_map, void __iomem *hdm, int which,
>  			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
>  {
> +	u64 size, base, skip, dpa_size, lo, hi;
>  	struct cxl_endpoint_decoder *cxled;
> -	u64 size, base, skip, dpa_size;
>  	bool committed;
>  	u32 remainder;
>  	int i, rc;
> @@ -801,8 +800,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  							which, info);
>  
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
> -	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> -	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> +	lo = readl(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
> +	hi = readl(hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(which));
> +	base = (hi << 32) + lo;
> +	lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> +	hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which));
> +	size = (hi << 32) + lo;
>  	committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
>  	cxld->commit = cxl_decoder_commit;
>  	cxld->reset = cxl_decoder_reset;
> @@ -865,8 +868,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		return rc;
>  
>  	if (!info) {
> -		target_list.value =
> -			ioread64_hi_lo(hdm + CXL_HDM_DECODER0_TL_LOW(which));
> +		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
> +		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
> +		target_list.value = (hi << 32) + lo;
>  		for (i = 0; i < cxld->interleave_ways; i++)
>  			target_map[i] = target_list.target_id[i];
>  
> @@ -883,7 +887,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  			port->id, cxld->id, size, cxld->interleave_ways);
>  		return -ENXIO;
>  	}
> -	skip = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SKIP_LOW(which));
> +	lo = readl(hdm + CXL_HDM_DECODER0_SKIP_LOW(which));
> +	hi = readl(hdm + CXL_HDM_DECODER0_SKIP_HIGH(which));
> +	skip = (hi << 32) + lo;
>  	cxled = to_cxl_endpoint_decoder(&cxld->dev);
>  	rc = devm_cxl_dpa_reserve(cxled, *dpa_base + skip, dpa_size, skip);
>  	if (rc) {
>