Re: [PATCH 00/12] treewide: Fix GENMASK misuses

[Andrzej Hajda]

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