Message ID | 1622105322-2975-1-git-send-email-sharathv@codeaurora.org |
---|---|
Headers | show |
Series | net: qualcomm: rmnet: Enable Mapv5 | expand |
On 2021-05-27 02:48, Sharath Chandra Vurukala wrote: > This series introduces the MAPv5 packet format. > > Patch 0 documents the MAPv4/v5. > Patch 1 introduces the MAPv5 and the Inline checksum offload for > RX/Ingress. > Patch 2 introduces the MAPv5 and the Inline checksum offload for > TX/Egress. > > A new checksum header format is used as part of MAPv5.For RX > checksum > offload, > the checksum is verified by the HW and the validity is marked in the > checksum > header of MAPv5. For TX, the required metadata is filled up so > hardware > can > compute the checksum. > > v1->v2: > - Fixed the compilation errors, warnings reported by kernel test > robot. > - Checksum header definition is expanded to support big, little > endian > formats as mentioned by Jakub. > > v2->v3: > - Fixed compilation errors reported by kernel bot for big endian > flavor. > > v3->v4: > - Made changes to use masks instead of C bit-fields as suggested by > Jakub/Alex. > > v4->v5: > - Corrected checkpatch errors and warnings reported by patchwork. > > v5->v6: > - Corrected the bug identified by Alex and incorporated all his > comments. > > v6->v7: > - Removed duplicate inclusion of linux/bitfield.h in > rmnet_map_data.c > > Sharath Chandra Vurukala (3): > docs: networking: Add documentation for MAPv5 > net: ethernet: rmnet: Support for ingress MAPv5 checksum offload > net: ethernet: rmnet: Add support for MAPv5 egress packets > > .../device_drivers/cellular/qualcomm/rmnet.rst | 126 > +++++++++++++++-- > drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h | 4 +- > .../net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 34 +++-- > drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 11 +- > .../net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 151 > +++++++++++++++++++-- > drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c | 3 +- > include/linux/if_rmnet.h | 26 +++- > include/uapi/linux/if_link.h | 2 + > 8 files changed, 320 insertions(+), 37 deletions(-) For the series- Reviewed-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
On Thu, 27 May 2021 14:18:41 +0530 Sharath Chandra Vurukala wrote: > Adding support for processing of MAPv5 downlink packets. > It involves parsing the Mapv5 packet and checking the csum header > to know whether the hardware has validated the checksum and is > valid or not. > > Based on the checksum valid bit the corresponding stats are > incremented and skb->ip_summed is marked either CHECKSUM_UNNECESSARY > or left as CHEKSUM_NONE to let network stack revalidate the checksum > and update the respective snmp stats. > > Current MAPV1 header has been modified, the reserved field in the > Mapv1 header is now used for next header indication. > > Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org> > @@ -300,8 +301,11 @@ struct rmnet_map_header *rmnet_map_add_map_header(struct sk_buff *skb, > struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb, > struct rmnet_port *port) > { > + struct rmnet_map_v5_csum_header *next_hdr = NULL; > + void *data = skb->data; > struct rmnet_map_header *maph; Please maintain reverse xmas tree ordering > struct sk_buff *skbn; > + u8 nexthdr_type; > u32 packet_len; > > if (skb->len == 0) > @@ -310,8 +314,18 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb, > maph = (struct rmnet_map_header *)skb->data; > packet_len = ntohs(maph->pkt_len) + sizeof(*maph); > > - if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) > + if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) { > packet_len += sizeof(struct rmnet_map_dl_csum_trailer); > + } else if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) { > + if (!(maph->flags & MAP_CMD_FLAG)) { > + packet_len += sizeof(*next_hdr); > + if (maph->flags & MAP_NEXT_HEADER_FLAG) > + next_hdr = (data + sizeof(*maph)); brackets unnecessary > + else > + /* Mapv5 data pkt without csum hdr is invalid */ > + return NULL; > + } > + } > > if (((int)skb->len - (int)packet_len) < 0) > return NULL; > @@ -320,6 +334,13 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb, > if (!maph->pkt_len) > return NULL; > > + if (next_hdr) { > + nexthdr_type = u8_get_bits(next_hdr->header_info, > + MAPV5_HDRINFO_HDR_TYPE_FMASK); > + if (nexthdr_type != RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD) > + return NULL; > + } > + > skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC); > if (!skbn) > return NULL; > @@ -414,3 +435,37 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb, > > priv->stats.csum_sw++; > } > + > +/* Process a MAPv5 packet header */ > +int rmnet_map_process_next_hdr_packet(struct sk_buff *skb, > + u16 len) > +{ > + struct rmnet_priv *priv = netdev_priv(skb->dev); > + struct rmnet_map_v5_csum_header *next_hdr; > + u8 nexthdr_type; > + int rc = 0; rc is not meaningfully used > + next_hdr = (struct rmnet_map_v5_csum_header *)(skb->data + > + sizeof(struct rmnet_map_header)); > + > + nexthdr_type = u8_get_bits(next_hdr->header_info, > + MAPV5_HDRINFO_HDR_TYPE_FMASK); > + > + if (nexthdr_type == RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD) { > + if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) { > + priv->stats.csum_sw++; > + } else if (next_hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG) { > + priv->stats.csum_ok++; > + skb->ip_summed = CHECKSUM_UNNECESSARY; > + } else { > + priv->stats.csum_valid_unset++; > + } > + > + /* Pull csum v5 header */ > + skb_pull(skb, sizeof(*next_hdr)); > + } else { > + return -EINVAL; flip condition, return early > + } > + > + return rc; > +} > diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h > index 4efb537..8502ccc 100644 > --- a/include/linux/if_rmnet.h > +++ b/include/linux/if_rmnet.h > @@ -1,5 +1,5 @@ > /* SPDX-License-Identifier: GPL-2.0-only > - * Copyright (c) 2013-2019, The Linux Foundation. All rights reserved. > + * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved. > */ > > #ifndef _LINUX_IF_RMNET_H_ > @@ -14,8 +14,10 @@ struct rmnet_map_header { > /* rmnet_map_header flags field: > * PAD_LEN: number of pad bytes following packet data > * CMD: 1 = packet contains a MAP command; 0 = packet contains data > + * NEXT_HEADER 1 = packet contains V5 CSUM header 0 = no V5 CSUM header Colon missing? > */ > #define MAP_PAD_LEN_MASK GENMASK(5, 0) > +#define MAP_NEXT_HEADER_FLAG BIT(6) > #define MAP_CMD_FLAG BIT(7) > > struct rmnet_map_dl_csum_trailer { > @@ -45,4 +47,26 @@ struct rmnet_map_ul_csum_header { > #define MAP_CSUM_UL_UDP_FLAG BIT(14) > #define MAP_CSUM_UL_ENABLED_FLAG BIT(15) > > +/* MAP CSUM headers */ > +struct rmnet_map_v5_csum_header { > + u8 header_info; > + u8 csum_info; > + __be16 reserved; > +} __aligned(1); __aligned() seems rather pointless here but ok. > +/* v5 header_info field > + * NEXT_HEADER: Represents whether there is any other header double space > + * HEADER TYPE: represents the type of this header On previous line you used _ for a space, and started from capital letter. Please be consistent. > + * > + * csum_info field > + * CSUM_VALID_OR_REQ: > + * 1 = for UL, checksum computation is requested. > + * 1 = for DL, validated the checksum and has found it valid > + */ > + > +#define MAPV5_HDRINFO_NXT_HDR_FLAG BIT(0) > +#define MAPV5_HDRINFO_HDR_TYPE_FMASK GENMASK(7, 1) > +#define MAPV5_CSUMINFO_VALID_FLAG BIT(7) > + > +#define RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD 2 > #endif /* !(_LINUX_IF_RMNET_H_) */
On 5/28/21 5:58 PM, Jakub Kicinski wrote: > On Thu, 27 May 2021 14:18:41 +0530 Sharath Chandra Vurukala wrote: >> Adding support for processing of MAPv5 downlink packets. >> It involves parsing the Mapv5 packet and checking the csum header >> to know whether the hardware has validated the checksum and is >> valid or not. Nice review Jakub. I will wait for version 8 and will review that. -Alex >> >> Based on the checksum valid bit the corresponding stats are >> incremented and skb->ip_summed is marked either CHECKSUM_UNNECESSARY >> or left as CHEKSUM_NONE to let network stack revalidate the checksum >> and update the respective snmp stats. >> >> Current MAPV1 header has been modified, the reserved field in the >> Mapv1 header is now used for next header indication. >> >> Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org> > >> @@ -300,8 +301,11 @@ struct rmnet_map_header *rmnet_map_add_map_header(struct sk_buff *skb, >> struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb, >> struct rmnet_port *port) >> { >> + struct rmnet_map_v5_csum_header *next_hdr = NULL; >> + void *data = skb->data; >> struct rmnet_map_header *maph; > > Please maintain reverse xmas tree ordering > >> struct sk_buff *skbn; >> + u8 nexthdr_type; >> u32 packet_len; >> >> if (skb->len == 0) >> @@ -310,8 +314,18 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb, >> maph = (struct rmnet_map_header *)skb->data; >> packet_len = ntohs(maph->pkt_len) + sizeof(*maph); >> >> - if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) >> + if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) { >> packet_len += sizeof(struct rmnet_map_dl_csum_trailer); >> + } else if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) { >> + if (!(maph->flags & MAP_CMD_FLAG)) { >> + packet_len += sizeof(*next_hdr); >> + if (maph->flags & MAP_NEXT_HEADER_FLAG) >> + next_hdr = (data + sizeof(*maph)); > > brackets unnecessary > >> + else >> + /* Mapv5 data pkt without csum hdr is invalid */ >> + return NULL; >> + } >> + } >> >> if (((int)skb->len - (int)packet_len) < 0) >> return NULL; >> @@ -320,6 +334,13 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb, >> if (!maph->pkt_len) >> return NULL; >> >> + if (next_hdr) { >> + nexthdr_type = u8_get_bits(next_hdr->header_info, >> + MAPV5_HDRINFO_HDR_TYPE_FMASK); >> + if (nexthdr_type != RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD) >> + return NULL; >> + } >> + >> skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC); >> if (!skbn) >> return NULL; >> @@ -414,3 +435,37 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb, >> >> priv->stats.csum_sw++; >> } >> + >> +/* Process a MAPv5 packet header */ >> +int rmnet_map_process_next_hdr_packet(struct sk_buff *skb, >> + u16 len) >> +{ >> + struct rmnet_priv *priv = netdev_priv(skb->dev); >> + struct rmnet_map_v5_csum_header *next_hdr; >> + u8 nexthdr_type; >> + int rc = 0; > > rc is not meaningfully used > >> + next_hdr = (struct rmnet_map_v5_csum_header *)(skb->data + >> + sizeof(struct rmnet_map_header)); >> + >> + nexthdr_type = u8_get_bits(next_hdr->header_info, >> + MAPV5_HDRINFO_HDR_TYPE_FMASK); >> + >> + if (nexthdr_type == RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD) { >> + if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) { >> + priv->stats.csum_sw++; >> + } else if (next_hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG) { >> + priv->stats.csum_ok++; >> + skb->ip_summed = CHECKSUM_UNNECESSARY; >> + } else { >> + priv->stats.csum_valid_unset++; >> + } >> + >> + /* Pull csum v5 header */ >> + skb_pull(skb, sizeof(*next_hdr)); >> + } else { >> + return -EINVAL; > > flip condition, return early > >> + } >> + >> + return rc; >> +} >> diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h >> index 4efb537..8502ccc 100644 >> --- a/include/linux/if_rmnet.h >> +++ b/include/linux/if_rmnet.h >> @@ -1,5 +1,5 @@ >> /* SPDX-License-Identifier: GPL-2.0-only >> - * Copyright (c) 2013-2019, The Linux Foundation. All rights reserved. >> + * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved. >> */ >> >> #ifndef _LINUX_IF_RMNET_H_ >> @@ -14,8 +14,10 @@ struct rmnet_map_header { >> /* rmnet_map_header flags field: >> * PAD_LEN: number of pad bytes following packet data >> * CMD: 1 = packet contains a MAP command; 0 = packet contains data >> + * NEXT_HEADER 1 = packet contains V5 CSUM header 0 = no V5 CSUM header > > Colon missing? > >> */ >> #define MAP_PAD_LEN_MASK GENMASK(5, 0) >> +#define MAP_NEXT_HEADER_FLAG BIT(6) >> #define MAP_CMD_FLAG BIT(7) >> >> struct rmnet_map_dl_csum_trailer { >> @@ -45,4 +47,26 @@ struct rmnet_map_ul_csum_header { >> #define MAP_CSUM_UL_UDP_FLAG BIT(14) >> #define MAP_CSUM_UL_ENABLED_FLAG BIT(15) >> >> +/* MAP CSUM headers */ >> +struct rmnet_map_v5_csum_header { >> + u8 header_info; >> + u8 csum_info; >> + __be16 reserved; >> +} __aligned(1); > > __aligned() seems rather pointless here but ok. > >> +/* v5 header_info field >> + * NEXT_HEADER: Represents whether there is any other header > > double space > >> + * HEADER TYPE: represents the type of this header > > On previous line you used _ for a space, and started from capital > letter. Please be consistent. > >> + * >> + * csum_info field >> + * CSUM_VALID_OR_REQ: >> + * 1 = for UL, checksum computation is requested. >> + * 1 = for DL, validated the checksum and has found it valid >> + */ >> + >> +#define MAPV5_HDRINFO_NXT_HDR_FLAG BIT(0) >> +#define MAPV5_HDRINFO_HDR_TYPE_FMASK GENMASK(7, 1) >> +#define MAPV5_CSUMINFO_VALID_FLAG BIT(7) >> + >> +#define RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD 2 >> #endif /* !(_LINUX_IF_RMNET_H_) */
On 2021-05-29 04:28, Jakub Kicinski wrote: > On Thu, 27 May 2021 14:18:41 +0530 Sharath Chandra Vurukala wrote: >> Adding support for processing of MAPv5 downlink packets. >> It involves parsing the Mapv5 packet and checking the csum header >> to know whether the hardware has validated the checksum and is >> valid or not. >> >> Based on the checksum valid bit the corresponding stats are >> incremented and skb->ip_summed is marked either CHECKSUM_UNNECESSARY >> or left as CHEKSUM_NONE to let network stack revalidate the checksum >> and update the respective snmp stats. >> >> Current MAPV1 header has been modified, the reserved field in the >> Mapv1 header is now used for next header indication. >> >> Signed-off-by: Sharath Chandra Vurukala <sharathv@codeaurora.org> > >> @@ -300,8 +301,11 @@ struct rmnet_map_header >> *rmnet_map_add_map_header(struct sk_buff *skb, >> struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb, >> struct rmnet_port *port) >> { >> + struct rmnet_map_v5_csum_header *next_hdr = NULL; >> + void *data = skb->data; >> struct rmnet_map_header *maph; > > Please maintain reverse xmas tree ordering Thanks Jakub for the review, I will address all the comments that you gave in the next subsequent patch. > >> struct sk_buff *skbn; >> + u8 nexthdr_type; >> u32 packet_len; >> >> if (skb->len == 0) >> @@ -310,8 +314,18 @@ struct sk_buff *rmnet_map_deaggregate(struct >> sk_buff *skb, >> maph = (struct rmnet_map_header *)skb->data; >> packet_len = ntohs(maph->pkt_len) + sizeof(*maph); >> >> - if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) >> + if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) { >> packet_len += sizeof(struct rmnet_map_dl_csum_trailer); >> + } else if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) { >> + if (!(maph->flags & MAP_CMD_FLAG)) { >> + packet_len += sizeof(*next_hdr); >> + if (maph->flags & MAP_NEXT_HEADER_FLAG) >> + next_hdr = (data + sizeof(*maph)); > > brackets unnecessary > Will take care in next patch. >> + else >> + /* Mapv5 data pkt without csum hdr is invalid */ >> + return NULL; >> + } >> + } >> >> if (((int)skb->len - (int)packet_len) < 0) >> return NULL; >> @@ -320,6 +334,13 @@ struct sk_buff *rmnet_map_deaggregate(struct >> sk_buff *skb, >> if (!maph->pkt_len) >> return NULL; >> >> + if (next_hdr) { >> + nexthdr_type = u8_get_bits(next_hdr->header_info, >> + MAPV5_HDRINFO_HDR_TYPE_FMASK); >> + if (nexthdr_type != RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD) >> + return NULL; >> + } >> + >> skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC); >> if (!skbn) >> return NULL; >> @@ -414,3 +435,37 @@ void rmnet_map_checksum_uplink_packet(struct >> sk_buff *skb, >> >> priv->stats.csum_sw++; >> } >> + >> +/* Process a MAPv5 packet header */ >> +int rmnet_map_process_next_hdr_packet(struct sk_buff *skb, >> + u16 len) >> +{ >> + struct rmnet_priv *priv = netdev_priv(skb->dev); >> + struct rmnet_map_v5_csum_header *next_hdr; >> + u8 nexthdr_type; >> + int rc = 0; > > rc is not meaningfully used > >> + next_hdr = (struct rmnet_map_v5_csum_header *)(skb->data + >> + sizeof(struct rmnet_map_header)); >> + >> + nexthdr_type = u8_get_bits(next_hdr->header_info, >> + MAPV5_HDRINFO_HDR_TYPE_FMASK); >> + >> + if (nexthdr_type == RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD) { >> + if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) { >> + priv->stats.csum_sw++; >> + } else if (next_hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG) { >> + priv->stats.csum_ok++; >> + skb->ip_summed = CHECKSUM_UNNECESSARY; >> + } else { >> + priv->stats.csum_valid_unset++; >> + } >> + >> + /* Pull csum v5 header */ >> + skb_pull(skb, sizeof(*next_hdr)); >> + } else { >> + return -EINVAL; > > flip condition, return early > Sure will take care in next patch. >> + } >> + >> + return rc; >> +} >> diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h >> index 4efb537..8502ccc 100644 >> --- a/include/linux/if_rmnet.h >> +++ b/include/linux/if_rmnet.h >> @@ -1,5 +1,5 @@ >> /* SPDX-License-Identifier: GPL-2.0-only >> - * Copyright (c) 2013-2019, The Linux Foundation. All rights >> reserved. >> + * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights >> reserved. >> */ >> >> #ifndef _LINUX_IF_RMNET_H_ >> @@ -14,8 +14,10 @@ struct rmnet_map_header { >> /* rmnet_map_header flags field: >> * PAD_LEN: number of pad bytes following packet data >> * CMD: 1 = packet contains a MAP command; 0 = packet contains data >> + * NEXT_HEADER 1 = packet contains V5 CSUM header 0 = no V5 CSUM >> header > > Colon missing? > >> */ >> #define MAP_PAD_LEN_MASK GENMASK(5, 0) >> +#define MAP_NEXT_HEADER_FLAG BIT(6) >> #define MAP_CMD_FLAG BIT(7) >> >> struct rmnet_map_dl_csum_trailer { >> @@ -45,4 +47,26 @@ struct rmnet_map_ul_csum_header { >> #define MAP_CSUM_UL_UDP_FLAG BIT(14) >> #define MAP_CSUM_UL_ENABLED_FLAG BIT(15) >> >> +/* MAP CSUM headers */ >> +struct rmnet_map_v5_csum_header { >> + u8 header_info; >> + u8 csum_info; >> + __be16 reserved; >> +} __aligned(1); > > __aligned() seems rather pointless here but ok. > >> +/* v5 header_info field >> + * NEXT_HEADER: Represents whether there is any other header > > double space > >> + * HEADER TYPE: represents the type of this header > > On previous line you used _ for a space, and started from capital > letter. Please be consistent. > Sure, will take care in next patch. >> + * >> + * csum_info field >> + * CSUM_VALID_OR_REQ: >> + * 1 = for UL, checksum computation is requested. >> + * 1 = for DL, validated the checksum and has found it valid >> + */ >> + >> +#define MAPV5_HDRINFO_NXT_HDR_FLAG BIT(0) >> +#define MAPV5_HDRINFO_HDR_TYPE_FMASK GENMASK(7, 1) >> +#define MAPV5_CSUMINFO_VALID_FLAG BIT(7) >> + >> +#define RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD 2 >> #endif /* !(_LINUX_IF_RMNET_H_) */