linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v2 0/6] net: qualcomm: rmnet: stop using C bit-fields
@ 2021-03-06  3:15 Alex Elder
  2021-03-06  3:15 ` [PATCH net-next v2 1/6] net: qualcomm: rmnet: mark trailer field endianness Alex Elder
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Alex Elder @ 2021-03-06  3:15 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel

Version 2 of this series fixes the code in the final patch that
encoded the RMNet checksum offload header inserted for outgoing
packets.  It was erroneously using be16_encode_bits() to generate
the value to be written into the header, where it should have been
using u16_encode_bits().  Bjorn noticed this, as did the Intel
kernel test robot.

This version of these patches has been tested as follows:
  - ICMP (ping) and TCP (wget), and UDP (iperf) packet transfer in
    both directions over an RMNet link (using IPA, with IPv4 only).
    This used QMAPv4, with checksum offload in both directions
    enabled (and aggregation enabled for inbound data).  Inbound
    checksum values were verified good (for TCP and UDP).  I
    presume the TX checksum was acceptable but did nothing special
    to confirm that.  Checksum verification was enabled with:
      ethtool -K rmnet_data0 tx on rx on
  - Format of the new and old structures were compared, bit by bit,
    after assigning various values to the fields using both the
    old and new structure definitions (and access methods).
  - The patches were all run through sparse.  No new errors are
    reported.  (A "no newline at end of file" warning is reported
    for "rmnet_vnd.c"; and ip_fast_csum() defined in ARM64
    "checksum.h" produces a warning.)

I've added Reviewed-by from Bjorn to the first five patches, and
Reported-by from the kernel test robot on the last one

Below is description that was sent with version 1.

					-Alex

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH net-next v2 1/6] net: qualcomm: rmnet: mark trailer field endianness
  2021-03-06  3:15 [PATCH net-next v2 0/6] net: qualcomm: rmnet: stop using C bit-fields Alex Elder
@ 2021-03-06  3:15 ` Alex Elder
  2021-03-06  3:15 ` [PATCH net-next v2 2/6] net: qualcomm: rmnet: simplify some byte order logic Alex Elder
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Alex Elder @ 2021-03-06  3:15 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next v2 2/6] net: qualcomm: rmnet: simplify some byte order logic
  2021-03-06  3:15 [PATCH net-next v2 0/6] net: qualcomm: rmnet: stop using C bit-fields Alex Elder
  2021-03-06  3:15 ` [PATCH net-next v2 1/6] net: qualcomm: rmnet: mark trailer field endianness Alex Elder
@ 2021-03-06  3:15 ` Alex Elder
  2021-03-06  3:15 ` [PATCH net-next v2 3/6] net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros Alex Elder
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Alex Elder @ 2021-03-06  3:15 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next v2 3/6] net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros
  2021-03-06  3:15 [PATCH net-next v2 0/6] net: qualcomm: rmnet: stop using C bit-fields Alex Elder
  2021-03-06  3:15 ` [PATCH net-next v2 1/6] net: qualcomm: rmnet: mark trailer field endianness Alex Elder
  2021-03-06  3:15 ` [PATCH net-next v2 2/6] net: qualcomm: rmnet: simplify some byte order logic Alex Elder
@ 2021-03-06  3:15 ` Alex Elder
  2021-03-06  3:15 ` [PATCH net-next v2 4/6] net: qualcomm: rmnet: use field masks instead of C bit-fields Alex Elder
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Alex Elder @ 2021-03-06  3:15 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next v2 4/6] net: qualcomm: rmnet: use field masks instead of C bit-fields
  2021-03-06  3:15 [PATCH net-next v2 0/6] net: qualcomm: rmnet: stop using C bit-fields Alex Elder
                   ` (2 preceding siblings ...)
  2021-03-06  3:15 ` [PATCH net-next v2 3/6] net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros Alex Elder
