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

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

[Alex Elder]

This series converts data structures defined in <linux/if_rmnet.h>
so they use integral field values with bitfield masks rather than
rely on C bit-fields.

I first proposed doing something like this long ago when my confusion
about this code (and the memory layout it was supposed to represent)
led me to believe it was erroneous:
  https://lore.kernel.org/netdev/20190520135354.18628-1-elder@linaro.org/

It came up again recently, when Sharath Chandra Vurukala proposed
a new structure in "if_rmnet.h", again using C bit-fields.  I asked
whether the new structure could use field masks, and Jakub requested
that this be done.
  https://lore.kernel.org/netdev/1613079324-20166-1-git-send-email-sharathv@codeaurora.org/
I volunteered to convert the existing RMNet code to use bitfield
masks, and that is what I'm doing here.

The first three patches are more or less preparation work for the
last three.
  - The first marks two fields in an existing structure explicitly
    big endian.  They are unused by current code, so this should
    have no impact.
  - The second simplifies some code that computes the value of a
    field in a header in a somewhat obfuscated way.
  - The third eliminates some trivial accessor macros, open-coding
    them instead.  I believe the accessors actually do more harm
    than good.
  - The last three convert the structures defined in "if_rmnet.h"
    so they are defined only with integral fields, each having
    well-defined byte order.  Where sub-fields are needed, field
    masks are defined so they can be encoded or extracted using
    functions like be16_get_bits() or u8_encode_bits(), defined
    in <linux/bitfield.h>.  The three structures converted are,
    in order:  rmnet_map_header, rmnet_map_dl_csum_trailer, and
    rmnet_map_ul_csum_header.

					-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.20.1

