From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <linux-pci@vger.kernel.org>,
<nvdimm@lists.linux.dev>, <ben.widawsky@intel.com>
Subject: Re: [PATCH 2/2] cxl/core/port: Handle invalid decoders
Date: Tue, 1 Feb 2022 12:59:54 +0000 [thread overview]
Message-ID: <20220201125954.000038c4@Huawei.com> (raw)
In-Reply-To: <164317464918.3438644.12371149695618136198.stgit@dwillia2-desk3.amr.corp.intel.com>
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);
>
prev parent reply other threads:[~2022-02-01 12:59 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220201125954.000038c4@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=ben.widawsky@intel.com \
--cc=dan.j.williams@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=nvdimm@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).