[PATCH 0/4] cxl/region: Endpoint decoder fixes

[PATCH 0/4] cxl/region: Endpoint decoder fixes

[Dan Williams]

It turns out that the driver was only programming "mid-level" decoders
and not endpoint decoders. This was missed because the default settings
still allowed traffic to flow, just not to the expected DPA, and not
without aliasing.

The following fixes address that problem, but this still wants a test
environment that validates the expected DPA relative to a given HPA.
Jonathan caught this bug while trying to develop such an environment for
QEMU, but this likely also wants a cxl_test scheme to do the same. In
the meantime here is the fixup to program interleave ways + granularity
and some miscellaneous fixups for build robot reports.

These apply against current cxl/pending.

---

Dan Williams (4):
      cxl/region: Fix decoder interleave programming
      cxl/region: Move HPA setup to cxl_region_attach()
      cxl/region: Fix port setup uninitialized variable warnings
      cxl/region: Fix region commit uninitialized variable warning


 drivers/cxl/core/hdm.c    |   17 ++---------------
 drivers/cxl/core/region.c |   36 +++++++++++++++++++++++++++---------
 2 files changed, 29 insertions(+), 24 deletions(-)

base-commit: 65fc1c3d26b96002a5aa1f4012fae4dc98fd5683

[PATCH 1/4] cxl/region: Fix decoder interleave programming

[Dan Williams]

Jonathan notes:

"Curiously interleave ways = 1 for the EPs which is obviously wrong"

...while testing the latest CXL development branch on QEMU.

It turns out the region creation process failed to program the endpoint
decoders. This was missed because the default settings of x1 at 4K
intereleave still results in the region appearing to function. Jonathan
caught the bug by reverse mapping the translations that need to happen
for the QEMU support.

Link: https://lore.kernel.org/r/62e95fdf9f6e2_30440294e4@dwillia2-xfh.jf.intel.com.notmuch
Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index cf5d5811fe4c..8e6ff3f39755 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1294,6 +1294,9 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		p->state = CXL_CONFIG_ACTIVE;
 	}
 
+	cxled->cxld.interleave_ways = p->interleave_ways;
+	cxled->cxld.interleave_granularity = p->interleave_granularity;
+
 	return 0;
 
 err_decrement:

[PATCH 2/4] cxl/region: Move HPA setup to cxl_region_attach()

[Dan Williams]

A recent bug fix added the setup of the endpoint decoder interleave
geometry settings to cxl_region_attach(). Move the HPA setup there as
well to keep all endpoint decoder parameter setting in a central
location.

Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c    |   17 ++---------------
 drivers/cxl/core/region.c |    4 ++++
 2 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 8143e2615957..5aadefd670d1 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -499,20 +499,6 @@ static void cxld_set_type(struct cxl_decoder *cxld, u32 *ctrl)
 			  CXL_HDM_DECODER0_CTRL_TYPE);
 }
 
