From: Alex Elder <hidden> Date: 2021-03-15 13:35:46
The main reason for version 4 of this series is that a bug was
introduced in version 3, and that is fixed.
But a nice note from Vladimir Oltean got me thinking about the
necessity of using accessors defined in <linux/bitfield.h>, and I
concluded there was no need. So this version simplifies things
further, using bitwise AND and OR operators (rather than, e.g.,
u8_get_bits()) to access all values encoded in bit fields.
This version has been tested using IPv4 with checksum offload
enabled and disabled. Traffic over the link included ICMP (ping),
UDP (iperf), and TCP (wget).
Version 3 of this series used BIT() rather than GENMASK() to define
single-bit masks, and bitwise AND operators to access them.
Version 2 fixed bugs in the way the value written into the header
was computed in version 1.
The series was first posted here:
https://lore.kernel.org/netdev/20210304223431.15045-1-elder@linaro.org/
-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 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 | 10 +--
.../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, 69 insertions(+), 89 deletions(-)
--
2.27.0
From: Alex Elder <hidden> Date: 2021-03-15 13:35:46
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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
include/linux/if_rmnet.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Alex Elder <hidden> Date: 2021-03-15 13:35:46
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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
.../ethernet/qualcomm/rmnet/rmnet_map_data.c | 22 ++++++++++---------
1 file changed, 12 insertions(+), 10 deletions(-)
From: Alex Elder <hidden> Date: 2021-03-15 13:35:47
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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
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(-)
@@ -330,7 +330,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,returnNULL;/* Some hardware can send us empty frames. Catch them */-if(ntohs(maph->pkt_len)==0)+if(!maph->pkt_len)returnNULL;skbn=alloc_skb(packet_len+RMNET_MAP_DEAGGR_SPACING,GFP_ATOMIC);
From: Alex Elder <hidden> Date: 2021-03-15 13:35:47
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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
v3: - Use BIT(x) and don't use u8_get_bits() for the checksum valid flag
.../ethernet/qualcomm/rmnet/rmnet_map_data.c | 2 +-
include/linux/if_rmnet.h | 17 +++++++----------
2 files changed, 8 insertions(+), 11 deletions(-)
From: Alex Elder <hidden> Date: 2021-03-15 13:35:47
The actual layout of bits defined in C bit-fields (e.g. int foo : 3)
is implementation-defined. Structures defined in <linux/if_rmnet.h>
address this by specifying all bit-fields twice, to cover two
possible layouts.
I think this pattern is repetitive and noisy, and I find the whole
notion of compiler "bitfield endianness" to be non-intuitive.
Stop using C bit-fields for the command/data flag and the pad length
fields in the rmnet_map structure, and define a single-byte flags
field instead. Define a mask for the single bit "command" flag,
and another mask for the encoded pad length. The content of both
fields can be accessed using a simple bitwise AND operation.
Signed-off-by: Alex Elder <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
v4: - Don't use u8_get_bits() to access the pad length
- Added BUILD_BUG_ON() to ensure field width is adequate
v3: - Use BIT(x) and don't use u8_get_bits() for the command flag
.../ethernet/qualcomm/rmnet/rmnet_handlers.c | 4 ++--
.../ethernet/qualcomm/rmnet/rmnet_map_data.c | 4 +++-
include/linux/if_rmnet.h | 23 ++++++++-----------
3 files changed, 15 insertions(+), 16 deletions(-)
@@ -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=padding&MAP_PAD_LEN_MASK;returnmap_header;}
From: Alex Elder <hidden> Date: 2021-03-15 13:35:47
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 masks to encode or get values within it. The content of
these fields can be accessed using simple bitwise AND and OR
operations on the (host byte order) value of the new structure
member.
Previously rmnet_map_ipv4_ul_csum_header() would update C bit-field
values in host byte order, then forcibly fix their byte order using
a combination of byte swap operations and types.
Instead, just compute the value that needs to go into the new
structure member and save it with a simple byte-order conversion.
Make similar simplifications in rmnet_map_ipv6_ul_csum_header().
Finally, in rmnet_map_checksum_uplink_packet() a set of assignments
zeroes every field in the upload checksum header. Replace that with
a single memset() operation.
Signed-off-by: Alex Elder <redacted>
---
v4: - Don't use u16_get_bits() to access the checksum field offset
v3: - Use BIT(x) and don't use u16_get_bits() for single-bit flags
v2: - Fixed to use u16_encode_bits() instead of be16_encode_bits().
.../ethernet/qualcomm/rmnet/rmnet_map_data.c | 34 ++++++-------------
include/linux/if_rmnet.h | 21 ++++++------
2 files changed, 21 insertions(+), 34 deletions(-)
From: Alexander Duyck <hidden> Date: 2021-03-15 16:03:48
On Mon, Mar 15, 2021 at 6:36 AM Alex Elder [off-list ref] wrote:
quoted hunk
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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
.../ethernet/qualcomm/rmnet/rmnet_map_data.c | 22 ++++++++++---------
1 file changed, 12 insertions(+), 10 deletions(-)
Rather than using skb_transport_header the correct pointer to use is
probably skb_checksum_start. The two are essentially synonymous but
the checksumming code is supposed to use skb_checksum_start.
Alternatively you could look at possibly using skb_network_header_len
as that would be the same value assuming that both headers are the
outer headers. Then you could avoid the extra pointer overhead.
From: Alexander Duyck <hidden> Date: 2021-03-15 16:13:23
On Mon, Mar 15, 2021 at 6:36 AM Alex Elder [off-list ref] wrote:
The main reason for version 4 of this series is that a bug was
introduced in version 3, and that is fixed.
But a nice note from Vladimir Oltean got me thinking about the
necessity of using accessors defined in <linux/bitfield.h>, and I
concluded there was no need. So this version simplifies things
further, using bitwise AND and OR operators (rather than, e.g.,
u8_get_bits()) to access all values encoded in bit fields.
This version has been tested using IPv4 with checksum offload
enabled and disabled. Traffic over the link included ICMP (ping),
UDP (iperf), and TCP (wget).
Version 3 of this series used BIT() rather than GENMASK() to define
single-bit masks, and bitwise AND operators to access them.
Version 2 fixed bugs in the way the value written into the header
was computed in version 1.
The series was first posted here:
https://lore.kernel.org/netdev/20210304223431.15045-1-elder@linaro.org/
-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 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 | 10 +--
.../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, 69 insertions(+), 89 deletions(-)
Other than the minor nit I pointed out in patch 2 the set looks good to me.
Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
From: Alex Elder <hidden> Date: 2021-03-15 17:13:06
On 3/15/21 11:02 AM, Alexander Duyck wrote:
On Mon, Mar 15, 2021 at 6:36 AM Alex Elder [off-list ref] wrote:
quoted
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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
.../ethernet/qualcomm/rmnet/rmnet_map_data.c | 22 ++++++++++---------
1 file changed, 12 insertions(+), 10 deletions(-)
Rather than using skb_transport_header the correct pointer to use is
probably skb_checksum_start. The two are essentially synonymous but
the checksumming code is supposed to use skb_checksum_start.
That's a great suggestion. I was mimicking the existing
code but it would be much better to use that.
Alternatively you could look at possibly using skb_network_header_len
as that would be the same value assuming that both headers are the
outer headers. Then you could avoid the extra pointer overhead.
Actually, I think this is better still, because the purpose
here is to tell the hardware where to start checksumming.
I.e., it's unrelated to the SKB, and hides the calculation.
Thank you. I'll put together version 5, incorporating your
suggestion.
-Alex