[alsa-devel] [PATCH 00/12] treewide: Fix GENMASK misuses
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(-)
Arguments are supposed to be ordered high then low.
Signed-off-by: Joe Perches joe@perches.com --- sound/soc/codecs/wcd-clsh-v2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wcd-clsh-v2.c b/sound/soc/codecs/wcd-clsh-v2.c index c397d713f01a..cc5a9c9b918b 100644 --- a/sound/soc/codecs/wcd-clsh-v2.c +++ b/sound/soc/codecs/wcd-clsh-v2.c @@ -65,7 +65,7 @@ struct wcd_clsh_ctrl { #define WCD9XXX_FLYBACK_EN_PWDN_WITH_DELAY 0 #define WCD9XXX_RX_BIAS_FLYB_BUFF WCD9335_REG(0x6, 0xC7) #define WCD9XXX_RX_BIAS_FLYB_VNEG_5_UA_MASK GENMASK(7, 4) -#define WCD9XXX_RX_BIAS_FLYB_VPOS_5_UA_MASK GENMASK(0, 3) +#define WCD9XXX_RX_BIAS_FLYB_VPOS_5_UA_MASK GENMASK(3, 0) #define WCD9XXX_HPH_L_EN WCD9335_REG(0x6, 0xD3) #define WCD9XXX_HPH_CONST_SEL_L_MASK GENMASK(7, 3) #define WCD9XXX_HPH_CONST_SEL_BYPASS 0
The patch
ASoC: wcd9335: Fix misuse of GENMASK macro
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.4
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From f7408a3d5b5fd10571a653d1a81ce9167c62727f Mon Sep 17 00:00:00 2001
From: Joe Perches joe@perches.com Date: Tue, 9 Jul 2019 22:04:25 -0700 Subject: [PATCH] ASoC: wcd9335: Fix misuse of GENMASK macro
Arguments are supposed to be ordered high then low.
Signed-off-by: Joe Perches joe@perches.com Link: https://lore.kernel.org/r/92e31a9f321fe731d428ec3ec9d4654ea8a16d1b.156273488... Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/wcd-clsh-v2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wcd-clsh-v2.c b/sound/soc/codecs/wcd-clsh-v2.c index c397d713f01a..cc5a9c9b918b 100644 --- a/sound/soc/codecs/wcd-clsh-v2.c +++ b/sound/soc/codecs/wcd-clsh-v2.c @@ -65,7 +65,7 @@ struct wcd_clsh_ctrl { #define WCD9XXX_FLYBACK_EN_PWDN_WITH_DELAY 0 #define WCD9XXX_RX_BIAS_FLYB_BUFF WCD9335_REG(0x6, 0xC7) #define WCD9XXX_RX_BIAS_FLYB_VNEG_5_UA_MASK GENMASK(7, 4) -#define WCD9XXX_RX_BIAS_FLYB_VPOS_5_UA_MASK GENMASK(0, 3) +#define WCD9XXX_RX_BIAS_FLYB_VPOS_5_UA_MASK GENMASK(3, 0) #define WCD9XXX_HPH_L_EN WCD9335_REG(0x6, 0xD3) #define WCD9XXX_HPH_CONST_SEL_L_MASK GENMASK(7, 3) #define WCD9XXX_HPH_CONST_SEL_BYPASS 0
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
participants (6)
-
Andrzej Hajda -
David Miller -
Joe Perches -
Johannes Berg -
Mark Brown -
Russell King - ARM Linux admin