-static void cxld_set_hpa(struct cxl_decoder *cxld, u64 *base, u64 *size)
-{
-	struct cxl_region *cxlr = cxld->region;
-	struct cxl_region_params *p = &cxlr->params;
-
-	cxld->hpa_range = (struct range) {
-		.start = p->res->start,
-		.end = p->res->end,
-	};
-
-	*base = p->res->start;
-	*size = resource_size(p->res);
-}
-
 static void cxld_clear_hpa(struct cxl_decoder *cxld)
 {
 	cxld->hpa_range = (struct range) {
@@ -601,7 +587,8 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
 	cxld_set_interleave(cxld, &ctrl);
 	cxld_set_type(cxld, &ctrl);
-	cxld_set_hpa(cxld, &base, &size);
+	base = cxld->hpa_range.start;
+	size = range_len(&cxld->hpa_range);
 
 	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
 	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 8e6ff3f39755..a073f16355ca 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1296,6 +1296,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 
 	cxled->cxld.interleave_ways = p->interleave_ways;
 	cxled->cxld.interleave_granularity = p->interleave_granularity;
+	cxled->cxld.hpa_range = (struct range) {
+		.start = p->res->start,
+		.end = p->res->end,
+	};
 
 	return 0;
 

[PATCH 3/4] cxl/region: Fix port setup uninitialized variable warnings

[Dan Williams]

0day robot reports:

drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'eiw'.
drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'peig'.
drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'peiw'.

...which are all valid reports. Add debug statement to consume the,
albeit unexpected, errors.

Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index a073f16355ca..5c931b6eb4e7 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1059,8 +1059,21 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 		parent_iw = parent_cxld->interleave_ways;
 	}
 
-	granularity_to_cxl(parent_ig, &peig);
-	ways_to_cxl(parent_iw, &peiw);
+	rc = granularity_to_cxl(parent_ig, &peig);
+	if (rc) {
+		dev_dbg(&cxlr->dev, "%s:%s: invalid parent granularity: %d\n",
+			dev_name(parent_port->uport),
+			dev_name(&parent_port->dev), parent_ig);
+		return rc;
+	}
+
+	rc = ways_to_cxl(parent_iw, &peiw);
+	if (rc) {
+		dev_dbg(&cxlr->dev, "%s:%s: invalid parent interleave: %d\n",
+			dev_name(parent_port->uport),
+			dev_name(&parent_port->dev), parent_iw);
+		return rc;
+	}
 
 	iw = cxl_rr->nr_targets;
 	ways_to_cxl(iw, &eiw);

[PATCH 4/4] cxl/region: Fix region commit uninitialized variable warning

[Dan Williams]

0day robot reports:

drivers/cxl/core/region.c:196 cxl_region_decode_commit() error: uninitialized symbol 'rc'.

The re-checking of loop termination conditions to determine "success"
makes it hard to see that @rc is initialized in all cases. Remove those
to make it explicit that @rc reflects a commit error and that the rest
of logic is concerned with unwinding committed decoders.

Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c931b6eb4e7..a68e4e0cf169 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -159,7 +159,7 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
 static int cxl_region_decode_commit(struct cxl_region *cxlr)
 {
 	struct cxl_region_params *p = &cxlr->params;
-	int i, rc;
+	int i, rc = 0;
 
 	for (i = 0; i < p->nr_targets; i++) {
 		struct cxl_endpoint_decoder *cxled = p->targets[i];
@@ -180,7 +180,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
 		}
 
 		/* success, all decoders up to the root are programmed */
-		if (is_cxl_root(iter))
+		if (rc == 0)
 			continue;
 
 		/* programming @iter failed, teardown */
@@ -192,14 +192,12 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
 		}
 
 		cxled->cxld.reset(&cxled->cxld);
-		if (i == 0)
-			return rc;
-		break;
+		goto err;
 	}
 
-	if (i >= p->nr_targets)
-		return 0;
+	return 0;
 
+err:
 	/* undo the targets that were successfully committed */
 	cxl_region_decode_reset(cxlr, i);
 	return rc;

Re: [PATCH 1/4] cxl/region: Fix decoder interleave programming

[Jonathan Cameron]

On Wed, 03 Aug 2022 00:24:23 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan notes:
> 
> "Curiously interleave ways = 1 for the EPs which is obviously wrong"
> 
> ...while testing the latest CXL development branch on QEMU.
> 
> It turns out the region creation process failed to program the endpoint
> decoders. This was missed because the default settings of x1 at 4K
> intereleave still results in the region appearing to function. Jonathan
> caught the bug by reverse mapping the translations that need to happen
> for the QEMU support.
> 
> Link: https://lore.kernel.org/r/62e95fdf9f6e2_30440294e4@dwillia2-xfh.jf.intel.com.notmuch
> Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Looks good to me.  Minimal testing done so far.
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/region.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index cf5d5811fe4c..8e6ff3f39755 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1294,6 +1294,9 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		p->state = CXL_CONFIG_ACTIVE;
>  	}
>  
> +	cxled->cxld.interleave_ways = p->interleave_ways;
> +	cxled->cxld.interleave_granularity = p->interleave_granularity;
> +
>  	return 0;
>  
>  err_decrement:
> 

Re: [PATCH 2/4] cxl/region: Move HPA setup to cxl_region_attach()

[Jonathan Cameron]

