| Message ID | 165951148105.967013.14191992449932268431.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Accepted |
| Commit | 69c9961387f244077101de3ce4e272717617dc87 |
| Headers | show |
| Series | cxl/region: Endpoint decoder fixes | expand |
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; >
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; >
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; > >
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.
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!
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;
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(-)