From: Alex Elder <hidden> Date: 2021-03-06 03:17:18
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
From: Alex Elder <hidden> Date: 2021-03-06 03:17:19
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-06 03:17:19
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-06 03:17:19
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>
---
.../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-06 03:17:19
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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
.../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(-)
@@ -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);returnmap_header;}
From: Alex Elder <hidden> Date: 2021-03-06 03:17:19
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 <redacted>
Reported-by: kernel test robot <redacted>
---
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: Alex Elder <hidden> Date: 2021-03-06 03:17:19
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: David Laight <hidden> Date: 2021-03-08 10:14:52
From: Alex Elder
quoted hunk
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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
.../ethernet/qualcomm/rmnet/rmnet_map_data.c | 2 +-
include/linux/if_rmnet.h | 17 +++++++----------
2 files changed, 8 insertions(+), 11 deletions(-)
@@ -365,7 +365,7 @@ int rmnet_map_checksum_downlink_packet(struct sk_buff *skb, u16 len)csum_trailer=(structrmnet_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)
From: David Laight <hidden> Date: 2021-03-08 10:19:44
From: Alex Elder
quoted hunk
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 <redacted>
Reported-by: kernel test robot <redacted>
---
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: Alex Elder <hidden> Date: 2021-03-08 13:40:27
On 3/8/21 4:13 AM, David Laight wrote:
From: Alex Elder
quoted
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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
.../ethernet/qualcomm/rmnet/rmnet_map_data.c | 2 +-
include/linux/if_rmnet.h | 17 +++++++----------
2 files changed, 8 insertions(+), 11 deletions(-)
@@ -365,7 +365,7 @@ int rmnet_map_checksum_downlink_packet(struct sk_buff *skb, u16 len)csum_trailer=(structrmnet_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)
From: David Laight <hidden> Date: 2021-03-08 13:56:41
...
quoted
quoted
- 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)
From: Alex Elder <hidden> Date: 2021-03-08 14:00:28
On 3/8/21 4:18 AM, David Laight wrote:
From: Alex Elder
quoted
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 <redacted>
Reported-by: kernel test robot <redacted>
---
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(-)
@@ -198,23 +198,19 @@ rmnet_map_ipv4_ul_csum_header(void *iphdr,structrmnet_map_ul_csum_header*ul_header,structsk_buff*skb){-__be16*hdr=(__be16*)ul_header;structiphdr*ip4h=iphdr;u16offset;+u16val;offset=skb_transport_header(skb)-(unsignedchar*)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((__forceu16)*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)
From: David Laight <hidden> Date: 2021-03-08 14:11:51
...
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)
From: Alex Elder <hidden> Date: 2021-03-08 14:20:30
On 3/8/21 7:53 AM, David Laight wrote:
...
quoted
quoted
quoted
- 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)