On Wed, 03 Aug 2022 00:24:29 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> A recent bug fix added the setup of the endpoint decoder interleave
> geometry settings to cxl_region_attach(). Move the HPA setup there as
> well to keep all endpoint decoder parameter setting in a central
> location.
> 
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Makes sense to me plus works for the random test I hit it with.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/hdm.c    |   17 ++---------------
>  drivers/cxl/core/region.c |    4 ++++
>  2 files changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 8143e2615957..5aadefd670d1 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -499,20 +499,6 @@ static void cxld_set_type(struct cxl_decoder *cxld, u32 *ctrl)
>  			  CXL_HDM_DECODER0_CTRL_TYPE);
>  }
>  
> -static void cxld_set_hpa(struct cxl_decoder *cxld, u64 *base, u64 *size)
> -{
> -	struct cxl_region *cxlr = cxld->region;
> -	struct cxl_region_params *p = &cxlr->params;
> -
> -	cxld->hpa_range = (struct range) {
> -		.start = p->res->start,
> -		.end = p->res->end,
> -	};
> -
> -	*base = p->res->start;
> -	*size = resource_size(p->res);
> -}
> -
>  static void cxld_clear_hpa(struct cxl_decoder *cxld)
>  {
>  	cxld->hpa_range = (struct range) {
> @@ -601,7 +587,8 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
>  	cxld_set_interleave(cxld, &ctrl);
>  	cxld_set_type(cxld, &ctrl);
> -	cxld_set_hpa(cxld, &base, &size);
> +	base = cxld->hpa_range.start;
> +	size = range_len(&cxld->hpa_range);
>  
>  	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
>  	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 8e6ff3f39755..a073f16355ca 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1296,6 +1296,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  
>  	cxled->cxld.interleave_ways = p->interleave_ways;
>  	cxled->cxld.interleave_granularity = p->interleave_granularity;
> +	cxled->cxld.hpa_range = (struct range) {
> +		.start = p->res->start,
> +		.end = p->res->end,
> +	};
>  
>  	return 0;
>  
> 

Re: [PATCH 3/4] cxl/region: Fix port setup uninitialized variable warnings

[Jonathan Cameron]

On Wed, 03 Aug 2022 00:24:34 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> 0day robot reports:
> 
> drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'eiw'.
> drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'peig'.
> drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'peiw'.
> 
> ...which are all valid reports. Add debug statement to consume the,
> albeit unexpected, errors.
> 
> Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/region.c |   17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index a073f16355ca..5c931b6eb4e7 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1059,8 +1059,21 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  		parent_iw = parent_cxld->interleave_ways;
>  	}
>  
> -	granularity_to_cxl(parent_ig, &peig);
> -	ways_to_cxl(parent_iw, &peiw);
> +	rc = granularity_to_cxl(parent_ig, &peig);
> +	if (rc) {
> +		dev_dbg(&cxlr->dev, "%s:%s: invalid parent granularity: %d\n",
> +			dev_name(parent_port->uport),
> +			dev_name(&parent_port->dev), parent_ig);
> +		return rc;
> +	}
> +
> +	rc = ways_to_cxl(parent_iw, &peiw);
> +	if (rc) {
> +		dev_dbg(&cxlr->dev, "%s:%s: invalid parent interleave: %d\n",
> +			dev_name(parent_port->uport),
> +			dev_name(&parent_port->dev), parent_iw);
> +		return rc;
> +	}
>  
>  	iw = cxl_rr->nr_targets;
>  	ways_to_cxl(iw, &eiw);
> 

Re: [PATCH 4/4] cxl/region: Fix region commit uninitialized variable warning

[Jonathan Cameron]

On Wed, 03 Aug 2022 00:24:41 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> 0day robot reports:
> 
> drivers/cxl/core/region.c:196 cxl_region_decode_commit() error: uninitialized symbol 'rc'.
> 
> The re-checking of loop termination conditions to determine "success"
> makes it hard to see that @rc is initialized in all cases. Remove those
> to make it explicit that @rc reflects a commit error and that the rest
> of logic is concerned with unwinding committed decoders.
> 
> Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
One comment inline but this looks basically fine to me.
Whilst it's more indented, I'd personally slightly prefer the
bad path indented rather than using continue for the good path.

Either way

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/region.c |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c931b6eb4e7..a68e4e0cf169 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -159,7 +159,7 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
>  static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> -	int i, rc;
> +	int i, rc = 0;
>  
>  	for (i = 0; i < p->nr_targets; i++) {
>  		struct cxl_endpoint_decoder *cxled = p->targets[i];
> @@ -180,7 +180,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  		}
>  
>  		/* success, all decoders up to the root are programmed */
> -		if (is_cxl_root(iter))
> +		if (rc == 0)
Personally slight preference for
if (rc) {
	error handling.
}

because it makes me think a tiny bit less :)

