Re: [PATCH net-next v7 2/3] net: ethernet: rmnet: Support for ingress MAPv5 checksum offload

[Jakub Kicinski]

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@xxxxxxxxxxxxxx>

> @@ -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_) */