treewide: Fix GENMASK misuses

Subject: [PATCH 09/12] rtw88: Fix misuse of GENMASK macro

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/net/wireless/realtek/rtw88/rtw8822b.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/realtek/rtw88/rtw8822b.c +1 -1

index 1172f6c0605b..d61d534396c7 100644
@@ -997,7 +997,7 @@ static void rtw8822b_do_iqk(struct rtw_dev *rtwdev)
 	rtw_write_rf(rtwdev, RF_PATH_A, RF_DTXLOK, RFREG_MASK, 0x0);
 
 	reload = !!rtw_read32_mask(rtwdev, REG_IQKFAILMSK, BIT(16));
-	iqk_fail_mask = rtw_read32_mask(rtwdev, REG_IQKFAILMSK,
 GENMASK(0, 7));
+	iqk_fail_mask = rtw_read32_mask(rtwdev, REG_IQKFAILMSK,
 GENMASK(7, 0));
 	rtw_dbg(rtwdev, RTW_DBG_PHY,

"iqk counter=%d reload=%d do_iqk_cnt=%d
n_iqk_fail(mask)=0x%02x\n",
counter, reload, ++do_iqk_cnt, iqk_fail_mask);
--

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/media/sunxi/cedrus/cedrus_regs.h +1 -1

index 3e9931416e45..ddd29788d685 100644
@@ -110,7 +110,7 @@
 #define VE_DEC_MPEG_MBADDR			(VE_ENGINE_DEC_MPEG + 0x10)
 
 #define VE_DEC_MPEG_MBADDR_X(w)			(((w) << 8) & GENMASK(15, 8))
-#define VE_DEC_MPEG_MBADDR_Y(h)			(((h) << 0) & GENMASK(0, 7))
+#define VE_DEC_MPEG_MBADDR_Y(h)			(((h) << 0) & GENMASK(7, 0))
 
 #define VE_DEC_MPEG_CTRL			(VE_ENGINE_DEC_MPEG + 0x14)
 

Signature · 1 line

2.15.0

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-gic-v3-its.c +0 -0

index 35500801dc2b..730fbe0e2a9d 100644

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

Quoted 7 lines · Joe Perches

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()?

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

Quoted 4 lines · Johannes Berg

Quoted 3 lines · Joe Perches

These GENMASK uses are inverted argument order and the actual masks produced are incorrect. Fix them.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 5 lines · Russell King - ARM Linux admin

Quoted 4 lines · Johannes Berg

Quoted 3 lines · Joe Perches

These GENMASK uses are inverted argument order and the actual masks produced are incorrect. Fix them.

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

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.

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.

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 <[email protected]>

---
 drivers/iio/adc/max9611.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/adc/max9611.c +1 -1

index 917223d5ff5b..0e3c6529fc4c 100644
@@ -83,7 +83,7 @@
 #define MAX9611_TEMP_MAX_POS		0x7f80
 #define MAX9611_TEMP_MAX_NEG		0xff80
 #define MAX9611_TEMP_MIN_NEG		0xd980
-#define MAX9611_TEMP_MASK		GENMASK(7, 15)
+#define MAX9611_TEMP_MASK		GENMASK(15, 7)
 #define MAX9611_TEMP_SHIFT		0x07
 #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
 #define MAX9611_TEMP_SCALE_NUM		1000000

Quoted 3 lines · On Tue, 9 Jul 2019 22:04:17 -0700 Joe Perches <[email protected]> wrote:

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

Applied to the fixes-togreg branch of iio.git and marked for stable etc.

Thanks,

Jonathan

Quoted 17 lines

---
 drivers/iio/adc/max9611.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/adc/max9611.c +1 -1

index 917223d5ff5b..0e3c6529fc4c 100644
@@ -83,7 +83,7 @@
 #define MAX9611_TEMP_MAX_POS		0x7f80
 #define MAX9611_TEMP_MAX_NEG		0xff80
 #define MAX9611_TEMP_MIN_NEG		0xd980
-#define MAX9611_TEMP_MASK		GENMASK(7, 15)
+#define MAX9611_TEMP_MASK		GENMASK(15, 7)
 #define MAX9611_TEMP_SHIFT		0x07
 #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
 #define MAX9611_TEMP_SCALE_NUM		1000000

Quoted 7 lines · Jonathan Cameron

Quoted 3 lines · On Tue, 9 Jul 2019 22:04:17 -0700 Joe Perches <[email protected]> wrote:

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

Quoted 2 lines

Applied to the fixes-togreg branch of iio.git and marked for stable etc.

This mask is used in an init function called from a probe.

I don't have this hardware but it looks as if it could never have worked so I doubt the driver and the hardware have ever been tested.

Does anyone have this device in actual use?

	regval = ret & MAX9611_TEMP_MASK;

	if ((regval > MAX9611_TEMP_MAX_POS &&
	     regval < MAX9611_TEMP_MIN_NEG) ||
	     regval > MAX9611_TEMP_MAX_NEG) {
		dev_err(max9611->dev,
			"Invalid value received from ADC 0x%4x: aborting\n",
			regval);
		return -EIO;
	}

Quoted 21 lines

Thanks,

Jonathan

Quoted 17 lines

---
 drivers/iio/adc/max9611.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/adc/max9611.c +1 -1

index 917223d5ff5b..0e3c6529fc4c 100644
@@ -83,7 +83,7 @@
 #define MAX9611_TEMP_MAX_POS		0x7f80
 #define MAX9611_TEMP_MAX_NEG		0xff80
 #define MAX9611_TEMP_MIN_NEG		0xd980
-#define MAX9611_TEMP_MASK		GENMASK(7, 15)
+#define MAX9611_TEMP_MASK		GENMASK(15, 7)
 #define MAX9611_TEMP_SHIFT		0x07
 #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
 #define MAX9611_TEMP_SCALE_NUM		1000000

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/clocksource/timer-npcm7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/clocksource/timer-npcm7xx.c +0 -0

index 7a9bb5532d99..8a30da7f083b 100644

drivers/clocksource/timer-npcm7xx.c +0 -1

 #define NPCM7XX_Tx_MIN_PRESCALE                0x1
 #define NPCM7XX_Tx_TDR_MASK_BITS       24
 #define NPCM7XX_Tx_MAX_CNT             0xFFFFFF
--

2.15.0

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/phy/amlogic/phy-meson-g12a-usb2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/phy/amlogic/phy-meson-g12a-usb2.c +0 -0

index 9065ffc85eb4..cd7eccab2649 100644

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/mmc/host/meson-mx-sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/host/meson-mx-sdio.c +0 -0

index 2d736e416775..ba9a63db73da 100644

From f7408a3d5b5fd10571a653d1a81ce9167c62727f Mon Sep 17 00:00:00 2001

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/mmc/host/meson-mx-sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/host/meson-mx-sdio.c +1 -2

index 2d736e416775..ba9a63db73da 100644
@@ -73,7 +73,7 @@
        #define MESON_MX_SDIO_IRQC_IF_CONFIG_MASK               GENMASK(7, 6)
        #define MESON_MX_SDIO_IRQC_FORCE_DATA_CLK               BIT(8)
        #define MESON_MX_SDIO_IRQC_FORCE_DATA_CMD               BIT(9)
-       #define MESON_MX_SDIO_IRQC_FORCE_DATA_DAT_MASK          GENMASK(10, 13)
+       #define MESON_MX_SDIO_IRQC_FORCE_DATA_DAT_MASK          GENMASK(13, 10)
        #define MESON_MX_SDIO_IRQC_SOFT_RESET                   BIT(15)
        #define MESON_MX_SDIO_IRQC_FORCE_HALT                   BIT(30)
        #define MESON_MX_SDIO_IRQC_HALT_HOLE                    BIT(31)