>  			continue;
>  
>  		/* programming @iter failed, teardown */
> @@ -192,14 +192,12 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  		}
>  
>  		cxled->cxld.reset(&cxled->cxld);
> -		if (i == 0)

Possibly worth calling out in patch description that
cxl_region_decode_reset() is a noop if i == 0
so this special path wasn't needed before this patch.

> -			return rc;
> -		break;
> +		goto err;
>  	}
>  
> -	if (i >= p->nr_targets)
> -		return 0;
> +	return 0;
>  
> +err:
>  	/* undo the targets that were successfully committed */
>  	cxl_region_decode_reset(cxlr, i);
>  	return rc;
> 

Re: [PATCH 3/4] cxl/region: Fix port setup uninitialized variable warnings

[Ira Weiny]

On Wed, Aug 03, 2022 at 12:24:34AM -0700, Dan Williams wrote:
> 0day robot reports:
> 
> drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'eiw'.
> drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'peig'.
> drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'peiw'.
> 
> ...which are all valid reports. Add debug statement to consume the,
> albeit unexpected, errors.
> 
> Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c |   17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index a073f16355ca..5c931b6eb4e7 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1059,8 +1059,21 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  		parent_iw = parent_cxld->interleave_ways;
>  	}
>  
> -	granularity_to_cxl(parent_ig, &peig);
> -	ways_to_cxl(parent_iw, &peiw);
> +	rc = granularity_to_cxl(parent_ig, &peig);
> +	if (rc) {
> +		dev_dbg(&cxlr->dev, "%s:%s: invalid parent granularity: %d\n",
> +			dev_name(parent_port->uport),
> +			dev_name(&parent_port->dev), parent_ig);
> +		return rc;
> +	}
> +
> +	rc = ways_to_cxl(parent_iw, &peiw);
> +	if (rc) {
> +		dev_dbg(&cxlr->dev, "%s:%s: invalid parent interleave: %d\n",
> +			dev_name(parent_port->uport),
> +			dev_name(&parent_port->dev), parent_iw);
> +		return rc;
> +	}
>  
>  	iw = cxl_rr->nr_targets;
>  	ways_to_cxl(iw, &eiw);

Do you need to do something here to fix the potential uninitialized use of eiw?

Ira

> 

Re: [PATCH 4/4] cxl/region: Fix region commit uninitialized variable warning

[Ira Weiny]

On Wed, Aug 03, 2022 at 12:24:41AM -0700, Dan Williams wrote:
> 0day robot reports:
> 
> drivers/cxl/core/region.c:196 cxl_region_decode_commit() error: uninitialized symbol 'rc'.
> 
> The re-checking of loop termination conditions to determine "success"
> makes it hard to see that @rc is initialized in all cases. Remove those
> to make it explicit that @rc reflects a commit error and that the rest
> of logic is concerned with unwinding committed decoders.
> 
> Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c931b6eb4e7..a68e4e0cf169 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -159,7 +159,7 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
>  static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> -	int i, rc;
> +	int i, rc = 0;
>  
>  	for (i = 0; i < p->nr_targets; i++) {
>  		struct cxl_endpoint_decoder *cxled = p->targets[i];
> @@ -180,7 +180,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  		}
>  
>  		/* success, all decoders up to the root are programmed */
> -		if (is_cxl_root(iter))
> +		if (rc == 0)
>  			continue;
>  
>  		/* programming @iter failed, teardown */
> @@ -192,14 +192,12 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  		}
>  
>  		cxled->cxld.reset(&cxled->cxld);
> -		if (i == 0)
> -			return rc;
> -		break;
> +		goto err;
>  	}
>  
> -	if (i >= p->nr_targets)
> -		return 0;
> +	return 0;
>  
> +err:
>  	/* undo the targets that were successfully committed */
>  	cxl_region_decode_reset(cxlr, i);

Doesn't this take care of teardown now?  Does the for loop even need to do
teardown now?

Ira

>  	return rc;
> 

Re: [PATCH 4/4] cxl/region: Fix region commit uninitialized variable warning

[Ira Weiny]