@ 2021-03-06  3:15 ` Alex Elder
  2021-03-06  3:15 ` [PATCH net-next v2 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer Alex Elder
  2021-03-06  3:15 ` [PATCH net-next v2 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header Alex Elder
  5 siblings, 0 replies; 14+ messages in thread
From: Alex Elder @ 2021-03-06  3:15 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel

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>
Reviewed-by: Bjorn Andersson <bjorn.andersson@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.27.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next v2 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer
  2021-03-06  3:15 [PATCH net-next v2 0/6] net: qualcomm: rmnet: stop using C bit-fields Alex Elder
                   ` (3 preceding siblings ...)
  2021-03-06  3:15 ` [PATCH net-next v2 4/6] net: qualcomm: rmnet: use field masks instead of C bit-fields Alex Elder
@ 2021-03-06  3:15 ` Alex Elder
  2021-03-08 10:13   ` David Laight
  2021-03-06  3:15 ` [PATCH net-next v2 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header Alex Elder
  5 siblings, 1 reply; 14+ messages in thread
From: Alex Elder @ 2021-03-06  3:15 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next v2 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header
  2021-03-06  3:15 [PATCH net-next v2 0/6] net: qualcomm: rmnet: stop using C bit-fields Alex Elder
                   ` (4 preceding siblings ...)
  2021-03-06  3:15 ` [PATCH net-next v2 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer Alex Elder
@ 2021-03-06  3:15 ` Alex Elder
  2021-03-08 10:18   ` David Laight
  5 siblings, 1 reply; 14+ messages in thread
From: Alex Elder @ 2021-03-06  3:15 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel, kernel test robot

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>
Reported-by: kernel test robot <lkp@intel.com>
---
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 29d485b868a65..b76ad48da7325 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 = u16_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 |= u16_encode_bits(1, MAP_CSUM_UL_UDP_FMASK);
+	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 = u16_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 |= u16_encode_bits(1, MAP_CSUM_UL_UDP_FMASK);
+	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 1fbb7531238b6..9ff09a2bcf9e1 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_FMASK		GENMASK(14, 14)
+#define MAP_CSUM_UL_ENABLED_FMASK	GENMASK(15, 15)
+
 #endif /* !(_LINUX_IF_RMNET_H_) */
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* RE: [PATCH net-next v2 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer
  2021-03-06  3:15 ` [PATCH net-next v2 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer Alex Elder
@ 2021-03-08 10:13   ` David Laight
  2021-03-08 13:39     ` Alex Elder
  0 siblings, 1 reply; 14+ messages in thread
From: David Laight @ 2021-03-08 10:13 UTC (permalink / raw)
  To: 'Alex Elder', subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel

From: Alex Elder
> Sent: 06 March 2021 03:16
> 
> 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>
> ---
>  .../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)) {

Is that just an overcomplicated way of saying:
	if (!(csum_trailer->flags & MAP_CSUM_DL_VALID_FMASK)) {

    David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH net-next v2 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header
  2021-03-06  3:15 ` [PATCH net-next v2 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header Alex Elder
@ 2021-03-08 10:18   ` David Laight
  2021-03-08 13:59     ` Alex Elder
  0 siblings, 1 reply; 14+ messages in thread
From: David Laight @ 2021-03-08 10:18 UTC (permalink / raw)
  To: 'Alex Elder', subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel, kernel test robot

From: Alex Elder
> Sent: 06 March 2021 03:16
> 
> 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>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
> 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 29d485b868a65..b76ad48da7325 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 = u16_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 |= u16_encode_bits(1, MAP_CSUM_UL_UDP_FMASK);
> +	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);

Isn't this potentially misaligned?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next v2 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer
  2021-03-08 10:13   ` David Laight
@ 2021-03-08 13:39     ` Alex Elder
  2021-03-08 13:53       ` David Laight
  0 siblings, 1 reply; 14+ messages in thread
From: Alex Elder @ 2021-03-08 13:39 UTC (permalink / raw)
  To: David Laight, subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel

On 3/8/21 4:13 AM, David Laight wrote:
> From: Alex Elder
>> Sent: 06 March 2021 03:16
>>
>> 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>
>> ---
>>   .../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)) {
> 
> Is that just an overcomplicated way of saying:
> 	if (!(csum_trailer->flags & MAP_CSUM_DL_VALID_FMASK)) {

Yes it is.  I defined and used all the field masks in a
consistent way, but I do think it will read better the
way you suggest.  Bjorn also asked me privately whether
GENMASK(15, 15) was just the same as BIT(15) (it is).

I will post version 3 of the series, identifying which
fields are single bit/Boolean.  For those I will define
the value using BIT() and will set/extract using simple
AND/OR operators.  I won't use the _FMASK suffix on such
fields.

Thanks a lot for your comment/question/suggestion.  I
like it.

					-Alex

>      David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH net-next v2 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer
  2021-03-08 13:39     ` Alex Elder
@ 2021-03-08 13:53       ` David Laight
  2021-03-08 14:19         ` Alex Elder
  0 siblings, 1 reply; 14+ messages in thread
From: David Laight @ 2021-03-08 13:53 UTC (permalink / raw)
  To: 'Alex Elder', subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel

...
> >> -	if (!csum_trailer->valid) {
> >> +	if (!u8_get_bits(csum_trailer->flags, MAP_CSUM_DL_VALID_FMASK)) {
> >
> > Is that just an overcomplicated way of saying:
> > 	if (!(csum_trailer->flags & MAP_CSUM_DL_VALID_FMASK)) {
> 
> Yes it is.  I defined and used all the field masks in a
> consistent way, but I do think it will read better the
> way you suggest.  Bjorn also asked me privately whether
> GENMASK(15, 15) was just the same as BIT(15) (it is).
> 
> I will post version 3 of the series, identifying which
> fields are single bit/Boolean.  For those I will define
> the value using BIT() and will set/extract using simple
> AND/OR operators.  I won't use the _FMASK suffix on such
> fields.

Even for the checksum offset a simple 'offset << CONSTANT'
is enough.
If it is the bottom bits then even that isn't really needed.
You might want to mask off high bits - but that is an error
path that needs to have been checked earlier.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next v2 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header
  2021-03-08 10:18   ` David Laight
@ 2021-03-08 13:59     ` Alex Elder
  2021-03-08 14:10       ` David Laight
  0 siblings, 1 reply; 14+ messages in thread
From: Alex Elder @ 2021-03-08 13:59 UTC (permalink / raw)
  To: David Laight, subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel, kernel test robot

On 3/8/21 4:18 AM, David Laight wrote:
> From: Alex Elder
>> Sent: 06 March 2021 03:16
>>
>> 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>
>> Reported-by: kernel test robot <lkp@intel.com>
>> ---
>> 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 29d485b868a65..b76ad48da7325 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 = u16_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 |= u16_encode_bits(1, MAP_CSUM_UL_UDP_FMASK);
>> +	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);
> 
> Isn't this potentially misaligned?

At first I thought you were talking about column alignment.

The short answer:  Yes (at least it's possible)!  And
that's a problem elsewhere in the driver.  I noticed
that before and confirmed that unaligned accesses *do*
occur in this driver.

I would want to fix that comprehensively (and separate
from this patch), and not just in this one spot.  I have
not done it because I am not set up to readily test the
change; unaligned access does not cause a fault on aarch64
(or at least on the platforms I'm using).

Sort of related, I have been meaning to eliminate the
pointless __aligned(1) tags on rmnet structures defined
in <linux/if_rmnet.h>.  It wouldn't hurt to use __packed,
though I think they're all 4 or 8 bytes naturally anyway.
Perhaps marking them __aligned(4) would help identify
potential unaligned accesses?

Anyway, assuming you're not talking about tab stops, yes
it's possible this assignment is misaligned.  But it's a
bigger problem than what you point out.  I will take this
as a sign that I'm not the only one who has this concern,
meaning I should maybe bump the priority on getting this
alignment thing fixed.

					-Alex

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH net-next v2 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header
  2021-03-08 13:59     ` Alex Elder
@ 2021-03-08 14:10       ` David Laight
  0 siblings, 0 replies; 14+ messages in thread
From: David Laight @ 2021-03-08 14:10 UTC (permalink / raw)
  To: 'Alex Elder', subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel, kernel test robot

...
> Sort of related, I have been meaning to eliminate the
> pointless __aligned(1) tags on rmnet structures defined
> in <linux/if_rmnet.h>.  It wouldn't hurt to use __packed,
> though I think they're all 4 or 8 bytes naturally anyway.
> Perhaps marking them __aligned(4) would help identify
> potential unaligned accesses?

Don't use __packed (etc) unless the data might be misaligned.
If the architecture doesn't support misaligned memory
accesses then the compiler has to generate code that
does byte accesses and shifts.

__aligned(4) is mostly useful for structures that have
to have an 8-byte field on a 4-byte boundary.
(As happens in the x86 compat32 code.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next v2 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer
  2021-03-08 13:53       ` David Laight
@ 2021-03-08 14:19         ` Alex Elder
  0 siblings, 0 replies; 14+ messages in thread
From: Alex Elder @ 2021-03-08 14:19 UTC (permalink / raw)
  To: David Laight, subashab, stranche, davem, kuba
  Cc: sharathv, bjorn.andersson, evgreen, cpratapa, elder, netdev,
	linux-kernel

On 3/8/21 7:53 AM, David Laight wrote:
> ...
>>>> -	if (!csum_trailer->valid) {
>>>> +	if (!u8_get_bits(csum_trailer->flags, MAP_CSUM_DL_VALID_FMASK)) {
>>>
>>> Is that just an overcomplicated way of saying:
>>> 	if (!(csum_trailer->flags & MAP_CSUM_DL_VALID_FMASK)) {
>>
>> Yes it is.  I defined and used all the field masks in a
>> consistent way, but I do think it will read better the
>> way you suggest.  Bjorn also asked me privately whether
>> GENMASK(15, 15) was just the same as BIT(15) (it is).
>>
>> I will post version 3 of the series, identifying which
>> fields are single bit/Boolean.  For those I will define
>> the value using BIT() and will set/extract using simple
>> AND/OR operators.  I won't use the _FMASK suffix on such
>> fields.
> 
> Even for the checksum offset a simple 'offset << CONSTANT'
> is enough.

I do not want the code to assume the field resides in the
bottom of the register.

> If it is the bottom bits then even that isn't really needed.
> You might want to mask off high bits - but that is an error
> path that needs to have been checked earlier.

Using u32_get_bits() (etc.) is a general-purpose way of
setting and extracting bit fields from registers.  In a
way, it might be overkill here.  On the other hand, we
can expect additional structures to be defined in
<linux/if_rmnet.h>.  If we handle fields in this common
way now, they can be handled the same way for new
structures.

I think your implication is right, that this could be
done possibly more simply without the helper functions.

Let me do the BIT() fix on version 3.  If others echo
your thoughts about not using the u32_get_bits() family
of helper functions, I can address that in version 4.

					-Alex


> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-03-08 14:20 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-06  3:15 [PATCH net-next v2 0/6] net: qualcomm: rmnet: stop using C bit-fields Alex Elder
2021-03-06  3:15 ` [PATCH net-next v2 1/6] net: qualcomm: rmnet: mark trailer field endianness Alex Elder
2021-03-06  3:15 ` [PATCH net-next v2 2/6] net: qualcomm: rmnet: simplify some byte order logic Alex Elder
2021-03-06  3:15 ` [PATCH net-next v2 3/6] net: qualcomm: rmnet: kill RMNET_MAP_GET_*() accessor macros Alex Elder
2021-03-06  3:15 ` [PATCH net-next v2 4/6] net: qualcomm: rmnet: use field masks instead of C bit-fields Alex Elder
2021-03-06  3:15 ` [PATCH net-next v2 5/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum trailer Alex Elder
2021-03-08 10:13   ` David Laight
2021-03-08 13:39     ` Alex Elder
2021-03-08 13:53       ` David Laight
2021-03-08 14:19         ` Alex Elder
2021-03-06  3:15 ` [PATCH net-next v2 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header Alex Elder
2021-03-08 10:18   ` David Laight
2021-03-08 13:59     ` Alex Elder
2021-03-08 14:10       ` David Laight

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).