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

[Dan Williams <dan.j.williams@intel.com>]

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!