--

2.15.0

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

Acked-by Yan-Hsuan Chuang <[email protected]>

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/media/sunxi/cedrus/cedrus_regs.h +0 -0

index 3e9931416e45..ddd29788d685 100644

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/aspeed/aspeed_gfx.h +0 -0

index a10358bb61ec..095ea03e5833 100644

This macro is easy to misuse as it's odd argument order.

If specified with simple decimal values, make sure the arguments are ordered high then low.

Also check if any argument is > 32 where instead of GENMASK, GENMASK_ULL should be used.

Signed-off-by Joe Perches <[email protected]>

---
 scripts/checkpatch.pl | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

scripts/checkpatch.pl +15 -0

index 6cb99ec62000..d37bbe33524b 100755
@@ -6368,6 +6368,21 @@ sub process {
 			     "switch default: should use break\n" . $herectx);
 		}
 
+# check for misuses of GENMASK
+		if ($line =~ /\b(GENMASK(?:_ULL)?)\s*\(\s*(\d+)\s*,\s*(\d+)\s*\)/) {
+			my $type = $1;
+			my $high = $2;
+			my $low = $3;
+			if ($high < $low) {
+				ERROR("GENMASK",
+				      "$type argument order is high then low\n" . $herecurr);
+			}
+			if ($type eq "GENMASK" && ($high >= 32 || $low >= 32)) {
+				ERROR("GENMASK",
+				      "$type with arguments >= 32 should use GENMASK_ULL\n" . $herecurr);
+			}
+		}
+
 # check for gcc specific __FUNCTION__
 		if ($line =~ /\b__FUNCTION__\b/) {
 			if (WARN("USE_FUNC",

Quoted 14 lines · Joe Perches

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/media/sunxi/cedrus/cedrus_regs.h +0 -0

index 3e9931416e45..ddd29788d685 100644
@@ -110,7 +110,7 @@
 #define VE_DEC_MPEG_MBADDR			(VE_ENGINE_DEC_MPEG + 0x10)
 

drivers/staging/media/sunxi/cedrus/cedrus_regs.h line 115

Quoted 5 lines

drivers/staging/media/sunxi/cedrus/cedrus_regs.h +1 -1

 #define VE_DEC_MPEG_MBADDR_X(w)			(((w) << 8) & GENMASK(15, 8))
-#define VE_DEC_MPEG_MBADDR_Y(h)			(((h) << 0) & GENMASK(0, 7))
+#define VE_DEC_MPEG_MBADDR_Y(h)			(((h) << 0) & GENMASK(7, 0))
 
 #define VE_DEC_MPEG_CTRL			(VE_ENGINE_DEC_MPEG + 0x14)

Greg? ping?

Quoted 19 lines · Joe Perches

Quoted 18 lines · Joe Perches

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/media/sunxi/cedrus/cedrus_regs.h +1 -1

index 3e9931416e45..ddd29788d685 100644
@@ -110,7 +110,7 @@
 #define VE_DEC_MPEG_MBADDR			(VE_ENGINE_DEC_MPEG + 0x10)
 
 #define VE_DEC_MPEG_MBADDR_X(w)			(((w) << 8) & GENMASK(15, 8))
-#define VE_DEC_MPEG_MBADDR_Y(h)			(((h) << 0) & GENMASK(0, 7))
+#define VE_DEC_MPEG_MBADDR_Y(h)			(((h) << 0) & GENMASK(7, 0))
 

Quoted 5 lines

drivers/staging/media/sunxi/cedrus/cedrus_regs.h line 115

Quoted 1 line

drivers/staging/media/sunxi/cedrus/cedrus_regs.h +0 -0

 #define VE_DEC_MPEG_CTRL			(VE_ENGINE_DEC_MPEG + 0x14)

Greg? ping?

I am not the maintainer of drivers/staging/media, that is Mauro.

Quoted 20 lines · Greg Kroah-Hartman

Quoted 19 lines · Joe Perches

Quoted 18 lines · Joe Perches

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/media/sunxi/cedrus/cedrus_regs.h +1 -1

index 3e9931416e45..ddd29788d685 100644
@@ -110,7 +110,7 @@
 #define VE_DEC_MPEG_MBADDR			(VE_ENGINE_DEC_MPEG + 0x10)
 
 #define VE_DEC_MPEG_MBADDR_X(w)			(((w) << 8) & GENMASK(15, 8))
-#define VE_DEC_MPEG_MBADDR_Y(h)			(((h) << 0) & GENMASK(0, 7))
+#define VE_DEC_MPEG_MBADDR_Y(h)			(((h) << 0) & GENMASK(7, 0))
 

Quoted 7 lines

Quoted 5 lines

drivers/staging/media/sunxi/cedrus/cedrus_regs.h line 115

Quoted 1 line

drivers/staging/media/sunxi/cedrus/cedrus_regs.h +0 -0

 #define VE_DEC_MPEG_CTRL			(VE_ENGINE_DEC_MPEG + 0x14)

Greg? ping?

I am not the maintainer of drivers/staging/media, that is Mauro.

Maybe you want:
---

MAINTAINERS +0 -0

index f7dc5bad396a..2fb95ad6ebef 100644

Quoted 7 lines · Joe Perches

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/aspeed/aspeed_gfx.h int aspeed_gfx_create_output(struct drm_device *drm); · line 77

Quoted 10 lines

drivers/gpu/drm/aspeed/aspeed_gfx.h +1 -1

index a10358bb61ec..095ea03e5833 100644
@@ -74,7 +74,7 @@ int aspeed_gfx_create_output(struct drm_device *drm);
 /* CTRL2 */
 #define CRT_CTRL_DAC_EN			BIT(0)
 #define CRT_CTRL_VBLANK_LINE(x)		(((x) << 20) & CRT_CTRL_VBLANK_LINE_MASK)
-#define CRT_CTRL_VBLANK_LINE_MASK	GENMASK(20, 31)
+#define CRT_CTRL_VBLANK_LINE_MASK	GENMASK(31, 20)

ping?

Quoted 8 lines · Joe Perches

Quoted 7 lines · Joe Perches

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 10 lines

drivers/gpu/drm/aspeed/aspeed_gfx.h int aspeed_gfx_create_output(struct drm_device *drm); · line 77

Quoted 10 lines

drivers/gpu/drm/aspeed/aspeed_gfx.h +1 -1

index a10358bb61ec..095ea03e5833 100644
@@ -74,7 +74,7 @@ int aspeed_gfx_create_output(struct drm_device *drm);
 /* CTRL2 */
 #define CRT_CTRL_DAC_EN			BIT(0)
 #define CRT_CTRL_VBLANK_LINE(x)		(((x) << 20) & CRT_CTRL_VBLANK_LINE_MASK)
-#define CRT_CTRL_VBLANK_LINE_MASK	GENMASK(20, 31)
+#define CRT_CTRL_VBLANK_LINE_MASK	GENMASK(31, 20)

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 10 lines · Andrew Jeffery

Quoted 8 lines · Joe Perches

Quoted 7 lines · Joe Perches

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

Quoted 10 lines

drivers/gpu/drm/aspeed/aspeed_gfx.h int aspeed_gfx_create_output(struct drm_device *drm); · line 77

Quoted 10 lines

drivers/gpu/drm/aspeed/aspeed_gfx.h +1 -1

index a10358bb61ec..095ea03e5833 100644
@@ -74,7 +74,7 @@ int aspeed_gfx_create_output(struct drm_device *drm);
 /* CTRL2 */
 #define CRT_CTRL_DAC_EN                  BIT(0)
 #define CRT_CTRL_VBLANK_LINE(x)          (((x) << 20) & CRT_CTRL_VBLANK_LINE_MASK)
-#define CRT_CTRL_VBLANK_LINE_MASK        GENMASK(20, 31)
+#define CRT_CTRL_VBLANK_LINE_MASK        GENMASK(31, 20)

Reviewed-by Andrew Jeffery <[email protected]>

This hardly needs a review, it needs to be applied. There's a nominal git tree for aspeed here:

T: git git://git.kernel.org/pub/scm/linux/kernel/git/joel/aspeed.git

But who's going to do apply this?

Quoted 14 lines · Joe Perches

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/media/sunxi/cedrus/cedrus_regs.h +0 -0

index 3e9931416e45..ddd29788d685 100644
@@ -110,7 +110,7 @@
 #define VE_DEC_MPEG_MBADDR			(VE_ENGINE_DEC_MPEG + 0x10)
 

drivers/staging/media/sunxi/cedrus/cedrus_regs.h line 115

Quoted 5 lines

drivers/staging/media/sunxi/cedrus/cedrus_regs.h +1 -1

 #define VE_DEC_MPEG_MBADDR_X(w)			(((w) << 8) & GENMASK(15, 8))
-#define VE_DEC_MPEG_MBADDR_Y(h)			(((h) << 0) & GENMASK(0, 7))
+#define VE_DEC_MPEG_MBADDR_Y(h)			(((h) << 0) & GENMASK(7, 0))
 
 #define VE_DEC_MPEG_CTRL			(VE_ENGINE_DEC_MPEG + 0x14)

Greg? ping?

Quoted 28 lines · Joe Perches

Quoted 9 lines · Andrew Jeffery

Quoted 8 lines · Joe Perches

Quoted 7 lines · Joe Perches

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

Quoted 10 lines

drivers/gpu/drm/aspeed/aspeed_gfx.h int aspeed_gfx_create_output(struct drm_device *drm); · line 77

Quoted 10 lines

drivers/gpu/drm/aspeed/aspeed_gfx.h +1 -1

index a10358bb61ec..095ea03e5833 100644
@@ -74,7 +74,7 @@ int aspeed_gfx_create_output(struct drm_device *drm);
 /* CTRL2 */
 #define CRT_CTRL_DAC_EN                  BIT(0)
 #define CRT_CTRL_VBLANK_LINE(x)          (((x) << 20) & CRT_CTRL_VBLANK_LINE_MASK)
-#define CRT_CTRL_VBLANK_LINE_MASK        GENMASK(20, 31)
+#define CRT_CTRL_VBLANK_LINE_MASK        GENMASK(31, 20)

Reviewed-by Andrew Jeffery <[email protected]>

This hardly needs a review, it needs to be applied. There's a nominal git tree for aspeed here:

T: git git://git.kernel.org/pub/scm/linux/kernel/git/joel/aspeed.git

Quoted 1 line

But who's going to do apply this?

This is a DRM patch, so it goes through the DRM tree. I am a co-maintainer there and can apply it once I remember how to drive the tools.

(FYI, this macro is not used by the current driver).

It'd can't be done as it's used in declarations
and included in asm files and it uses the UL()
macro.

Trimming CC-list.

Quoted 3 lines

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(-)

include/linux/bits.h +7 -2

index 669d69441a62..52e747d27f87 100644
@@ -2,6 +2,7 @@
 #ifndef __LINUX_BITS_H
 #define __LINUX_BITS_H
 
+#include <linux/build_bug.h>
 #include <linux/const.h>
 #include <asm/bitsperlong.h>
 

@@ -19,11 +20,15 @@
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
 #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 */

Quoted 9 lines · Jonathan Cameron

Quoted 3 lines · On Tue, 9 Jul 2019 22:04:17 -0700 Joe Perches <[email protected]> wrote:

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

Applied to the fixes-togreg branch of iio.git and marked for stable etc.

This mask is used in an init function called from a probe.

I don't have this hardware but it looks as if it could never have worked so I doubt the driver and the hardware have ever been tested.

Does anyone have this device in actual use?

	regval = ret & MAX9611_TEMP_MASK;

	if ((regval > MAX9611_TEMP_MAX_POS &&
	     regval < MAX9611_TEMP_MIN_NEG) ||
	     regval > MAX9611_TEMP_MAX_NEG) {

		dev_err(max9611->dev,
			"Invalid value received from ADC 0x%4x: aborting\n",
			regval);
		return -EIO;
	}

Quoted 21 lines

Thanks,

Jonathan

Quoted 17 lines

---
 drivers/iio/adc/max9611.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/adc/max9611.c +1 -1

index 917223d5ff5b..0e3c6529fc4c 100644
@@ -83,7 +83,7 @@
 #define MAX9611_TEMP_MAX_POS		0x7f80
 #define MAX9611_TEMP_MAX_NEG		0xff80
 #define MAX9611_TEMP_MIN_NEG		0xd980
-#define MAX9611_TEMP_MASK		GENMASK(7, 15)
+#define MAX9611_TEMP_MASK		GENMASK(15, 7)
 #define MAX9611_TEMP_SHIFT		0x07
 #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
 #define MAX9611_TEMP_SCALE_NUM		1000000

Hello,
so I finally run some test and...

Quoted 10 lines · Joe Perches

Quoted 9 lines · Jonathan Cameron

Quoted 3 lines · On Tue, 9 Jul 2019 22:04:17 -0700 Joe Perches <[email protected]> wrote:

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

Applied to the fixes-togreg branch of iio.git and marked for stable etc.

I don't see it in v5.3-rc2, has it been collected or are we still in time for an additional fix?

Quoted 8 lines

This mask is used in an init function called from a probe.

I don't have this hardware but it looks as if it could never have worked so I doubt the driver and the hardware have ever been tested.

Does anyone have this device in actual use?

Because it turns out this is 2 times embarrassing. The mask definition is indeed wrong, as Joe reported and fixed, but also this line

Quoted 2 lines

	regval = ret & MAX9611_TEMP_MASK;  

is very wrong as regval is read as:

        ret = max9611_read_single(max9611, CONF_TEMP, &regval);

So that should actually be:

        regval &= MAX9611_TEMP_MASK;

not
regval = ret & MAX9611_TEMP_MASK;
Ups...

Yes, it worked by chance, as regval was always 0, which is in the range of acceptable temperatures :/

Quoted 4 lines

	if ((regval > MAX9611_TEMP_MAX_POS &&
	     regval < MAX9611_TEMP_MIN_NEG) ||
	     regval > MAX9611_TEMP_MAX_NEG) {  

Also reading this condition and how I had defined the temperature calculation formula makes me wonder if this it totally correct, but for the moment:

1) if Joe's patch has been collected, I can send an additional patch to fix how regval is computed.
2) If Joe's patch still have to be collected, the regval computation might be fixed there.

Sorry for taking so long to get back to you and thanks for noticing.

Thanks
j

Quoted 29 lines

		dev_err(max9611->dev,
			"Invalid value received from ADC 0x%4x: aborting\n",
			regval);
		return -EIO;
	}

Quoted 21 lines

Thanks,

Jonathan

Quoted 17 lines

---
 drivers/iio/adc/max9611.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/adc/max9611.c +1 -1

index 917223d5ff5b..0e3c6529fc4c 100644
@@ -83,7 +83,7 @@
 #define MAX9611_TEMP_MAX_POS		0x7f80
 #define MAX9611_TEMP_MAX_NEG		0xff80
 #define MAX9611_TEMP_MIN_NEG		0xd980
-#define MAX9611_TEMP_MASK		GENMASK(7, 15)
+#define MAX9611_TEMP_MASK		GENMASK(15, 7)
 #define MAX9611_TEMP_SHIFT		0x07
 #define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
 #define MAX9611_TEMP_SCALE_NUM		1000000  

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_OR_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

---
Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 7 of the patches were not in Linus tree, and 2 were not in linux-next.

Also, there's currently no asm users of bits.h, but since it was made asm-compatible just two weeks ago it would be a shame to break it right away...

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_OR_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

---
Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 7 of the patches were not in Linus tree, and 2 were not in linux-next.

Also, there's currently no asm users of bits.h, but since it was made asm-compatible just two weeks ago it would be a shame to break it right away...

 include/linux/bits.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

include/linux/bits.h +8 -0

index 669d69441a62..73489579eef9 100644
@@ -18,12 +18,22 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l)  BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+               __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0))
+#else
+#define GENMASK_INPUT_CHECK(h, l) 0
+#endif
+
 #define GENMASK(h, l) \

