[PATCH net-next v3 0/6] net: qualcomm: rmnet: stop using C bit-fields

[PATCH net-next v3 0/6] net: qualcomm: rmnet: stop using C bit-fields

[Alex Elder]

Version 3 of this series uses BIT() rather than GENMASK() to define
single-bit masks.  It then uses a simple AND (&) operation rather
than (e.g.) u8_get_bits() to access such flags.  This was suggested
by David Laight and really prefer the result.  With Bjorn's
permission I have preserved his Reviewed-by tags on the first five
patches.

Version 2 fixed bugs in the way the value written into the header
was computed.

The series was first posted here:
  https://lore.kernel.org/netdev/20210304223431.15045-1-elder@linaro.org/
Below is a summary of the original description.

This series converts data structures defined in <linux/if_rmnet.h>
so they use integral field values with bitfield masks rather than
relying on C bit-fields.
  - The first three patches lay the ground work for the others.
      - The first adds endianness notation to a structure.
      - The second simplifies a bit of complicated code.
      - The third open-codes some macros that needlessly
        obscured some simple code.
  - Each of the last three patches converts one of the structures
    defined in <linux/if_rmnet.h> so it no longer uses C bit-fields.

    					-Alex

Alex Elder (6):
  net: qualcomm: rmnet: mark trailer field endianness
  net: qualcomm: rmnet: simplify some byte order logic
  net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros
  net: qualcomm: rmnet: use field masks instead of C bit-fields
  net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer
  net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header

 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  | 11 ++--
 .../net/ethernet/qualcomm/rmnet/rmnet_map.h   | 12 ----
 .../qualcomm/rmnet/rmnet_map_command.c        | 11 +++-
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 60 ++++++++---------
 include/linux/if_rmnet.h                      | 65 +++++++++----------
 5 files changed, 70 insertions(+), 89 deletions(-)

-- 
2.27.0

[PATCH net-next v3 1/6] net: qualcomm: rmnet: mark trailer field endianness

[Alex Elder]

The fields in the checksum trailer structure used for QMAP protocol
RX packets are all big-endian format, so define them that way.

It turns out these fields are never actually used by the RMNet code.
The start offset is always assumed to be zero, and the length is
taken from the other packet headers.  So making these fields
explicitly big endian has no effect on the behavior of the code.

Signed-off-by: Alex Elder <elder@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 include/linux/if_rmnet.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
index 9661416a9bb47..8c7845baf3837 100644
--- a/include/linux/if_rmnet.h
+++ b/include/linux/if_rmnet.h
@@ -32,8 +32,8 @@ struct rmnet_map_dl_csum_trailer {
 #else
 #error	"Please fix <asm/byteorder.h>"
 #endif
-	u16 csum_start_offset;
-	u16 csum_length;
+	__be16 csum_start_offset;
+	__be16 csum_length;
 	__be16 csum_value;
 } __aligned(1);
 
-- 
2.27.0

[PATCH net-next v3 2/6] net: qualcomm: rmnet: simplify some byte order logic

[Alex Elder]

In rmnet_map_ipv4_ul_csum_header() and rmnet_map_ipv6_ul_csum_header()
the offset within a packet at which checksumming should commence is
calculated.  This calculation involves byte swapping and a forced type
conversion that makes it hard to understand.

Simplify this by computing the offset in host byte order, then
converting the result when assigning it into the header field.

