| Message ID | cover.1562734889.git.joe@perches.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | treewide: Fix GENMASK misuses | expand |
On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote: > These GENMASK uses are inverted argument order and the > actual masks produced are incorrect. Fix them. > > Add checkpatch tests to help avoid more misuses too. > > Joe Perches (12): > checkpatch: Add GENMASK tests IMHO this doesn't make a lot of sense as a checkpatch test - just throw in a BUILD_BUG_ON()? johannes
On Wed, Jul 10, 2019 at 11:17:31AM +0200, Johannes Berg wrote: > On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote: > > These GENMASK uses are inverted argument order and the > > actual masks produced are incorrect. Fix them. > > > > Add checkpatch tests to help avoid more misuses too. > > > > Joe Perches (12): > > checkpatch: Add GENMASK tests > > IMHO this doesn't make a lot of sense as a checkpatch test - just throw > in a BUILD_BUG_ON()? My personal take on this is that GENMASK() is really not useful, it's just pure obfuscation and leads to exactly these kinds of mistakes. Yes, I fully understand the argument that you can just specify the start and end bits, and it _in theory_ makes the code more readable. However, the problem is when writing code. GENMASK(a, b). Is a the starting bit or ending bit? Is b the number of bits? It's confusing and causes mistakes resulting in incorrect code. A BUILD_BUG_ON() can catch some of the cases, but not all of them. For example: GENMASK(6, 2) would satisify the requirement that a > b, so a BUILD_BUG_ON() will not trigger, but was the author meaning 0x3c or 0xc0? Personally, I've decided I am _not_ going to use GENMASK() in my code because I struggle to get the macro arguments correct - I'm _much_ happier, and it is way more reliable for me to write the mask in hex notation. I think this is where use of a ternary operator would come in use. The normal way of writing a number of bits tends to be "a:b", so if GENMASK took something like GENMASK(6:2), then I'd have less issue with it, because it's argument is then in a familiar notation. Yes, I'm sure that someone will point out that the GENMASK arguments are just in the same order, but that doesn't prevent _me_ frequently getting it wrong - and that's the point. The macro seems to me to cause more problems than it solves.
On Wed, 2019-07-10 at 10:43 +0100, Russell King - ARM Linux admin wrote: > On Wed, Jul 10, 2019 at 11:17:31AM +0200, Johannes Berg wrote: > > On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote: > > > These GENMASK uses are inverted argument order and the > > > actual masks produced are incorrect. Fix them. > > > > > > Add checkpatch tests to help avoid more misuses too. > > > > > > Joe Perches (12): > > > checkpatch: Add GENMASK tests > > > > IMHO this doesn't make a lot of sense as a checkpatch test - just throw > > in a BUILD_BUG_ON()? I tried that. It'd can't be done as it's used in declarations and included in asm files and it uses the UL() macro. I also tried just making it do the right thing whatever the argument order. Oh well. > My personal take on this is that GENMASK() is really not useful, it's > just pure obfuscation and leads to exactly these kinds of mistakes. > > Yes, I fully understand the argument that you can just specify the > start and end bits, and it _in theory_ makes the code more readable. > > However, the problem is when writing code. GENMASK(a, b). Is a the > starting bit or ending bit? Is b the number of bits? It's confusing > and causes mistakes resulting in incorrect code. A BUILD_BUG_ON() > can catch some of the cases, but not all of them. It's a horrid little macro and I agree with Russell. I also think if it existed at all it should have been GENMASK(low, high) not GENMASK(high, low). I
On Wed, 2019-07-10 at 08:45 -0700, Joe Perches wrote: > On Wed, 2019-07-10 at 10:43 +0100, Russell King - ARM Linux admin wrote: > > On Wed, Jul 10, 2019 at 11:17:31AM +0200, Johannes Berg wrote: > > > On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote: > > > > These GENMASK uses are inverted argument order and the > > > > actual masks produced are incorrect. Fix them. > > > > > > > > Add checkpatch tests to help avoid more misuses too. > > > > > > > > Joe Perches (12): > > > > checkpatch: Add GENMASK tests > > > > > > IMHO this doesn't make a lot of sense as a checkpatch test - just throw > > > in a BUILD_BUG_ON()? > > I tried that. > > It'd can't be done as it's used in declarations > and included in asm files and it uses the UL() > macro. > > I also tried just making it do the right thing > whatever the argument order. I forgot. I also made all those arguments when it was introduced in 2013. https://lore.kernel.org/patchwork/patch/414248/ > Oh well. yeah.
From: Joe Perches <joe@perches.com> Date: Tue, 9 Jul 2019 22:04:13 -0700 > These GENMASK uses are inverted argument order and the > actual masks produced are incorrect. Fix them. > > Add checkpatch tests to help avoid more misuses too. Patches #7 and #8 applied to 'net', with appropriate Fixes tags added to #8.
Hi Joe, On 10.07.2019 07:04, Joe Perches wrote: > These GENMASK uses are inverted argument order and the > actual masks produced are incorrect. Fix them. > > Add checkpatch tests to help avoid more misuses too. > > Joe Perches (12): > checkpatch: Add GENMASK tests > clocksource/drivers/npcm: Fix misuse of GENMASK macro > drm: aspeed_gfx: Fix misuse of GENMASK macro > iio: adc: max9611: Fix misuse of GENMASK macro > irqchip/gic-v3-its: Fix misuse of GENMASK macro > mmc: meson-mx-sdio: Fix misuse of GENMASK macro > net: ethernet: mediatek: Fix misuses of GENMASK macro > net: stmmac: Fix misuses of GENMASK macro > rtw88: Fix misuse of GENMASK macro > phy: amlogic: G12A: Fix misuse of GENMASK macro > staging: media: cedrus: Fix misuse of GENMASK macro > ASoC: wcd9335: Fix misuse of GENMASK macro > > drivers/clocksource/timer-npcm7xx.c | 2 +- > drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +- > drivers/iio/adc/max9611.c | 2 +- > drivers/irqchip/irq-gic-v3-its.c | 2 +- > drivers/mmc/host/meson-mx-sdio.c | 2 +- > drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +- > drivers/net/ethernet/mediatek/mtk_sgmii.c | 2 +- > drivers/net/ethernet/stmicro/stmmac/descs.h | 2 +- > drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 4 ++-- > drivers/net/wireless/realtek/rtw88/rtw8822b.c | 2 +- > drivers/phy/amlogic/phy-meson-g12a-usb2.c | 2 +- > drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +- > scripts/checkpatch.pl | 15 +++++++++++++++ > sound/soc/codecs/wcd-clsh-v2.c | 2 +- > 14 files changed, 29 insertions(+), 14 deletions(-) > After adding following compile time check: ------ diff --git a/Makefile b/Makefile index 5102b2bbd224..ac4ea5f443a9 100644 --- a/Makefile +++ b/Makefile @@ -457,7 +457,7 @@ KBUILD_AFLAGS := -D__ASSEMBLY__ -fno-PIE KBUILD_CFLAGS := -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 \ + -Wno-format-security -Werror=div-by-zero \ -std=gnu89 KBUILD_CPPFLAGS := -D__KERNEL__ KBUILD_AFLAGS_KERNEL := diff --git a/include/linux/bits.h b/include/linux/bits.h index 669d69441a62..61d74b103055 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -19,11 +19,11 @@ * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000. */ #define GENMASK(h, l) \ - (((~UL(0)) - (UL(1) << (l)) + 1) & \ + (((~UL(0)) - (UL(1) << (l)) + 1 + 0/((h) >= (l))) & \ (~UL(0) >> (BITS_PER_LONG - 1 - (h)))) #define GENMASK_ULL(h, l) \ - (((~ULL(0)) - (ULL(1) << (l)) + 1) & \ + (((~ULL(0)) - (ULL(1) << (l)) + 1 + 0/((h) >= (l))) & \ (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))) #endif /* __LINUX_BITS_H */ ------- I was able to detect one more GENMASK misue (AARCH64, allyesconfig): CC drivers/phy/rockchip/phy-rockchip-inno-hdmi.o In file included from ../include/linux/bitops.h:5:0, from ../include/linux/kernel.h:12, from ../include/linux/clk.h:13, from ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:9: ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c: In function ‘inno_hdmi_phy_rk3328_power_on’: ../include/linux/bits.h:22:37: error: division by zero [-Werror=div-by-zero] (((~UL(0)) - (UL(1) << (l)) + 1 + 0/((h) >= (l))) & \ ^ ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:24:42: note: in expansion of macro ‘GENMASK’ #define UPDATE(x, h, l) (((x) << (l)) & GENMASK((h), (l))) ^~~~~~~ ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:201:50: note: in expansion of macro ‘UPDATE’ #define RK3328_TERM_RESISTOR_CALIB_SPEED_7_0(x) UPDATE(x, 7, 9) ^~~~~~ ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:1046:26: note: in expansion of macro ‘RK3328_TERM_RESISTOR_CALIB_SPEED_7_0’ inno_write(inno, 0xc6, RK3328_TERM_RESISTOR_CALIB_SPEED_7_0(v)); Of course I do not advise to add the check as is to Kernel - it is undefined behavior area AFAIK. Regards Andrzej