Quoted 29 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_OR_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

---
Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 7 of the patches were not in Linus tree, and 2 were not in linux-next.

Quoted 3 lines

Also, there's currently no asm users of bits.h, but since it was made asm-compatible just two weeks ago it would be a shame to break it right away...

include/linux/bits.h

Quoted 1 line

include/linux/bits.h +0 -0

include/linux/bits.h line 37

Quoted 11 lines

#define GENMASK(h, l) \

include/linux/bits.h +4 -2

+       (GENMASK_INPUT_CHECK(h, l) + \
        (((~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) \
+       (GENMASK_INPUT_CHECK(h, l) + \
        (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
-        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
+        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h)))))

This is getting cluttered with so many parentheses.

One way of clean-up is to rename the current macros as follows:

   GENMASK()    ->  __GENMASK()
   GENMASK_UL() ->  __GENMASK_ULL()

Then,

#define GENMASK(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
#define GENMASK_ULL(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))

Quoted 3 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

A few things:

o Reading the final code is a bit confusing. Perhaps add a comment description saying it's not checked in asm .h uses.

o Maybe use:
#define GENMASK_INPUT_CHECK(h, l) UL(0)

o The compiler error message when the arguments are in the wrong order isn't obvious. Is there some way to improve the compiler error output, maybe by using BUILD_BUG_ON_MSG or some other mechanism?