On Wed, Aug 03, 2022 at 02:49:37PM -0700, Ira wrote:
> On Wed, Aug 03, 2022 at 12:24:41AM -0700, Dan Williams wrote:
> > 0day robot reports:
> > 
> > drivers/cxl/core/region.c:196 cxl_region_decode_commit() error: uninitialized symbol 'rc'.
> > 
> > The re-checking of loop termination conditions to determine "success"
> > makes it hard to see that @rc is initialized in all cases. Remove those
> > to make it explicit that @rc reflects a commit error and that the rest
> > of logic is concerned with unwinding committed decoders.
> > 
> > Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/region.c |   12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 5c931b6eb4e7..a68e4e0cf169 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -159,7 +159,7 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
> >  static int cxl_region_decode_commit(struct cxl_region *cxlr)
> >  {
> >  	struct cxl_region_params *p = &cxlr->params;
> > -	int i, rc;
> > +	int i, rc = 0;
> >  
> >  	for (i = 0; i < p->nr_targets; i++) {
> >  		struct cxl_endpoint_decoder *cxled = p->targets[i];
> > @@ -180,7 +180,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
> >  		}
> >  
> >  		/* success, all decoders up to the root are programmed */
> > -		if (is_cxl_root(iter))
> > +		if (rc == 0)
> >  			continue;
> >  
> >  		/* programming @iter failed, teardown */
> > @@ -192,14 +192,12 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
> >  		}
> >  
> >  		cxled->cxld.reset(&cxled->cxld);
> > -		if (i == 0)
> > -			return rc;
> > -		break;
> > +		goto err;
> >  	}
> >  
> > -	if (i >= p->nr_targets)
> > -		return 0;
> > +	return 0;
> >  
> > +err:
> >  	/* undo the targets that were successfully committed */
> >  	cxl_region_decode_reset(cxlr, i);
> 
> Doesn't this take care of teardown now?  Does the for loop even need to do
> teardown now?

Never mind I think I see how this logic is the same.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> 
> Ira
> 
> >  	return rc;
> > 

Re: [PATCH 3/4] cxl/region: Fix port setup uninitialized variable warnings

[Dan Williams]

Ira Weiny wrote:
> On Wed, Aug 03, 2022 at 12:24:34AM -0700, Dan Williams wrote:
> > 0day robot reports:
> > 
> > drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'eiw'.
> > drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'peig'.
> > drivers/cxl/core/region.c:1068 cxl_port_setup_targets() error: uninitialized symbol 'peiw'.
> > 
> > ...which are all valid reports. Add debug statement to consume the,
> > albeit unexpected, errors.
> > 
> > Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/region.c |   17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index a073f16355ca..5c931b6eb4e7 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1059,8 +1059,21 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  		parent_iw = parent_cxld->interleave_ways;
> >  	}
> >  
> > -	granularity_to_cxl(parent_ig, &peig);
> > -	ways_to_cxl(parent_iw, &peiw);
> > +	rc = granularity_to_cxl(parent_ig, &peig);
> > +	if (rc) {
> > +		dev_dbg(&cxlr->dev, "%s:%s: invalid parent granularity: %d\n",
> > +			dev_name(parent_port->uport),
> > +			dev_name(&parent_port->dev), parent_ig);
> > +		return rc;
> > +	}
> > +
> > +	rc = ways_to_cxl(parent_iw, &peiw);
> > +	if (rc) {
> > +		dev_dbg(&cxlr->dev, "%s:%s: invalid parent interleave: %d\n",
> > +			dev_name(parent_port->uport),
> > +			dev_name(&parent_port->dev), parent_iw);
> > +		return rc;
> > +	}
> >  
> >  	iw = cxl_rr->nr_targets;
> >  	ways_to_cxl(iw, &eiw);
> 
> Do you need to do something here to fix the potential uninitialized use of eiw?

Yup, good catch. Added:

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 7b794147bf7d..ab99c1c3b2e9 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1004,7 +1004,13 @@ static int cxl_port_setup_targets(struct cxl_port *port,
        }
 
        iw = cxl_rr->nr_targets;
-       ways_to_cxl(iw, &eiw);
+       rc = ways_to_cxl(iw, &eiw);
+       if (rc) {
+               dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
+                       dev_name(port->uport), dev_name(&port->dev), iw);
+               return rc;
+       }
+
        if (cxl_rr->nr_targets > 1) {
                u32 address_bit = max(peig + peiw, eiw + peig);
 

Re: [PATCH 4/4] cxl/region: Fix region commit uninitialized variable warning

[Dan Williams]

Ira Weiny wrote:
> On Wed, Aug 03, 2022 at 12:24:41AM -0700, Dan Williams wrote:
> > 0day robot reports:
> > 
> > drivers/cxl/core/region.c:196 cxl_region_decode_commit() error: uninitialized symbol 'rc'.
> > 
> > The re-checking of loop termination conditions to determine "success"
> > makes it hard to see that @rc is initialized in all cases. Remove those
> > to make it explicit that @rc reflects a commit error and that the rest
> > of logic is concerned with unwinding committed decoders.
> > 
> > Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/region.c |   12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 5c931b6eb4e7..a68e4e0cf169 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -159,7 +159,7 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
> >  static int cxl_region_decode_commit(struct cxl_region *cxlr)
> >  {
> >  	struct cxl_region_params *p = &cxlr->params;
> > -	int i, rc;
> > +	int i, rc = 0;
> >  
> >  	for (i = 0; i < p->nr_targets; i++) {
> >  		struct cxl_endpoint_decoder *cxled = p->targets[i];
> > @@ -180,7 +180,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
> >  		}
> >  
> >  		/* success, all decoders up to the root are programmed */
> > -		if (is_cxl_root(iter))
> > +		if (rc == 0)
> >  			continue;
> >  
> >  		/* programming @iter failed, teardown */
> > @@ -192,14 +192,12 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
> >  		}
> >  
> >  		cxled->cxld.reset(&cxled->cxld);
> > -		if (i == 0)
> > -			return rc;
> > -		break;
> > +		goto err;
> >  	}
> >  
> > -	if (i >= p->nr_targets)
> > -		return 0;
> > +	return 0;
> >  
> > +err:
> >  	/* undo the targets that were successfully committed */
> >  	cxl_region_decode_reset(cxlr, i);
> 
> Doesn't this take care of teardown now?  Does the for loop even need to do
> teardown now?
> 

cxl_region_decode_reset() takes care of fully committed positions in the
region. The teardown within the for loop handles cleanups in the middle
of that process. I.e. cxl_region_decode_reset() would end up doing
unwind work on decoders that were never set up in the first instance if
it was called to handle all failures.

Re: [PATCH 4/4] cxl/region: Fix region commit uninitialized variable warning

[Dan Williams]

Ira Weiny wrote:
> On Wed, Aug 03, 2022 at 02:49:37PM -0700, Ira wrote:
> > On Wed, Aug 03, 2022 at 12:24:41AM -0700, Dan Williams wrote:
> > > 0day robot reports:
> > > 
> > > drivers/cxl/core/region.c:196 cxl_region_decode_commit() error: uninitialized symbol 'rc'.
> > > 
> > > The re-checking of loop termination conditions to determine "success"
> > > makes it hard to see that @rc is initialized in all cases. Remove those
> > > to make it explicit that @rc reflects a commit error and that the rest
> > > of logic is concerned with unwinding committed decoders.
> > > 
> > > Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  drivers/cxl/core/region.c |   12 +++++-------
> > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > > index 5c931b6eb4e7..a68e4e0cf169 100644
> > > --- a/drivers/cxl/core/region.c
> > > +++ b/drivers/cxl/core/region.c
> > > @@ -159,7 +159,7 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
> > >  static int cxl_region_decode_commit(struct cxl_region *cxlr)
> > >  {
> > >  	struct cxl_region_params *p = &cxlr->params;
> > > -	int i, rc;
> > > +	int i, rc = 0;
> > >  
> > >  	for (i = 0; i < p->nr_targets; i++) {
> > >  		struct cxl_endpoint_decoder *cxled = p->targets[i];
> > > @@ -180,7 +180,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
> > >  		}
> > >  
> > >  		/* success, all decoders up to the root are programmed */
> > > -		if (is_cxl_root(iter))
> > > +		if (rc == 0)
> > >  			continue;
> > >  
> > >  		/* programming @iter failed, teardown */
> > > @@ -192,14 +192,12 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
> > >  		}
> > >  
> > >  		cxled->cxld.reset(&cxled->cxld);
> > > -		if (i == 0)
> > > -			return rc;
> > > -		break;
> > > +		goto err;
> > >  	}
> > >  
> > > -	if (i >= p->nr_targets)
> > > -		return 0;
> > > +	return 0;
> > >  
> > > +err:
> > >  	/* undo the targets that were successfully committed */
> > >  	cxl_region_decode_reset(cxlr, i);
> > 
> > Doesn't this take care of teardown now?  Does the for loop even need to do
> > teardown now?
> 
> Never mind I think I see how this logic is the same.

Ah cool, disregard my last reply.

> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Thanks!