Re: [PATCH net-next v7 3/3] net: ethernet: rmnet: Add support for MAPv5 egress packets

[sharathv]

On 2021-05-29 04:41, Jakub Kicinski wrote:
> On Thu, 27 May 2021 14:18:42 +0530 Sharath Chandra Vurukala wrote:
> > Adding support for MAPv5 egress packets.
> >
> > This involves adding the MAPv5 header and setting the 
> > csum_valid_required
> > in the checksum header to request HW compute the checksum.
> >
> > Corresponding stats are incremented based on whether the checksum is
> > computed in software or HW.
> >
> > New stat has been added which represents the count of packets whose
> > checksum is calculated by the HW.
> >
> > Signed-off-by: Sharath Chandra Vurukala <sharathv@xxxxxxxxxxxxxx>
>
> > +static void rmnet_map_v5_checksum_uplink_packet(struct sk_buff *skb,
> > +						struct rmnet_port *port,
> > +						struct net_device *orig_dev)
> > +{
> > +	struct rmnet_priv *priv = netdev_priv(orig_dev);
> > +	struct rmnet_map_v5_csum_header *ul_header;
> > +
> > +	if (!(port->data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5))
> > +		return;
>
> how can we get here if this condition is not met? Looks like defensive
> programming.

Yes we get here only for the MAPv5 case, as you think this is just a 
defensive code.
will remove this in next patch.

> > +	ul_header = skb_push(skb, sizeof(*ul_header));
>
> Are you making sure you can modify head? I only see a check if there is
> enough headroom but not if head is writable (skb_cow_head()).

TSkb_cow_head() changes will be done in the rmnet_map_egress_handler() 
in the next patch.

> > +	memset(ul_header, 0, sizeof(*ul_header));
> > +	ul_header->header_info = 
> > u8_encode_bits(RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD,
> > +						MAPV5_HDRINFO_HDR_TYPE_FMASK);
>
> Is prepending the header required even when packet doesn't need
> checksuming?
>
> > +	if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > +		void *iph = (char *)ul_header + sizeof(*ul_header);
>
> ip_hdr(skb)

> > +		__sum16 *check;
> > +		void *trans;
> > +		u8 proto;
> > +
> > +		if (skb->protocol == htons(ETH_P_IP)) {
> > +			u16 ip_len = ((struct iphdr *)iph)->ihl * 4;
> > +
> > +			proto = ((struct iphdr *)iph)->protocol;
> > +			trans = iph + ip_len;
> > +		} else if (skb->protocol == htons(ETH_P_IPV6)) {
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +			u16 ip_len = sizeof(struct ipv6hdr);
> > +
> > +			proto = ((struct ipv6hdr *)iph)->nexthdr;
> > +			trans = iph + ip_len;
> > +#else
> > +			priv->stats.csum_err_invalid_ip_version++;
> > +			goto sw_csum;
> > +#endif /* CONFIG_IPV6 */
> > +		} else {
> > +			priv->stats.csum_err_invalid_ip_version++;
> > +			goto sw_csum;
> > +		}
> > +
> > +		check = rmnet_map_get_csum_field(proto, trans);
> > +		if (check) {
> > +			skb->ip_summed = CHECKSUM_NONE;
> > +			/* Ask for checksum offloading */
> > +			ul_header->csum_info |= MAPV5_CSUMINFO_VALID_FLAG;
> > +			priv->stats.csum_hw++;
> > +			return;
>
> Please try to keep the success path unindented.

Sure will take care of these comments in next patch.
> > +		}
> > +	}
> > +
> > +sw_csum:
> > +	priv->stats.csum_sw++;
> > +}