Quoted 13 lines · Masahiro Yamada

Quoted 3 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

This is getting cluttered with so many parentheses.

One way of clean-up is to rename the current macros as follows:

   GENMASK()    ->  __GENMASK()
   GENMASK_UL() ->  __GENMASK_ULL()

Quoted 4 lines

Then,

#define GENMASK(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
#define GENMASK_ULL(h, l) (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))

Much nicer. It may be better still to use avoid
multiple dereferences of each argument.

Also it'd be useful to rename h and l to something like high_bit and low_bit or high and low.

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_ON_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

If successful, BUILD_BUG_OR_ZERO() returns 0 of type size_t. To avoid problems with implicit conversions, cast the result of BUILD_BUG_OR_ZERO to unsigned long.

Since both BUILD_BUG_ON_ZERO() and __is_constexpr() only uses sizeof() on the arguments passed to them, neither of them evaluate the expression unless it is a VLA. Therefore, GENMASK(1, x++) still behaves as expected.

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

---
Changes in v2:
- Add comment about why inputs are not checked when used in asm file
- Use UL(0) instead of 0
- Extract mask creation in a separate macro to improve readability
- Use high and low instead of h and l (part of this was extracted to a separate patch)
- Updated commit message

Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 6 of the patches are not in Linus tree, and 2 are not in linux-next.

Quoted 24 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_OR_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

Quoted 8 lines

---
Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 7 of the patches were not in Linus tree, and 2 were not in linux-next.

Also, there's currently no asm users of bits.h, but since it was made asm-compatible just two weeks ago it would be a shame to break it right away...

[]

include/linux/bits.h

Quoted 1 line

include/linux/bits.h +0 -0

[]

include/linux/bits.h line 26

Quoted 10 lines

include/linux/bits.h +6 -0

@@ -18,12 +18,22 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l)  BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+             __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0))
+#else
+#define GENMASK_INPUT_CHECK(h, l) 0

A few things:

o Reading the final code is a bit confusing. Perhaps add a comment description saying it's not checked in asm .h uses.

o Maybe use:
#define GENMASK_INPUT_CHECK(h, l) UL(0)

o The compiler error message when the arguments are in the wrong order isn't obvious. Is there some way to improve the compiler error output, maybe by using BUILD_BUG_ON_MSG or some other mechanism?

Quoted 49 lines · Joe Perches

Quoted 24 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_OR_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

Quoted 8 lines

---
Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 7 of the patches were not in Linus tree, and 2 were not in linux-next.

Also, there's currently no asm users of bits.h, but since it was made asm-compatible just two weeks ago it would be a shame to break it right away...

[]

include/linux/bits.h

Quoted 1 line

include/linux/bits.h +0 -0

[]

include/linux/bits.h line 26

Quoted 10 lines

include/linux/bits.h +6 -0

@@ -18,12 +18,22 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l)  BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+             __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0))
+#else
+#define GENMASK_INPUT_CHECK(h, l) 0

A few things:

Quoted 6 lines

o Reading the final code is a bit confusing. Perhaps add a comment description saying it's not checked in asm .h uses.

o Maybe use:
#define GENMASK_INPUT_CHECK(h, l) UL(0)

Why?

Quoted 54 lines · Masahiro Yamada

Quoted 53 lines · Joe Perches

Quoted 24 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_OR_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

Quoted 8 lines

---
Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 7 of the patches were not in Linus tree, and 2 were not in linux-next.

Also, there's currently no asm users of bits.h, but since it was made asm-compatible just two weeks ago it would be a shame to break it right away...

[]

include/linux/bits.h

Quoted 1 line

include/linux/bits.h +0 -0

[]

include/linux/bits.h line 26

Quoted 10 lines

include/linux/bits.h +6 -0

@@ -18,12 +18,22 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l)  BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+             __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0))
+#else
+#define GENMASK_INPUT_CHECK(h, l) 0

A few things:

o Reading the final code is a bit confusing. Perhaps add a comment description saying it's not checked in asm .h uses.

Quoted 4 lines

Quoted 2 lines

o Maybe use:
#define GENMASK_INPUT_CHECK(h, l) UL(0)

Why?

Consistency with the uses in what's now called __GENMASK

Quoted 56 lines · Joe Perches

Quoted 54 lines · Masahiro Yamada

Quoted 53 lines · Joe Perches

Quoted 24 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_OR_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

Quoted 8 lines

---
Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 7 of the patches were not in Linus tree, and 2 were not in linux-next.

Also, there's currently no asm users of bits.h, but since it was made asm-compatible just two weeks ago it would be a shame to break it right away...

[]

include/linux/bits.h

Quoted 1 line

include/linux/bits.h +0 -0

[]

include/linux/bits.h line 26

Quoted 10 lines

include/linux/bits.h +6 -0

@@ -18,12 +18,22 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l)  BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+             __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0))
+#else
+#define GENMASK_INPUT_CHECK(h, l) 0

A few things:

o Reading the final code is a bit confusing. Perhaps add a comment description saying it's not checked in asm .h uses.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

o Maybe use:
#define GENMASK_INPUT_CHECK(h, l) UL(0)

Why?

Consistency with the uses in what's now called __GENMASK

Inconsistent with __GENMASK_ULL.

Quoted 60 lines · Masahiro Yamada

Quoted 59 lines · Joe Perches

Quoted 57 lines · Masahiro Yamada

Quoted 55 lines · Joe Perches

Quoted 24 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_OR_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

Quoted 8 lines

---
Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 7 of the patches were not in Linus tree, and 2 were not in linux-next.

Also, there's currently no asm users of bits.h, but since it was made asm-compatible just two weeks ago it would be a shame to break it right away...

[]

include/linux/bits.h

Quoted 1 line

include/linux/bits.h +0 -0

[]

include/linux/bits.h line 26

Quoted 10 lines

include/linux/bits.h +6 -0

@@ -18,12 +18,22 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l)  BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+             __is_constexpr(h) && __is_constexpr(l), (l) > (h), 0))
+#else
+#define GENMASK_INPUT_CHECK(h, l) 0

A few things:

o Reading the final code is a bit confusing. Perhaps add a comment description saying it's not checked in asm .h uses.

o Maybe use:
#define GENMASK_INPUT_CHECK(h, l) UL(0)

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Why?

Consistency with the uses in what's now called __GENMASK

Inconsistent with __GENMASK_ULL.

Would you prefer to add GENMASK_ULL_INPUT_CHECK?

Or replace UL(0) with
0 and then probably move the cast of BUILD_BUG_OR_ZERO (to avoid GENMASK be of type size_t) to GENMASK and GENMASK_ULL?

BTW, v2 is already inconsistent.
If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',, you would have to cast (low) > (high) as well:

               (unsigned long)((low) > (high)), UL(0))))

This is totally redundant, and weird.

Quoted 5 lines · Masahiro Yamada

BTW, v2 is already inconsistent.
If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',, you would have to cast (low) > (high) as well:

Quoted 3 lines

               (unsigned long)((low) > (high)), UL(0))))

This is totally redundant, and weird.

I take back this comment.
You added (unsigned long) to the beginning of this macro. So, the type is consistent, but I believe all casts should be removed.

Quoted 8 lines · Masahiro Yamada

Quoted 5 lines · Masahiro Yamada

BTW, v2 is already inconsistent.
If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',, you would have to cast (low) > (high) as well:

Quoted 7 lines

Quoted 3 lines

               (unsigned long)((low) > (high)), UL(0))))

This is totally redundant, and weird.

I take back this comment.
You added (unsigned long) to the beginning of this macro. So, the type is consistent, but I believe all casts should be removed.

Maybe you're right. BUILD_BUG_ON_ZERO returns size_t regardless of inputs. I was worried that on some platform, size_t would be larger than unsigned long (as far as I could see, the standard does not give any guarantees), and thus all of a sudden GENMASK would be 8 bytes instead of 4, but perhaps that is not a problem?

Quoted 18 lines · Rikard Falkeborn

Quoted 15 lines · Masahiro Yamada

Quoted 8 lines · Masahiro Yamada

BTW, v2 is already inconsistent.
If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',, you would have to cast (low) > (high) as well:

               (unsigned long)((low) > (high)), UL(0))))

This is totally redundant, and weird.

I take back this comment.
You added (unsigned long) to the beginning of this macro. So, the type is consistent, but I believe all casts should be removed.

Quoted 5 lines

Maybe you're right. BUILD_BUG_ON_ZERO returns size_t regardless of inputs. I was worried that on some platform, size_t would be larger than unsigned long (as far as I could see, the standard does not give any guarantees), and thus all of a sudden GENMASK would be 8 bytes instead of 4, but perhaps that is not a problem?

How about adding (int) cast to BUILD_BUG_ON_ZERO() ?

Quoted 27 lines · Masahiro Yamada

Quoted 23 lines · Rikard Falkeborn

Quoted 8 lines · Masahiro Yamada

Quoted 5 lines · Masahiro Yamada

BTW, v2 is already inconsistent.
If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',, you would have to cast (low) > (high) as well:

Quoted 7 lines

Quoted 3 lines

               (unsigned long)((low) > (high)), UL(0))))

This is totally redundant, and weird.

I take back this comment.
You added (unsigned long) to the beginning of this macro. So, the type is consistent, but I believe all casts should be removed.

Maybe you're right. BUILD_BUG_ON_ZERO returns size_t regardless of inputs. I was worried that on some platform, size_t would be larger than unsigned long (as far as I could see, the standard does not give any guarantees), and thus all of a sudden GENMASK would be 8 bytes instead of 4, but perhaps that is not a problem?

Quoted 1 line

How about adding (int) cast to BUILD_BUG_ON_ZERO() ?

I'll have a look.

Hi Rikard,

Quoted 29 lines · Rikard Falkeborn

Quoted 27 lines · Masahiro Yamada

Quoted 23 lines · Rikard Falkeborn

Quoted 8 lines · Masahiro Yamada

Quoted 5 lines · Masahiro Yamada

BTW, v2 is already inconsistent.
If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',, you would have to cast (low) > (high) as well:

Quoted 7 lines

Quoted 3 lines

               (unsigned long)((low) > (high)), UL(0))))

This is totally redundant, and weird.

I take back this comment.
You added (unsigned long) to the beginning of this macro. So, the type is consistent, but I believe all casts should be removed.

Maybe you're right. BUILD_BUG_ON_ZERO returns size_t regardless of inputs. I was worried that on some platform, size_t would be larger than unsigned long (as far as I could see, the standard does not give any guarantees), and thus all of a sudden GENMASK would be 8 bytes instead of 4, but perhaps that is not a problem?

Quoted 3 lines

Quoted 1 line

How about adding (int) cast to BUILD_BUG_ON_ZERO() ?

I'll have a look.

I found a more important problem in this patch.

You used __is_constexpr(), which is defined in <linux/kernel.h>.

This header does not include <linux/kernel.h>,
so this header is not self-contained anymore.

The following test code fails to build:

#include <linux/bits.h>
unsigned long foo(unsigned long in_bits)

{
        return in_bits & GENMASK(5, 3);
}

However, you cannot include <linux/kernel.h> from <linux/bits.h>. See the log of 8bd9cb51daac89337295b6f037b0486911e1b408

This header was split out to not pull in <linux/bitops.h> Including <linux/kernel.h> pulls in <linux/bitops.h> again.

In summary, please use __builtin_constant_p()
instead of __is_constexpr().

You can shorten __builtin_constant_p(high) && __builtin_constant_p(low) into __builtin_constant_p((low) > (high)).

How about this?

#define GENMASK_INPUT_CHECK(high, low) \
       BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
              __builtin_constant_p((low) > (high)), (low) > (high), 0))

Signature · 2 lines

Best Regards
Masahiro Yamada

Quoted 4 lines · Masahiro Yamada

How about this?

#define GENMASK_INPUT_CHECK(high, low) \
       BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
              __builtin_constant_p((low) > (high)), (low) > (high), 0))

Thanks for the feedback, your version looks much cleaner than mine. I *think* I had a reason for using __is_constexpr() instead of __builtin_constant_p but I'll try a full rebuild to see if something comes up.

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_ON_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

If successful, BUILD_BUG_OR_ZERO() returns 0 of type size_t. To avoid problems with implicit conversions, cast the result of BUILD_BUG_OR_ZERO to unsigned long.

Since both BUILD_BUG_ON_ZERO() and __is_constexpr() only uses sizeof() on the arguments passed to them, neither of them evaluate the expression unless it is a VLA. Therefore, GENMASK(1, x++) still behaves as expected.

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Quoted 27 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_ON_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

If successful, BUILD_BUG_OR_ZERO() returns 0 of type size_t. To avoid problems with implicit conversions, cast the result of BUILD_BUG_OR_ZERO to unsigned long.

Since both BUILD_BUG_ON_ZERO() and __is_constexpr() only uses sizeof() on the arguments passed to them, neither of them evaluate the expression unless it is a VLA. Therefore, GENMASK(1, x++) still behaves as expected.

Quoted 5 lines

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Who is going to fix the fallout ? For example, arm64:defconfig no longer compiles with this patch applied.

It seems to me that the benefit of catching misuses of GENMASK is much less than the fallout from no longer compiling kernels, since those kernels won't get any test coverage at all anymore.

Quoted 38 lines · Guenter Roeck

Quoted 27 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_ON_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

If successful, BUILD_BUG_OR_ZERO() returns 0 of type size_t. To avoid problems with implicit conversions, cast the result of BUILD_BUG_OR_ZERO to unsigned long.

Since both BUILD_BUG_ON_ZERO() and __is_constexpr() only uses sizeof() on the arguments passed to them, neither of them evaluate the expression unless it is a VLA. Therefore, GENMASK(1, x++) still behaves as expected.

Quoted 5 lines

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Who is going to fix the fallout ? For example, arm64:defconfig no longer compiles with this patch applied.

Quoted 3 lines

It seems to me that the benefit of catching misuses of GENMASK is much less than the fallout from no longer compiling kernels, since those kernels won't get any test coverage at all anymore.

We cannot apply this until we fix all errors.

I do not understand why Andrew picked up this so soon.

Quoted 46 lines · Masahiro Yamada

Quoted 35 lines · Guenter Roeck

Quoted 27 lines · Rikard Falkeborn

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both the arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_ON_ZERO() must be used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead of __builtin_constant_p().

If successful, BUILD_BUG_OR_ZERO() returns 0 of type size_t. To avoid problems with implicit conversions, cast the result of BUILD_BUG_OR_ZERO to unsigned long.

Since both BUILD_BUG_ON_ZERO() and __is_constexpr() only uses sizeof() on the arguments passed to them, neither of them evaluate the expression unless it is a VLA. Therefore, GENMASK(1, x++) still behaves as expected.

Quoted 5 lines

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm compatible, disable the checks if the file is included in an asm file.

Quoted 6 lines

Who is going to fix the fallout ? For example, arm64:defconfig no longer compiles with this patch applied.

It seems to me that the benefit of catching misuses of GENMASK is much less than the fallout from no longer compiling kernels, since those kernels won't get any test coverage at all anymore.

We cannot apply this until we fix all errors.

Quoted 2 lines

I do not understand why Andrew picked up this so soon.

The same was done with the fallthrough warning in mainline, which still results in all "sh" builds failing there (and in -next, obviously). I don't understand the logic either, but maybe it is the new normal.

Guenter

Quoted 9 lines · Rikard Falkeborn

Quoted 4 lines · Masahiro Yamada

How about this?

#define GENMASK_INPUT_CHECK(high, low) \
       BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
              __builtin_constant_p((low) > (high)), (low) > (high), 0))

Thanks for the feedback, your version looks much cleaner than mine. I *think* I had a reason for using __is_constexpr() instead of __builtin_constant_p but I'll try a full rebuild to see if something comes up.

Perhaps a statement expression so high and low aren't possibly evaluated multiple times?

#define GENMASK_INPUT_CHECK(high, low) \

({								\
	typeof(high) _high = high;				\
	typeof(low) _low = low;					\
	BUILD_BUG_ON_ZERO(__builtin_constant_p(_low > _high,	\
					       _low > _high,	\
					       0))		\
})

On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <[email protected]> wrote:

Quoted 6 lines

Who is going to fix the fallout ? For example, arm64:defconfig no longer compiles with this patch applied.

It seems to me that the benefit of catching misuses of GENMASK is much less than the fallout from no longer compiling kernels, since those kernels won't get any test coverage at all anymore.

We cannot apply this until we fix all errors. I do not understand why Andrew picked up this so soon.

Quoted 1 line · Masahiro Yamada

On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <[email protected]> wrote:

[]

Quoted 7 lines

Quoted 6 lines

Who is going to fix the fallout ? For example, arm64:defconfig no longer compiles with this patch applied.

It seems to me that the benefit of catching misuses of GENMASK is much less than the fallout from no longer compiling kernels, since those kernels won't get any test coverage at all anymore.

Quoted 2 lines

We cannot apply this until we fix all errors. I do not understand why Andrew picked up this so soon.

I think it makes complete sense to break -next (not mainline) and force
people to fix defects. Especially these types of defects that are
trivial to fix.

Quoted 10 lines · Joe Perches

Quoted 1 line · Masahiro Yamada

On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <[email protected]> wrote:

[]

Quoted 7 lines

Quoted 6 lines

Who is going to fix the fallout ? For example, arm64:defconfig no longer compiles with this patch applied.

It seems to me that the benefit of catching misuses of GENMASK is much less than the fallout from no longer compiling kernels, since those kernels won't get any test coverage at all anymore.

Quoted 7 lines

Quoted 2 lines

We cannot apply this until we fix all errors. I do not understand why Andrew picked up this so soon.

I think it makes complete sense to break -next (not mainline) and force people to fix defects. Especially these types of defects that are trivial to fix.

I don't think this (from next-20190807):

Build results:

	total: 158 pass: 137 fail: 21
Qemu test results:
	total: 391 pass: 318 fail: 73

is very useful. The situation is bad enough for newly introduced problems. It is all but impossible to get fixes for all problems discovered (or introduced) by adding checks like this one. In some cases, no one will care. In others, no one will pick up patches. Sometimes people won't know or realize that they are expected to fix something. Making the situation worse, the failures introduced by the new checks will hide other accumulating problems.

arch/sh has failed to build in mainline since 7/27 and in -next since next-20190711, due to the added "fallthrough" warning. I don't think that is too useful either. Ok, that situation may be a sign that the architecture isn't maintained as well as it should, but I don't think that this warrants breaking it on purpose in the hope to trigger some kind of reaction.

I don't mind if new checks are introduced, and I agree that it is useful and makes sense. But the checks should only be introduced after a reasonable attempt was made to fix _all_ associated problems. That doesn't mean that the entire work has to be done by the person introducing the check, but I do see that person responsible for making sure (or a reasonable definition of "make sure") that all problems are fixed before actually introducing the check. Yes, I understand, this is a lot of work, but adding checks and letting all hell break loose can not be the answer.

Quoted 40 lines · Guenter Roeck

Quoted 10 lines · Joe Perches

Quoted 1 line · Masahiro Yamada

On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <[email protected]> wrote:

[]

Quoted 7 lines

Quoted 6 lines

Who is going to fix the fallout ? For example, arm64:defconfig no longer compiles with this patch applied.

It seems to me that the benefit of catching misuses of GENMASK is much less than the fallout from no longer compiling kernels, since those kernels won't get any test coverage at all anymore.

Quoted 7 lines

Quoted 2 lines

