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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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;
>