Re: [PATCH v3 net-next 1/2] ravb: Add Rx checksum offload support for GbEth

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/1/24 10:45 PM, Biju Das wrote:

> TOE has hardware support for calculating IP header and TCP/UDP/ICMP
> checksum for both IPv4 and IPv6.
> 
> Add Rx checksum offload supported by TOE for IPv4 and TCP/UDP protocols.
> 
> For Rx, the 4-byte result of checksum calculation is attached to the
> Ethernet frames.First 2-bytes is result of IPv4 header checksum and next
> 2-bytes is TCP/UDP/ICMP checksum.
> 
> If a frame does not have checksum error, 0x0000 is attached as checksum
> calculation result. For unsupported frames 0xFFFF is attached as checksum
> calculation result. In case of an IPv6 packet, IPv4 checksum is always set
> to 0xFFFF.
> 
> We can test this functionality by the below commands
> 
> ethtool -K eth0 rx on --> to turn on Rx checksum offload
> ethtool -K eth0 rx off --> to turn off Rx checksum offload
> 
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index e0f8276cffed..a7fe9d8b6b41 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -206,6 +206,7 @@ enum ravb_reg {
>  	RFCR	= 0x0760,
>  	MAFCR	= 0x0778,

   Would be good to add this comment here:

	/* TOE registers */

>  	CSR0    = 0x0800,	/* RZ/G2L only */
> +	CSR2    = 0x0808,	/* RZ/G2L only */
>  };
>  
>  
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 0e3731f50fc2..c4dc6ec54287 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -522,6 +522,23 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>  	return -ENOMEM;
>  }
>  
> +static void ravb_csum_init_gbeth(struct net_device *ndev)
> +{
> +	if (!(ndev->features & NETIF_F_RXCSUM))
> +		goto done;
> +
> +	ravb_write(ndev, 0, CSR0);
> +	if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) {
> +		netdev_err(ndev, "Timeout enabling hardware checksum\n");
> +		ndev->features &= ~NETIF_F_RXCSUM;
> +	} else {
> +		ravb_write(ndev, CSR2_ALL, CSR2);

   Does it make sense to set the IPv6 specific bits if we don't yet
support IPv6 checksumming anyways?

> +	}
> +
> +done:
> +	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
> +}
> +
>  static void ravb_emac_init_gbeth(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
[...]
> @@ -734,6 +752,31 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
>  	}
>  }
>  
> +static void ravb_rx_csum_gbeth(struct sk_buff *skb)
> +{
> +	__wsum csum_ip_hdr, csum_proto;
> +	u8 *hw_csum;
> +
> +	/* The hardware checksum status is contained in sizeof(__sum16) * 2 = 4
> +	 * bytes appended to packet data. First 2 bytes is ip header csum and
> +	 * last 2 bytes is protocol csum.

   Hm, maybe spell out csum as checksum?

> +	 */
> +	if (unlikely(skb->len < sizeof(__sum16) * 2))
> +		return;
> +
> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
> +	csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
> +
> +	hw_csum -= sizeof(__sum16);
> +	csum_ip_hdr = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
> +	skb_trim(skb, skb->len - 2 * sizeof(__sum16));
> +
> +	/* TODO: IPV6 Rx csum */
> +	if (skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr && !csum_proto)
> +		/* Hardware validated our checksum */
> +		skb->ip_summed = CHECKSUM_UNNECESSARY;

   I think you need {} because of the comment (but would be good without it
as well)...

[...]
> @@ -2334,11 +2381,49 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  }
>  
> +static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum ravb_reg reg,
> +				     u32 val, u32 mask)

   I'd suggest to mimic ravb_wait() with the the mask param followed by
the val[ue] param...

> +{
> +	u32 csr0;
> +	int ret;
> +
> +	csr0 = ravb_read(ndev, CSR0);

   Hm... I think we already know CSR0's value as ravb_csum_init_gbeth()
sets it to (CSR0_TPE | CSR0_RPE) on exit...

> +	ravb_write(ndev, csr0 & ~mask, CSR0);
> +	ret = ravb_wait(ndev, CSR0, mask, 0);
> +	if (!ret)
> +		ravb_write(ndev, val, reg);
> +
> +	ravb_write(ndev, csr0, CSR0);
> +
> +	return ret;
> +}
> +
>  static int ravb_set_features_gbeth(struct net_device *ndev,
>  				   netdev_features_t features)
>  {
> -	/* Place holder */
> -	return 0;
> +	netdev_features_t changed = ndev->features ^ features;
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	unsigned long flags;
> +	int ret = 0;
> +	u32 val;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	if (changed & NETIF_F_RXCSUM) {
> +		if (features & NETIF_F_RXCSUM)
> +			val = CSR2_ALL;

   Again, does it make sense to set the IPv6 bits in CSR2?

> +		else
> +			val = 0;
> +
> +		ret = ravb_endisable_csum_gbeth(ndev, CSR2, val, CSR0_RPE);
> +		if (ret)
> +			goto done;
> +	}
> +
> +	ndev->features = features;
> +done:
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return ret;
>  }
>  
>  static int ravb_set_features_rcar(struct net_device *ndev,
[...]

MBR, Sergey




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux