apparent big-endian bugs in dwc-xlgmac

In xlgmac_dev_xmit():

                        /* Mark it as a CONTEXT descriptor */
                        dma_desc->desc3 = XLGMAC_SET_REG_BITS_LE(
                                                dma_desc->desc3,
                                                TX_CONTEXT_DESC3_CTXT_POS,
                                                TX_CONTEXT_DESC3_CTXT_LEN,
                                                1);

Looking at XLGMAC_SET_REG_BITS_LE() we see this:

#define XLGMAC_SET_REG_BITS_LE(var, pos, len, val) ({                   \
        typeof(var) _var = (var);                                       \
        typeof(pos) _pos = (pos);                                       \
        typeof(len) _len = (len);                                       \
        typeof(val) _val = (val);                                       \
        _val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos);         \
        _var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val;         \
        cpu_to_le32(_var);                                              \

})

That thing assumes var to be host-endian and has a little-endian result. Unfortunately, we feed it a little-endian and store the result back into the same place. That might work if the original values *was* host-endian and we wanted to end up with little-endian. However, that is immediately followed by

                        /* Indicate this descriptor contains the MSS */
                        dma_desc->desc3 = XLGMAC_SET_REG_BITS_LE(
                                                dma_desc->desc3,
                                                TX_CONTEXT_DESC3_TCMSSV_POS,
                                                TX_CONTEXT_DESC3_TCMSSV_LEN,
                                                1);

where we operate on the now definitely little-endian value. That really can't be right. I don't have the hardware in question, so I can't test that, but it smells like this needs something like diff below, making XLGMAC_SET_REG_BITS_LE take le32 and return le32. GET side of things is le32 -> u32; definition looks correct, but slightly misannotated.

Signed-off-by Al Viro <[email protected]>

---

drivers/net/ethernet/synopsys/dwc-xlgmac.h +1 -1

index cab3e40a86b9..e95c4c250e16 100644
@@ -106,7 +106,7 @@
 #define XLGMAC_GET_REG_BITS_LE(var, pos, len) ({			\
 	typeof(pos) _pos = (pos);					\
 	typeof(len) _len = (len);					\
-	typeof(var) _var = le32_to_cpu((var));				\
+	u32 _var = le32_to_cpu((var));				\
 	((_var) & GENMASK(_pos + _len - 1, _pos)) >> (_pos);		\
 })
 

Hi AI Viro,

drivers/net/ethernet/synopsys/dwc-xlgmac.h line 132

Quoted 11 lines

drivers/net/ethernet/synopsys/dwc-xlgmac.h +2 -2

@@ -125,8 +125,8 @@
 	typeof(len) _len = (len);					\
 	typeof(val) _val = (val);					\
 	_val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos);		\
-	_var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val;		\
-	cpu_to_le32(_var);						\
+	(_var & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | 	\
+		cpu_to_le32(_val);					\
 })
 
 struct xlgmac_pdata;

Make sense.  But I think what you want is fix as follows. Right ?

What for? Sure, this variant will work, but why bother with

	a = le32_to_cpu(b);
	(cpu_to_le32(a) & ....) | ....
and how is that better than
	(b & ...) | ...

IDGI... Mind you, I'm not sure if there is any point keeping _var in that thing, seeing that we use var only once - might be better off with

	((var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | 	\
		cpu_to_le32(_val);					\

Quoted 10 lines · Al Viro

What for? Sure, this variant will work, but why bother with

	a = le32_to_cpu(b);
	(cpu_to_le32(a) & ....) | ....
and how is that better than
	(b & ...) | ...

IDGI... Mind you, I'm not sure if there is any point keeping _var in that thing, seeing that we use var only once - might be better off with

	((var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | 	\
		cpu_to_le32(_val);					\

FWIW, seeing how many drivers end up open-coding that, I'm rather tempted to add to linux/bitops.h or linux/byteorder/generic.h the following:

static inline __le16 le16_replace_bits(__le16 old, u16 v, int bit, int size)

{
	__le16 mask = cpu_to_le16(GENMASK(bit + size - 1, bit));
	return (old & ~mask) | (cpu_to_le16(v << bit) & mask);
}

static inline __le32 le32_replace_bits(__le32 old, u32 v, int bit, int size)

{
	__le32 mask = cpu_to_le32(GENMASK(bit + size - 1, bit));
	return (old & ~mask) | (cpu_to_le32(v << bit) & mask);
}

static inline __le64 le64_replace_bits(__le64 old, u64 v, int bit, int size)

{
	__le64 mask = cpu_to_le64(GENMASK_ULL(bit + size - 1, bit));
	return (old & ~mask) | (cpu_to_le64(v << bit) & mask);
}

static inline __be16 be16_replace_bits(__be16 old, u16 v, int bit, int size)

{
	__be16 mask = cpu_to_be16(GENMASK(bit + size - 1, bit));
	return (old & ~mask) | (cpu_to_be16(v << bit) & mask);
}

static inline __be32 be32_replace_bits(__be32 old, u32 v, int bit, int size)

{
	__be32 mask = cpu_to_be32(GENMASK(bit + size - 1, bit));
	return (old & ~mask) | (cpu_to_be32(v << bit) & mask);
}

static inline __be64 be64_replace_bits(__be64 old, u64 v, int bit, int size)

{
	__be64 mask = cpu_to_be64(GENMASK_ULL(bit + size - 1, bit));
	return (old & ~mask) | (cpu_to_be64(v << bit) & mask);
}

static inline u16 le16_get_bits(__le16 v, int bit, int size)

{
	return (le16_to_cpu(v) >> bit) & (BIT(size) - 1);
}

static inline u32 le32_get_bits(__le32 v, int bit, int size)

{
	return (le32_to_cpu(v) >> bit) & (BIT(size) - 1);
}

static inline u64 le64_get_bits(__le64 v, int bit, int size)

{
	return (le64_to_cpu(v) >> bit) & (BIT_ULL(size) - 1);
}

static inline u16 be16_get_bits(__be16 v, int bit, int size)

{
	return (be16_to_cpu(v) >> bit) & (BIT(size) - 1);
}

static inline u32 be32_get_bits(__be32 v, int bit, int size)

{
	return (be32_to_cpu(v) >> bit) & (BIT(size) - 1);
}

static inline u64 be64_get_bits(__be64 v, int bit, int size)

{
	return (be64_to_cpu(v) >> bit) & (BIT_ULL(size) - 1);
}

and let drivers use those...

Essentially, it gives helpers for work with bitfields in fixed-endian. Suppose we have e.g. a little-endian 32bit value with fixed layout; expressing that as a bitfield would go like

	struct foo {
		unsigned foo:4;		/* bits 0..3 */
		unsigned :2;
		unsigned bar:12;	/* bits 6..17 */
		unsigned baz:14;	/* bits 18..31 */
	}

Even for host-endian it doesn't work all that well - you end up with ifdefs in structure definition and generated code stinks. For fixed-endian it gets really painful, and people tend to use explicit shift-and-mask kind of macros for accessing the fields (and often enough get the endianness conversions wrong, at that). With these primitives

struct foo v <=> __le32 v
v.foo = i ? 1 : 2 <=> v = le32_replace_bits(v, i ? 1 : 2, 0, 4)
f(4 + v.baz) <=> f(4 + le32_get_bits(v, 18, 14))

Quoted 15 lines · Al Viro

Essentially, it gives helpers for work with bitfields in fixed-endian. Suppose we have e.g. a little-endian 32bit value with fixed layout; expressing that as a bitfield would go like

	struct foo {
		unsigned foo:4;		/* bits 0..3 */
		unsigned :2;
		unsigned bar:12;	/* bits 6..17 */
		unsigned baz:14;	/* bits 18..31 */
	}

Even for host-endian it doesn't work all that well - you end up with ifdefs in structure definition and generated code stinks. For fixed-endian it gets really painful, and people tend to use explicit shift-and-mask kind of macros for accessing the fields (and often enough get the endianness conversions wrong, at that). With these primitives

Quoted 3 lines

struct foo v <=> __le32 v
v.foo = i ? 1 : 2 <=> v = le32_replace_bits(v, i ? 1 : 2, 0, 4)
f(4 + v.baz) <=> f(4 + le32_get_bits(v, 18, 14))

Looks very useful. The [start bit, size] pair may not land itself too nicely to creating defines, though. Which is why in include/linux/bitfield.h we tried to use a shifted mask and work backwards from that single value what the start and size are. commit 3e9b3112ec74 ("add basic register-field manipulation macros") has the description. Could a similar trick perhaps be applicable here?

Umm... What's wrong with

#define FIELD_FOO 0,4
#define FIELD_BAR 6,12
#define FIELD_BAZ 18,14

A macro can bloody well expand to any sequence of tokens - le32_get_bits(v, FIELD_BAZ) will become le32_get_bits(v, 18, 14) just fine. What's the problem with that?

Quoted 8 lines · Al Viro

Umm... What's wrong with

#define FIELD_FOO 0,4
#define FIELD_BAR 6,12
#define FIELD_BAZ 18,14

A macro can bloody well expand to any sequence of tokens - le32_get_bits(v, FIELD_BAZ) will become le32_get_bits(v, 18, 14) just fine. What's the problem with that?

FWIW, if you want to use the mask, __builtin_ffsll() is not the only way to do it - you don't need the shift. Multiplier would do just as well, and that can be had easier. If mask = (2*a + 1)<<n = ((2*a)<<n) ^ (1<<n), then

	mask - 1 = ((2*a) << n) + ((1<<n) - 1) = ((2*n) << n) ^ ((1<<n) - 1)
	mask ^ (mask - 1) = (1<<n) + ((1<<n) - 1)
and
	mask & (mask ^ (mask - 1)) = 1<<n.

IOW, with

static __always_inline u64 mask_to_multiplier(u64 mask)

{
	return mask & (mask ^ (mask - 1));
}

we could do

static __always_inline __le64 le64_replace_bits(__le64 old, u64 v, u64 mask)

{
	__le64 m = cpu_to_le64(mask);
	return (old & ~m) | (cpu_to_le64(v * mask_to_multiplier(mask)) & m);
}

static __always_inline u64 le64_get_bits(__le64 v, u64 mask)

{
	return (le64_to_cpu(v) & mask) / mask_to_multiplier(mask);
}

etc. Compiler will turn those into shifts... I can live with either calling conventions.

Comments?

Quoted 4 lines

static __always_inline u64 mask_to_multiplier(u64 mask)

{
	return mask & (mask ^ (mask - 1));
}

Very nice! The compilation-time check if the value can fit in a field covered by the mask (if they're both known) did help me catch bugs early a few times over the years, so if it could be preserved we can maybe even drop the FIELD_* macros and just use this approach?

Quoted 4 lines · Jakub Kicinski

Quoted 4 lines

static __always_inline u64 mask_to_multiplier(u64 mask)

{
	return mask & (mask ^ (mask - 1));
}  

D'oh. Even simpler than that, of course -

static __always_inline u64 mask_to_multiplier(u64 mask) {
return mask & -mask;
}

Quoted 4 lines

Very nice! The compilation-time check if the value can fit in a field covered by the mask (if they're both known) did help me catch bugs early a few times over the years, so if it could be preserved we can maybe even drop the FIELD_* macros and just use this approach?

Umm... Something like this, perhaps? Same bunch, plus u{16,32,64}_... variants for host-endian. Adding sanity check on mask is also not hard, but I don't know how useful it actually is...

include/linux/byteorder/generic.h +3 -0

index 451aaa0786ae..a032de9aa03d 100644
@@ -187,4 +187,36 @@ static inline void be32_to_cpu_array(u32 *dst, const __be32 *src, size_t len)
 		dst[i] = be32_to_cpu(src[i]);
 }
 
+extern void __compiletime_error("value doesn't fit into mask")
+__field_overflow(void);
+static __always_inline u64 mask_to_multiplier(u64 mask)

include/linux/byteorder/generic.h +19 -0

+		    (val & ~(mask/mask_to_multiplier(mask))))		\
+				    __field_overflow();			\
+	return (old & ~m) |						\
+		(to(val * mask_to_multiplier(mask)) & m);		\
+}									\
+static __always_inline base type##_get_bits(__##type v, base mask)	\
+{									\
+	return (from(v) & mask)/mask_to_multiplier(mask);		\
+}
+#define __MAKE_OP(size)							\
+	____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu)	\
+	____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu)	\
+	____MAKE_OP(u##size,u##size,,)
+__MAKE_OP(16)
+__MAKE_OP(32)
+__MAKE_OP(64)
+#undef __MAKE_OP
+#undef ____MAKE_OP
+
 #endif /* _LINUX_BYTEORDER_GENERIC_H */

include/linux/byteorder/generic.h static inline void be32_to_cpu_array(u32 *dst, const __be32 *src, size_t len) · line 202

Quoted 5 lines

include/linux/byteorder/generic.h +5 -0

+static __always_inline __##type type##_replace_bits(__##type old,	\
+					base val, base mask)		\
+{									\
+	__##type m = to(mask);						\
+        if (__builtin_constant_p(val) &&				\

Is the lack of a __builtin_constant_p(mask) test intentional? Sometimes the bitfield is a packed array and people may have a helper to which only the mask is passed as non-constant and the value is implied by the helper, thus constant.

Quoted 10 lines · Jakub Kicinski

include/linux/byteorder/generic.h static inline void be32_to_cpu_array(u32 *dst, const __be32 *src, size_t len) · line 202

Quoted 5 lines

include/linux/byteorder/generic.h +5 -0

+static __always_inline __##type type##_replace_bits(__##type old,	\
+					base val, base mask)		\
+{									\
+	__##type m = to(mask);						\
+        if (__builtin_constant_p(val) &&				\  

Is the lack of a __builtin_constant_p(mask) test intentional? Sometimes the bitfield is a packed array and people may have a helper to which only the mask is passed as non-constant and the value is implied by the helper, thus constant.

If the mask in non-constant, we probably shouldn't be using that at all; could you show a real-world example where that would be the case?

Quoted 12 lines · Al Viro

Quoted 10 lines · Jakub Kicinski

include/linux/byteorder/generic.h static inline void be32_to_cpu_array(u32 *dst, const __be32 *src, size_t len) · line 202

Quoted 5 lines

include/linux/byteorder/generic.h +5 -0

+static __always_inline __##type type##_replace_bits(__##type old,	\
+					base val, base mask)		\
+{									\
+	__##type m = to(mask);						\
+        if (__builtin_constant_p(val) &&				\  

Is the lack of a __builtin_constant_p(mask) test intentional? Sometimes the bitfield is a packed array and people may have a helper to which only the mask is passed as non-constant and the value is implied by the helper, thus constant.

Quoted 2 lines

If the mask in non-constant, we probably shouldn't be using that at all; could you show a real-world example where that would be the case?

FIELD_* macros explicitly forbid this, since the code would be... suboptimal with the runtime ffsl. Real life examples are the hackish macro NFP_ETH_SET_BIT_CONFIG() in

drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c

I remember there was also some Renesas code.. maybe this:

https://patchwork.kernel.org/patch/9881279/

it looks like cpg_z_clk_recalc_rate() and cpg_z2_clk_recalc_rate() only differ in mask.

Quoted 20 lines · Jakub Kicinski

Quoted 12 lines · Al Viro

Quoted 10 lines · Jakub Kicinski

include/linux/byteorder/generic.h static inline void be32_to_cpu_array(u32 *dst, const __be32 *src, size_t len) · line 202

Quoted 5 lines

include/linux/byteorder/generic.h +5 -0

+static __always_inline __##type type##_replace_bits(__##type old,	\
+					base val, base mask)		\
+{									\
+	__##type m = to(mask);						\
+        if (__builtin_constant_p(val) &&				\    

Is the lack of a __builtin_constant_p(mask) test intentional? Sometimes the bitfield is a packed array and people may have a helper to which only the mask is passed as non-constant and the value is implied by the helper, thus constant.

Quoted 2 lines

If the mask in non-constant, we probably shouldn't be using that at all; could you show a real-world example where that would be the case?

FIELD_* macros explicitly forbid this, since the code would be... suboptimal with the runtime ffsl. Real life examples are the hackish macro NFP_ETH_SET_BIT_CONFIG() in

Quoted 1 line

drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c

Why not simply make nfp_eth_set_bit_config() static inline and be done with that? It's not that large and there are only few callers, so...

Quoted 6 lines

I remember there was also some Renesas code.. maybe this:

https://patchwork.kernel.org/patch/9881279/

it looks like cpg_z_clk_recalc_rate() and cpg_z2_clk_recalc_rate() only differ in mask.

*shrug*

That thing would expand into "reg &= 15" in one case and "reg >>= 8; reg &= 15" in another. Either of which is cheaper than a function call, and definitely cheaper than any kind of dynamic calculation of shift, no matter how implemented.

It used to be __always_inline, but apparently LLVM/clang doesn't propagate constants :(

4e59532541c8 ("nfp: don't depend on compiler constant propagation")

Quoted 4 lines · Jakub Kicinski

It used to be __always_inline, but apparently LLVM/clang doesn't propagate constants :(

4e59532541c8 ("nfp: don't depend on compiler constant propagation")

Doesn't propagate constants or doesn't have exact same set of rules for __builtin_constant_p()? IOW, if you dropped that BUILD_BUG_ON(), what would be left after optimizations?

Quoted 5 lines · Al Viro

Quoted 3 lines · Jakub Kicinski

It used to be __always_inline, but apparently LLVM/clang doesn't propagate constants :(

Quoted 5 lines

Quoted 1 line

4e59532541c8 ("nfp: don't depend on compiler constant propagation")

Doesn't propagate constants or doesn't have exact same set of rules for __builtin_constant_p()? IOW, if you dropped that BUILD_BUG_ON(), what would be left after optimizations?

Hm. You're right. It just doesn't recognize the parameter as constant in __builtin_constant_p().

Next question: where do we put that bunch? I've put it into linux/byteorder/generic.h, so that anything picking fixed-endian primitives would pick those as well; I hadn't thought of linux/bitfield.h at the time. We certainly could put it there instead - it's never pulled by other headers, so adding #include <asm/byteorder.h> into linux/bitfield.h is not going to cause header order problems. Not sure...

Linus, do you have any preferences in that area?

The following primitives are defined in linux/bitfield.h:

* u32 le32_get_bits(__le32 val, u32 field) extracts the contents of the bitfield specified by @field in little-endian 32bit value @val and converts it to host-endian.

* void le32p_replace_bits(__le32 *p, u32 v, u32 field) replaces the contents of the bitfield specified by @field in little-endian 32bit object pointet to by *p with the value of @v. New value is given in host-endian and stored as little-endian.

* __le32 le32_replace_bits(__le32 old, u32 v, u32 field) is equivalent to ({__le32 tmp = old; le32p_replace_bits(&old, v, field); tmp;}) In other words, instead of modifying an object in memory, it takes the initial value and returns the modified one.

Looks great to me!

Quoted 11 lines · Al Viro

The following primitives are defined in linux/bitfield.h:

* u32 le32_get_bits(__le32 val, u32 field) extracts the contents of the bitfield specified by @field in little-endian 32bit value @val and converts it to host-endian.

* void le32p_replace_bits(__le32 *p, u32 v, u32 field) replaces the contents of the bitfield specified by @field in little-endian 32bit object pointet to by *p with the value of @v. New value is given in host-endian and stored as little-endian.

Quoted 4 lines

* __le32 le32_replace_bits(__le32 old, u32 v, u32 field) is equivalent to ({__le32 tmp = old; le32p_replace_bits(&old, v, field); tmp;}) In other words, instead of modifying an object in memory, it takes the initial value and returns the modified one.

the current macros take filed/mask as first param, not sure if it's worth maintaining the order

IMO it's not worth the trouble; let's go with the check inside of static inline and accept that on clang builds it'll do nothing.

Quoted 15 lines · Jie Deng

Hi AI Viro,

drivers/net/ethernet/synopsys/dwc-xlgmac.h line 132

Quoted 11 lines

drivers/net/ethernet/synopsys/dwc-xlgmac.h +2 -2

@@ -125,8 +125,8 @@
 	typeof(len) _len = (len);					\
 	typeof(val) _val = (val);					\
 	_val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos);		\
-	_var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val;		\
-	cpu_to_le32(_var);						\
+	(_var & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | 	\
+		cpu_to_le32(_val);					\
 })
 
 struct xlgmac_pdata;

Make sense.  But I think what you want is fix as follows. Right ?

drivers/net/ethernet/synopsys/dwc-xlgmac.h line 130

Quoted 9 lines

drivers/net/ethernet/synopsys/dwc-xlgmac.h +3 -3

@@ -125,8 +125,8 @@
 	typeof(len) _len = (len);						\
- 	typeof(val) _val = (val);						\
+       u32 _var = le32_to_cpu((var));                                          \ 	
        _val = (_val << _pos) & GENMASK(_pos + _len - 1, _pos);			\
-	_var = (_var & ~GENMASK(_pos + _len - 1, _pos)) | _val;			\
-	cpu_to_le32(_var);							\
+	(cpu_to_le32(_var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | 	\
+		cpu_to_le32(_val);						\
 })

What for? Sure, this variant will work, but why bother with

	a = le32_to_cpu(b);
	(cpu_to_le32(a) & ....) | ....
and how is that better than
	(b & ...) | ...

IDGI... Mind you, I'm not sure if there is any point keeping _var in that thing, seeing that we use var only once - might be better off with

	((var) & ~cpu_to_le32(GENMASK(_pos + _len - 1, _pos))) | 	\
		cpu_to_le32(_val);					\