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
