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
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 */
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
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;}
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;}
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) {
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;@@ -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) {
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.
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;@@ -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) {
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;@@ -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) {
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)
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");- }
- 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)
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");- }
- 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)
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");- }
- 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)
linux-stable-mirror@lists.linaro.org
-
Alison Schofield -
Dan Williams -
Dave Jiang -
Jonathan Cameron