Re: [PATCH 00/12] treewide: Fix GENMASK misuses
On Sat, 2019-07-27 at 21:54 +0200, Rikard Falkeborn wrote: > Trimming CC-list. > > > It'd can't be done as it's used in declarations > > and included in asm files and it uses the UL() > > macro. > > Can the BUILD_BUG_ON_ZERO() macro be used instead? It works in > declarations. I don't know if it works in asm-files, but the below > changes builds an x86-64 allyesconfig without problems (after the rest > of this series have been applied. Maybe, fine by me if it works. Perhaps you should submit this and let the build-bot verify it. > /Rikard > > --- > include/linux/bits.h | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/include/linux/bits.h b/include/linux/bits.h > index 669d69441a62..52e747d27f87 100644 > --- a/include/linux/bits.h > +++ b/include/linux/bits.h > @@ -2,6 +2,7 @@ > #ifndef __LINUX_BITS_H > #define __LINUX_BITS_H > > +#include > #include > #include > > @@ -19,11 +20,15 @@ > * GENMASK_ULL(39, 21) gives us the 64bit vector 0x00e0. > */ > #define GENMASK(h, l) \ > + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ > + __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0)) + \ > (((~UL(0)) - (UL(1) << (l)) + 1) & \ > - (~UL(0) >> (BITS_PER_LONG - 1 - (h > + (~UL(0) >> (BITS_PER_LONG - 1 - (h) > > #define GENMASK_ULL(h, l) \ > + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ > + __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0)) + \ > (((~ULL(0)) - (ULL(1) << (l)) + 1) & \ > - (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h > + (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h) > > #endif /* __LINUX_BITS_H */
Re: [PATCH 00/12] treewide: Fix GENMASK misuses
Trimming CC-list. > It'd can't be done as it's used in declarations > and included in asm files and it uses the UL() > macro. Can the BUILD_BUG_ON_ZERO() macro be used instead? It works in declarations. I don't know if it works in asm-files, but the below changes builds an x86-64 allyesconfig without problems (after the rest of this series have been applied. /Rikard --- include/linux/bits.h | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/include/linux/bits.h b/include/linux/bits.h index 669d69441a62..52e747d27f87 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -2,6 +2,7 @@ #ifndef __LINUX_BITS_H #define __LINUX_BITS_H +#include #include #include @@ -19,11 +20,15 @@ * GENMASK_ULL(39, 21) gives us the 64bit vector 0x00e0. */ #define GENMASK(h, l) \ + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ + __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0)) + \ (((~UL(0)) - (UL(1) << (l)) + 1) & \ -(~UL(0) >> (BITS_PER_LONG - 1 - (h +(~UL(0) >> (BITS_PER_LONG - 1 - (h) #define GENMASK_ULL(h, l) \ + (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ + __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0)) + \ (((~ULL(0)) - (ULL(1) << (l)) + 1) & \ -(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h +(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h) #endif /* __LINUX_BITS_H */ -- 2.22.0
Re: [PATCH 00/12] treewide: Fix GENMASK misuses
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 0x00e0. */ #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
[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(-) -- 2.15.0