Signed-off-by: Alex Elder <elder@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 22 ++++++++++---------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 21d38167f9618..bd1aa11c9ce59 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -197,12 +197,13 @@ rmnet_map_ipv4_ul_csum_header(void *iphdr,
 			      struct rmnet_map_ul_csum_header *ul_header,
 			      struct sk_buff *skb)
 {
-	struct iphdr *ip4h = (struct iphdr *)iphdr;
-	__be16 *hdr = (__be16 *)ul_header, offset;
+	__be16 *hdr = (__be16 *)ul_header;
+	struct iphdr *ip4h = iphdr;
+	u16 offset;
+
+	offset = skb_transport_header(skb) - (unsigned char *)iphdr;
+	ul_header->csum_start_offset = htons(offset);
 
-	offset = htons((__force u16)(skb_transport_header(skb) -
-				     (unsigned char *)iphdr));
-	ul_header->csum_start_offset = offset;
 	ul_header->csum_insert_offset = skb->csum_offset;
 	ul_header->csum_enabled = 1;
 	if (ip4h->protocol == IPPROTO_UDP)
@@ -239,12 +240,13 @@ rmnet_map_ipv6_ul_csum_header(void *ip6hdr,
 			      struct rmnet_map_ul_csum_header *ul_header,
 			      struct sk_buff *skb)
 {
-	struct ipv6hdr *ip6h = (struct ipv6hdr *)ip6hdr;
-	__be16 *hdr = (__be16 *)ul_header, offset;
+	__be16 *hdr = (__be16 *)ul_header;
+	struct ipv6hdr *ip6h = ip6hdr;
+	u16 offset;
+
+	offset = skb_transport_header(skb) - (unsigned char *)ip6hdr;
+	ul_header->csum_start_offset = htons(offset);
 
-	offset = htons((__force u16)(skb_transport_header(skb) -
-				     (unsigned char *)ip6hdr));
-	ul_header->csum_start_offset = offset;
 	ul_header->csum_insert_offset = skb->csum_offset;
 	ul_header->csum_enabled = 1;
 
-- 
2.27.0

[PATCH net-next v3 3/6] net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros

[Alex Elder]

The following macros, defined in "rmnet_map.h", assume a socket
buffer is provided as an argument without any real indication this
is the case.
    RMNET_MAP_GET_MUX_ID()
    RMNET_MAP_GET_CD_BIT()
    RMNET_MAP_GET_PAD()
    RMNET_MAP_GET_CMD_START()
    RMNET_MAP_GET_LENGTH()
What they hide is pretty trivial accessing of fields in a structure,
and it's much clearer to see this if we do these accesses directly.

So rather than using these accessor macros, assign a local
variable of the map header pointer type to the socket buffer data
pointer, and derereference that pointer variable.

In "rmnet_map_data.c", use sizeof(object) rather than sizeof(type)
in one spot.  Also, there's no need to byte swap 0; it's all zeros
irrespective of endianness.

Signed-off-by: Alex Elder <elder@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 10 ++++++----
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h      | 12 ------------
 .../net/ethernet/qualcomm/rmnet/rmnet_map_command.c  | 11 ++++++++---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c |  4 ++--
 4 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 3d00b32323084..2a6b2a609884c 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -56,20 +56,22 @@ static void
 __rmnet_map_ingress_handler(struct sk_buff *skb,
 			    struct rmnet_port *port)
 {
+	struct rmnet_map_header *map_header = (void *)skb->data;
 	struct rmnet_endpoint *ep;
 	u16 len, pad;
 	u8 mux_id;
 
-	if (RMNET_MAP_GET_CD_BIT(skb)) {
+	if (map_header->cd_bit) {
+		/* Packet contains a MAP command (not data) */
 		if (port->data_format & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
 			return rmnet_map_command(skb, port);
 
 		goto free_skb;
 	}
 
-	mux_id = RMNET_MAP_GET_MUX_ID(skb);
-	pad = RMNET_MAP_GET_PAD(skb);
-	len = RMNET_MAP_GET_LENGTH(skb) - pad;
+	mux_id = map_header->mux_id;
+	pad = map_header->pad_len;
+	len = ntohs(map_header->pkt_len) - pad;
 
 	if (mux_id >= RMNET_MAX_LOGICAL_EP)
 		goto free_skb;
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index 576501db2a0bc..2aea153f42473 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -32,18 +32,6 @@ enum rmnet_map_commands {
 	RMNET_MAP_COMMAND_ENUM_LENGTH
 };
 
-#define RMNET_MAP_GET_MUX_ID(Y) (((struct rmnet_map_header *) \
-				 (Y)->data)->mux_id)
-#define RMNET_MAP_GET_CD_BIT(Y) (((struct rmnet_map_header *) \
-				(Y)->data)->cd_bit)
-#define RMNET_MAP_GET_PAD(Y) (((struct rmnet_map_header *) \
-				(Y)->data)->pad_len)
-#define RMNET_MAP_GET_CMD_START(Y) ((struct rmnet_map_control_command *) \
-				    ((Y)->data + \
-				      sizeof(struct rmnet_map_header)))
-#define RMNET_MAP_GET_LENGTH(Y) (ntohs(((struct rmnet_map_header *) \
-					(Y)->data)->pkt_len))
-
 #define RMNET_MAP_COMMAND_REQUEST     0
 #define RMNET_MAP_COMMAND_ACK         1
 #define RMNET_MAP_COMMAND_UNSUPPORTED 2
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
index beaee49621287..add0f5ade2e61 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
@@ -12,12 +12,13 @@ static u8 rmnet_map_do_flow_control(struct sk_buff *skb,
 				    struct rmnet_port *port,
 				    int enable)
 {
+	struct rmnet_map_header *map_header = (void *)skb->data;
 	struct rmnet_endpoint *ep;
 	struct net_device *vnd;
 	u8 mux_id;
 	int r;
 
-	mux_id = RMNET_MAP_GET_MUX_ID(skb);
+	mux_id = map_header->mux_id;
 
 	if (mux_id >= RMNET_MAX_LOGICAL_EP) {
 		kfree_skb(skb);
@@ -49,6 +50,7 @@ static void rmnet_map_send_ack(struct sk_buff *skb,
 			       unsigned char type,
 			       struct rmnet_port *port)
 {
+	struct rmnet_map_header *map_header = (void *)skb->data;
 	struct rmnet_map_control_command *cmd;
 	struct net_device *dev = skb->dev;
 
@@ -58,7 +60,8 @@ static void rmnet_map_send_ack(struct sk_buff *skb,
 
 	skb->protocol = htons(ETH_P_MAP);
 
-	cmd = RMNET_MAP_GET_CMD_START(skb);
+	/* Command data immediately follows the MAP header */
+	cmd = (struct rmnet_map_control_command *)(map_header + 1);
 	cmd->cmd_type = type & 0x03;
 
 	netif_tx_lock(dev);
@@ -71,11 +74,13 @@ static void rmnet_map_send_ack(struct sk_buff *skb,
  */
 void rmnet_map_command(struct sk_buff *skb, struct rmnet_port *port)
 {
+	struct rmnet_map_header *map_header = (void *)skb->data;
 	struct rmnet_map_control_command *cmd;
 	unsigned char command_name;
 	unsigned char rc = 0;
 
-	cmd = RMNET_MAP_GET_CMD_START(skb);
+	/* Command data immediately follows the MAP header */
+	cmd = (struct rmnet_map_control_command *)(map_header + 1);
 	command_name = cmd->command_name;
 
 	switch (command_name) {
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index bd1aa11c9ce59..fd55269c2ce3c 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -321,7 +321,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
 		return NULL;
 
 	maph = (struct rmnet_map_header *)skb->data;
-	packet_len = ntohs(maph->pkt_len) + sizeof(struct rmnet_map_header);
+	packet_len = ntohs(maph->pkt_len) + sizeof(*maph);
 
 	if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
 		packet_len += sizeof(struct rmnet_map_dl_csum_trailer);
@@ -330,7 +330,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
 		return NULL;
 
 	/* Some hardware can send us empty frames. Catch them */
-	if (ntohs(maph->pkt_len) == 0)
+	if (!maph->pkt_len)
 		return NULL;
 
 	skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
-- 
2.27.0

[PATCH net-next v3 4/6] net: qualcomm: rmnet: use field masks instead of C bit-fields

[Alex Elder]

The actual layout of bits defined in C bit-fields (e.g. int foo : 3)
is implementation-defined.  Structures defined in <linux/if_rmnet.h>
address this by specifying all bit-fields twice, to cover two
possible layouts.

I think this pattern is repetitive and noisy, and I find the whole
notion of compiler "bitfield endianness" to be non-intuitive.

Stop using C bit-fields for the command/data flag and the pad length
fields in the rmnet_map structure, and define a single-byte flags
field instead.  Define a mask for the single bit "command" flag
encoded in the flags field.  Define a field mask for the encoded pad
length, and access it using functions defined in <linux/bitfield.h>.

Signed-off-by: Alex Elder <elder@linaro.org>
---
v3: Use BIT(x) and don't use u8_get_bits() for the command flag

 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  |  5 ++--
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  |  4 +++-
 include/linux/if_rmnet.h                      | 23 ++++++++-----------
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 2a6b2a609884c..28d355b094683 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -4,6 +4,7 @@
  * RMNET Data ingress/egress handler
  */
 
+#include <linux/bitfield.h>
 #include <linux/netdevice.h>
 #include <linux/netdev_features.h>
 #include <linux/if_arp.h>
@@ -61,7 +62,7 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 	u16 len, pad;
 	u8 mux_id;
 
-	if (map_header->cd_bit) {
+	if (map_header->flags & MAP_CMD_FLAG) {
 		/* Packet contains a MAP command (not data) */
 		if (port->data_format & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
 			return rmnet_map_command(skb, port);
@@ -70,7 +71,7 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 	}
 
 	mux_id = map_header->mux_id;
-	pad = map_header->pad_len;
+	pad = u8_get_bits(map_header->flags, MAP_PAD_LEN_FMASK);
 	len = ntohs(map_header->pkt_len) - pad;
 
 	if (mux_id >= RMNET_MAX_LOGICAL_EP)
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index fd55269c2ce3c..3291f252d81b0 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -4,6 +4,7 @@
  * RMNET Data MAP protocol
  */
 
+#include <linux/bitfield.h>
 #include <linux/netdevice.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
@@ -299,7 +300,8 @@ struct rmnet_map_header *rmnet_map_add_map_header(struct sk_buff *skb,
 
 done:
 	map_header->pkt_len = htons(map_datalen + padding);
-	map_header->pad_len = padding & 0x3F;
+	/* This is a data packet, so the CMD bit is 0 */
+	map_header->flags = u8_encode_bits(padding, MAP_PAD_LEN_FMASK);
 
 	return map_header;
 }
diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
index 8c7845baf3837..22ccc89bb5d8e 100644
--- a/include/linux/if_rmnet.h
+++ b/include/linux/if_rmnet.h
@@ -6,21 +6,18 @@
 #define _LINUX_IF_RMNET_H_
 
 struct rmnet_map_header {
-#if defined(__LITTLE_ENDIAN_BITFIELD)
-	u8  pad_len:6;
-	u8  reserved_bit:1;
-	u8  cd_bit:1;
-#elif defined (__BIG_ENDIAN_BITFIELD)
-	u8  cd_bit:1;
-	u8  reserved_bit:1;
-	u8  pad_len:6;
-#else
-#error	"Please fix <asm/byteorder.h>"
-#endif
-	u8  mux_id;
-	__be16 pkt_len;
+	u8 flags;			/* MAP_*_FMASK */
+	u8 mux_id;
+	__be16 pkt_len;			/* Length of packet, including pad */
 }  __aligned(1);
 
+/* rmnet_map_header flags field:
+ *  CMD:	1 = packet contains a MAP command; 0 = packet contains data
+ *  PAD_LEN:	number of pad bytes following packet data
+ */
+#define MAP_CMD_FLAG			BIT(7)
+#define MAP_PAD_LEN_FMASK		GENMASK(5, 0)
+
 struct rmnet_map_dl_csum_trailer {
 	u8  reserved1;
 #if defined(__LITTLE_ENDIAN_BITFIELD)
-- 
2.27.0

[PATCH net-next v3 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer

[Alex Elder]

Replace the use of C bit-fields in the rmnet_map_dl_csum_trailer
structure with a single one-byte field, using constant field masks
to encode or get at embedded values.

Signed-off-by: Alex Elder <elder@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
v3: Use BIT(x) and don't use u8_get_bits() for the checksum valid flag

 .../ethernet/qualcomm/rmnet/rmnet_map_data.c    |  2 +-
 include/linux/if_rmnet.h                        | 17 +++++++----------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 3291f252d81b0..72dbbe2c27bd7 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -365,7 +365,7 @@ int rmnet_map_checksum_downlink_packet(struct sk_buff *skb, u16 len)
 
 	csum_trailer = (struct rmnet_map_dl_csum_trailer *)(skb->data + len);
 
-	if (!csum_trailer->valid) {
+	if (csum_trailer->flags & MAP_CSUM_DL_VALID_FLAG) {
 		priv->stats.csum_valid_unset++;
 		return -EINVAL;
 	}
diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
index 22ccc89bb5d8e..a848bb2e0dad0 100644
--- a/include/linux/if_rmnet.h
+++ b/include/linux/if_rmnet.h
@@ -19,21 +19,18 @@ struct rmnet_map_header {
 #define MAP_PAD_LEN_FMASK		GENMASK(5, 0)
 
 struct rmnet_map_dl_csum_trailer {
-	u8  reserved1;
-#if defined(__LITTLE_ENDIAN_BITFIELD)
-	u8  valid:1;
-	u8  reserved2:7;
-#elif defined (__BIG_ENDIAN_BITFIELD)
-	u8  reserved2:7;
-	u8  valid:1;
-#else
-#error	"Please fix <asm/byteorder.h>"
-#endif
+	u8 reserved1;
+	u8 flags;			/* MAP_CSUM_DL_*_FMASK */
 	__be16 csum_start_offset;
 	__be16 csum_length;
 	__be16 csum_value;
 } __aligned(1);
 
+/* rmnet_map_dl_csum_trailer flags field:
+ *  VALID:	1 = checksum and length valid; 0 = ignore them
+ */
+#define MAP_CSUM_DL_VALID_FLAG		BIT(0)
+
 struct rmnet_map_ul_csum_header {
 	__be16 csum_start_offset;
 #if defined(__LITTLE_ENDIAN_BITFIELD)
-- 
2.27.0

Re: [PATCH net-next v3 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer

[Alex Elder]

On 3/9/21 6:48 AM, Alex Elder wrote:
> Replace the use of C bit-fields in the rmnet_map_dl_csum_trailer
> structure with a single one-byte field, using constant field masks
> to encode or get at embedded values.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> v3: Use BIT(x) and don't use u8_get_bits() for the checksum valid flag
> 
>   .../ethernet/qualcomm/rmnet/rmnet_map_data.c    |  2 +-
>   include/linux/if_rmnet.h                        | 17 +++++++----------
>   2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 3291f252d81b0..72dbbe2c27bd7 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -365,7 +365,7 @@ int rmnet_map_checksum_downlink_packet(struct sk_buff *skb, u16 len)
>   
>   	csum_trailer = (struct rmnet_map_dl_csum_trailer *)(skb->data + len);
>   
> -	if (!csum_trailer->valid) {
> +	if (csum_trailer->flags & MAP_CSUM_DL_VALID_FLAG) {

This was the actual problem.  The result of the new
test is the reverse of what it should be.

In any case, I'm taking a break from this for a while.

					-Alex

>   		priv->stats.csum_valid_unset++;
>   		return -EINVAL;
>   	}
> diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
> index 22ccc89bb5d8e..a848bb2e0dad0 100644
> --- a/include/linux/if_rmnet.h
> +++ b/include/linux/if_rmnet.h
> @@ -19,21 +19,18 @@ struct rmnet_map_header {
>   #define MAP_PAD_LEN_FMASK		GENMASK(5, 0)
>   
>   struct rmnet_map_dl_csum_trailer {
> -	u8  reserved1;
> -#if defined(__LITTLE_ENDIAN_BITFIELD)
> -	u8  valid:1;
> -	u8  reserved2:7;
> -#elif defined (__BIG_ENDIAN_BITFIELD)
> -	u8  reserved2:7;
> -	u8  valid:1;
> -#else
> -#error	"Please fix <asm/byteorder.h>"
> -#endif
> +	u8 reserved1;
> +	u8 flags;			/* MAP_CSUM_DL_*_FMASK */
>   	__be16 csum_start_offset;
>   	__be16 csum_length;
>   	__be16 csum_value;
>   } __aligned(1);
>   
> +/* rmnet_map_dl_csum_trailer flags field:
> + *  VALID:	1 = checksum and length valid; 0 = ignore them
> + */
> +#define MAP_CSUM_DL_VALID_FLAG		BIT(0)
> +
>   struct rmnet_map_ul_csum_header {
>   	__be16 csum_start_offset;
>   #if defined(__LITTLE_ENDIAN_BITFIELD)
> 

[PATCH net-next v3 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header

[Alex Elder]

Replace the use of C bit-fields in the rmnet_map_ul_csum_header
structure with a single two-byte (big endian) structure member,
and use bit or field masks to encode or get values within it.

Previously rmnet_map_ipv4_ul_csum_header() would update C bit-field
values in host byte order, then forcibly fix their byte order using
a combination of byte swap operations and types.

Instead, just compute the value that needs to go into the new
structure member and save it with a simple byte-order conversion.

Make similar simplifications in rmnet_map_ipv6_ul_csum_header().

Finally, in rmnet_map_checksum_uplink_packet() a set of assignments
zeroes every field in the upload checksum header.  Replace that with
a single memset() operation.

Signed-off-by: Alex Elder <elder@linaro.org>
Reported-by: kernel test robot <lkp@intel.com>
---
v3: Use BIT(x) and don't use u16_get_bits() for single-bit flags
v2: Fixed to use u16_encode_bits() instead of be16_encode_bits().

 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 34 ++++++-------------
 include/linux/if_rmnet.h                      | 21 ++++++------
 2 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 72dbbe2c27bd7..1ddc3440c8b48 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -198,23 +198,19 @@ rmnet_map_ipv4_ul_csum_header(void *iphdr,
 			      struct rmnet_map_ul_csum_header *ul_header,
 			      struct sk_buff *skb)
 {
-	__be16 *hdr = (__be16 *)ul_header;
 	struct iphdr *ip4h = iphdr;
 	u16 offset;
+	u16 val;
 
 	offset = skb_transport_header(skb) - (unsigned char *)iphdr;
 	ul_header->csum_start_offset = htons(offset);
 
-	ul_header->csum_insert_offset = skb->csum_offset;
-	ul_header->csum_enabled = 1;
+	val = MAP_CSUM_UL_ENABLED_FLAG;
 	if (ip4h->protocol == IPPROTO_UDP)
-		ul_header->udp_ind = 1;
-	else
-		ul_header->udp_ind = 0;
+		val |= MAP_CSUM_UL_UDP_FLAG;
+	val |= u16_encode_bits(skb->csum_offset, MAP_CSUM_UL_OFFSET_FMASK);
 
-	/* Changing remaining fields to network order */
-	hdr++;
-	*hdr = htons((__force u16)*hdr);
+	ul_header->csum_info = htons(val);
 
 	skb->ip_summed = CHECKSUM_NONE;
 
@@ -241,24 +237,19 @@ rmnet_map_ipv6_ul_csum_header(void *ip6hdr,
 			      struct rmnet_map_ul_csum_header *ul_header,
 			      struct sk_buff *skb)
 {
-	__be16 *hdr = (__be16 *)ul_header;
 	struct ipv6hdr *ip6h = ip6hdr;
 	u16 offset;
+	u16 val;
 
 	offset = skb_transport_header(skb) - (unsigned char *)ip6hdr;
 	ul_header->csum_start_offset = htons(offset);
 
-	ul_header->csum_insert_offset = skb->csum_offset;
-	ul_header->csum_enabled = 1;
-
+	val = MAP_CSUM_UL_ENABLED_FLAG;
 	if (ip6h->nexthdr == IPPROTO_UDP)
-		ul_header->udp_ind = 1;
-	else
-		ul_header->udp_ind = 0;
+		val |= MAP_CSUM_UL_UDP_FLAG;
+	val |= u16_encode_bits(skb->csum_offset, MAP_CSUM_UL_OFFSET_FMASK);
 
-	/* Changing remaining fields to network order */
-	hdr++;
-	*hdr = htons((__force u16)*hdr);
+	ul_header->csum_info = htons(val);
 
 	skb->ip_summed = CHECKSUM_NONE;
 
@@ -425,10 +416,7 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,
 	}
 
 sw_csum:
-	ul_header->csum_start_offset = 0;
-	ul_header->csum_insert_offset = 0;
-	ul_header->csum_enabled = 0;
-	ul_header->udp_ind = 0;
+	memset(ul_header, 0, sizeof(*ul_header));
 
 	priv->stats.csum_sw++;
 }
diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
index a848bb2e0dad0..141754d0078cc 100644
--- a/include/linux/if_rmnet.h
+++ b/include/linux/if_rmnet.h
@@ -33,17 +33,16 @@ struct rmnet_map_dl_csum_trailer {
 
 struct rmnet_map_ul_csum_header {
 	__be16 csum_start_offset;
-#if defined(__LITTLE_ENDIAN_BITFIELD)
-	u16 csum_insert_offset:14;
-	u16 udp_ind:1;
-	u16 csum_enabled:1;
-#elif defined (__BIG_ENDIAN_BITFIELD)
-	u16 csum_enabled:1;
-	u16 udp_ind:1;
-	u16 csum_insert_offset:14;
-#else
-#error	"Please fix <asm/byteorder.h>"
-#endif
+	__be16 csum_info;		/* MAP_CSUM_UL_*_FMASK */
 } __aligned(1);
 
+/* csum_info field:
+ *  OFFSET:	where (offset in bytes) to insert computed checksum
+ *  UDP:	1 = UDP checksum (zero checkum means no checksum)
+ *  ENABLED:	1 = checksum computation requested
+ */
+#define MAP_CSUM_UL_OFFSET_FMASK	GENMASK(13, 0)
+#define MAP_CSUM_UL_UDP_FLAG		BIT(14)
+#define MAP_CSUM_UL_ENABLED_FLAG	BIT(15)
+
 #endif /* !(_LINUX_IF_RMNET_H_) */
-- 
2.27.0

Re: [PATCH net-next v3 0/6] net: qualcomm: rmnet: stop using C bit-fields

[Alex Elder]

On 3/9/21 6:48 AM, Alex Elder wrote:
> Version 3 of this series uses BIT() rather than GENMASK() to define
> single-bit masks.  It then uses a simple AND (&) operation rather
> than (e.g.) u8_get_bits() to access such flags.  This was suggested
> by David Laight and really prefer the result.  With Bjorn's
> permission I have preserved his Reviewed-by tags on the first five
> patches.

Nice as all this looks, it doesn't *work*.  I did some very basic
testing before sending out version 3, but not enough.  (More on
the problem, below).

		--> I retract this series <--

I will send out an update (version 4).  But I won't be doing it
for a few more days.

The problem is that the BIT() flags are defined in host byte
order.  But the values they're compared against are not always
(or perhaps, never) in host byte order.

I regret the error, and will do a complete set of testing on
version 4 before sending it out for review.

					-Alex

> Version 2 fixed bugs in the way the value written into the header
> was computed.
> 
> The series was first posted here:
>    https://lore.kernel.org/netdev/20210304223431.15045-1-elder@linaro.org/
> Below is a summary of the original description.
> 
> This series converts data structures defined in <linux/if_rmnet.h>
> so they use integral field values with bitfield masks rather than
> relying on C bit-fields.
>    - The first three patches lay the ground work for the others.
>        - The first adds endianness notation to a structure.
>        - The second simplifies a bit of complicated code.
>        - The third open-codes some macros that needlessly
>          obscured some simple code.
>    - Each of the last three patches converts one of the structures
>      defined in <linux/if_rmnet.h> so it no longer uses C bit-fields.
> 
>      					-Alex
> 
> Alex Elder (6):
>    net: qualcomm: rmnet: mark trailer field endianness
>    net: qualcomm: rmnet: simplify some byte order logic
>    net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros
>    net: qualcomm: rmnet: use field masks instead of C bit-fields
>    net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer
>    net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header
> 
>   .../ethernet/qualcomm/rmnet/rmnet_handlers.c  | 11 ++--
>   .../net/ethernet/qualcomm/rmnet/rmnet_map.h   | 12 ----
>   .../qualcomm/rmnet/rmnet_map_command.c        | 11 +++-
>   .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 60 ++++++++---------
>   include/linux/if_rmnet.h                      | 65 +++++++++----------
>   5 files changed, 70 insertions(+), 89 deletions(-)
> 

Re: [PATCH net-next v3 0/6] net: qualcomm: rmnet: stop using C bit-fields

[Vladimir Oltean]

Hi Alex,

On Tue, Mar 09, 2021 at 05:39:20PM -0600, Alex Elder wrote:
> On 3/9/21 6:48 AM, Alex Elder wrote:
> > Version 3 of this series uses BIT() rather than GENMASK() to define
> > single-bit masks.  It then uses a simple AND (&) operation rather
> > than (e.g.) u8_get_bits() to access such flags.  This was suggested
> > by David Laight and really prefer the result.  With Bjorn's
> > permission I have preserved his Reviewed-by tags on the first five
> > patches.
>
> Nice as all this looks, it doesn't *work*.  I did some very basic
> testing before sending out version 3, but not enough.  (More on
> the problem, below).
>
> 		--> I retract this series <--
>
> I will send out an update (version 4).  But I won't be doing it
> for a few more days.
>
> The problem is that the BIT() flags are defined in host byte
> order.  But the values they're compared against are not always
> (or perhaps, never) in host byte order.
>
> I regret the error, and will do a complete set of testing on
> version 4 before sending it out for review.

I think I understand some of your pain. I had a similar situation trying
to write a driver for hardware with very strange bitfield organization,
and my top priority was actually maintaining a set of bitfield definitions
that could be taken directly from the user manual of said piece of
hardware (and similar to you, I dislike C bitfields). What I came up
with was an entirely new API called packing() which is described here:
https://www.kernel.org/doc/html/latest/core-api/packing.html
It doesn't have any users except code added by me (some in Ethernet fast
paths), and it has some limitations (mainly that it only has support for
u64 CPU words), but on the other hand, it's easy to understand, easy to
use, supports any bit/byte layout under the sun, doesn't suffer from
unaligned memory access issues due to its byte-by-byte approach, and is
completely independent of host endianness.
That said, I'm not completely happy with it because it has slightly
higher overhead compared to typical bitfield accessors. I've been on the
fence about even deleting it, considering that it's been two years since
it's in mainline and it hasn't gained much of a traction. So I would
rather try to work my way around a different API in the sja1105 driver.

Have you noticed this API and decided to not use it for whatever reason?
Could you let me know what that was? Even better, in your quest to fix
the rmnet driver, have you seen any API that is capable of extracting a
bitfield that spans two 64-bit halves of an 128 bit word in a custom bit
layout?

Re: [PATCH net-next v3 0/6] net: qualcomm: rmnet: stop using C bit-fields

[Alex Elder]

On 3/9/21 6:27 PM, Vladimir Oltean wrote:
> Hi Alex,
> 
> On Tue, Mar 09, 2021 at 05:39:20PM -0600, Alex Elder wrote:
>> On 3/9/21 6:48 AM, Alex Elder wrote:
>>> Version 3 of this series uses BIT() rather than GENMASK() to define
>>> single-bit masks.  It then uses a simple AND (&) operation rather
>>> than (e.g.) u8_get_bits() to access such flags.  This was suggested
>>> by David Laight and really prefer the result.  With Bjorn's
>>> permission I have preserved his Reviewed-by tags on the first five
>>> patches.
>>
>> Nice as all this looks, it doesn't *work*.  I did some very basic
>> testing before sending out version 3, but not enough.  (More on
>> the problem, below).
>>
>> 		--> I retract this series <--
>>
>> I will send out an update (version 4).  But I won't be doing it
>> for a few more days.
>>
>> The problem is that the BIT() flags are defined in host byte
>> order.  But the values they're compared against are not always
>> (or perhaps, never) in host byte order.
>>
>> I regret the error, and will do a complete set of testing on
>> version 4 before sending it out for review.
> 
> I think I understand some of your pain. I had a similar situation trying

I appreciate your saying that.  In this case these were
mistakes of my own making, but it has been surprisingly
tricky to make sense of exactly how bits are laid out
under various scenarios, old and new.

> to write a driver for hardware with very strange bitfield organization,
> and my top priority was actually maintaining a set of bitfield definitions
> that could be taken directly from the user manual of said piece of
> hardware (and similar to you, I dislike C bitfields). What I came up

That seems like a reasonable thing to do.  The conventional
layout of big endian bytes and bits in network documentation
is great, but it is also different from other conventions
used elsewhere, and sometimes that too can lead to confusion.

For example, network specs list tend to show groups of 4 bytes
in  a row with high-order byte *and bit* numbered 0:

  high order byte
|       0       |          1          |    2    |     3    |
|0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|   ...   |  ...  |31|
  ^-- high bit

While SCSI shows 1 byte rows, high-order bit numbered 7:

Byte| 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 |
  0  | ^-- high bit                  |
  1  |

> with was an entirely new API called packing() which is described here:
> https://www.kernel.org/doc/html/latest/core-api/packing.html

I was not aware of that.  And I have now looked at it and have
a few comments, below.

> It doesn't have any users except code added by me (some in Ethernet fast
> paths), and it has some limitations (mainly that it only has support for
> u64 CPU words), but on the other hand, it's easy to understand, easy to
> use, supports any bit/byte layout under the sun, doesn't suffer from
> unaligned memory access issues due to its byte-by-byte approach, and is
> completely independent of host endianness.
> That said, I'm not completely happy with it because it has slightly
> higher overhead compared to typical bitfield accessors. I've been on the
> fence about even deleting it, considering that it's been two years since
> it's in mainline and it hasn't gained much of a traction. So I would
> rather try to work my way around a different API in the sja1105 driver.
> 
> Have you noticed this API and decided to not use it for whatever reason?

I had not noticed it before you brought it to my attention.

> Could you let me know what that was? Even better, in your quest to fix
> the rmnet driver, have you seen any API that is capable of extracting a
> bitfield that spans two 64-bit halves of an 128 bit word in a custom bit
> layout?

I have not seen an interface that attempts to handle things that
span multiple 64-bit units of memory.

Your document uses big endian 64-bit word (not u64) as its "no
quirks" example (listing high-order bytes first, and numbering
bytes--and bits--starting at higher numbers).  My only objection
to that is that you should probably call it __be64.  Otherwise
I'm just pointing out that it is a convention, and as pointed
out above, it isn't universal.

For my purposes, all registers are 32 bits.  So to use your
packing() interface I would have to implement 32-bit versions
of the function.  But that's not really a big deal.  I do
see what you're doing, defining exactly what you want to
do in the arguments passed to packing(), and allowing the
one function to both encode and extract values in a 64-bit
register regardless of endianness.

Having looked at this, though, I prefer the functions and
macros defined in <linux/bitfield.h>.  Probably the biggest
reason is a bias I have about the fact that a single mask
value represents both the position and width of a field
within a register.  I independently "invented/discovered"
this and implemented it for my own use, only to learn later
it had already been done.  So I switched to the functions
that were already upstream.

The "bitfield.h" functions *require* the field mask to
be a runtime constant.  That is great, because it means
that determining the width and position of fields from
the mask can be done at compile time, and can be done
as efficiently as possible.  The downside is that it
would occasionally be nice to be able to supply a variable
mask value.  I've had to go to some lengths in the IPA
driver to get a result that compiles in some cases.

But because you asked, I'll list some other reasons why
I prefer those functions over your packing() function.
- As mentioned, a single mask defines both the position and
   size of a field.  It is specified in "natural" host byte
   order, but provides options for encoding or extracting
   values for big- or little-endian objects as well.
- I prefer to have the size of the integral types (and the
   operations on them) to be explicit.  I.e., yes, "int"
   is supposed to be the most efficient type on a machine,
   but in working with the hardware I like declaring exactly
   what size object I'm dealing with.  The "bitfield.h"
   functions have that, for every standard word size.
- I think that your one function, although convenient,
   is too general-purpose.  I'd rather *at least* have
   one function for encoding values and a second for
   decoding.
- In contrast, the "bitfield.h" functions each do one
   thing--either encode or extract, on an object of a
   known size and endianness.  The arguments passed are
   minimal but sufficient to perform each operation.
- I prefer having the encoding function *return* the
   encoded value rather than passing the address of
   an object to be updated.
- Although I recognize exactly the problem it solves,
   the QUIRKS flags you have aren't a lot more intuitive
   to me than the __LITTLE_ENDIAN_BITFIELD symbol is.
   At least the names you use offer a little more clarity.

None of these are strong criticisms.  In fact I like that
you created a function/system to solve this problem.  But
you requested some reasoning, so I wanted to offer some.

In this particular RMNet series, I followed conventions
I used for the IPA driver.  In that case, there are many
registers with fields, so it was important to follow
some consistent patterns.  For this case, not nearly as
much generality is needed, so I could probably get away
with simpler masks, possibly without even using the
u16_encode_bits() functions.  Not sure what I'll settle
on but I plan to post version 4 of the series early
next week.

Thanks for your note.  I do appreciate having someone
say "hey, I've been there."  And I'm glad to now be
aware of the packing() function.
	
					-Alex