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

[sharathv@codeaurora.org]

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@codeaurora.org>
> 
>> +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++;
>> +}