Re: [PATCH v3 10/14] cxl/region: Collect host bridge decoders

From: Dan Williams <dan.j.williams@intel.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: linux-cxl@vger.kernel.org, patches@lists.linux.dev,
	 Alison Schofield <alison.schofield@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	 Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	 Bjorn Helgaas <helgaas@kernel.org>,
	Linux NVDIMM <nvdimm@lists.linux.dev>,
	 Linux PCI <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v3 10/14] cxl/region: Collect host bridge decoders
Date: Fri, 18 Feb 2022 15:42:50 -0800	[thread overview]
Message-ID: <CAPcyv4hF=DWWszAhrOTiBLFxm5s8gcJ_TcdVz9UNfYEuXNiJTw@mail.gmail.com> (raw)
In-Reply-To: <20220128002707.391076-11-ben.widawsky@intel.com>

On Thu, Jan 27, 2022 at 4:27 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Part of host bridge verification in the CXL Type 3 Memory Device
> Software Guide calculates the host bridge interleave target list (6th
> step in the flow chart), ie. verification and state update are done in
> the same step. Host bridge verification is already in place, so go ahead
> and store the decoders with their target lists.
>
> Switches are implemented in a separate patch.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/region.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index 145d7bb02714..b8982be13bfe 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -428,6 +428,7 @@ static bool region_hb_rp_config_valid(struct cxl_region *cxlr,
>                 return simple_config(cxlr, hbs[0]);
>
>         for (i = 0; i < hb_count; i++) {
> +               struct cxl_decoder *cxld;
>                 int idx, position_mask;
>                 struct cxl_dport *rp;
>                 struct cxl_port *hb;
> @@ -486,6 +487,18 @@ static bool region_hb_rp_config_valid(struct cxl_region *cxlr,
>                                                 "One or more devices are not connected to the correct Host Bridge Root Port\n");
>                                         goto err;
>                                 }
> +
> +                               if (!state_update)
> +                                       continue;
> +
> +                               if (dev_WARN_ONCE(&cxld->dev,
> +                                                 port_grouping >= cxld->nr_targets,
> +                                                 "Invalid port grouping %d/%d\n",
> +                                                 port_grouping, cxld->nr_targets))
> +                                       goto err;
> +
> +                               cxld->interleave_ways++;
> +                               cxld->target[port_grouping] = get_rp(ep);

There is not enough context in the changelog to understand what this
code is doing, but I do want to react to all this caching of objects
without references. I'd prefer helpers that walk the device that are
already synced with device_del() events than worry about these caches
and when to invalidate their references.

>                         }
>                 }
>         }
> @@ -538,7 +551,7 @@ static bool rootd_valid(const struct cxl_region *cxlr,
>
>  struct rootd_context {
>         const struct cxl_region *cxlr;
> -       struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
> +       const struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
>         int count;
>  };
>
> @@ -564,7 +577,7 @@ static struct cxl_decoder *find_rootd(const struct cxl_region *cxlr,
>         struct rootd_context ctx;
>         struct device *ret;
>
> -       ctx.cxlr = cxlr;
> +       ctx.cxlr = (struct cxl_region *)cxlr;

If const requires casting then don't use const.

>
>         ret = device_find_child((struct device *)&root->dev, &ctx, rootd_match);
>         if (ret)
> --
> 2.35.0
>