[PATCH net-next 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>
---
 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.20.1

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

[Bjorn Andersson]

On Thu 04 Mar 16:34 CST 2021, Alex Elder wrote:

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

Regards,
Bjorn

> ---
>  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.20.1
> 

[PATCH net-next 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>
---
 .../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.20.1

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

[Bjorn Andersson]

On Thu 04 Mar 16:34 CST 2021, Alex Elder wrote:

> 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>
> ---
>  .../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) -

Just curious, why does this require a __force, or even a cast?


Regardless, your proposed way of writing it is easier to read.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> -				     (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.20.1
> 

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

[Alex Elder]

On 3/4/21 10:07 PM, Bjorn Andersson wrote:
> On Thu 04 Mar 16:34 CST 2021, Alex Elder wrote:
> 
>> 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>
>> ---
>>   .../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) -
> 
> Just curious, why does this require a __force, or even a cast?

The argument to htons() has type __u16.  In this case it
is passed the difference between pointers, which will
have type ptrdiff_t, which is certainly bigger than
16 bits.  I don't think the __force is needed, but the
cast to u16 might just be making the conversion to the
smaller type explicit.  Here too though, I don't think
it's necessary.

					-Alex

> Regardless, your proposed way of writing it is easier to read.
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Regards,
> Bjorn
> 
>> -				     (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.20.1
>>

[PATCH net-next 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>
---
 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.20.1

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

[Bjorn Andersson]

On Thu 04 Mar 16:34 CST 2021, Alex Elder wrote:

> 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.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Alex Elder <elder@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.20.1
> 

[PATCH net-next 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.  Instead, define a single-byte
flags field, and use the functions defined in <linux/bitfield.h>,
along with field mask constants to extract or assign values within
that field.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 .../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..30f8e2f02696b 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 (u8_get_bits(map_header->flags, MAP_CMD_FMASK)) {
 		/* 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..4824c6328a82c 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_FMASK			GENMASK(7, 7)
+#define MAP_PAD_LEN_FMASK		GENMASK(5, 0)
+
 struct rmnet_map_dl_csum_trailer {
 	u8  reserved1;
 #if defined(__LITTLE_ENDIAN_BITFIELD)
-- 
2.20.1

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

[Bjorn Andersson]

On Thu 04 Mar 16:34 CST 2021, Alex Elder wrote:

> 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.  Instead, define a single-byte
> flags field, and use the functions defined in <linux/bitfield.h>,
> along with field mask constants to extract or assign values within
> that field.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  .../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..30f8e2f02696b 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 (u8_get_bits(map_header->flags, MAP_CMD_FMASK)) {
>  		/* 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..4824c6328a82c 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_FMASK			GENMASK(7, 7)
> +#define MAP_PAD_LEN_FMASK		GENMASK(5, 0)
> +
>  struct rmnet_map_dl_csum_trailer {
>  	u8  reserved1;
>  #if defined(__LITTLE_ENDIAN_BITFIELD)
> -- 
> 2.20.1
> 

[PATCH net-next 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>
---
 .../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..29d485b868a65 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 (!u8_get_bits(csum_trailer->flags, MAP_CSUM_DL_VALID_FMASK)) {
 		priv->stats.csum_valid_unset++;
 		return -EINVAL;
 	}
diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
index 4824c6328a82c..1fbb7531238b6 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_FMASK		GENMASK(0, 0)
+
 struct rmnet_map_ul_csum_header {
 	__be16 csum_start_offset;
 #if defined(__LITTLE_ENDIAN_BITFIELD)
-- 
2.20.1

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

[Bjorn Andersson]

On Thu 04 Mar 16:34 CST 2021, 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.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  .../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..29d485b868a65 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 (!u8_get_bits(csum_trailer->flags, MAP_CSUM_DL_VALID_FMASK)) {
>  		priv->stats.csum_valid_unset++;
>  		return -EINVAL;
>  	}
> diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
> index 4824c6328a82c..1fbb7531238b6 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_FMASK		GENMASK(0, 0)
> +
>  struct rmnet_map_ul_csum_header {
>  	__be16 csum_start_offset;
>  #if defined(__LITTLE_ENDIAN_BITFIELD)
> -- 
> 2.20.1
> 

[PATCH net-next 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 field masks to encode or get values within it.

Previously rmnet_map_ipv4_ul_csum_header() would update values in
the host byte-order fields, and then forcibly fix their byte order
using a combination of byte order 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>
---
 .../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 29d485b868a65..db76bbf000aa1 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 = be16_encode_bits(1, MAP_CSUM_UL_ENABLED_FMASK);
 	if (ip4h->protocol == IPPROTO_UDP)
-		ul_header->udp_ind = 1;
-	else
-		ul_header->udp_ind = 0;
+		val |= be16_encode_bits(1, MAP_CSUM_UL_UDP_FMASK);
+	val |= be16_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 = be16_encode_bits(1, MAP_CSUM_UL_ENABLED_FMASK);
 	if (ip6h->nexthdr == IPPROTO_UDP)
-		ul_header->udp_ind = 1;
-	else
-		ul_header->udp_ind = 0;
+		val |= be16_encode_bits(1, MAP_CSUM_UL_UDP_FMASK);
+	val |= be16_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 1fbb7531238b6..149d696feb520 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:
+ *  ENABLED:	1 = checksum computation requested
+ *  UDP:	1 = UDP checksum (zero checkum means no checksum)
+ *  OFFSET:	where (offset in bytes) to insert computed checksum
+ */
+#define MAP_CSUM_UL_OFFSET_FMASK	GENMASK(13, 0)
+#define MAP_CSUM_UL_UDP_FMASK		GENMASK(14, 14)
+#define MAP_CSUM_UL_ENABLED_FMASK	GENMASK(15, 15)
+
 #endif /* !(_LINUX_IF_RMNET_H_) */
-- 
2.20.1

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

[Bjorn Andersson]

On Thu 04 Mar 16:34 CST 2021, Alex Elder wrote:

> 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 field masks to encode or get values within it.
> 
> Previously rmnet_map_ipv4_ul_csum_header() would update values in
> the host byte-order fields, and then forcibly fix their byte order
> using a combination of byte order 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>
> ---
>  .../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 29d485b868a65..db76bbf000aa1 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 = be16_encode_bits(1, MAP_CSUM_UL_ENABLED_FMASK);

Why are you using be16_ here? Won't that cancel the htons() below?

Regards,
Bjorn

>  	if (ip4h->protocol == IPPROTO_UDP)
> -		ul_header->udp_ind = 1;
> -	else
> -		ul_header->udp_ind = 0;
> +		val |= be16_encode_bits(1, MAP_CSUM_UL_UDP_FMASK);
> +	val |= be16_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 = be16_encode_bits(1, MAP_CSUM_UL_ENABLED_FMASK);
>  	if (ip6h->nexthdr == IPPROTO_UDP)
> -		ul_header->udp_ind = 1;
> -	else
> -		ul_header->udp_ind = 0;
> +		val |= be16_encode_bits(1, MAP_CSUM_UL_UDP_FMASK);
> +	val |= be16_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 1fbb7531238b6..149d696feb520 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:
> + *  ENABLED:	1 = checksum computation requested
> + *  UDP:	1 = UDP checksum (zero checkum means no checksum)
> + *  OFFSET:	where (offset in bytes) to insert computed checksum
> + */
> +#define MAP_CSUM_UL_OFFSET_FMASK	GENMASK(13, 0)
> +#define MAP_CSUM_UL_UDP_FMASK		GENMASK(14, 14)
> +#define MAP_CSUM_UL_ENABLED_FMASK	GENMASK(15, 15)
> +
>  #endif /* !(_LINUX_IF_RMNET_H_) */
> -- 
> 2.20.1
> 

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

[Alex Elder]

On 3/4/21 11:26 PM, Bjorn Andersson wrote:
> On Thu 04 Mar 16:34 CST 2021, Alex Elder wrote:
> 
>> 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 field masks to encode or get values within it.
>>
>> Previously rmnet_map_ipv4_ul_csum_header() would update values in
>> the host byte-order fields, and then forcibly fix their byte order
>> using a combination of byte order 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>
>> ---
>>   .../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 29d485b868a65..db76bbf000aa1 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 = be16_encode_bits(1, MAP_CSUM_UL_ENABLED_FMASK);
> 
> Why are you using be16_ here? Won't that cancel the htons() below?

Yes.  This was a mistake, and as you know, the kernel test
robot caught the problem with assigning a big endian value
to a host byte order variable.  This cannot have worked
before and I'm not sure what happened.

I've updated this series and am re-testing everything.  That
includes running the patches through sparse, but also includes
running with checksum offload enabled and verifying errors
are not reported when checksum offload active.

					-Alex

> Regards,
> Bjorn
> 
>>   	if (ip4h->protocol == IPPROTO_UDP)
>> -		ul_header->udp_ind = 1;
>> -	else
>> -		ul_header->udp_ind = 0;
>> +		val |= be16_encode_bits(1, MAP_CSUM_UL_UDP_FMASK);
>> +	val |= be16_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 = be16_encode_bits(1, MAP_CSUM_UL_ENABLED_FMASK);
>>   	if (ip6h->nexthdr == IPPROTO_UDP)
>> -		ul_header->udp_ind = 1;
>> -	else
>> -		ul_header->udp_ind = 0;
>> +		val |= be16_encode_bits(1, MAP_CSUM_UL_UDP_FMASK);
>> +	val |= be16_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 1fbb7531238b6..149d696feb520 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:
>> + *  ENABLED:	1 = checksum computation requested
>> + *  UDP:	1 = UDP checksum (zero checkum means no checksum)
>> + *  OFFSET:	where (offset in bytes) to insert computed checksum
>> + */
>> +#define MAP_CSUM_UL_OFFSET_FMASK	GENMASK(13, 0)
>> +#define MAP_CSUM_UL_UDP_FMASK		GENMASK(14, 14)
>> +#define MAP_CSUM_UL_ENABLED_FMASK	GENMASK(15, 15)
>> +
>>   #endif /* !(_LINUX_IF_RMNET_H_) */
>> -- 
>> 2.20.1
>>

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

[Alex Elder]

On 3/4/21 4:34 PM, Alex Elder wrote:
> This series converts data structures defined in <linux/if_rmnet.h>
> so they use integral field values with bitfield masks rather than
> rely on C bit-fields.

Whoops!  I forgot to check if net-next was open.  I'm very
sorry about that...

   http://vger.kernel.org/~davem/net-next.html

					-Alex

> I first proposed doing something like this long ago when my confusion
> about this code (and the memory layout it was supposed to represent)
> led me to believe it was erroneous:
>    https://lore.kernel.org/netdev/20190520135354.18628-1-elder@linaro.org/
> 
> It came up again recently, when Sharath Chandra Vurukala proposed
> a new structure in "if_rmnet.h", again using C bit-fields.  I asked
> whether the new structure could use field masks, and Jakub requested
> that this be done.
>    https://lore.kernel.org/netdev/1613079324-20166-1-git-send-email-sharathv@codeaurora.org/
> I volunteered to convert the existing RMNet code to use bitfield
> masks, and that is what I'm doing here.
> 
> The first three patches are more or less preparation work for the
> last three.
>    - The first marks two fields in an existing structure explicitly
>      big endian.  They are unused by current code, so this should
>      have no impact.
>    - The second simplifies some code that computes the value of a
>      field in a header in a somewhat obfuscated way.
>    - The third eliminates some trivial accessor macros, open-coding
>      them instead.  I believe the accessors actually do more harm
>      than good.
>    - The last three convert the structures defined in "if_rmnet.h"
>      so they are defined only with integral fields, each having
>      well-defined byte order.  Where sub-fields are needed, field
>      masks are defined so they can be encoded or extracted using
>      functions like be16_get_bits() or u8_encode_bits(), defined
>      in <linux/bitfield.h>.  The three structures converted are,
>      in order:  rmnet_map_header, rmnet_map_dl_csum_trailer, and
>      rmnet_map_ul_csum_header.
> 
> 					-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 0/6] net: qualcomm: rmnet: stop using C bit-fields

[subashab]

On 2021-03-04 15:34, Alex Elder wrote:
> This series converts data structures defined in <linux/if_rmnet.h>
> so they use integral field values with bitfield masks rather than
> rely on C bit-fields.
> 
> I first proposed doing something like this long ago when my confusion
> about this code (and the memory layout it was supposed to represent)
> led me to believe it was erroneous:
>   
> https://lore.kernel.org/netdev/20190520135354.18628-1-elder@linaro.org/
> 
> It came up again recently, when Sharath Chandra Vurukala proposed
> a new structure in "if_rmnet.h", again using C bit-fields.  I asked
> whether the new structure could use field masks, and Jakub requested
> that this be done.
> 
> https://lore.kernel.org/netdev/1613079324-20166-1-git-send-email-sharathv@
> codeaurora.org/
> I volunteered to convert the existing RMNet code to use bitfield
> masks, and that is what I'm doing here.
> 
> The first three patches are more or less preparation work for the
> last three.
>   - The first marks two fields in an existing structure explicitly
>     big endian.  They are unused by current code, so this should
>     have no impact.
>   - The second simplifies some code that computes the value of a
>     field in a header in a somewhat obfuscated way.
>   - The third eliminates some trivial accessor macros, open-coding
>     them instead.  I believe the accessors actually do more harm
>     than good.
>   - The last three convert the structures defined in "if_rmnet.h"
>     so they are defined only with integral fields, each having
>     well-defined byte order.  Where sub-fields are needed, field
>     masks are defined so they can be encoded or extracted using
>     functions like be16_get_bits() or u8_encode_bits(), defined
>     in <linux/bitfield.h>.  The three structures converted are,
>     in order:  rmnet_map_header, rmnet_map_dl_csum_trailer, and
>     rmnet_map_ul_csum_header.
> 
> 					-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(-)

Can you share what all tests have been done with these patches

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

[Alex Elder]

On 3/4/21 9:44 PM, subashab@codeaurora.org wrote:
> 
> Can you share what all tests have been done with these patches

I'm testing with all of them applied and "it works."  On
the first three I think they're simple enough that you
can see by inspection they should be OK.  For the rest
I tested more carefully.

For runtime testing, I have used them on IPA v3.5.1 and
IPA v4.2 platforms, running repeated ping and other network
traffic tests over an rmnet connection.

For unit testing, I did essentially the following.  I'll
use the MAP header structure as an example, but I did
this on all structures I modified.

	struct rmnet_map_header_new new;
	struct rmnet_map_header *old = (void *)&new;
	u8 val;

	val = u8_encode_bits(1, MAP_CMD_FMASK);
	val |= u8_encode_bits(0x23, MAP_PAD_LEN_FMASK);
	new.flags = val;
	new.mux_id = 0x45;
	new.pkt_len = htons(0x6789);

	printk("pad_len: 0x%x (want 0x23)\n", old->pad_len);
	printk("reserved_bit: 0x%x (want 0x0)\n", old->reserved_bit);
	printk("cd_bit: 0x%x (want 0x1)\n", old->cd_bit);
	printk("mux_id: 0x%x (want 0x45)\n", old->mux_id);
	printk("pkt_len: 0x%x (want 0x6789)\n", ntohs(old->pkt_len));

I didn't do *exactly* or *only* this, but basically the
process was manipulating the values assigned using the
old structure then verifying it has the same representation
in the new structure using the new access methods (and vice
versa).

I suspect you have a much better ability to test than
I do, and I would really prefer to see this get tested
rigorously if possible.

					-Alex