Re: [PATCH] ath10k: remove unused ath10k_get_ring_byte function
On 3/22/23 1:40 PM, Simon Horman wrote: On Wed, Mar 22, 2023 at 08:28:55AM -0400, Tom Rix wrote: clang with W=1 reports drivers/net/wireless/ath/ath10k/ce.c:88:1: error: unused function 'ath10k_get_ring_byte' [-Werror,-Wunused-function] ath10k_get_ring_byte(unsigned int offset, ^ This function is not used so remove it. Signed-off-by: Tom Rix Hi Tom, this looks good. But this patch applied, and with clang 11.0.2, make CC=clang W=1 tells me: drivers/net/wireless/ath/ath10k/ce.c:80:19: error: unused function 'shadow_dst_wr_ind_addr' [-Werror,-Wunused-function] static inline u32 shadow_dst_wr_ind_addr(struct ath10k *ar, ^ drivers/net/wireless/ath/ath10k/ce.c:434:20: error: unused function 'ath10k_ce_error_intr_enable' [-Werror,-Wunused-function] static inline void ath10k_ce_error_intr_enable(struct ath10k *ar, ^ Perhaps those functions should be removed too? I believe these were removed with c3ab8c9a296 ("wifi: ath10k: Remove the unused function shadow_dst_wr_ind_addr() and ath10k_ce_error_intr_enable()") Tom --- drivers/net/wireless/ath/ath10k/ce.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c index b656cfc03648..c27b8204718a 100644 --- a/drivers/net/wireless/ath/ath10k/ce.c +++ b/drivers/net/wireless/ath/ath10k/ce.c @@ -84,13 +84,6 @@ ath10k_set_ring_byte(unsigned int offset, return ((offset << addr_map->lsb) & addr_map->mask); } -static inline unsigned int -ath10k_get_ring_byte(unsigned int offset, -struct ath10k_hw_ce_regs_addr_map *addr_map) -{ - return ((offset & addr_map->mask) >> (addr_map->lsb)); -} - static inline u32 ath10k_ce_read32(struct ath10k *ar, u32 offset) { struct ath10k_ce *ce = ath10k_ce_priv(ar); -- 2.27.0 ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH] ath10k: remove unused ath10k_get_ring_byte function
clang with W=1 reports drivers/net/wireless/ath/ath10k/ce.c:88:1: error: unused function 'ath10k_get_ring_byte' [-Werror,-Wunused-function] ath10k_get_ring_byte(unsigned int offset, ^ This function is not used so remove it. Signed-off-by: Tom Rix --- drivers/net/wireless/ath/ath10k/ce.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c index b656cfc03648..c27b8204718a 100644 --- a/drivers/net/wireless/ath/ath10k/ce.c +++ b/drivers/net/wireless/ath/ath10k/ce.c @@ -84,13 +84,6 @@ ath10k_set_ring_byte(unsigned int offset, return ((offset << addr_map->lsb) & addr_map->mask); } -static inline unsigned int -ath10k_get_ring_byte(unsigned int offset, -struct ath10k_hw_ce_regs_addr_map *addr_map) -{ - return ((offset & addr_map->mask) >> (addr_map->lsb)); -} - static inline u32 ath10k_ce_read32(struct ath10k *ar, u32 offset) { struct ath10k_ce *ce = ath10k_ce_priv(ar); -- 2.27.0 ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [RFC] treewide: cleanup unreachable breaks
On 10/19/20 4:05 PM, Jason Gunthorpe wrote: > On Mon, Oct 19, 2020 at 12:42:15PM -0700, Nick Desaulniers wrote: >> On Sat, Oct 17, 2020 at 10:43 PM Greg KH wrote: >>> On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: >>>> From: Tom Rix >>>> >>>> This is a upcoming change to clean up a new warning treewide. >>>> I am wondering if the change could be one mega patch (see below) or >>>> normal patch per file about 100 patches or somewhere half way by collecting >>>> early acks. >>> Please break it up into one-patch-per-subsystem, like normal, and get it >>> merged that way. >>> >>> Sending us a patch, without even a diffstat to review, isn't going to >>> get you very far... >> Tom, >> If you're able to automate this cleanup, I suggest checking in a >> script that can be run on a directory. Then for each subsystem you >> can say in your commit "I ran scripts/fix_whatever.py on this subdir." >> Then others can help you drive the tree wide cleanup. Then we can >> enable -Wunreachable-code-break either by default, or W=2 right now >> might be a good idea. > I remember using clang-modernize in the past to fix issues very > similar to this, if clang machinery can generate the warning, can't > something like clang-tidy directly generate the patch? Yes clang-tidy and similar are good tools. Sometimes they change too much and your time shifts from editing to analyzing and dropping changes. I am looking at them for auto changing api. When i have something greater than half baked i will post. Tom > > You can send me a patch for drivers/infiniband/* as well > > Thanks, > Jason > ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [RFC] treewide: cleanup unreachable breaks
On 10/19/20 12:42 PM, Nick Desaulniers wrote: > On Sat, Oct 17, 2020 at 10:43 PM Greg KH wrote: >> On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: >>> From: Tom Rix >>> >>> This is a upcoming change to clean up a new warning treewide. >>> I am wondering if the change could be one mega patch (see below) or >>> normal patch per file about 100 patches or somewhere half way by collecting >>> early acks. >> Please break it up into one-patch-per-subsystem, like normal, and get it >> merged that way. >> >> Sending us a patch, without even a diffstat to review, isn't going to >> get you very far... > Tom, > If you're able to automate this cleanup, I suggest checking in a > script that can be run on a directory. Then for each subsystem you > can say in your commit "I ran scripts/fix_whatever.py on this subdir." > Then others can help you drive the tree wide cleanup. Then we can > enable -Wunreachable-code-break either by default, or W=2 right now > might be a good idea. I should have waited for Joe Perches's fixer addition to checkpatch :) The easy fixes I did only cover about 1/2 of the problems. Remaining are mostly nested switches, which from a complexity standpoint is bad. > > Ah, George (gbiv@, cc'ed), did an analysis recently of > `-Wunreachable-code-loop-increment`, `-Wunreachable-code-break`, and > `-Wunreachable-code-return` for Android userspace. From the review: > ``` > Spoilers: of these, it seems useful to turn on > -Wunreachable-code-loop-increment and -Wunreachable-code-return by > default for Android In my simple add-a-cflag bot, i see there are about 250 issues for -Wunreachable-code-return. I'll see about doing this one next. > ... > While these conventions about always having break arguably became > obsolete when we enabled -Wfallthrough, my sample turned up zero > potential bugs caught by this warning, and we'd need to put a lot of > effort into getting a clean tree. So this warning doesn't seem to be > worth it. > ``` > Looks like there's an order of magnitude of `-Wunreachable-code-break` > than the other two. > > We probably should add all 3 to W=2 builds (wrapped in cc-option). > I've filed https://github.com/ClangBuiltLinux/linux/issues/1180 to > follow up on. Yes, i think think these should be added. Tom ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH] wireless: remove unneeded break
On 10/19/20 9:20 AM, Joe Perches wrote: > On Mon, 2020-10-19 at 10:54 -0500, Gustavo A. R. Silva wrote: >> On 10/19/20 10:21, Joe Perches wrote: >>> On Mon, 2020-10-19 at 17:14 +0200, Christian Lamparter wrote: >>>> On 19/10/2020 17:05, t...@redhat.com wrote: >>>>> From: Tom Rix >>>>> >>>>> A break is not needed if it is preceded by a return or goto >>>>> >>>>> Signed-off-by: Tom Rix >>>>> diff --git a/drivers/net/wireless/intersil/p54/eeprom.c >>>>> b/drivers/net/wireless/intersil/p54/eeprom.c > [] >>>>> @@ -870,7 +870,6 @@ int p54_parse_eeprom(struct ieee80211_hw *dev, void >>>>> *eeprom, int len) >>>>> } else { >>>>> goto good_eeprom; >>>>> } >>>>> - break; >>>> Won't the compiler (gcc) now complain about a missing fallthrough >>>> annotation? >> Clang would definitely complain about this. > As far as I can tell, clang 10.0.0 doesn't complain. > > This compiles without fallthrough complaint > > from make V=1 W=123 CC=clang drivers/net/wireless/intersil/p54/eeprom.o > with -Wimplicit-fallthrough added > > $ clang -Wp,-MMD,drivers/net/wireless/intersil/p54/.eeprom.o.d -nostdinc > -isystem /usr/local/lib/clang/10.0.0/include -I./arch/x86/include > -I./arch/x86/include/generated -I./include -I./arch/x86/include/uapi > -I./arch/x86/include/generated/uapi -I./include/uapi > -I./include/generated/uapi -include ./include/linux/kconfig.h -include > ./include/linux/compiler_types.h -D__KERNEL__ -Qunused-arguments > -DKBUILD_EXTRA_WARN1 -DKBUILD_EXTRA_WARN2 -DKBUILD_EXTRA_WARN3 -Wall -Wundef > -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common > -fshort-wchar -fno-PIE -Werror=implicit-function-declaration > -Werror=implicit-int -Wno-format-security -std=gnu89 -no-integrated-as > -Werror=unknown-warning-option -mno-sse -mno-mmx -mno-sse2 -mno-3dnow > -mno-avx -m64 -mno-80387 -mstack-alignment=8 -mtune=generic -mno-red-zone > -mcmodel=kernel -DCONFIG_X86_X32_ABI -Wno-sign-compare > -fno-asynchronous-unwind-tables -mretpoline-external-thunk > -fno-delete-null-pointer-checks -Wno-address-of-packed-member -O2 > -Wframe-larger-than=2048 -fstack-protector-strong > -Wno-format-invalid-specifier -Wno-gnu -mno-global-merge > -Wno-unused-const-variable -ftrivial-auto-var-init=pattern -pg -mfentry > -DCC_USING_FENTRY -falign-functions=32 -Wdeclaration-after-statement -Wvla > -Wno-pointer-sign -Wno-array-bounds -fno-strict-overflow -fno-stack-check > -Werror=date-time -Werror=incompatible-pointer-types -fcf-protection=none > -Wextra -Wunused -Wno-unused-parameter -Wmissing-declarations > -Wmissing-format-attribute -Wmissing-prototypes -Wold-style-definition > -Wmissing-include-dirs -Wunused-const-variable > -Wno-missing-field-initializers -Wno-sign-compare -Wno-type-limits > -Wcast-align -Wdisabled-optimization -Wnested-externs -Wshadow > -Wmissing-field-initializers -Wtype-limits -Wunused-macros > -Wbad-function-cast -Wcast-qual -Wconversion -Wpacked -Wpadded > -Wpointer-arith -Wredundant-decls -Wsign-compare -Wswitch-default > -fsanitize=kernel-address -mllvm -asan-mapping-offset=0xdc00 > -mllvm -asan-globals=1 -mllvm -asan-instrumentation-with-call-threshold=0 > -mllvm -asan-stack=0 --param asan-instrument-allocas=1 > -fsanitize-coverage=trace-pc -fsanitize-coverage=trace-cmp > -Wimplicit-fallthrough > -DKBUILD_MODFILE='"drivers/net/wireless/intersil/p54/p54common"' > -DKBUILD_BASENAME='"eeprom"' -DKBUILD_MODNAME='"p54common"' -c -o > drivers/net/wireless/intersil/p54/eeprom.o > drivers/net/wireless/intersil/p54/eeprom.c > I did not intend for if-else; break; changes to be in the patchset. I will kick this out and respin the patch after i get the first pass of the other changes out. Sorry, Tom ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [RFC] treewide: cleanup unreachable breaks
On 10/17/20 10:43 PM, Greg KH wrote: > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: >> From: Tom Rix >> >> This is a upcoming change to clean up a new warning treewide. >> I am wondering if the change could be one mega patch (see below) or >> normal patch per file about 100 patches or somewhere half way by collecting >> early acks. > Please break it up into one-patch-per-subsystem, like normal, and get it > merged that way. OK. Thanks, Tom > > Sending us a patch, without even a diffstat to review, isn't going to > get you very far... > > thanks, > > greg k-h > ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k