| Message ID | 20240206121301.7225-3-fabio.maria.de.francesco@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add cond_guard() to conditional guards | expand |
On 2/6/24 5:13 AM, Fabio M. De Francesco wrote: > Use cond_guard() in show_target() to not open code an up_read() in an 'out' > block. If the down_read_interruptible() fails, the statement passed to the > second argument of cond_guard() returns -EINTR. > > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Suggested-by: Ira Weiny <ira.weiny@intel.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/region.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 0f05692bfec3..eb5c36462c0a 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -666,28 +666,20 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos) > { > struct cxl_region_params *p = &cxlr->params; > struct cxl_endpoint_decoder *cxled; > - int rc; > > - rc = down_read_interruptible(&cxl_region_rwsem); > - if (rc) > - return rc; > + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); > > if (pos >= p->interleave_ways) { > dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos, > p->interleave_ways); > - rc = -ENXIO; > - goto out; > + return -ENXIO; > } > > cxled = p->targets[pos]; > if (!cxled) > - rc = sysfs_emit(buf, "\n"); > - else > - rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev)); > -out: > - up_read(&cxl_region_rwsem); > + return sysfs_emit(buf, "\n"); > > - return rc; > + return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev)); > } > > static int match_free_decoder(struct device *dev, void *data)
Hi Fabio, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.8-rc3 next-20240206] [cannot apply to cxl/next cxl/pending] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Fabio-M-De-Francesco/cleanup-Add-cond_guard-to-conditional-guards/20240206-203110 base: linus/master patch link: https://lore.kernel.org/r/20240206121301.7225-3-fabio.maria.de.francesco%40linux.intel.com patch subject: [PATCH 2/2 v3] cxl/region: Use cond_guard() in show_targetN() config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240207/202402070919.0zuYCxMS-lkp@intel.com/config) compiler: s390-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240207/202402070919.0zuYCxMS-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202402070919.0zuYCxMS-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/cxl/core/region.c: In function 'show_targetN': >> drivers/cxl/core/region.c:670:70: warning: suggest braces around empty body in an 'else' statement [-Wempty-body] 670 | cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); | ^ vim +/else +670 drivers/cxl/core/region.c 664 665 static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos) 666 { 667 struct cxl_region_params *p = &cxlr->params; 668 struct cxl_endpoint_decoder *cxled; 669 > 670 cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); 671 672 if (pos >= p->interleave_ways) { 673 dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos, 674 p->interleave_ways); 675 return -ENXIO; 676 } 677 678 cxled = p->targets[pos]; 679 if (!cxled) 680 return sysfs_emit(buf, "\n"); 681 682 return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev)); 683 } 684
On Wednesday, 7 February 2024 02:54:34 CET kernel test robot wrote: > Hi Fabio, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on linus/master] > [also build test WARNING on v6.8-rc3 next-20240206] > [cannot apply to cxl/next cxl/pending] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: > https://github.com/intel-lab-lkp/linux/commits/Fabio-M-De-Francesco/cleanup > -Add-cond_guard-to-conditional-guards/20240206-203110 base: linus/master > patch link: > https://lore.kernel.org/r/20240206121301.7225-3-fabio.maria.de.francesco%40 > linux.intel.com patch subject: [PATCH 2/2 v3] cxl/region: Use cond_guard() > in show_targetN() config: s390-allyesconfig > (https://download.01.org/0day-ci/archive/20240207/202402070919.0zuYCxMS-lkp > @intel.com/config) compiler: s390-linux-gcc (GCC) 13.2.0 > reproduce (this is a W=1 build): > (https://download.01.org/0day-ci/archive/20240207/202402070919.0zuYCxMS-lkp > @intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version > of the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: > | https://lore.kernel.org/oe-kbuild-all/202402070919.0zuYCxMS-lkp@intel.com > | / > All warnings (new ones prefixed by >>): > > drivers/cxl/core/region.c: In function 'show_targetN': > >> drivers/cxl/core/region.c:670:70: warning: suggest braces around empty > >> body in an 'else' statement [-Wempty-body] > 670 | cond_guard(rwsem_read_intr, return -EINTR, > &cxl_region_rwsem); > | ^ I think that this warning deserves attention and braces should be added around the 'else' empty body. I'm going to send v4: #define cond_guard(_name, _ret, args...) \ CLASS(_name, scope)(args); \ if (!__guard_ptr(_name)(&scope)) _ret; \ else { } Any comments? Fabio > > vim +/else +670 drivers/cxl/core/region.c > > 664 > 665 static size_t show_targetN(struct cxl_region *cxlr, char *buf, int > pos) 666 { > 667 struct cxl_region_params *p = &cxlr->params; > 668 struct cxl_endpoint_decoder *cxled; > 669 > > > 670 cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); > > 671 > 672 if (pos >= p->interleave_ways) { > 673 dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos, > 674 p->interleave_ways); > 675 return -ENXIO; > 676 } > 677 > 678 cxled = p->targets[pos]; > 679 if (!cxled) > 680 return sysfs_emit(buf, "\n"); > 681 > 682 return sysfs_emit(buf, "%s\n", dev_name(&cxled- >cxld.dev)); > 683 } > 684
Fabio M. De Francesco wrote: > On Wednesday, 7 February 2024 02:54:34 CET kernel test robot wrote: > > Hi Fabio, > > > > kernel test robot noticed the following build warnings: > > > > [auto build test WARNING on linus/master] > > [also build test WARNING on v6.8-rc3 next-20240206] > > [cannot apply to cxl/next cxl/pending] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > url: > > https://github.com/intel-lab-lkp/linux/commits/Fabio-M-De-Francesco/cleanup > > -Add-cond_guard-to-conditional-guards/20240206-203110 base: linus/master > > patch link: > > https://lore.kernel.org/r/20240206121301.7225-3-fabio.maria.de.francesco%40 > > linux.intel.com patch subject: [PATCH 2/2 v3] cxl/region: Use cond_guard() > > in show_targetN() config: s390-allyesconfig > > (https://download.01.org/0day-ci/archive/20240207/202402070919.0zuYCxMS-lkp > > @intel.com/config) compiler: s390-linux-gcc (GCC) 13.2.0 > > reproduce (this is a W=1 build): > > (https://download.01.org/0day-ci/archive/20240207/202402070919.0zuYCxMS-lkp > > @intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version > > of the same patch/commit), kindly add following tags > > > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: > > | https://lore.kernel.org/oe-kbuild-all/202402070919.0zuYCxMS-lkp@intel.com > > | / > > All warnings (new ones prefixed by >>): > > > > drivers/cxl/core/region.c: In function 'show_targetN': > > >> drivers/cxl/core/region.c:670:70: warning: suggest braces around empty > > >> body in an 'else' statement [-Wempty-body] > > 670 | cond_guard(rwsem_read_intr, return -EINTR, > > &cxl_region_rwsem); > > | > ^ > > I think that this warning deserves attention and braces should be added around > the 'else' empty body. I'm going to send v4: > > #define cond_guard(_name, _ret, args...) \ > CLASS(_name, scope)(args); \ > if (!__guard_ptr(_name)(&scope)) _ret; \ > else { } > I think this is a good addition. If the user forgets ';' at the end of the cond_guard() we could have a hidden side effect similar to what Dan was concerned about... This guarantees that won't happen. Score one for the bots! Ira
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 0f05692bfec3..eb5c36462c0a 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -666,28 +666,20 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos) { struct cxl_region_params *p = &cxlr->params; struct cxl_endpoint_decoder *cxled; - int rc; - rc = down_read_interruptible(&cxl_region_rwsem); - if (rc) - return rc; + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); if (pos >= p->interleave_ways) { dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos, p->interleave_ways); - rc = -ENXIO; - goto out; + return -ENXIO; } cxled = p->targets[pos]; if (!cxled) - rc = sysfs_emit(buf, "\n"); - else - rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev)); -out: - up_read(&cxl_region_rwsem); + return sysfs_emit(buf, "\n"); - return rc; + return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev)); } static int match_free_decoder(struct device *dev, void *data)