* [PATCH 0/2] cxl/port: Robustness fixes for decoder enumeration
@ 2022-01-26 5:23 Dan Williams
2022-01-26 5:24 ` [PATCH 1/2] cxl/core/port: Fix / relax decoder target enumeration Dan Williams
2022-01-26 5:24 ` [PATCH 2/2] cxl/core/port: Handle invalid decoders Dan Williams
0 siblings, 2 replies; 5+ messages in thread
From: Dan Williams @ 2022-01-26 5:23 UTC (permalink / raw)
To: linux-cxl; +Cc: linux-pci, nvdimm, ben.widawsky
Further testing of the decoder toplogy enumeration patches found cases
where the driver is too strict about what it accepts. First, there is no
expectation that the decoder's target list is valid when the decoder is
disabled. Make decoder_populate_targets() failures non-fatal on disabled
decoders. Second, if the decoder emits out-of-bounds / reserved values
at init warn and continue if at least one valid decoder was found. This
future-proofs the driver against changes to the interleave_ways
encoding, at least for continuing to operate decoders that conform to
current expectations.
Applies on top of:
https://lore.kernel.org/r/164298411792.3018233.7493009997525360044.stgit@dwillia2-desk3.amr.corp.intel.com
---
Dan Williams (2):
cxl/core/port: Fix / relax decoder target enumeration
cxl/core/port: Handle invalid decoders
drivers/cxl/acpi.c | 2 +-
drivers/cxl/core/hdm.c | 36 ++++++++++++++++++++++++++++++------
drivers/cxl/core/port.c | 5 ++++-
3 files changed, 35 insertions(+), 8 deletions(-)
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/2] cxl/core/port: Fix / relax decoder target enumeration 2022-01-26 5:23 [PATCH 0/2] cxl/port: Robustness fixes for decoder enumeration Dan Williams @ 2022-01-26 5:24 ` Dan Williams 2022-02-01 12:55 ` Jonathan Cameron 2022-01-26 5:24 ` [PATCH 2/2] cxl/core/port: Handle invalid decoders Dan Williams 1 sibling, 1 reply; 5+ messages in thread From: Dan Williams @ 2022-01-26 5:24 UTC (permalink / raw) To: linux-cxl; +Cc: linux-pci, nvdimm, ben.widawsky If the decoder is not presently active the target_list may not be accurate. Perform a best effort mapping and assume that it will be fixed up when the decoder is enabled. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/acpi.c | 2 +- drivers/cxl/core/port.c | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index df6691d0a6d0..f64d98bfcb3b 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -15,7 +15,7 @@ static unsigned long cfmws_to_decoder_flags(int restrictions) { - unsigned long flags = 0; + unsigned long flags = CXL_DECODER_F_ENABLE; if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) flags |= CXL_DECODER_F_TYPE2; diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 224a4853a33e..e75e0d4fb894 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1263,8 +1263,11 @@ int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map) port = to_cxl_port(cxld->dev.parent); if (!is_endpoint_decoder(dev)) { rc = decoder_populate_targets(cxld, port, target_map); - if (rc) + if (rc && (cxld->flags & CXL_DECODER_F_ENABLE)) { + dev_err(&port->dev, + "Failed to populate active decoder targets\n"); return rc; + } } rc = dev_set_name(dev, "decoder%d.%d", port->id, cxld->id); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] cxl/core/port: Fix / relax decoder target enumeration 2022-01-26 5:24 ` [PATCH 1/2] cxl/core/port: Fix / relax decoder target enumeration Dan Williams @ 2022-02-01 12:55 ` Jonathan Cameron 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Cameron @ 2022-02-01 12:55 UTC (permalink / raw) To: Dan Williams; +Cc: linux-cxl, linux-pci, nvdimm, ben.widawsky On Tue, 25 Jan 2022 21:24:04 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > If the decoder is not presently active the target_list may not be > accurate. Perform a best effort mapping and assume that it will be fixed > up when the decoder is enabled. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Make sense. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/acpi.c | 2 +- > drivers/cxl/core/port.c | 5 ++++- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index df6691d0a6d0..f64d98bfcb3b 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -15,7 +15,7 @@ > > static unsigned long cfmws_to_decoder_flags(int restrictions) > { > - unsigned long flags = 0; > + unsigned long flags = CXL_DECODER_F_ENABLE; > > if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) > flags |= CXL_DECODER_F_TYPE2; > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 224a4853a33e..e75e0d4fb894 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1263,8 +1263,11 @@ int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map) > port = to_cxl_port(cxld->dev.parent); > if (!is_endpoint_decoder(dev)) { > rc = decoder_populate_targets(cxld, port, target_map); > - if (rc) > + if (rc && (cxld->flags & CXL_DECODER_F_ENABLE)) { > + dev_err(&port->dev, > + "Failed to populate active decoder targets\n"); > return rc; > + } > } > > rc = dev_set_name(dev, "decoder%d.%d", port->id, cxld->id); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] cxl/core/port: Handle invalid decoders 2022-01-26 5:23 [PATCH 0/2] cxl/port: Robustness fixes for decoder enumeration Dan Williams 2022-01-26 5:24 ` [PATCH 1/2] cxl/core/port: Fix / relax decoder target enumeration Dan Williams @ 2022-01-26 5:24 ` Dan Williams 2022-02-01 12:59 ` Jonathan Cameron 1 sibling, 1 reply; 5+ messages in thread From: Dan Williams @ 2022-01-26 5:24 UTC (permalink / raw) To: linux-cxl; +Cc: linux-pci, nvdimm, ben.widawsky In case init_hdm_decoder() finds invalid settings, skip to the next valid decoder. Only fail port enumeration if zero valid decoders are found. This protects the driver init against broken hardware and / or future interleave capabilities. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/hdm.c | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index c966ab0d51fe..4955ba16c9c8 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -150,8 +150,8 @@ static int to_interleave_ways(u32 ctrl) } } -static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map, - void __iomem *hdm, int which) +static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, + int *target_map, void __iomem *hdm, int which) { u64 size, base; u32 ctrl; @@ -167,6 +167,11 @@ static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map, if (!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED)) size = 0; + if (base == U64_MAX || size == U64_MAX) { + dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n", + port->id, cxld->id); + return -ENXIO; + } cxld->decoder_range = (struct range) { .start = base, @@ -180,6 +185,12 @@ static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map, cxld->flags |= CXL_DECODER_F_LOCK; } cxld->interleave_ways = to_interleave_ways(ctrl); + if (!cxld->interleave_ways) { + dev_warn(&port->dev, + "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n", + port->id, cxld->id, ctrl); + return -ENXIO; + } cxld->interleave_granularity = to_interleave_granularity(ctrl); if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl)) @@ -188,12 +199,14 @@ static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map, cxld->target_type = CXL_DECODER_ACCELERATOR; if (is_cxl_endpoint(to_cxl_port(cxld->dev.parent))) - return; + return 0; target_list.value = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_TL_LOW(which)); for (i = 0; i < cxld->interleave_ways; i++) target_map[i] = target_list.target_id[i]; + + return 0; } /** @@ -204,7 +217,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm) { void __iomem *hdm = cxlhdm->regs.hdm_decoder; struct cxl_port *port = cxlhdm->port; - int i, committed; + int i, committed, failed; u32 ctrl; /* @@ -224,7 +237,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm) if (committed != cxlhdm->decoder_count) msleep(20); - for (i = 0; i < cxlhdm->decoder_count; i++) { + for (i = 0, failed = 0; i < cxlhdm->decoder_count; i++) { int target_map[CXL_DECODER_MAX_INTERLEAVE] = { 0 }; int rc, target_count = cxlhdm->target_count; struct cxl_decoder *cxld; @@ -239,7 +252,13 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm) return PTR_ERR(cxld); } - init_hdm_decoder(cxld, target_map, cxlhdm->regs.hdm_decoder, i); + rc = init_hdm_decoder(port, cxld, target_map, + cxlhdm->regs.hdm_decoder, i); + if (rc) { + put_device(&cxld->dev); + failed++; + continue; + } rc = add_hdm_decoder(port, cxld, target_map); if (rc) { dev_warn(&port->dev, @@ -248,6 +267,11 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm) } } + if (failed == cxlhdm->decoder_count) { + dev_err(&port->dev, "No valid decoders found\n"); + return -ENXIO; + } + return 0; } EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_decoders, CXL); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] cxl/core/port: Handle invalid decoders 2022-01-26 5:24 ` [PATCH 2/2] cxl/core/port: Handle invalid decoders Dan Williams @ 2022-02-01 12:59 ` Jonathan Cameron 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Cameron @ 2022-02-01 12:59 UTC (permalink / raw) To: Dan Williams; +Cc: linux-cxl, linux-pci, nvdimm, ben.widawsky On Tue, 25 Jan 2022 21:24:09 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > In case init_hdm_decoder() finds invalid settings, skip to the next > valid decoder. Only fail port enumeration if zero valid decoders are > found. This protects the driver init against broken hardware and / or > future interleave capabilities. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> One comment inline, but I'm fine with this as it is. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/hdm.c | 36 ++++++++++++++++++++++++++++++------ > 1 file changed, 30 insertions(+), 6 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index c966ab0d51fe..4955ba16c9c8 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -150,8 +150,8 @@ static int to_interleave_ways(u32 ctrl) > } > } > > -static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map, > - void __iomem *hdm, int which) > +static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > + int *target_map, void __iomem *hdm, int which) > { > u64 size, base; > u32 ctrl; > @@ -167,6 +167,11 @@ static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map, > > if (!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED)) > size = 0; > + if (base == U64_MAX || size == U64_MAX) { > + dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n", > + port->id, cxld->id); > + return -ENXIO; > + } > > cxld->decoder_range = (struct range) { > .start = base, > @@ -180,6 +185,12 @@ static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map, > cxld->flags |= CXL_DECODER_F_LOCK; > } > cxld->interleave_ways = to_interleave_ways(ctrl); > + if (!cxld->interleave_ways) { > + dev_warn(&port->dev, > + "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n", > + port->id, cxld->id, ctrl); > + return -ENXIO; > + } > cxld->interleave_granularity = to_interleave_granularity(ctrl); > > if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl)) > @@ -188,12 +199,14 @@ static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map, > cxld->target_type = CXL_DECODER_ACCELERATOR; > > if (is_cxl_endpoint(to_cxl_port(cxld->dev.parent))) > - return; > + return 0; > > target_list.value = > ioread64_hi_lo(hdm + CXL_HDM_DECODER0_TL_LOW(which)); > for (i = 0; i < cxld->interleave_ways; i++) > target_map[i] = target_list.target_id[i]; > + > + return 0; > } > > /** > @@ -204,7 +217,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm) > { > void __iomem *hdm = cxlhdm->regs.hdm_decoder; > struct cxl_port *port = cxlhdm->port; > - int i, committed; > + int i, committed, failed; > u32 ctrl; > > /* > @@ -224,7 +237,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm) > if (committed != cxlhdm->decoder_count) > msleep(20); > > - for (i = 0; i < cxlhdm->decoder_count; i++) { > + for (i = 0, failed = 0; i < cxlhdm->decoder_count; i++) { > int target_map[CXL_DECODER_MAX_INTERLEAVE] = { 0 }; > int rc, target_count = cxlhdm->target_count; > struct cxl_decoder *cxld; > @@ -239,7 +252,13 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm) > return PTR_ERR(cxld); > } > > - init_hdm_decoder(cxld, target_map, cxlhdm->regs.hdm_decoder, i); > + rc = init_hdm_decoder(port, cxld, target_map, > + cxlhdm->regs.hdm_decoder, i); > + if (rc) { > + put_device(&cxld->dev); > + failed++; Not sure I'd have done it this way around, as opposed to inited++ or similar, but up to you. > + continue; > + } > rc = add_hdm_decoder(port, cxld, target_map); > if (rc) { > dev_warn(&port->dev, > @@ -248,6 +267,11 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm) > } > } > > + if (failed == cxlhdm->decoder_count) { > + dev_err(&port->dev, "No valid decoders found\n"); > + return -ENXIO; > + } > + > return 0; > } > EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_decoders, CXL); > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-01 12:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-26 5:23 [PATCH 0/2] cxl/port: Robustness fixes for decoder enumeration Dan Williams 2022-01-26 5:24 ` [PATCH 1/2] cxl/core/port: Fix / relax decoder target enumeration Dan Williams 2022-02-01 12:55 ` Jonathan Cameron 2022-01-26 5:24 ` [PATCH 2/2] cxl/core/port: Handle invalid decoders Dan Williams 2022-02-01 12:59 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).