Message ID | cover.1562734889.git.joe@perches.com (mailing list archive) |
---|---|
Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1hl4nV-0003tY-Aq; Wed, 10 Jul 2019 05:05:41 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726067AbfGJFEf (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 10 Jul 2019 01:04:35 -0400 Received: from smtprelay0012.hostedemail.com ([216.40.44.12]:43575 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725791AbfGJFEf (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 10 Jul 2019 01:04:35 -0400 Received: from filter.hostedemail.com (clb03-v110.bra.tucows.net [216.40.38.60]) by smtprelay07.hostedemail.com (Postfix) with ESMTP id 2D69B181D3403; Wed, 10 Jul 2019 05:04:33 +0000 (UTC) X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: 2, 0, 0, , d41d8cd98f00b204, joe@perches.com, :::::::::::::::::::::::::::::::::::::::, RULES_HIT:41:355:379:541:973:982:988:989:1260:1345:1437:1534:1541:1711:1730:1747:1777:1792:1801:2198:2199:2393:2559:2562:2731:3138:3139:3140:3141:3142:3352:3865:3866:3867:3868:4250:4605:5007:6261:6737:10004:10848:11026:11658:11914:12043:12048:12297:12679:12895:13069:13161:13229:13311:13357:14096:14181:14384:14394:14581:14721:21080:21220:21451:21627:30054, 0, RBL:23.242.196.136:@perches.com:.lbl8.mailshell.net-62.8.0.180 64.201.201.201, CacheIP:none, Bayesian:0.5, 0.5, 0.5, Netcheck:none, DomainCache:0, MSF:not bulk, SPF:fn, MSBL:0, DNSBL:neutral, Custom_rules:0:0:0, LFtime:26, LUA_SUMMARY:none X-HE-Tag: help91_a3a1c1ec484c X-Filterd-Recvd-Size: 2833 Received: from joe-laptop.perches.com (cpe-23-242-196-136.socal.res.rr.com [23.242.196.136]) (Authenticated sender: joe@perches.com) by omf06.hostedemail.com (Postfix) with ESMTPA; Wed, 10 Jul 2019 05:04:30 +0000 (UTC) From: Joe Perches <joe@perches.com> To: Andrew Morton <akpm@linux-foundation.org>, Patrick Venture <venture@google.com>, Nancy Yuen <yuenn@google.com>, Benjamin Fair <benjaminfair@google.com>, Andrew Jeffery <andrew@aj.id.au>, openbmc@lists.ozlabs.org, linux-kernel@vger.kernel.org, linux-aspeed@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, netdev@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, linux-wireless@vger.kernel.org, linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org, linux-iio@vger.kernel.org, linux-mmc@vger.kernel.org, devel@driverdev.osuosl.org, alsa-devel@alsa-project.org Subject: [PATCH 00/12] treewide: Fix GENMASK misuses Date: Tue, 9 Jul 2019 22:04:13 -0700 Message-Id: <cover.1562734889.git.joe@perches.com> X-Mailer: git-send-email 2.15.0 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Series |
treewide: Fix GENMASK misuses
|
|
Message
Joe Perches
July 10, 2019, 5:04 a.m. UTC
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(-)
Comments
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