We cannot apply this until we fix all errors. I do not understand why Andrew picked up this so soon.

I think it makes complete sense to break -next (not mainline) and force people to fix defects. Especially these types of defects that are trivial to fix.

I don't think this (from next-20190807):

Build results:

	total: 158 pass: 137 fail: 21
Qemu test results:
	total: 391 pass: 318 fail: 73

is very useful. The situation is bad enough for newly introduced problems. It is all but impossible to get fixes for all problems discovered (or introduced) by adding checks like this one. In some cases, no one will care. In others, no one will pick up patches. Sometimes people won't know or realize that they are expected to fix something. Making the situation worse, the failures introduced by the new checks will hide other accumulating problems.

arch/sh has failed to build in mainline since 7/27 and in -next since next-20190711, due to the added "fallthrough" warning. I don't think that is too useful either. Ok, that situation may be a sign that the architecture isn't maintained as well as it should, but I don't think that this warrants breaking it on purpose in the hope to trigger some kind of reaction.

Quoted 8 lines

I don't mind if new checks are introduced, and I agree that it is useful and makes sense. But the checks should only be introduced after a reasonable attempt was made to fix _all_ associated problems. That doesn't mean that the entire work has to be done by the person introducing the check, but I do see that person responsible for making sure (or a reasonable definition of "make sure") that all problems are fixed before actually introducing the check. Yes, I understand, this is a lot of work, but adding checks and letting all hell break loose can not be the answer.

No hell is unleashed.

It's -next, an integration build, not mainline.

Be a little more verbose to improve readability.

Signed-off-by Rikard Falkeborn <[email protected]>

---
Changes in v2:
- This patch is new in v2

 include/linux/bits.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

include/linux/bits.h +8 -9

index 669d69441a62..d4466aa42a9c 100644
@@ -14,16 +14,16 @@
 #define BITS_PER_BYTE          8
 
 /*
- * Create a contiguous bitmask starting at bit position @l and ending at
- * position @h. For example
+ * Create a contiguous bitmask starting at bit position @low and ending at
+ * position @high. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
-#define GENMASK(h, l) \
-       (((~UL(0)) - (UL(1) << (l)) + 1) & \
-        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
+#define GENMASK(high, low) \
+       (((~UL(0)) - (UL(1) << (low)) + 1) & \
+        (~UL(0) >> (BITS_PER_LONG - 1 - (high))))
 
-#define GENMASK_ULL(h, l) \
-       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
-        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
+#define GENMASK_ULL(high, low) \
+       (((~ULL(0)) - (ULL(1) << (low)) + 1) & \
+        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (high))))
 
 #endif /* __LINUX_BITS_H */
--

2.22.0

Quoted 4 lines · Rikard Falkeborn

Be a little more verbose to improve readability.

Signed-off-by Rikard Falkeborn <[email protected]>

BTW, I do not understand what the improvement is.
I tend to regard this as a noise commit.

Quoted 3 lines · Rikard Falkeborn

Be a little more verbose to improve readability.

Signed-off-by Rikard Falkeborn <[email protected]>

BTW, I do not understand what the improvement is.
I tend to regard this as a noise commit.

Quoted 40 lines

---
Changes in v2:
- This patch is new in v2

 include/linux/bits.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

include/linux/bits.h +8 -9

index 669d69441a62..d4466aa42a9c 100644
@@ -14,16 +14,16 @@
 #define BITS_PER_BYTE          8
 
 /*
- * Create a contiguous bitmask starting at bit position @l and ending at
- * position @h. For example
+ * Create a contiguous bitmask starting at bit position @low and ending at
+ * position @high. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
-#define GENMASK(h, l) \
-       (((~UL(0)) - (UL(1) << (l)) + 1) & \
-        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
+#define GENMASK(high, low) \
+       (((~UL(0)) - (UL(1) << (low)) + 1) & \
+        (~UL(0) >> (BITS_PER_LONG - 1 - (high))))
 
-#define GENMASK_ULL(h, l) \
-       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
-        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
+#define GENMASK_ULL(high, low) \
+       (((~ULL(0)) - (ULL(1) << (low)) + 1) & \
+        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (high))))
 
 #endif /* __LINUX_BITS_H */
--

2.22.0

Hello,

A new attempt to try to add build time validity checks of GENMASK (and GENMASK_ULL) inputs. There main differences from v2:

Remove a define of BUILD_BUG_ON in x86/boot to avoid a compiler warning about redefining BUILD_BUG_ON. Instead, use the common one from include/.

Drop patch 2 in v2 where GENMASK arguments where made more verbose.

Add a cast in the BUILD_BUG_ON_ZERO macro change the type to int to avoid the somewhat clumpsy casts of BUILD_BUG_ON_ZERO. The second patch in this series adds such a cast to BUILD_BUG_ON_ZERO, which makes it possible to avoid casts when using BUILD_BUG_ON_ZERO in patch 3.

I have checked all users of BUILD_BUG_ON_ZERO and I did not find a case where adding a cast to int would affect existing users but I'd feel much more comfortable if someone else double (or tripple) checked (there are ~80 instances plus ~10 copies in tools). Perhaps I should have CC:d maintainers of files using BUILD_BUG_ON_ZERO?

Finally, use __builtin_constant_p instead of __is_constexpr. This avoids pulling in kernel.h in bits.h.

Quoted 7 lines · Joe Perches

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/phy/amlogic/phy-meson-g12a-usb2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 6 lines

drivers/phy/amlogic/phy-meson-g12a-usb2.c +0 -0

index 9065ffc85eb4..cd7eccab2649 100644

drivers/phy/amlogic/phy-meson-g12a-usb2.c line 72

Quoted 8 lines

drivers/phy/amlogic/phy-meson-g12a-usb2.c +1 -1

@@ -66,7 +66,7 @@
 #define PHY_CTRL_R14						0x38
 	#define PHY_CTRL_R14_I_RDP_EN				BIT(0)
 	#define PHY_CTRL_R14_I_RPU_SW1_EN			BIT(1)
-	#define PHY_CTRL_R14_I_RPU_SW2_EN			GENMASK(2, 3)
+	#define PHY_CTRL_R14_I_RPU_SW2_EN			GENMASK(3, 2)
 	#define PHY_CTRL_R14_PG_RSTN				BIT(4)
 	#define PHY_CTRL_R14_I_C2L_DATA_16_8			BIT(5)
 	#define PHY_CTRL_R14_I_C2L_ASSERT_SINGLE_EN_ZERO	BIT(6)

Reviewed-by Neil Armstrong <[email protected]>

Quoted 8 lines · Neil Armstrong

Quoted 7 lines · Joe Perches

Arguments are supposed to be ordered high then low.

Signed-off-by Joe Perches <[email protected]>

---
 drivers/phy/amlogic/phy-meson-g12a-usb2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 16 lines

Quoted 6 lines

drivers/phy/amlogic/phy-meson-g12a-usb2.c +0 -0

index 9065ffc85eb4..cd7eccab2649 100644

drivers/phy/amlogic/phy-meson-g12a-usb2.c line 72

Quoted 8 lines

drivers/phy/amlogic/phy-meson-g12a-usb2.c +1 -1

@@ -66,7 +66,7 @@
 #define PHY_CTRL_R14						0x38
 	#define PHY_CTRL_R14_I_RDP_EN				BIT(0)
 	#define PHY_CTRL_R14_I_RPU_SW1_EN			BIT(1)
