Re: [PATCH net-next 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header
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 --- .../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 " -#endif + __be16
Re: [PATCH net-next 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header
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 > --- > .../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 " > -#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
[PATCH net-next 6/6] net: qualcomm: rmnet: don't use C bit-fields in rmnet checksum header
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 --- .../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 " -#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