diff mbox series

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

Message ID 20220128002707.391076-11-ben.widawsky@intel.com (mailing list archive)
State New, archived
Headers show
Series CXL Region driver | expand

Commit Message

Ben Widawsky Jan. 28, 2022, 12:27 a.m. UTC
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(-)

Comments

Jonathan Cameron Feb. 1, 2022, 6:21 p.m. UTC | #1
On Thu, 27 Jan 2022 16:27:03 -0800
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>
Looks like a little bit of code got in here that I think
belongs in a different patch.

> ---
>  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);
>  			}
>  		}
>  	}
> @@ -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;

Why is this here?  If it's needed, that need doesn't seem to have come in
as part of this patch.

>  
>  	ret = device_find_child((struct device *)&root->dev, &ctx, rootd_match);
>  	if (ret)
Dan Williams Feb. 18, 2022, 11:42 p.m. UTC | #2
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
>
diff mbox series

Patch

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);
 			}
 		}
 	}
@@ -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;
 
 	ret = device_find_child((struct device *)&root->dev, &ctx, rootd_match);
 	if (ret)