-	#define PHY_CTRL_R14_I_RPU_SW2_EN			GENMASK(2, 3)
+	#define PHY_CTRL_R14_I_RPU_SW2_EN			GENMASK(3, 2)
 	#define PHY_CTRL_R14_PG_RSTN				BIT(4)
 	#define PHY_CTRL_R14_I_C2L_DATA_16_8			BIT(5)
 	#define PHY_CTRL_R14_I_C2L_ASSERT_SINGLE_EN_ZERO	BIT(6)

Reviewed-by Neil Armstrong <[email protected]>

Shouldn't this go to stable trees as well?

-Kishon

Hello,

A new attempt to try to add build time validity checks of GENMASK (and GENMASK_ULL) inputs. There main differences from v2:

Remove a define of BUILD_BUG_ON in x86/boot to avoid a compiler warning about redefining BUILD_BUG_ON. Instead, use the common one from include/.

Drop patch 2 in v2 where GENMASK arguments where made more verbose.

Add a cast in the BUILD_BUG_ON_ZERO macro change the type to int to avoid the somewhat clumpsy casts of BUILD_BUG_ON_ZERO. The second patch in this series adds such a cast to BUILD_BUG_ON_ZERO, which makes it possible to avoid casts when using BUILD_BUG_ON_ZERO in patch 3.

I have checked all users of BUILD_BUG_ON_ZERO and I did not find a case where adding a cast to int would affect existing users but I'd feel much more comfortable if someone else double (or tripple) checked (there are ~80 instances plus ~10 copies in tools). Perhaps I should have CC:d maintainers of files using BUILD_BUG_ON_ZERO?

Finally, use __builtin_constant_p instead of __is_constexpr. This avoids pulling in kernel.h in bits.h.

Joe Perches sent a patch series to fix existing misuses, currently there are five such misuses (which patches pending) left in Linus tree and two (with patches pending) in linux-next. Those patches should fix all "simple" misuses of GENMASK (cases where the arguments are numerical constants). Pushing v2 to linux-next also revealed an arm-specific misuse where GENMASK was used in another macro (and also broke the arm-builds). There is a patch to fix that by Nathan Chancellor (not in linux-next yet). Those patches should be merged before the last patch of this series to avoid breaking builds.

Having BUILD_BUG_ON_ZERO produce a value of type size_t leads to awkward casts in cases where the result needs to be signed, or of smaller type than size_t. To avoid this, cast the value to int instead and rely on implicit type conversions when a larger or unsigned type is needed.

Suggested-by Masahiro Yamada <[email protected]>

Signed-off-by Rikard Falkeborn <[email protected]>

Changes in v3:
- This patch is new in v3

 include/linux/build_bug.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

include/linux/build_bug.h +2 -3

index 0fe5426f2bdc..e3a0be2c90ad 100644
@@ -9,11 +9,11 @@
 #else /* __CHECKER__ */
 /*
  * Force a compilation error if condition is true, but also produce a
- * result (of value 0 and type size_t), so the expression can be used
+ * result (of value 0 and type int), so the expression can be used
  * e.g. in a structure initializer (or where-ever else comma expressions
  * aren't permitted).
  */
-#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
+#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
 #endif /* __CHECKER__ */
 
 /* Force a compilation error if a constant expression is not a power of 2 */
--

2.22.0

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_ON_ZERO() must be used instead of BUILD_BUG_ON().

__builtin_constant_p does not evaluate is argument, it only checks if it is a constant or not at compile time, and __builtin_choose_expr does not evaluate the expression that is not chosen. Therefore, GENMASK(x++, 0) does only evaluate x++ once.

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since BUILD_BUG_OR_ZERO() is not asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

---
Changes in v3:
- Changed back to shorter macro argument names
- Remove casts and use 0 instead of UL(0) in GENMASK_INPUT_CHECK(), since all results in GENMASK_INPUT_CHECK() are now ints. Update commit message to reflect that.

Changes in v2:
- Add comment about why inputs are not checked when used in asm file
- Use UL(0) instead of 0
- Extract mask creation in a separate macro to improve readability
- Use high and low instead of h and l (part of this was extracted to a separate patch)
- Updated commit message

Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 5 of the patches are not in Linus tree, and 2 are not in linux-next. There is also a patch pending by Nathan Chancellor that also needs to be merged before this patch is merged to avoid build failures.

 include/linux/bits.h | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

include/linux/bits.h +19 -3

index 669d69441a62..4ba0fb609239 100644
@@ -18,12 +18,29 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
-#define GENMASK(h, l) \
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l) \
+       (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+               __builtin_constant_p((l) > (h)), (l) > (h), 0)))
+#else
+/*
+ * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
+ * disable the input check if that is the case.
+ */
+#define GENMASK_INPUT_CHECK(h, l) 0
+#endif
+
+#define __GENMASK(h, l) \
        (((~UL(0)) - (UL(1) << (l)) + 1) & \
         (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
+#define GENMASK(h, l) \
+       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
 
-#define GENMASK_ULL(h, l) \
+#define __GENMASK_ULL(h, l) \
        (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
         (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
+#define GENMASK_ULL(h, l) \
+       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
 
 #endif /* __LINUX_BITS_H */
--

2.22.0

GENMASK() and GENMASK_ULL() are supposed to be called with the high bit as the first argument and the low bit as the second argument. Mixing them will return a mask with zero bits set.

Recent commits show getting this wrong is not uncommon, see e.g. commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK macro").

To prevent such mistakes from appearing again, add compile time sanity checking to the arguments of GENMASK() and GENMASK_ULL(). If both arguments are known at compile time, and the low bit is higher than the high bit, break the build to detect the mistake immediately.

Since GENMASK() is used in declarations, BUILD_BUG_ON_ZERO() must be used instead of BUILD_BUG_ON().

__builtin_constant_p does not evaluate is argument, it only checks if it is a constant or not at compile time, and __builtin_choose_expr does not evaluate the expression that is not chosen. Therefore, GENMASK(x++, 0) does only evaluate x++ once.

Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends available in assembly") made the macros in linux/bits.h available in assembly. Since BUILD_BUG_OR_ZERO() is not asm compatible, disable the checks if the file is included in an asm file.

Signed-off-by Rikard Falkeborn <[email protected]>

---
Changes in v3:
- Changed back to shorter macro argument names
- Remove casts and use 0 instead of UL(0) in GENMASK_INPUT_CHECK(), since all results in GENMASK_INPUT_CHECK() are now ints. Update commit message to reflect that.

Changes in v2:
- Add comment about why inputs are not checked when used in asm file
- Use UL(0) instead of 0
- Extract mask creation in a separate macro to improve readability
- Use high and low instead of h and l (part of this was extracted to a separate patch)
- Updated commit message

Joe Perches sent a series to fix the existing misuses of GENMASK() that needs to be merged before this to avoid build failures. Currently, 5 of the patches are not in Linus tree, and 2 are not in linux-next. There is also a patch pending by Nathan Chancellor that also needs to be merged before this patch is merged to avoid build failures.

 include/linux/bits.h | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

include/linux/bits.h +15 -1

index 669d69441a62..4ba0fb609239 100644
@@ -18,12 +18,29 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
-#define GENMASK(h, l) \
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(h, l) \
+       (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+               __builtin_constant_p((l) > (h)), (l) > (h), 0)))
+#else
+/*
+ * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
+ * disable the input check if that is the case.
+ */
+#define GENMASK_INPUT_CHECK(h, l) 0
+#endif
+
+#define __GENMASK(h, l) \
        (((~UL(0)) - (UL(1) << (l)) + 1) & \
         (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
+#define GENMASK(h, l) \