* [PATCH 0/3] Fix clang -Wunused-but-set-variable warnings @ 2021-07-14 9:17 Bill Wendling 2021-07-14 9:17 ` [PATCH 1/3] base: remove unused variable 'no_warn' Bill Wendling ` (3 more replies) 0 siblings, 4 replies; 36+ messages in thread From: Bill Wendling @ 2021-07-14 9:17 UTC (permalink / raw) To: linux-kernel, netdev, linux-scsi, clang-built-linux, Nathan Chancellor, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen Cc: Bill Wendling These patches clean up warnings from clang's '-Wunused-but-set-variable' flag. Bill Wendling (3): base: remove unused variable 'no_warn' bnx2x: remove unused variable 'cur_data_offset' scsi: qla2xxx: remove unused variable 'status' drivers/base/module.c | 6 ++---- drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 6 ------ drivers/scsi/qla2xxx/qla_nx.c | 2 -- 3 files changed, 2 insertions(+), 12 deletions(-) -- 2.32.0.93.g670b81a890-goog ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/3] base: remove unused variable 'no_warn' 2021-07-14 9:17 [PATCH 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling @ 2021-07-14 9:17 ` Bill Wendling 2021-07-14 12:15 ` kernel test robot 2021-07-14 9:17 ` [PATCH 2/3] bnx2x: remove unused variable 'cur_data_offset' Bill Wendling ` (2 subsequent siblings) 3 siblings, 1 reply; 36+ messages in thread From: Bill Wendling @ 2021-07-14 9:17 UTC (permalink / raw) To: linux-kernel, netdev, linux-scsi, clang-built-linux, Nathan Chancellor, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen Cc: Bill Wendling Fix the following build warning: drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] int no_warn; Signed-off-by: Bill Wendling <morbo@google.com> --- drivers/base/module.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/base/module.c b/drivers/base/module.c index 46ad4d636731..81d84a066a38 100644 --- a/drivers/base/module.c +++ b/drivers/base/module.c @@ -33,7 +33,6 @@ static void module_create_drivers_dir(struct module_kobject *mk) void module_add_driver(struct module *mod, struct device_driver *drv) { char *driver_name; - int no_warn; struct module_kobject *mk = NULL; if (!drv) @@ -59,12 +58,11 @@ void module_add_driver(struct module *mod, struct device_driver *drv) return; /* Don't check return codes; these calls are idempotent */ - no_warn = sysfs_create_link(&drv->p->kobj, &mk->kobj, "module"); + sysfs_create_link(&drv->p->kobj, &mk->kobj, "module"); driver_name = make_driver_name(drv); if (driver_name) { module_create_drivers_dir(mk); - no_warn = sysfs_create_link(mk->drivers_dir, &drv->p->kobj, - driver_name); + sysfs_create_link(mk->drivers_dir, &drv->p->kobj, driver_name); kfree(driver_name); } } -- 2.32.0.93.g670b81a890-goog ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 1/3] base: remove unused variable 'no_warn' 2021-07-14 9:17 ` [PATCH 1/3] base: remove unused variable 'no_warn' Bill Wendling @ 2021-07-14 12:15 ` kernel test robot 0 siblings, 0 replies; 36+ messages in thread From: kernel test robot @ 2021-07-14 12:15 UTC (permalink / raw) To: Bill Wendling, linux-kernel, netdev, linux-scsi, clang-built-linux, Nathan Chancellor, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller Cc: kbuild-all [-- Attachment #1: Type: text/plain, Size: 3478 bytes --] Hi Bill, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on scsi/for-next] [also build test WARNING on linux/master driver-core/driver-core-testing mkp-scsi/for-next ipvs/master linus/master v5.14-rc1 next-20210714] [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] url: https://github.com/0day-ci/linux/commits/Bill-Wendling/Fix-clang-Wunused-but-set-variable-warnings/20210714-172029 base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next config: powerpc-redwood_defconfig (attached as .config) compiler: powerpc-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/f6c4b007fc8c907719d883faae424f2cf3bb100c git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Bill-Wendling/Fix-clang-Wunused-but-set-variable-warnings/20210714-172029 git checkout f6c4b007fc8c907719d883faae424f2cf3bb100c # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): drivers/base/module.c: In function 'module_add_driver': >> drivers/base/module.c:61:2: warning: ignoring return value of 'sysfs_create_link', declared with attribute warn_unused_result [-Wunused-result] 61 | sysfs_create_link(&drv->p->kobj, &mk->kobj, "module"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/base/module.c:65:3: warning: ignoring return value of 'sysfs_create_link', declared with attribute warn_unused_result [-Wunused-result] 65 | sysfs_create_link(mk->drivers_dir, &drv->p->kobj, driver_name); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +/sysfs_create_link +61 drivers/base/module.c 32 33 void module_add_driver(struct module *mod, struct device_driver *drv) 34 { 35 char *driver_name; 36 struct module_kobject *mk = NULL; 37 38 if (!drv) 39 return; 40 41 if (mod) 42 mk = &mod->mkobj; 43 else if (drv->mod_name) { 44 struct kobject *mkobj; 45 46 /* Lookup built-in module entry in /sys/modules */ 47 mkobj = kset_find_obj(module_kset, drv->mod_name); 48 if (mkobj) { 49 mk = container_of(mkobj, struct module_kobject, kobj); 50 /* remember our module structure */ 51 drv->p->mkobj = mk; 52 /* kset_find_obj took a reference */ 53 kobject_put(mkobj); 54 } 55 } 56 57 if (!mk) 58 return; 59 60 /* Don't check return codes; these calls are idempotent */ > 61 sysfs_create_link(&drv->p->kobj, &mk->kobj, "module"); 62 driver_name = make_driver_name(drv); 63 if (driver_name) { 64 module_create_drivers_dir(mk); 65 sysfs_create_link(mk->drivers_dir, &drv->p->kobj, driver_name); 66 kfree(driver_name); 67 } 68 } 69 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 14516 bytes --] ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/3] base: remove unused variable 'no_warn' @ 2021-07-14 12:15 ` kernel test robot 0 siblings, 0 replies; 36+ messages in thread From: kernel test robot @ 2021-07-14 12:15 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 3562 bytes --] Hi Bill, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on scsi/for-next] [also build test WARNING on linux/master driver-core/driver-core-testing mkp-scsi/for-next ipvs/master linus/master v5.14-rc1 next-20210714] [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] url: https://github.com/0day-ci/linux/commits/Bill-Wendling/Fix-clang-Wunused-but-set-variable-warnings/20210714-172029 base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next config: powerpc-redwood_defconfig (attached as .config) compiler: powerpc-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/f6c4b007fc8c907719d883faae424f2cf3bb100c git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Bill-Wendling/Fix-clang-Wunused-but-set-variable-warnings/20210714-172029 git checkout f6c4b007fc8c907719d883faae424f2cf3bb100c # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): drivers/base/module.c: In function 'module_add_driver': >> drivers/base/module.c:61:2: warning: ignoring return value of 'sysfs_create_link', declared with attribute warn_unused_result [-Wunused-result] 61 | sysfs_create_link(&drv->p->kobj, &mk->kobj, "module"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/base/module.c:65:3: warning: ignoring return value of 'sysfs_create_link', declared with attribute warn_unused_result [-Wunused-result] 65 | sysfs_create_link(mk->drivers_dir, &drv->p->kobj, driver_name); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +/sysfs_create_link +61 drivers/base/module.c 32 33 void module_add_driver(struct module *mod, struct device_driver *drv) 34 { 35 char *driver_name; 36 struct module_kobject *mk = NULL; 37 38 if (!drv) 39 return; 40 41 if (mod) 42 mk = &mod->mkobj; 43 else if (drv->mod_name) { 44 struct kobject *mkobj; 45 46 /* Lookup built-in module entry in /sys/modules */ 47 mkobj = kset_find_obj(module_kset, drv->mod_name); 48 if (mkobj) { 49 mk = container_of(mkobj, struct module_kobject, kobj); 50 /* remember our module structure */ 51 drv->p->mkobj = mk; 52 /* kset_find_obj took a reference */ 53 kobject_put(mkobj); 54 } 55 } 56 57 if (!mk) 58 return; 59 60 /* Don't check return codes; these calls are idempotent */ > 61 sysfs_create_link(&drv->p->kobj, &mk->kobj, "module"); 62 driver_name = make_driver_name(drv); 63 if (driver_name) { 64 module_create_drivers_dir(mk); 65 sysfs_create_link(mk->drivers_dir, &drv->p->kobj, driver_name); 66 kfree(driver_name); 67 } 68 } 69 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 14516 bytes --] ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/3] bnx2x: remove unused variable 'cur_data_offset' 2021-07-14 9:17 [PATCH 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling 2021-07-14 9:17 ` [PATCH 1/3] base: remove unused variable 'no_warn' Bill Wendling @ 2021-07-14 9:17 ` Bill Wendling 2021-07-14 9:17 ` [PATCH 3/3] scsi: qla2xxx: remove unused variable 'status' Bill Wendling 2021-07-26 20:19 ` [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling 3 siblings, 0 replies; 36+ messages in thread From: Bill Wendling @ 2021-07-14 9:17 UTC (permalink / raw) To: linux-kernel, netdev, linux-scsi, clang-built-linux, Nathan Chancellor, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen Cc: Bill Wendling Fix the clang build warning: drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c:1862:13: error: variable 'cur_data_offset' set but not used [-Werror,-Wunused-but-set-variable] dma_addr_t cur_data_offset; Signed-off-by: Bill Wendling <morbo@google.com> --- drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c index 27943b0446c2..f255fd0b16db 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c @@ -1858,7 +1858,6 @@ void bnx2x_iov_adjust_stats_req(struct bnx2x *bp) { int i; int first_queue_query_index, num_queues_req; - dma_addr_t cur_data_offset; struct stats_query_entry *cur_query_entry; u8 stats_count = 0; bool is_fcoe = false; @@ -1879,10 +1878,6 @@ void bnx2x_iov_adjust_stats_req(struct bnx2x *bp) BNX2X_NUM_ETH_QUEUES(bp), is_fcoe, first_queue_query_index, first_queue_query_index + num_queues_req); - cur_data_offset = bp->fw_stats_data_mapping + - offsetof(struct bnx2x_fw_stats_data, queue_stats) + - num_queues_req * sizeof(struct per_queue_stats); - cur_query_entry = &bp->fw_stats_req-> query[first_queue_query_index + num_queues_req]; @@ -1933,7 +1928,6 @@ void bnx2x_iov_adjust_stats_req(struct bnx2x *bp) cur_query_entry->funcID, j, cur_query_entry->index); cur_query_entry++; - cur_data_offset += sizeof(struct per_queue_stats); stats_count++; /* all stats are coalesced to the leading queue */ -- 2.32.0.93.g670b81a890-goog ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 3/3] scsi: qla2xxx: remove unused variable 'status' 2021-07-14 9:17 [PATCH 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling 2021-07-14 9:17 ` [PATCH 1/3] base: remove unused variable 'no_warn' Bill Wendling 2021-07-14 9:17 ` [PATCH 2/3] bnx2x: remove unused variable 'cur_data_offset' Bill Wendling @ 2021-07-14 9:17 ` Bill Wendling 2021-07-26 20:19 ` [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling 3 siblings, 0 replies; 36+ messages in thread From: Bill Wendling @ 2021-07-14 9:17 UTC (permalink / raw) To: linux-kernel, netdev, linux-scsi, clang-built-linux, Nathan Chancellor, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen Cc: Bill Wendling Fix the clang build warning: drivers/scsi/qla2xxx/qla_nx.c:2209:6: error: variable 'status' set but not used [-Werror,-Wunused-but-set-variable] int status = 0; Signed-off-by: Bill Wendling <morbo@google.com> --- drivers/scsi/qla2xxx/qla_nx.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c index 615e44af1ca6..11aad97dfca8 100644 --- a/drivers/scsi/qla2xxx/qla_nx.c +++ b/drivers/scsi/qla2xxx/qla_nx.c @@ -2166,7 +2166,6 @@ qla82xx_poll(int irq, void *dev_id) struct qla_hw_data *ha; struct rsp_que *rsp; struct device_reg_82xx __iomem *reg; - int status = 0; uint32_t stat; uint32_t host_int = 0; uint16_t mb[8]; @@ -2195,7 +2194,6 @@ qla82xx_poll(int irq, void *dev_id) case 0x10: case 0x11: qla82xx_mbx_completion(vha, MSW(stat)); - status |= MBX_INTERRUPT; break; case 0x12: mb[0] = MSW(stat); -- 2.32.0.93.g670b81a890-goog ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings 2021-07-14 9:17 [PATCH 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling ` (2 preceding siblings ...) 2021-07-14 9:17 ` [PATCH 3/3] scsi: qla2xxx: remove unused variable 'status' Bill Wendling @ 2021-07-26 20:19 ` Bill Wendling 2021-07-26 20:19 ` [PATCH v2 1/3] base: mark 'no_warn' as unused Bill Wendling ` (3 more replies) 3 siblings, 4 replies; 36+ messages in thread From: Bill Wendling @ 2021-07-26 20:19 UTC (permalink / raw) To: linux-kernel, netdev, linux-scsi, clang-built-linux, Nathan Chancellor, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen Cc: Bill Wendling These patches clean up warnings from clang's '-Wunused-but-set-variable' flag. Changes for v2: - Mark "no_warn" as "__maybe_unused" to avoid separate warning. Bill Wendling (3): base: mark 'no_warn' as unused bnx2x: remove unused variable 'cur_data_offset' scsi: qla2xxx: remove unused variable 'status' drivers/base/module.c | 2 +- drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 6 ------ drivers/scsi/qla2xxx/qla_nx.c | 2 -- 3 files changed, 1 insertion(+), 9 deletions(-) -- 2.32.0.432.gabb21c7263-goog ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-26 20:19 ` [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling @ 2021-07-26 20:19 ` Bill Wendling 2021-07-26 20:47 ` Nathan Chancellor 2021-07-26 20:19 ` [PATCH v2 2/3] bnx2x: remove unused variable 'cur_data_offset' Bill Wendling ` (2 subsequent siblings) 3 siblings, 1 reply; 36+ messages in thread From: Bill Wendling @ 2021-07-26 20:19 UTC (permalink / raw) To: linux-kernel, netdev, linux-scsi, clang-built-linux, Nathan Chancellor, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen Cc: Bill Wendling Fix the following build warning: drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] int no_warn; This variable is used to remove another warning, but causes a warning itself. Mark it as 'unused' to avoid that. Signed-off-by: Bill Wendling <morbo@google.com> --- drivers/base/module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/module.c b/drivers/base/module.c index 46ad4d636731..10494336d601 100644 --- a/drivers/base/module.c +++ b/drivers/base/module.c @@ -33,7 +33,7 @@ static void module_create_drivers_dir(struct module_kobject *mk) void module_add_driver(struct module *mod, struct device_driver *drv) { char *driver_name; - int no_warn; + int __maybe_unused no_warn; struct module_kobject *mk = NULL; if (!drv) -- 2.32.0.432.gabb21c7263-goog ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-26 20:19 ` [PATCH v2 1/3] base: mark 'no_warn' as unused Bill Wendling @ 2021-07-26 20:47 ` Nathan Chancellor 2021-07-26 21:01 ` Bill Wendling 2021-07-27 5:27 ` Greg Kroah-Hartman 0 siblings, 2 replies; 36+ messages in thread From: Nathan Chancellor @ 2021-07-26 20:47 UTC (permalink / raw) To: Bill Wendling, Greg Kroah-Hartman, Rafael J. Wysocki Cc: Nick Desaulniers, clang-built-linux, linux-kernel + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c respectively, drop everyone else. Original post: https://lore.kernel.org/r/20210726201924.3202278-2-morbo@google.com/ On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > Fix the following build warning: > > drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] > int no_warn; > > This variable is used to remove another warning, but causes a warning > itself. Mark it as 'unused' to avoid that. > > Signed-off-by: Bill Wendling <morbo@google.com> Even though they evaluate to the same thing, it might be worth using "__always_unused" here because it is :) Regardless: Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > drivers/base/module.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/module.c b/drivers/base/module.c > index 46ad4d636731..10494336d601 100644 > --- a/drivers/base/module.c > +++ b/drivers/base/module.c > @@ -33,7 +33,7 @@ static void module_create_drivers_dir(struct module_kobject *mk) > void module_add_driver(struct module *mod, struct device_driver *drv) > { > char *driver_name; > - int no_warn; > + int __maybe_unused no_warn; > struct module_kobject *mk = NULL; > > if (!drv) > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-26 20:47 ` Nathan Chancellor @ 2021-07-26 21:01 ` Bill Wendling 2021-07-27 5:27 ` Greg Kroah-Hartman 1 sibling, 0 replies; 36+ messages in thread From: Bill Wendling @ 2021-07-26 21:01 UTC (permalink / raw) To: Nathan Chancellor Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Nick Desaulniers, clang-built-linux, LKML On Mon, Jul 26, 2021 at 1:47 PM Nathan Chancellor <nathan@kernel.org> wrote: > > + Greg and Rafael as the maintainer and reviewer of > drivers/base/module.c respectively, drop everyone else. > > Original post: > > https://lore.kernel.org/r/20210726201924.3202278-2-morbo@google.com/ > > On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > > Fix the following build warning: > > > > drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] > > int no_warn; > > > > This variable is used to remove another warning, but causes a warning > > itself. Mark it as 'unused' to avoid that. > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > Even though they evaluate to the same thing, it might be worth using > "__always_unused" here because it is :) > I thought about that, but went with the softer option in the (probably very) unlikely event that it will be used in the future. :-) I'll be happy to resubmit a new patch though. -bw > Regardless: > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > > --- > > drivers/base/module.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/base/module.c b/drivers/base/module.c > > index 46ad4d636731..10494336d601 100644 > > --- a/drivers/base/module.c > > +++ b/drivers/base/module.c > > @@ -33,7 +33,7 @@ static void module_create_drivers_dir(struct module_kobject *mk) > > void module_add_driver(struct module *mod, struct device_driver *drv) > > { > > char *driver_name; > > - int no_warn; > > + int __maybe_unused no_warn; > > struct module_kobject *mk = NULL; > > > > if (!drv) > > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-26 20:47 ` Nathan Chancellor 2021-07-26 21:01 ` Bill Wendling @ 2021-07-27 5:27 ` Greg Kroah-Hartman 2021-07-27 6:15 ` Bill Wendling 1 sibling, 1 reply; 36+ messages in thread From: Greg Kroah-Hartman @ 2021-07-27 5:27 UTC (permalink / raw) To: Nathan Chancellor Cc: Bill Wendling, Rafael J. Wysocki, Nick Desaulniers, clang-built-linux, linux-kernel On Mon, Jul 26, 2021 at 01:47:33PM -0700, Nathan Chancellor wrote: > + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c > respectively, drop everyone else. Odd no one cc:ed us originally, I guess they didn't want the patch ever merged? :( > > Original post: > > https://lore.kernel.org/r/20210726201924.3202278-2-morbo@google.com/ > > On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > > Fix the following build warning: > > > > drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] > > int no_warn; That's not going to be a good warning to ever have the kernel use due to how lots of hardware works (i.e. we need to do a read after a write but we can throw the read away as it does not matter). > > > > This variable is used to remove another warning, but causes a warning > > itself. Mark it as 'unused' to avoid that. > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > Even though they evaluate to the same thing, it might be worth using > "__always_unused" here because it is :) But it is not unused, the value is written into it. So this isn't ok, sometimes we want to write to variables but never care about the value, that does not mean the compiler should complain about it. thanks, greg k-h ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 5:27 ` Greg Kroah-Hartman @ 2021-07-27 6:15 ` Bill Wendling 2021-07-27 6:41 ` Greg Kroah-Hartman 0 siblings, 1 reply; 36+ messages in thread From: Bill Wendling @ 2021-07-27 6:15 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Nathan Chancellor, Rafael J. Wysocki, Nick Desaulniers, clang-built-linux, LKML On Mon, Jul 26, 2021 at 10:27 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Mon, Jul 26, 2021 at 01:47:33PM -0700, Nathan Chancellor wrote: > > + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c > > respectively, drop everyone else. > > Odd no one cc:ed us originally, I guess they didn't want the patch ever > merged? :( > > > > > Original post: > > > > https://lore.kernel.org/r/20210726201924.3202278-2-morbo@google.com/ > > > > On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > > > Fix the following build warning: > > > > > > drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] > > > int no_warn; > > That's not going to be a good warning to ever have the kernel use due to > how lots of hardware works (i.e. we need to do a read after a write but > we can throw the read away as it does not matter). > > > > > > > > This variable is used to remove another warning, but causes a warning > > > itself. Mark it as 'unused' to avoid that. > > > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > > > Even though they evaluate to the same thing, it might be worth using > > "__always_unused" here because it is :) > > But it is not unused, the value is written into it. > I believe that only matters if the variable is marked "volatile". Otherwise, the variable itself is never used. A "variable that's written to but not read from," in fact, is the whole reason for the warning. > So this isn't ok, sometimes we want to write to variables but never care > about the value, that does not mean the compiler should complain about > it. > Typically, if you don't care about the return value, you simply don't assign it to a variable (cf. printf). However, the functions that assign to "no_warn" have the "warn_unused_result" attribute. The fact that the variable is named "no_warn" seems to indicate that it's meant to remain unused, even if it probably should be checked. Would you rather the warning be turned off on some level? -bw ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 6:15 ` Bill Wendling @ 2021-07-27 6:41 ` Greg Kroah-Hartman 2021-07-27 7:08 ` Bill Wendling 2021-07-27 17:39 ` Nick Desaulniers 0 siblings, 2 replies; 36+ messages in thread From: Greg Kroah-Hartman @ 2021-07-27 6:41 UTC (permalink / raw) To: Bill Wendling Cc: Nathan Chancellor, Rafael J. Wysocki, Nick Desaulniers, clang-built-linux, LKML On Mon, Jul 26, 2021 at 11:15:52PM -0700, Bill Wendling wrote: > On Mon, Jul 26, 2021 at 10:27 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Mon, Jul 26, 2021 at 01:47:33PM -0700, Nathan Chancellor wrote: > > > + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c > > > respectively, drop everyone else. > > > > Odd no one cc:ed us originally, I guess they didn't want the patch ever > > merged? :( > > > > > > > > Original post: > > > > > > https://lore.kernel.org/r/20210726201924.3202278-2-morbo@google.com/ > > > > > > On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > > > > Fix the following build warning: > > > > > > > > drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] > > > > int no_warn; > > > > That's not going to be a good warning to ever have the kernel use due to > > how lots of hardware works (i.e. we need to do a read after a write but > > we can throw the read away as it does not matter). > > > > > > > > > > > > This variable is used to remove another warning, but causes a warning > > > > itself. Mark it as 'unused' to avoid that. > > > > > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > > > > > Even though they evaluate to the same thing, it might be worth using > > > "__always_unused" here because it is :) > > > > But it is not unused, the value is written into it. > > > I believe that only matters if the variable is marked "volatile". "volatile" means nothing anymore, never use it or even think about it again please :) > Otherwise, the variable itself is never used. A "variable that's > written to but not read from," in fact, is the whole reason for the > warning. But that is ok! Sometimes you need to do this with hardware (like all PCI devices). This is a legitimate code flow for many hardware types and if a C compiler thinks that this is not ok, then it is broken. So be VERY careful when changing drivers based on this warning. Because of this, I do not think you can enable it over the whole kernel without causing major problems in some areas. But that is independent of this specific issue you are trying to patch here, I say this to warn you of a number of stupid places where people have tried to "optimize away" reads based on this compiler warning in drivers, and we have had to add them back because it broke functionality. > > So this isn't ok, sometimes we want to write to variables but never care > > about the value, that does not mean the compiler should complain about > > it. > > > Typically, if you don't care about the return value, you simply don't > assign it to a variable (cf. printf). However, the functions that > assign to "no_warn" have the "warn_unused_result" attribute. The fact > that the variable is named "no_warn" seems to indicate that it's meant > to remain unused, even if it probably should be checked. These functions have warn_unused_result set on them because for 99% of the time, I want the value to be checked. But as you can see in this use, as per the comments in the code, we do not care about the result for a very good reason. So we just assign it to a variable to make the compiler quiet. > Would you rather the warning be turned off on some level? Which warning? The code here, as-is, is correct. We already have 1 compiler warning work around in place, do you want to add another one? How many can we stack on top of each other? And again, why did you not cc: the maintainers of this code for this change? That's not good... thanks, greg k-h ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 6:41 ` Greg Kroah-Hartman @ 2021-07-27 7:08 ` Bill Wendling 2021-07-27 7:12 ` Greg Kroah-Hartman 2021-07-27 17:39 ` Nick Desaulniers 1 sibling, 1 reply; 36+ messages in thread From: Bill Wendling @ 2021-07-27 7:08 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Nathan Chancellor, Rafael J. Wysocki, Nick Desaulniers, clang-built-linux, LKML On Mon, Jul 26, 2021 at 11:41 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Mon, Jul 26, 2021 at 11:15:52PM -0700, Bill Wendling wrote: > > On Mon, Jul 26, 2021 at 10:27 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > On Mon, Jul 26, 2021 at 01:47:33PM -0700, Nathan Chancellor wrote: > > > > + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c > > > > respectively, drop everyone else. > > > > > > Odd no one cc:ed us originally, I guess they didn't want the patch ever > > > merged? :( > > > I don't believe I saw you or Rafael listed in the "script/get_maintainers" output. I tried to copy everyone who showed up. > > > > > > > > Original post: > > > > > > > > https://lore.kernel.org/r/20210726201924.3202278-2-morbo@google.com/ > > > > > > > > On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > > > > > Fix the following build warning: > > > > > > > > > > drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] > > > > > int no_warn; > > > > > > That's not going to be a good warning to ever have the kernel use due to > > > how lots of hardware works (i.e. we need to do a read after a write but > > > we can throw the read away as it does not matter). > > > > > > > > > > > > > > > > This variable is used to remove another warning, but causes a warning > > > > > itself. Mark it as 'unused' to avoid that. > > > > > > > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > > > > > > > Even though they evaluate to the same thing, it might be worth using > > > > "__always_unused" here because it is :) > > > > > > But it is not unused, the value is written into it. > > > > > I believe that only matters if the variable is marked "volatile". > > "volatile" means nothing anymore, never use it or even think about it > again please :) > Never use what? ;-) > > Otherwise, the variable itself is never used. A "variable that's > > written to but not read from," in fact, is the whole reason for the > > warning. > > But that is ok! Sometimes you need to do this with hardware (like all > PCI devices). This is a legitimate code flow for many hardware types > and if a C compiler thinks that this is not ok, then it is broken. > Well, no. A C compiler cares about the C language. A variable that's assigned to but not otherwise used isn't useful in the language. Like most warnings, the compiler warns because these situations have led to errors in the past (e.g. maybe someone assigned to the wrong variable because they mistyped the name or something). So this is a perfectly valid warning for a C compiler to emit. This especially holds true when the function being called is marked as requiring the return value to be checked, as is the case with the functions whose values are assigned 'no_warn'. > So be VERY careful when changing drivers based on this warning. Because > of this, I do not think you can enable it over the whole kernel without > causing major problems in some areas. > > But that is independent of this specific issue you are trying to patch > here, I say this to warn you of a number of stupid places where people > have tried to "optimize away" reads based on this compiler warning in > drivers, and we have had to add them back because it broke > functionality. > I definitely agree that we shouldn't blindly remove code just because the return value assigned to a variable isn't used. That's not what this patch is doing. > > > So this isn't ok, sometimes we want to write to variables but never care > > > about the value, that does not mean the compiler should complain about > > > it. > > > > > Typically, if you don't care about the return value, you simply don't > > assign it to a variable (cf. printf). However, the functions that > > assign to "no_warn" have the "warn_unused_result" attribute. The fact > > that the variable is named "no_warn" seems to indicate that it's meant > > to remain unused, even if it probably should be checked. > > These functions have warn_unused_result set on them because for 99% of > the time, I want the value to be checked. But as you can see in this > use, as per the comments in the code, we do not care about the result > for a very good reason. So we just assign it to a variable to make the > compiler quiet. > Right. And because you[1] had to hack around that warning, it led to another warning. This is typical with hacks. [1] Okay, maybe not *you* explicitly, but "you" in the general sense of "the person who wrote this code." > > Would you rather the warning be turned off on some level? > > Which warning? > The one this patch is for. > The code here, as-is, is correct. We already have 1 compiler warning > work around in place, do you want to add another one? How many can we > stack on top of each other? > This one change should suffice. > And again, why did you not cc: the maintainers of this code for this > change? That's not good... > I guess the maintainers didn't show up in the scripts/get_maintainers list when I ran it. I CC'ed everyone that did show up. There's a long list of emails in the "To" section. I don't know what else to say... -bw ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 7:08 ` Bill Wendling @ 2021-07-27 7:12 ` Greg Kroah-Hartman 2021-07-27 7:15 ` Bill Wendling 0 siblings, 1 reply; 36+ messages in thread From: Greg Kroah-Hartman @ 2021-07-27 7:12 UTC (permalink / raw) To: Bill Wendling Cc: Nathan Chancellor, Rafael J. Wysocki, Nick Desaulniers, clang-built-linux, LKML On Tue, Jul 27, 2021 at 12:08:37AM -0700, Bill Wendling wrote: > On Mon, Jul 26, 2021 at 11:41 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Mon, Jul 26, 2021 at 11:15:52PM -0700, Bill Wendling wrote: > > > On Mon, Jul 26, 2021 at 10:27 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Jul 26, 2021 at 01:47:33PM -0700, Nathan Chancellor wrote: > > > > > + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c > > > > > respectively, drop everyone else. > > > > > > > > Odd no one cc:ed us originally, I guess they didn't want the patch ever > > > > merged? :( > > > > > I don't believe I saw you or Rafael listed in the > "script/get_maintainers" output. I tried to copy everyone who showed > up. $ ./scripts/get_maintainer.pl --file drivers/base/module.c Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS) "Rafael J. Wysocki" <rafael@kernel.org> (reviewer:DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS) linux-kernel@vger.kernel.org (open list) {sigh} ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 7:12 ` Greg Kroah-Hartman @ 2021-07-27 7:15 ` Bill Wendling 0 siblings, 0 replies; 36+ messages in thread From: Bill Wendling @ 2021-07-27 7:15 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Nathan Chancellor, Rafael J. Wysocki, Nick Desaulniers, clang-built-linux, LKML On Tue, Jul 27, 2021 at 12:12 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jul 27, 2021 at 12:08:37AM -0700, Bill Wendling wrote: > > On Mon, Jul 26, 2021 at 11:41 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > On Mon, Jul 26, 2021 at 11:15:52PM -0700, Bill Wendling wrote: > > > > On Mon, Jul 26, 2021 at 10:27 PM Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> wrote: > > > > > On Mon, Jul 26, 2021 at 01:47:33PM -0700, Nathan Chancellor wrote: > > > > > > + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c > > > > > > respectively, drop everyone else. > > > > > > > > > > Odd no one cc:ed us originally, I guess they didn't want the patch ever > > > > > merged? :( > > > > > > > I don't believe I saw you or Rafael listed in the > > "script/get_maintainers" output. I tried to copy everyone who showed > > up. > > $ ./scripts/get_maintainer.pl --file drivers/base/module.c > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS) > "Rafael J. Wysocki" <rafael@kernel.org> (reviewer:DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS) > linux-kernel@vger.kernel.org (open list) > I did it on the patches themselves, not the individual files. I thought I got everyone, but apparently didn't. :-( -bw ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 6:41 ` Greg Kroah-Hartman 2021-07-27 7:08 ` Bill Wendling @ 2021-07-27 17:39 ` Nick Desaulniers 2021-07-27 17:42 ` Nick Desaulniers ` (2 more replies) 1 sibling, 3 replies; 36+ messages in thread From: Nick Desaulniers @ 2021-07-27 17:39 UTC (permalink / raw) To: Greg Kroah-Hartman, Bill Wendling Cc: Nathan Chancellor, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Mon, Jul 26, 2021 at 11:41 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Jul 26, 2021 at 11:15:52PM -0700, Bill Wendling wrote: > > On Mon, Jul 26, 2021 at 10:27 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > On Mon, Jul 26, 2021 at 01:47:33PM -0700, Nathan Chancellor wrote: > > > > + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c > > > > respectively, drop everyone else. > > > > > > Odd no one cc:ed us originally, I guess they didn't want the patch ever > > > merged? :( Bill, $ wget https://lore.kernel.org/lkml/20210726201924.3202278-2-morbo@google.com/raw -O bill.patch $ ./scripts/get_maintainer.pl bill.patch Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS) "Rafael J. Wysocki" <rafael@kernel.org> (reviewer:DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS) Nathan Chancellor <nathan@kernel.org> (supporter:CLANG/LLVM BUILD SUPPORT) Nick Desaulniers <ndesaulniers@google.com> (supporter:CLANG/LLVM BUILD SUPPORT) linux-kernel@vger.kernel.org (open list) clang-built-linux@googlegroups.com (open list:CLANG/LLVM BUILD SUPPORT) You can use something like this shell function for small patches: $ which kpatch kpatch () { patch=$1 shift if [[ -d $patch ]] then echo "Directory given" else git send-email --cc-cmd="./scripts/get_maintainer.pl --norolestats $patch" $@ $patch fi } $ kpatch bill.patch --to "Greg Kroah-Hartman <gregkh@linuxfoundation.org>" --to "Rafael J. Wysocki <rafael@kernel.org>" (I recommend always putting the maintainers or people you'd expect to pick up the patches in To:.) > > > > > > > > > > > Original post: > > > > > > > > https://lore.kernel.org/r/20210726201924.3202278-2-morbo@google.com/ > > > > > > > > On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > > > > > Fix the following build warning: > > > > > > > > > > drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] > > > > > int no_warn; > > > > > > That's not going to be a good warning to ever have the kernel use due to > > > how lots of hardware works (i.e. we need to do a read after a write but > > > we can throw the read away as it does not matter). > > > > > > > > > > > > > > > > This variable is used to remove another warning, but causes a warning > > > > > itself. Mark it as 'unused' to avoid that. > > > > > > > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > > > > > > > Even though they evaluate to the same thing, it might be worth using > > > > "__always_unused" here because it is :) > > > > > > But it is not unused, the value is written into it. > > > > > I believe that only matters if the variable is marked "volatile". > > "volatile" means nothing anymore, never use it or even think about it > again please :) What Greg is getting at is that the use of the volatile keyword in variable declarations is slightly frowned on by the kernel community. It's less flexible than making accesses volatile qualified via casts. Then you have flexibility for some accesses to be volatile (ie. not CSE'd), and some not (ie. CSE'd), if needed. Though just because you assign to a variable doesn't mean that the compiler generates an access, especially if the result is unused. This warning is all about dead stores. The cast to a volatile qualified pointer then dereference is what guarantees the access. https://godbolt.org/z/7K7369bGG (To be explicit, IMO Greg's point about volatile stores is orthogonal to discussions about dead stores.) > > > Otherwise, the variable itself is never used. A "variable that's > > written to but not read from," in fact, is the whole reason for the > > warning. > > But that is ok! Sometimes you need to do this with hardware (like all > PCI devices). This is a legitimate code flow for many hardware types > and if a C compiler thinks that this is not ok, then it is broken. > > So be VERY careful when changing drivers based on this warning. Because > of this, I do not think you can enable it over the whole kernel without > causing major problems in some areas. > > But that is independent of this specific issue you are trying to patch > here, I say this to warn you of a number of stupid places where people > have tried to "optimize away" reads based on this compiler warning in > drivers, and we have had to add them back because it broke > functionality. > > > > So this isn't ok, sometimes we want to write to variables but never care > > > about the value, that does not mean the compiler should complain about > > > it. > > > > > Typically, if you don't care about the return value, you simply don't > > assign it to a variable (cf. printf). However, the functions that > > assign to "no_warn" have the "warn_unused_result" attribute. The fact > > that the variable is named "no_warn" seems to indicate that it's meant > > to remain unused, even if it probably should be checked. > > These functions have warn_unused_result set on them because for 99% of > the time, I want the value to be checked. But as you can see in this > use, as per the comments in the code, we do not care about the result > for a very good reason. So we just assign it to a variable to make the > compiler quiet. I think warn_unused_result should only really be used for functions where the return value should be used 100% of the time. If there are cases where it's ok to not check the return value, consider not using warn_unused_result on function declarations. That said, we have a very similar issue throughout LLVM that Bill should recognize. In LLVM, we have pretty aggressive usage of assertions. Rather than: assert(someReallyLongExpression && "error message"); where that statement might wrap across multiple lines, instead it might be clearer to write: bool IsOk = someReallyLongExpression; assert(IsOk && "error message"); which looks nicer but now produces -Wunused-but-set-variable on IsOk for release builds where assertions are disabled. The common fix in LLVM is to write: bool IsOk = someReallyLongExpression; assert(IsOk && "error message"); (void)IsOk; The cast to void is technically a use that doesn't result in a dead store. That pattern could be used in the kernel rather than int no_warn; no_warn = warn_unused_result_fn(); at least to avoid -Wunused-but-set-variable. Oh, looks like a curious difference between compilers: https://godbolt.org/z/GvznMM6o1 Filed https://bugs.llvm.org/show_bug.cgi?id=51228. So I guess we can't use the cast-to-void to avoid -Wunused-but-set-variable, since that triggers -Wunused-result, at least with GCC. :( Nevermind... Though I still think the use of warn_unused_result on sysfs_create_link() is worth revisiting. (Orthogonally, I wonder if C would have been more ergonomic or less to have all functions implicitly warn_unused_result then have callers be explicit when they didn't want a result, rather than what we have today which is the opposite. Maybe that's a terrible idea, but sometimes you can't tell until some volume of code has been written in such a language. Maybe it makes hello world less pretty, but maybe it avoids more bugs in real code.) > > > Would you rather the warning be turned off on some level? > > Which warning? > > The code here, as-is, is correct. We already have 1 compiler warning > work around in place, do you want to add another one? How many can we > stack on top of each other? Isn't -Wunused-but-set-variable enabled only for W=1 builds? > > And again, why did you not cc: the maintainers of this code for this > change? That's not good... > > thanks, > > greg k-h -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 17:39 ` Nick Desaulniers @ 2021-07-27 17:42 ` Nick Desaulniers 2021-07-27 17:59 ` Greg Kroah-Hartman 2021-07-27 18:32 ` Nathan Chancellor 2 siblings, 0 replies; 36+ messages in thread From: Nick Desaulniers @ 2021-07-27 17:42 UTC (permalink / raw) To: Bill Wendling Cc: Nathan Chancellor, Rafael J. Wysocki, clang-built-linux, LKML, Greg Kroah-Hartman On Tue, Jul 27, 2021 at 10:39 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Mon, Jul 26, 2021 at 11:41 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Jul 26, 2021 at 11:15:52PM -0700, Bill Wendling wrote: > > > On Mon, Jul 26, 2021 at 10:27 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Jul 26, 2021 at 01:47:33PM -0700, Nathan Chancellor wrote: > > > > > + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c > > > > > respectively, drop everyone else. > > > > > > > > Odd no one cc:ed us originally, I guess they didn't want the patch ever > > > > merged? :( Oh, I just noticed this patch was part of a series. Perhaps you ran get_maintainer.pl on one patch of the series, and used that list for all patches in the series? Since these patches are orthogonal (ie. they don't depend on any order relative to one another; they can go in separately via different trees and their maintainers) consider not using a series. That should save you from having to write a TPS Report^W^Wcover letter. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 17:39 ` Nick Desaulniers 2021-07-27 17:42 ` Nick Desaulniers @ 2021-07-27 17:59 ` Greg Kroah-Hartman 2021-07-27 18:31 ` Nick Desaulniers ` (2 more replies) 2021-07-27 18:32 ` Nathan Chancellor 2 siblings, 3 replies; 36+ messages in thread From: Greg Kroah-Hartman @ 2021-07-27 17:59 UTC (permalink / raw) To: Nick Desaulniers Cc: Bill Wendling, Nathan Chancellor, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Tue, Jul 27, 2021 at 10:39:49AM -0700, Nick Desaulniers wrote: > > > > > Original post: > > > > > > > > > > https://lore.kernel.org/r/20210726201924.3202278-2-morbo@google.com/ > > > > > > > > > > On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > > > > > > Fix the following build warning: > > > > > > > > > > > > drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] > > > > > > int no_warn; > > > > > > > > That's not going to be a good warning to ever have the kernel use due to > > > > how lots of hardware works (i.e. we need to do a read after a write but > > > > we can throw the read away as it does not matter). > > > > > > > > > > > > > > > > > > > > This variable is used to remove another warning, but causes a warning > > > > > > itself. Mark it as 'unused' to avoid that. > > > > > > > > > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > > > > > > > > > Even though they evaluate to the same thing, it might be worth using > > > > > "__always_unused" here because it is :) > > > > > > > > But it is not unused, the value is written into it. > > > > > > > I believe that only matters if the variable is marked "volatile". > > > > "volatile" means nothing anymore, never use it or even think about it > > again please :) > > What Greg is getting at is that the use of the volatile keyword in > variable declarations is slightly frowned on by the kernel community. > It's less flexible than making accesses volatile qualified via casts. > Then you have flexibility for some accesses to be volatile (ie. not > CSE'd), and some not (ie. CSE'd), if needed. > > Though just because you assign to a variable doesn't mean that the > compiler generates an access, especially if the result is unused. > This warning is all about dead stores. The cast to a volatile > qualified pointer then dereference is what guarantees the access. > > https://godbolt.org/z/7K7369bGG > > (To be explicit, IMO Greg's point about volatile stores is orthogonal > to discussions about dead stores.) I didn't bring up that dirty word, Bill did :) > > > Otherwise, the variable itself is never used. A "variable that's > > > written to but not read from," in fact, is the whole reason for the > > > warning. > > > > But that is ok! Sometimes you need to do this with hardware (like all > > PCI devices). This is a legitimate code flow for many hardware types > > and if a C compiler thinks that this is not ok, then it is broken. > > > > So be VERY careful when changing drivers based on this warning. Because > > of this, I do not think you can enable it over the whole kernel without > > causing major problems in some areas. > > > > But that is independent of this specific issue you are trying to patch > > here, I say this to warn you of a number of stupid places where people > > have tried to "optimize away" reads based on this compiler warning in > > drivers, and we have had to add them back because it broke > > functionality. > > > > > > So this isn't ok, sometimes we want to write to variables but never care > > > > about the value, that does not mean the compiler should complain about > > > > it. > > > > > > > Typically, if you don't care about the return value, you simply don't > > > assign it to a variable (cf. printf). However, the functions that > > > assign to "no_warn" have the "warn_unused_result" attribute. The fact > > > that the variable is named "no_warn" seems to indicate that it's meant > > > to remain unused, even if it probably should be checked. > > > > These functions have warn_unused_result set on them because for 99% of > > the time, I want the value to be checked. But as you can see in this > > use, as per the comments in the code, we do not care about the result > > for a very good reason. So we just assign it to a variable to make the > > compiler quiet. > > I think warn_unused_result should only really be used for functions > where the return value should be used 100% of the time. I too want a shiny new pony. But here in the real world, sometimes you have functions that for 99% of the users, you do want them to check the return value, but when you use them in core code or startup code, you "know" you are safe to ignore the return value. That is the case here. We have other fun examples of where people have tried to add error handling to code that runs at boot that have actually introduced security errors and they justify it with "but you have to check error values!" > If there are > cases where it's ok to not check the return value, consider not using > warn_unused_result on function declarations. Ok, so what do you do when you have a function like this where 99.9% of the users need to check this? Do I really need to write a wrapper function just for it so that I can use it "safely" in the core code instead? Something like: void do_safe_thing_and_ignore_the_world(...) { __unused int error; error = do_thing(...); } Or something else to get the compiler to be quiet about error being set and never used? There HAS to be that option somewhere anyway as we need it for other parts of the kernel where we do: write_bus(device, &value); value = read_bus(device); and then we ignore value as it is not needed, but yet we still HAVE to call read_bus() here, yet read_bus() is set as warn_unused_result() because, well, it is a read function :) > That said, we have a very similar issue throughout LLVM that Bill > should recognize. In LLVM, we have pretty aggressive usage of > assertions. Rather than: > > assert(someReallyLongExpression && "error message"); > > where that statement might wrap across multiple lines, instead it > might be clearer to write: > > bool IsOk = someReallyLongExpression; > assert(IsOk && "error message"); > > which looks nicer but now produces -Wunused-but-set-variable on IsOk > for release builds where assertions are disabled. The common fix in > LLVM is to write: > > bool IsOk = someReallyLongExpression; > assert(IsOk && "error message"); > (void)IsOk; > > The cast to void is technically a use that doesn't result in a dead > store. That pattern could be used in the kernel rather than > > int no_warn; > no_warn = warn_unused_result_fn(); > > at least to avoid -Wunused-but-set-variable. Oh, looks like a curious > difference between compilers: > https://godbolt.org/z/GvznMM6o1 > Filed https://bugs.llvm.org/show_bug.cgi?id=51228. So I guess we > can't use the cast-to-void to avoid -Wunused-but-set-variable, since > that triggers -Wunused-result, at least with GCC. :( Nevermind... > > Though I still think the use of warn_unused_result on > sysfs_create_link() is worth revisiting. Nope, not at all, I WANT users to check this as it is something that has caused problems in drivers and subsystems in the past. And doing the (void)sysfs_create_link(); hack is horrid, I thought we were better than that. Surely there is a "this variable is going to be assigned something but never used" option somewhere? This can't be the first time it has come up, right? > > > Would you rather the warning be turned off on some level? > > > > Which warning? > > > > The code here, as-is, is correct. We already have 1 compiler warning > > work around in place, do you want to add another one? How many can we > > stack on top of each other? > > Isn't -Wunused-but-set-variable enabled only for W=1 builds? No idea, as long as it is not a normal build option, that's fine. "W=1" is for kernel newbies wanting to clean up subsystems and get some patch counts merged :) thanks, greg k-h ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 17:59 ` Greg Kroah-Hartman @ 2021-07-27 18:31 ` Nick Desaulniers 2021-07-27 18:44 ` Greg Kroah-Hartman 2021-07-27 19:23 ` Bill Wendling 2021-07-27 20:13 ` Segher Boessenkool 2 siblings, 1 reply; 36+ messages in thread From: Nick Desaulniers @ 2021-07-27 18:31 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Bill Wendling, Nathan Chancellor, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Tue, Jul 27, 2021 at 10:59 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jul 27, 2021 at 10:39:49AM -0700, Nick Desaulniers wrote: > > If there are > > cases where it's ok to not check the return value, consider not using > > warn_unused_result on function declarations. > > Ok, so what do you do when you have a function like this where 99.9% of > the users need to check this? Do I really need to write a wrapper > function just for it so that I can use it "safely" in the core code > instead? > > Something like: > > void do_safe_thing_and_ignore_the_world(...) > { > __unused int error; > > error = do_thing(...); > } > > Or something else to get the compiler to be quiet about error being set > and never used? There HAS to be that option somewhere anyway as we need > it for other parts of the kernel where we do: > write_bus(device, &value); > value = read_bus(device); > and then we ignore value as it is not needed, but yet we still HAVE to > call read_bus() here, yet read_bus() is set as warn_unused_result() > because, well, it is a read function :) Such wrappers are trivial with __attribute__((alias(""))): https://godbolt.org/z/j5afPbGcM At least then it's very obvious if someone adds more call sites to such an alias. Then that calls for closer inspection in code review that yes, this is one of those 0.01% of cases. Since they occur 0.01% of the time, I don't expect such aliases to occur too frequently. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 18:31 ` Nick Desaulniers @ 2021-07-27 18:44 ` Greg Kroah-Hartman 2021-07-27 19:02 ` Nick Desaulniers 0 siblings, 1 reply; 36+ messages in thread From: Greg Kroah-Hartman @ 2021-07-27 18:44 UTC (permalink / raw) To: Nick Desaulniers Cc: Bill Wendling, Nathan Chancellor, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Tue, Jul 27, 2021 at 11:31:38AM -0700, Nick Desaulniers wrote: > On Tue, Jul 27, 2021 at 10:59 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Jul 27, 2021 at 10:39:49AM -0700, Nick Desaulniers wrote: > > > If there are > > > cases where it's ok to not check the return value, consider not using > > > warn_unused_result on function declarations. > > > > Ok, so what do you do when you have a function like this where 99.9% of > > the users need to check this? Do I really need to write a wrapper > > function just for it so that I can use it "safely" in the core code > > instead? > > > > Something like: > > > > void do_safe_thing_and_ignore_the_world(...) > > { > > __unused int error; > > > > error = do_thing(...); > > } > > > > Or something else to get the compiler to be quiet about error being set > > and never used? There HAS to be that option somewhere anyway as we need > > it for other parts of the kernel where we do: > > write_bus(device, &value); > > value = read_bus(device); > > and then we ignore value as it is not needed, but yet we still HAVE to > > call read_bus() here, yet read_bus() is set as warn_unused_result() > > because, well, it is a read function :) > > Such wrappers are trivial with __attribute__((alias(""))): > https://godbolt.org/z/j5afPbGcM > > At least then it's very obvious if someone adds more call sites to > such an alias. Then that calls for closer inspection in code review > that yes, this is one of those 0.01% of cases. Since they occur 0.01% > of the time, I don't expect such aliases to occur too frequently. That is just, well, horrible. Seriously horrible. Wow. And that is the "documented" way to do this? That feels like an abuse of the already-horrible-why-do-they-do-that-for-variables use of the alias attribute. How badly are compiler people going to complain to me about this if it's in this file? I can take a patch for that, but I feel the comments involved will make people, including myself when I have to look a the code again in 5 years, even more confused... ick, I feel dirty... greg k-h ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 18:44 ` Greg Kroah-Hartman @ 2021-07-27 19:02 ` Nick Desaulniers 0 siblings, 0 replies; 36+ messages in thread From: Nick Desaulniers @ 2021-07-27 19:02 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Bill Wendling, Nathan Chancellor, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Tue, Jul 27, 2021 at 11:45 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jul 27, 2021 at 11:31:38AM -0700, Nick Desaulniers wrote: > > On Tue, Jul 27, 2021 at 10:59 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Tue, Jul 27, 2021 at 10:39:49AM -0700, Nick Desaulniers wrote: > > > > If there are > > > > cases where it's ok to not check the return value, consider not using > > > > warn_unused_result on function declarations. > > > > > > Ok, so what do you do when you have a function like this where 99.9% of > > > the users need to check this? Do I really need to write a wrapper > > > function just for it so that I can use it "safely" in the core code > > > instead? > > > > > > Something like: > > > > > > void do_safe_thing_and_ignore_the_world(...) > > > { > > > __unused int error; > > > > > > error = do_thing(...); > > > } > > > > > > Or something else to get the compiler to be quiet about error being set > > > and never used? There HAS to be that option somewhere anyway as we need > > > it for other parts of the kernel where we do: > > > write_bus(device, &value); > > > value = read_bus(device); > > > and then we ignore value as it is not needed, but yet we still HAVE to > > > call read_bus() here, yet read_bus() is set as warn_unused_result() > > > because, well, it is a read function :) > > > > Such wrappers are trivial with __attribute__((alias(""))): > > https://godbolt.org/z/j5afPbGcM > > > > At least then it's very obvious if someone adds more call sites to > > such an alias. Then that calls for closer inspection in code review > > that yes, this is one of those 0.01% of cases. Since they occur 0.01% > > of the time, I don't expect such aliases to occur too frequently. > > That is just, well, horrible. Seriously horrible. Wow. Yeah, well, that's how I feel about warn_unused_result_except_I_didn't_mean_it. > And that is the "documented" way to do this? That feels like an abuse > of the already-horrible-why-do-they-do-that-for-variables use of the > alias attribute. You could also use #pragma's to disable the warning locally, with a good comment about why it's ok to ignore the return code. > How badly are compiler people going to complain to me about this if > it's in this file? > I can take a patch for that, but I feel the comments involved will make > people, including myself when I have to look a the code again in 5 > years, even more confused... > > ick, I feel dirty... > > greg k-h -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 17:59 ` Greg Kroah-Hartman 2021-07-27 18:31 ` Nick Desaulniers @ 2021-07-27 19:23 ` Bill Wendling 2021-07-27 20:13 ` Segher Boessenkool 2 siblings, 0 replies; 36+ messages in thread From: Bill Wendling @ 2021-07-27 19:23 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Nick Desaulniers, Nathan Chancellor, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Tue, Jul 27, 2021 at 10:59 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Jul 27, 2021 at 10:39:49AM -0700, Nick Desaulniers wrote: > > > > > > Original post: > > > > > > > > > > > > https://lore.kernel.org/r/20210726201924.3202278-2-morbo@google.com/ > > > > > > > > > > > > On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > > > > > > > Fix the following build warning: > > > > > > > > > > > > > > drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable] > > > > > > > int no_warn; > > > > > > > > > > That's not going to be a good warning to ever have the kernel use due to > > > > > how lots of hardware works (i.e. we need to do a read after a write but > > > > > we can throw the read away as it does not matter). > > > > > > > > > > > > > > > > > > > > > > > > This variable is used to remove another warning, but causes a warning > > > > > > > itself. Mark it as 'unused' to avoid that. > > > > > > > > > > > > > > Signed-off-by: Bill Wendling <morbo@google.com> > > > > > > > > > > > > Even though they evaluate to the same thing, it might be worth using > > > > > > "__always_unused" here because it is :) > > > > > > > > > > But it is not unused, the value is written into it. > > > > > > > > > I believe that only matters if the variable is marked "volatile". > > > > > > "volatile" means nothing anymore, never use it or even think about it > > > again please :) > > > > What Greg is getting at is that the use of the volatile keyword in > > variable declarations is slightly frowned on by the kernel community. > > It's less flexible than making accesses volatile qualified via casts. > > Then you have flexibility for some accesses to be volatile (ie. not > > CSE'd), and some not (ie. CSE'd), if needed. > > > > Though just because you assign to a variable doesn't mean that the > > compiler generates an access, especially if the result is unused. > > This warning is all about dead stores. The cast to a volatile > > qualified pointer then dereference is what guarantees the access. > > > > https://godbolt.org/z/7K7369bGG > > > > (To be explicit, IMO Greg's point about volatile stores is orthogonal > > to discussions about dead stores.) > > I didn't bring up that dirty word, Bill did :) > I brought it up only as a potential reason for the compiler *not* to emit the warning. We really shouldn't be spending this much time on it... > > > > Otherwise, the variable itself is never used. A "variable that's > > > > written to but not read from," in fact, is the whole reason for the > > > > warning. > > > > > > But that is ok! Sometimes you need to do this with hardware (like all > > > PCI devices). This is a legitimate code flow for many hardware types > > > and if a C compiler thinks that this is not ok, then it is broken. > > > > > > So be VERY careful when changing drivers based on this warning. Because > > > of this, I do not think you can enable it over the whole kernel without > > > causing major problems in some areas. > > > > > > But that is independent of this specific issue you are trying to patch > > > here, I say this to warn you of a number of stupid places where people > > > have tried to "optimize away" reads based on this compiler warning in > > > drivers, and we have had to add them back because it broke > > > functionality. > > > > > > > > So this isn't ok, sometimes we want to write to variables but never care > > > > > about the value, that does not mean the compiler should complain about > > > > > it. > > > > > > > > > Typically, if you don't care about the return value, you simply don't > > > > assign it to a variable (cf. printf). However, the functions that > > > > assign to "no_warn" have the "warn_unused_result" attribute. The fact > > > > that the variable is named "no_warn" seems to indicate that it's meant > > > > to remain unused, even if it probably should be checked. > > > > > > These functions have warn_unused_result set on them because for 99% of > > > the time, I want the value to be checked. But as you can see in this > > > use, as per the comments in the code, we do not care about the result > > > for a very good reason. So we just assign it to a variable to make the > > > compiler quiet. > > > > I think warn_unused_result should only really be used for functions > > where the return value should be used 100% of the time. > > I too want a shiny new pony. > You do? Ponies cost a lot of money and need ranches to live on and constant care...a lot of work. Cats are better. > But here in the real world, sometimes you have functions that for 99% of > the users, you do want them to check the return value, but when you use > them in core code or startup code, you "know" you are safe to ignore the > return value. > > That is the case here. We have other fun examples of where people have > tried to add error handling to code that runs at boot that have actually > introduced security errors and they justify it with "but you have to > check error values!" > That's fine, and I fully support this. But when you mark a function whose return value is 99.999999% checked except for the I-definitely-know-what-I'm-doing-you-stupid-compiler times, then you're going to get a warning from the compiler, because you've *told* the compiler that the return value needs to be checked, but the code doesn't check it. Compilers aren't mind readers. The option then is to tell the compiler that "Yes, I know what I'm doing, stop telling me otherwise" or disable the warning. As Nathan pointed out, the warning was disabled in an April commit I guess. > > If there are > > cases where it's ok to not check the return value, consider not using > > warn_unused_result on function declarations. > > Ok, so what do you do when you have a function like this where 99.9% of > the users need to check this? Do I really need to write a wrapper > function just for it so that I can use it "safely" in the core code > instead? > > Something like: > > void do_safe_thing_and_ignore_the_world(...) > { > __unused int error; > > error = do_thing(...); > } > > Or something else to get the compiler to be quiet about error being set > and never used? There HAS to be that option somewhere anyway as we need > it for other parts of the kernel where we do: > write_bus(device, &value); > value = read_bus(device); > and then we ignore value as it is not needed, but yet we still HAVE to > call read_bus() here, yet read_bus() is set as warn_unused_result() > because, well, it is a read function :) > We have a perfectly fine way of doing this, by marking the variable as "__maybe_unused". There's no need to come up with a convoluted workaround. Since we don't want to check the return value in roughly 0.1% of the use cases, adding the __maybe_unused attribute isn't a major headache. And it will prompt someone to really check whether it's the "right thing" to do or not, which is what warnings are meant for... > > That said, we have a very similar issue throughout LLVM that Bill > > should recognize. In LLVM, we have pretty aggressive usage of > > assertions. Rather than: > > > > assert(someReallyLongExpression && "error message"); > > > > where that statement might wrap across multiple lines, instead it > > might be clearer to write: > > > > bool IsOk = someReallyLongExpression; > > assert(IsOk && "error message"); > > > > which looks nicer but now produces -Wunused-but-set-variable on IsOk > > for release builds where assertions are disabled. The common fix in > > LLVM is to write: > > > > bool IsOk = someReallyLongExpression; > > assert(IsOk && "error message"); > > (void)IsOk; > > > > The cast to void is technically a use that doesn't result in a dead > > store. That pattern could be used in the kernel rather than > > > > int no_warn; > > no_warn = warn_unused_result_fn(); > > > > at least to avoid -Wunused-but-set-variable. Oh, looks like a curious > > difference between compilers: > > https://godbolt.org/z/GvznMM6o1 > > Filed https://bugs.llvm.org/show_bug.cgi?id=51228. So I guess we > > can't use the cast-to-void to avoid -Wunused-but-set-variable, since > > that triggers -Wunused-result, at least with GCC. :( Nevermind... > > > > Though I still think the use of warn_unused_result on > > sysfs_create_link() is worth revisiting. > > Nope, not at all, I WANT users to check this as it is something that has > caused problems in drivers and subsystems in the past. > > And doing the (void)sysfs_create_link(); hack is horrid, I thought we > were better than that. > > Surely there is a "this variable is going to be assigned something but > never used" option somewhere? This can't be the first time it has come > up, right? > > > > > Would you rather the warning be turned off on some level? > > > > > > Which warning? > > > > > > The code here, as-is, is correct. We already have 1 compiler warning > > > work around in place, do you want to add another one? How many can we > > > stack on top of each other? > > > > Isn't -Wunused-but-set-variable enabled only for W=1 builds? > > No idea, as long as it is not a normal build option, that's fine. "W=1" > is for kernel newbies wanting to clean up subsystems and get some patch > counts merged :) > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 17:59 ` Greg Kroah-Hartman 2021-07-27 18:31 ` Nick Desaulniers 2021-07-27 19:23 ` Bill Wendling @ 2021-07-27 20:13 ` Segher Boessenkool 2021-07-27 20:22 ` Bill Wendling 2 siblings, 1 reply; 36+ messages in thread From: Segher Boessenkool @ 2021-07-27 20:13 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Nick Desaulniers, Bill Wendling, Nathan Chancellor, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Tue, Jul 27, 2021 at 07:59:24PM +0200, Greg Kroah-Hartman wrote: > On Tue, Jul 27, 2021 at 10:39:49AM -0700, Nick Desaulniers wrote: > > I think warn_unused_result should only really be used for functions > > where the return value should be used 100% of the time. > > I too want a shiny new pony. > > But here in the real world, sometimes you have functions that for 99% of > the users, you do want them to check the return value, but when you use > them in core code or startup code, you "know" you are safe to ignore the > return value. > > That is the case here. We have other fun examples of where people have > tried to add error handling to code that runs at boot that have actually > introduced security errors and they justify it with "but you have to > check error values!" > > > If there are > > cases where it's ok to not check the return value, consider not using > > warn_unused_result on function declarations. > > Ok, so what do you do when you have a function like this where 99.9% of > the users need to check this? Do I really need to write a wrapper > function just for it so that I can use it "safely" in the core code > instead? > > Something like: > > void do_safe_thing_and_ignore_the_world(...) > { > __unused int error; > > error = do_thing(...); > } > > Or something else to get the compiler to be quiet about error being set > and never used? The simplest is to write if (do_thing()) { /* Nothing here, we can safely ignore the return value * here, because of X and Y and I don't know, I have no * idea actually why we can in this example. Hopefully * in real code people do have a good reason :-) */ } which should work in *any* compiler, doesn't need any extension, is quite elegant, and encourages documenting why we ignore the return value here. Segher ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 20:13 ` Segher Boessenkool @ 2021-07-27 20:22 ` Bill Wendling 2021-07-27 20:24 ` Bill Wendling 0 siblings, 1 reply; 36+ messages in thread From: Bill Wendling @ 2021-07-27 20:22 UTC (permalink / raw) To: Segher Boessenkool Cc: Greg Kroah-Hartman, Nick Desaulniers, Nathan Chancellor, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Tue, Jul 27, 2021 at 1:17 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Tue, Jul 27, 2021 at 07:59:24PM +0200, Greg Kroah-Hartman wrote: > > On Tue, Jul 27, 2021 at 10:39:49AM -0700, Nick Desaulniers wrote: > > > I think warn_unused_result should only really be used for functions > > > where the return value should be used 100% of the time. > > > > I too want a shiny new pony. > > > > But here in the real world, sometimes you have functions that for 99% of > > the users, you do want them to check the return value, but when you use > > them in core code or startup code, you "know" you are safe to ignore the > > return value. > > > > That is the case here. We have other fun examples of where people have > > tried to add error handling to code that runs at boot that have actually > > introduced security errors and they justify it with "but you have to > > check error values!" > > > > > If there are > > > cases where it's ok to not check the return value, consider not using > > > warn_unused_result on function declarations. > > > > Ok, so what do you do when you have a function like this where 99.9% of > > the users need to check this? Do I really need to write a wrapper > > function just for it so that I can use it "safely" in the core code > > instead? > > > > Something like: > > > > void do_safe_thing_and_ignore_the_world(...) > > { > > __unused int error; > > > > error = do_thing(...); > > } > > > > Or something else to get the compiler to be quiet about error being set > > and never used? > > The simplest is to write > if (do_thing()) { > /* Nothing here, we can safely ignore the return value > * here, because of X and Y and I don't know, I have no > * idea actually why we can in this example. Hopefully > * in real code people do have a good reason :-) > */ > } > > which should work in *any* compiler, doesn't need any extension, is > quite elegant, and encourages documenting why we ignore the return > value here. > Or better still, use sysfs_create_link_nowarn() instead of sysfs_create_link(). We'll just have to take the "__must_check" attribute off the sysfs_create_link_nowarn() declaration. -bw ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 20:22 ` Bill Wendling @ 2021-07-27 20:24 ` Bill Wendling 0 siblings, 0 replies; 36+ messages in thread From: Bill Wendling @ 2021-07-27 20:24 UTC (permalink / raw) To: Segher Boessenkool Cc: Greg Kroah-Hartman, Nick Desaulniers, Nathan Chancellor, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Tue, Jul 27, 2021 at 1:22 PM Bill Wendling <morbo@google.com> wrote: > > On Tue, Jul 27, 2021 at 1:17 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > > > On Tue, Jul 27, 2021 at 07:59:24PM +0200, Greg Kroah-Hartman wrote: > > > On Tue, Jul 27, 2021 at 10:39:49AM -0700, Nick Desaulniers wrote: > > > > I think warn_unused_result should only really be used for functions > > > > where the return value should be used 100% of the time. > > > > > > I too want a shiny new pony. > > > > > > But here in the real world, sometimes you have functions that for 99% of > > > the users, you do want them to check the return value, but when you use > > > them in core code or startup code, you "know" you are safe to ignore the > > > return value. > > > > > > That is the case here. We have other fun examples of where people have > > > tried to add error handling to code that runs at boot that have actually > > > introduced security errors and they justify it with "but you have to > > > check error values!" > > > > > > > If there are > > > > cases where it's ok to not check the return value, consider not using > > > > warn_unused_result on function declarations. > > > > > > Ok, so what do you do when you have a function like this where 99.9% of > > > the users need to check this? Do I really need to write a wrapper > > > function just for it so that I can use it "safely" in the core code > > > instead? > > > > > > Something like: > > > > > > void do_safe_thing_and_ignore_the_world(...) > > > { > > > __unused int error; > > > > > > error = do_thing(...); > > > } > > > > > > Or something else to get the compiler to be quiet about error being set > > > and never used? > > > > The simplest is to write > > if (do_thing()) { > > /* Nothing here, we can safely ignore the return value > > * here, because of X and Y and I don't know, I have no > > * idea actually why we can in this example. Hopefully > > * in real code people do have a good reason :-) > > */ > > } > > > > which should work in *any* compiler, doesn't need any extension, is > > quite elegant, and encourages documenting why we ignore the return > > value here. > > > Or better still, use sysfs_create_link_nowarn() instead of > sysfs_create_link(). We'll just have to take the "__must_check" > attribute off the sysfs_create_link_nowarn() declaration. > Strike that. I mistook the nowarn. -bw ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 17:39 ` Nick Desaulniers 2021-07-27 17:42 ` Nick Desaulniers 2021-07-27 17:59 ` Greg Kroah-Hartman @ 2021-07-27 18:32 ` Nathan Chancellor 2021-07-27 19:04 ` Nick Desaulniers 2021-07-27 19:12 ` Bill Wendling 2 siblings, 2 replies; 36+ messages in thread From: Nathan Chancellor @ 2021-07-27 18:32 UTC (permalink / raw) To: Nick Desaulniers, Greg Kroah-Hartman, Bill Wendling Cc: Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On 7/27/2021 10:39 AM, 'Nick Desaulniers' via Clang Built Linux wrote: > Isn't -Wunused-but-set-variable enabled only for W=1 builds? Maybe Bill's tree does not have commit 885480b08469 ("Makefile: Move -Wno-unused-but-set-variable out of GCC only block"), which disables the warning for clang just like GCC for regular builds? Cheers, Nathan ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 18:32 ` Nathan Chancellor @ 2021-07-27 19:04 ` Nick Desaulniers 2021-07-27 19:10 ` Nathan Chancellor 2021-07-27 19:12 ` Bill Wendling 1 sibling, 1 reply; 36+ messages in thread From: Nick Desaulniers @ 2021-07-27 19:04 UTC (permalink / raw) To: Nathan Chancellor, Bill Wendling Cc: Greg Kroah-Hartman, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Tue, Jul 27, 2021 at 11:32 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On 7/27/2021 10:39 AM, 'Nick Desaulniers' via Clang Built Linux wrote: > > Isn't -Wunused-but-set-variable enabled only for W=1 builds? > > Maybe Bill's tree does not have commit 885480b08469 ("Makefile: Move > -Wno-unused-but-set-variable out of GCC only block"), which disables the > warning for clang just like GCC for regular builds? Looks like 885480b08469, which landed in v5.13-rc1, so that's a possibility. Should that be sent to stable@ so that we don't observe these warnings for non-W=1 builds of stable branches with newer versions of clang? -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 19:04 ` Nick Desaulniers @ 2021-07-27 19:10 ` Nathan Chancellor 0 siblings, 0 replies; 36+ messages in thread From: Nathan Chancellor @ 2021-07-27 19:10 UTC (permalink / raw) To: Nick Desaulniers, Bill Wendling Cc: Greg Kroah-Hartman, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On 7/27/2021 12:04 PM, 'Nick Desaulniers' via Clang Built Linux wrote: > On Tue, Jul 27, 2021 at 11:32 AM Nathan Chancellor <nathan@kernel.org> wrote: >> >> On 7/27/2021 10:39 AM, 'Nick Desaulniers' via Clang Built Linux wrote: >>> Isn't -Wunused-but-set-variable enabled only for W=1 builds? >> >> Maybe Bill's tree does not have commit 885480b08469 ("Makefile: Move >> -Wno-unused-but-set-variable out of GCC only block"), which disables the >> warning for clang just like GCC for regular builds? > > Looks like 885480b08469, which landed in v5.13-rc1, so that's a > possibility. Should that be sent to stable@ so that we don't observe > these warnings for non-W=1 builds of stable branches with newer > versions of clang? It is already in all supported stable versions. Cheers, Nathan ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] base: mark 'no_warn' as unused 2021-07-27 18:32 ` Nathan Chancellor 2021-07-27 19:04 ` Nick Desaulniers @ 2021-07-27 19:12 ` Bill Wendling 1 sibling, 0 replies; 36+ messages in thread From: Bill Wendling @ 2021-07-27 19:12 UTC (permalink / raw) To: Nathan Chancellor Cc: Nick Desaulniers, Greg Kroah-Hartman, Rafael J. Wysocki, clang-built-linux, LKML, linux-toolchains On Tue, Jul 27, 2021 at 11:32 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On 7/27/2021 10:39 AM, 'Nick Desaulniers' via Clang Built Linux wrote: > > Isn't -Wunused-but-set-variable enabled only for W=1 builds? > > Maybe Bill's tree does not have commit 885480b08469 ("Makefile: Move > -Wno-unused-but-set-variable out of GCC only block"), which disables the > warning for clang just like GCC for regular builds? > Ah! I don't have that. It would technically make this patch unnecessary. Use that information as you see fit. -bw ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/3] bnx2x: remove unused variable 'cur_data_offset' 2021-07-26 20:19 ` [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling 2021-07-26 20:19 ` [PATCH v2 1/3] base: mark 'no_warn' as unused Bill Wendling @ 2021-07-26 20:19 ` Bill Wendling 2021-07-26 20:36 ` Nathan Chancellor 2021-07-26 20:19 ` [PATCH v2 3/3] scsi: qla2xxx: remove unused variable 'status' Bill Wendling 2021-07-29 3:38 ` [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings Martin K. Petersen 3 siblings, 1 reply; 36+ messages in thread From: Bill Wendling @ 2021-07-26 20:19 UTC (permalink / raw) To: linux-kernel, netdev, linux-scsi, clang-built-linux, Nathan Chancellor, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen Cc: Bill Wendling Fix the clang build warning: drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c:1862:13: error: variable 'cur_data_offset' set but not used [-Werror,-Wunused-but-set-variable] dma_addr_t cur_data_offset; Signed-off-by: Bill Wendling <morbo@google.com> --- drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c index 27943b0446c2..f255fd0b16db 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c @@ -1858,7 +1858,6 @@ void bnx2x_iov_adjust_stats_req(struct bnx2x *bp) { int i; int first_queue_query_index, num_queues_req; - dma_addr_t cur_data_offset; struct stats_query_entry *cur_query_entry; u8 stats_count = 0; bool is_fcoe = false; @@ -1879,10 +1878,6 @@ void bnx2x_iov_adjust_stats_req(struct bnx2x *bp) BNX2X_NUM_ETH_QUEUES(bp), is_fcoe, first_queue_query_index, first_queue_query_index + num_queues_req); - cur_data_offset = bp->fw_stats_data_mapping + - offsetof(struct bnx2x_fw_stats_data, queue_stats) + - num_queues_req * sizeof(struct per_queue_stats); - cur_query_entry = &bp->fw_stats_req-> query[first_queue_query_index + num_queues_req]; @@ -1933,7 +1928,6 @@ void bnx2x_iov_adjust_stats_req(struct bnx2x *bp) cur_query_entry->funcID, j, cur_query_entry->index); cur_query_entry++; - cur_data_offset += sizeof(struct per_queue_stats); stats_count++; /* all stats are coalesced to the leading queue */ -- 2.32.0.432.gabb21c7263-goog ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/3] bnx2x: remove unused variable 'cur_data_offset' 2021-07-26 20:19 ` [PATCH v2 2/3] bnx2x: remove unused variable 'cur_data_offset' Bill Wendling @ 2021-07-26 20:36 ` Nathan Chancellor 0 siblings, 0 replies; 36+ messages in thread From: Nathan Chancellor @ 2021-07-26 20:36 UTC (permalink / raw) To: Bill Wendling, linux-kernel, netdev, linux-scsi, clang-built-linux, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > Fix the clang build warning: > > drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c:1862:13: error: variable 'cur_data_offset' set but not used [-Werror,-Wunused-but-set-variable] > dma_addr_t cur_data_offset; > > Signed-off-by: Bill Wendling <morbo@google.com> It has been unused since the function's introduction in commit 67c431a5f2f3 ("bnx2x: Support statistics collection for VFs by the PF"), perhaps a leftover remnant from a previous version? Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c > index 27943b0446c2..f255fd0b16db 100644 > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c > @@ -1858,7 +1858,6 @@ void bnx2x_iov_adjust_stats_req(struct bnx2x *bp) > { > int i; > int first_queue_query_index, num_queues_req; > - dma_addr_t cur_data_offset; > struct stats_query_entry *cur_query_entry; > u8 stats_count = 0; > bool is_fcoe = false; > @@ -1879,10 +1878,6 @@ void bnx2x_iov_adjust_stats_req(struct bnx2x *bp) > BNX2X_NUM_ETH_QUEUES(bp), is_fcoe, first_queue_query_index, > first_queue_query_index + num_queues_req); > > - cur_data_offset = bp->fw_stats_data_mapping + > - offsetof(struct bnx2x_fw_stats_data, queue_stats) + > - num_queues_req * sizeof(struct per_queue_stats); > - > cur_query_entry = &bp->fw_stats_req-> > query[first_queue_query_index + num_queues_req]; > > @@ -1933,7 +1928,6 @@ void bnx2x_iov_adjust_stats_req(struct bnx2x *bp) > cur_query_entry->funcID, > j, cur_query_entry->index); > cur_query_entry++; > - cur_data_offset += sizeof(struct per_queue_stats); > stats_count++; > > /* all stats are coalesced to the leading queue */ > ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 3/3] scsi: qla2xxx: remove unused variable 'status' 2021-07-26 20:19 ` [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling 2021-07-26 20:19 ` [PATCH v2 1/3] base: mark 'no_warn' as unused Bill Wendling 2021-07-26 20:19 ` [PATCH v2 2/3] bnx2x: remove unused variable 'cur_data_offset' Bill Wendling @ 2021-07-26 20:19 ` Bill Wendling 2021-07-26 20:40 ` Nathan Chancellor 2021-07-27 3:14 ` Martin K. Petersen 2021-07-29 3:38 ` [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings Martin K. Petersen 3 siblings, 2 replies; 36+ messages in thread From: Bill Wendling @ 2021-07-26 20:19 UTC (permalink / raw) To: linux-kernel, netdev, linux-scsi, clang-built-linux, Nathan Chancellor, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen Cc: Bill Wendling Fix the clang build warning: drivers/scsi/qla2xxx/qla_nx.c:2209:6: error: variable 'status' set but not used [-Werror,-Wunused-but-set-variable] int status = 0; Signed-off-by: Bill Wendling <morbo@google.com> --- drivers/scsi/qla2xxx/qla_nx.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c index 615e44af1ca6..11aad97dfca8 100644 --- a/drivers/scsi/qla2xxx/qla_nx.c +++ b/drivers/scsi/qla2xxx/qla_nx.c @@ -2166,7 +2166,6 @@ qla82xx_poll(int irq, void *dev_id) struct qla_hw_data *ha; struct rsp_que *rsp; struct device_reg_82xx __iomem *reg; - int status = 0; uint32_t stat; uint32_t host_int = 0; uint16_t mb[8]; @@ -2195,7 +2194,6 @@ qla82xx_poll(int irq, void *dev_id) case 0x10: case 0x11: qla82xx_mbx_completion(vha, MSW(stat)); - status |= MBX_INTERRUPT; break; case 0x12: mb[0] = MSW(stat); -- 2.32.0.432.gabb21c7263-goog ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/3] scsi: qla2xxx: remove unused variable 'status' 2021-07-26 20:19 ` [PATCH v2 3/3] scsi: qla2xxx: remove unused variable 'status' Bill Wendling @ 2021-07-26 20:40 ` Nathan Chancellor 2021-07-27 3:14 ` Martin K. Petersen 1 sibling, 0 replies; 36+ messages in thread From: Nathan Chancellor @ 2021-07-26 20:40 UTC (permalink / raw) To: Bill Wendling, linux-kernel, linux-scsi, clang-built-linux, Nick Desaulniers, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote: > Fix the clang build warning: > > drivers/scsi/qla2xxx/qla_nx.c:2209:6: error: variable 'status' set but not used [-Werror,-Wunused-but-set-variable] > int status = 0; > > Signed-off-by: Bill Wendling <morbo@google.com> It has been unused since the function's introduction in commit a9083016a531 ("[SCSI] qla2xxx: Add ISP82XX support."). Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > drivers/scsi/qla2xxx/qla_nx.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c > index 615e44af1ca6..11aad97dfca8 100644 > --- a/drivers/scsi/qla2xxx/qla_nx.c > +++ b/drivers/scsi/qla2xxx/qla_nx.c > @@ -2166,7 +2166,6 @@ qla82xx_poll(int irq, void *dev_id) > struct qla_hw_data *ha; > struct rsp_que *rsp; > struct device_reg_82xx __iomem *reg; > - int status = 0; > uint32_t stat; > uint32_t host_int = 0; > uint16_t mb[8]; > @@ -2195,7 +2194,6 @@ qla82xx_poll(int irq, void *dev_id) > case 0x10: > case 0x11: > qla82xx_mbx_completion(vha, MSW(stat)); > - status |= MBX_INTERRUPT; > break; > case 0x12: > mb[0] = MSW(stat); > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/3] scsi: qla2xxx: remove unused variable 'status' 2021-07-26 20:19 ` [PATCH v2 3/3] scsi: qla2xxx: remove unused variable 'status' Bill Wendling 2021-07-26 20:40 ` Nathan Chancellor @ 2021-07-27 3:14 ` Martin K. Petersen 1 sibling, 0 replies; 36+ messages in thread From: Martin K. Petersen @ 2021-07-27 3:14 UTC (permalink / raw) To: Bill Wendling Cc: linux-kernel, netdev, linux-scsi, clang-built-linux, Nathan Chancellor, Nick Desaulniers, Ariel Elior, Sudarsana Kalluru, GR-everest-linux-l2, David S . Miller, Nilesh Javali, GR-QLogic-Storage-Upstream, James E . J . Bottomley, Martin K . Petersen Bill, > Fix the clang build warning: > > drivers/scsi/qla2xxx/qla_nx.c:2209:6: error: variable 'status' set but not used [-Werror,-Wunused-but-set-variable] > int status = 0; Applied to 5.15/scsi-staging, thanks! -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings 2021-07-26 20:19 ` [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling ` (2 preceding siblings ...) 2021-07-26 20:19 ` [PATCH v2 3/3] scsi: qla2xxx: remove unused variable 'status' Bill Wendling @ 2021-07-29 3:38 ` Martin K. Petersen 3 siblings, 0 replies; 36+ messages in thread From: Martin K. Petersen @ 2021-07-29 3:38 UTC (permalink / raw) To: linux-scsi, Sudarsana Kalluru, Nathan Chancellor, GR-QLogic-Storage-Upstream, linux-kernel, James E . J . Bottomley, Ariel Elior, Nilesh Javali, Nick Desaulniers, David S . Miller, GR-everest-linux-l2, netdev, Bill Wendling, clang-built-linux Cc: Martin K . Petersen On Mon, 26 Jul 2021 13:19:21 -0700, Bill Wendling wrote: > These patches clean up warnings from clang's '-Wunused-but-set-variable' flag. > > Changes for v2: > - Mark "no_warn" as "__maybe_unused" to avoid separate warning. > > Bill Wendling (3): > base: mark 'no_warn' as unused > bnx2x: remove unused variable 'cur_data_offset' > scsi: qla2xxx: remove unused variable 'status' > > [...] Applied to 5.15/scsi-queue, thanks! [3/3] scsi: qla2xxx: remove unused variable 'status' https://git.kernel.org/mkp/scsi/c/cb51bcd5c34b -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2021-07-29 3:39 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-14 9:17 [PATCH 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling 2021-07-14 9:17 ` [PATCH 1/3] base: remove unused variable 'no_warn' Bill Wendling 2021-07-14 12:15 ` kernel test robot 2021-07-14 12:15 ` kernel test robot 2021-07-14 9:17 ` [PATCH 2/3] bnx2x: remove unused variable 'cur_data_offset' Bill Wendling 2021-07-14 9:17 ` [PATCH 3/3] scsi: qla2xxx: remove unused variable 'status' Bill Wendling 2021-07-26 20:19 ` [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings Bill Wendling 2021-07-26 20:19 ` [PATCH v2 1/3] base: mark 'no_warn' as unused Bill Wendling 2021-07-26 20:47 ` Nathan Chancellor 2021-07-26 21:01 ` Bill Wendling 2021-07-27 5:27 ` Greg Kroah-Hartman 2021-07-27 6:15 ` Bill Wendling 2021-07-27 6:41 ` Greg Kroah-Hartman 2021-07-27 7:08 ` Bill Wendling 2021-07-27 7:12 ` Greg Kroah-Hartman 2021-07-27 7:15 ` Bill Wendling 2021-07-27 17:39 ` Nick Desaulniers 2021-07-27 17:42 ` Nick Desaulniers 2021-07-27 17:59 ` Greg Kroah-Hartman 2021-07-27 18:31 ` Nick Desaulniers 2021-07-27 18:44 ` Greg Kroah-Hartman 2021-07-27 19:02 ` Nick Desaulniers 2021-07-27 19:23 ` Bill Wendling 2021-07-27 20:13 ` Segher Boessenkool 2021-07-27 20:22 ` Bill Wendling 2021-07-27 20:24 ` Bill Wendling 2021-07-27 18:32 ` Nathan Chancellor 2021-07-27 19:04 ` Nick Desaulniers 2021-07-27 19:10 ` Nathan Chancellor 2021-07-27 19:12 ` Bill Wendling 2021-07-26 20:19 ` [PATCH v2 2/3] bnx2x: remove unused variable 'cur_data_offset' Bill Wendling 2021-07-26 20:36 ` Nathan Chancellor 2021-07-26 20:19 ` [PATCH v2 3/3] scsi: qla2xxx: remove unused variable 'status' Bill Wendling 2021-07-26 20:40 ` Nathan Chancellor 2021-07-27 3:14 ` Martin K. Petersen 2021-07-29 3:38 ` [PATCH v2 0/3] Fix clang -Wunused-but-set-variable warnings Martin K. Petersen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.