[v1] Extend GbEth checksum offload support to VLAN/IPv6 packets

[net-next PATCH 11/11] net: ravb: Add VLAN checksum support

Posted by Paul Barker 1 month, 4 weeks ago

From: Paul Barker <paul.barker.ct@bp.renesas.com>

The GbEth IP supports offloading checksum calculation for VLAN-tagged
packets, provided that the EtherType is 0x8100 and only one VLAN tag is
present.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 27 +++++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 2cb6c4ef1330..bfa834afc03a 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1056,6 +1056,7 @@ struct ravb_hw_info {
 	size_t gstrings_size;
 	netdev_features_t net_hw_features;
 	netdev_features_t net_features;
+	netdev_features_t vlan_features;
 	int stats_len;
 	u32 tccr_mask;
 	u32 tx_max_frame_size;
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 832132d44fb4..eb7499d42a9b 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 
 static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
 {
-	/* TODO: Need to add support for VLAN tag 802.1Q */
-	if (skb_vlan_tag_present(skb))
+	u16 net_protocol = ntohs(skb->protocol);
+
+	/* GbEth IP can calculate the checksum if:
+	 * - there are zero or one VLAN headers with TPID=0x8100
+	 * - the network protocol is IPv4 or IPv6
+	 * - the transport protocol is TCP, UDP or ICMP
+	 * - the packet is not fragmented
+	 */
+
+	if (skb_vlan_tag_present(skb) &&
+	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
 		return false;
 
-	switch (ntohs(skb->protocol)) {
+	if (net_protocol == ETH_P_8021Q) {
+		struct vlan_hdr vhdr, *vh;
+
+		vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);
+		if (!vh)
+			return false;
+
+		net_protocol = ntohs(vh->h_vlan_encapsulated_proto);
+	}
+
+	switch (net_protocol) {
 	case ETH_P_IP:
 		switch (ip_hdr(skb)->protocol) {
 		case IPPROTO_TCP:
@@ -2772,6 +2791,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.gstrings_size = sizeof(ravb_gstrings_stats_gbeth),
 	.net_hw_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
 	.net_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
+	.vlan_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
 	.tccr_mask = TCCR_TSRQ0,
 	.tx_max_frame_size = 1522,
@@ -2914,6 +2934,7 @@ static int ravb_probe(struct platform_device *pdev)
 
 	ndev->features = info->net_features;
 	ndev->hw_features = info->net_hw_features;
+	ndev->vlan_features = info->vlan_features;
 
 	error = reset_control_deassert(rstc);
 	if (error)
-- 
2.43.0

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

Posted by kernel test robot 1 month, 3 weeks ago

Hi Paul,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Barker/net-ravb-Factor-out-checksum-offload-enable-bits/20241001-001351
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240930160845.8520-12-paul%40pbarker.dev
patch subject: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support
config: arc-randconfig-r123-20241001 (https://download.01.org/0day-ci/archive/20241002/202410020057.Z9KJHqVt-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20241002/202410020057.Z9KJHqVt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410020057.Z9KJHqVt-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/renesas/ravb_main.c:2076:17: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/renesas/ravb_main.c: note: in included file (through include/linux/mutex.h, include/linux/notifier.h, include/linux/clk.h):
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true

vim +2076 drivers/net/ethernet/renesas/ravb_main.c

  2063	
  2064	static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
  2065	{
  2066		u16 net_protocol = ntohs(skb->protocol);
  2067	
  2068		/* GbEth IP can calculate the checksum if:
  2069		 * - there are zero or one VLAN headers with TPID=0x8100
  2070		 * - the network protocol is IPv4 or IPv6
  2071		 * - the transport protocol is TCP, UDP or ICMP
  2072		 * - the packet is not fragmented
  2073		 */
  2074	
  2075		if (skb_vlan_tag_present(skb) &&
> 2076		    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
  2077			return false;
  2078	
  2079		if (net_protocol == ETH_P_8021Q) {
  2080			struct vlan_hdr vhdr, *vh;
  2081	
  2082			vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);
  2083			if (!vh)
  2084				return false;
  2085	
  2086			net_protocol = ntohs(vh->h_vlan_encapsulated_proto);
  2087		}
  2088	
  2089		switch (net_protocol) {
  2090		case ETH_P_IP:
  2091			switch (ip_hdr(skb)->protocol) {
  2092			case IPPROTO_TCP:
  2093			case IPPROTO_UDP:
  2094			case IPPROTO_ICMP:
  2095				return true;
  2096			}
  2097			break;
  2098	
  2099		case ETH_P_IPV6:
  2100			switch (ipv6_hdr(skb)->nexthdr) {
  2101			case IPPROTO_TCP:
  2102			case IPPROTO_UDP:
  2103			case IPPROTO_ICMPV6:
  2104				return true;
  2105			}
  2106		}
  2107	
  2108		return false;
  2109	}
  2110	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

Posted by Sergey Shtylyov 1 month, 4 weeks ago

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> The GbEth IP supports offloading checksum calculation for VLAN-tagged
> packets, provided that the EtherType is 0x8100 and only one VLAN tag is
> present.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 832132d44fb4..eb7499d42a9b 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  
>  static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
>  {
> -	/* TODO: Need to add support for VLAN tag 802.1Q */
> -	if (skb_vlan_tag_present(skb))
> +	u16 net_protocol = ntohs(skb->protocol);
> +
> +	/* GbEth IP can calculate the checksum if:
> +	 * - there are zero or one VLAN headers with TPID=0x8100
> +	 * - the network protocol is IPv4 or IPv6
> +	 * - the transport protocol is TCP, UDP or ICMP
> +	 * - the packet is not fragmented
> +	 */
> +
> +	if (skb_vlan_tag_present(skb) &&
> +	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))

   Not sure I understand this check... Maybe s/==/!=/?

>  		return false;
>  
> -	switch (ntohs(skb->protocol)) {
> +	if (net_protocol == ETH_P_8021Q) {
> +		struct vlan_hdr vhdr, *vh;
> +
> +		vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);

   Hm, I thought the VLAN header starts at ETH_HLEN - 2, not at ETH_HLEN...

[...]

MBR, Sergey

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

Posted by Paul Barker 1 month, 3 weeks ago

On 30/09/2024 21:36, Sergey Shtylyov wrote:
> On 9/30/24 19:08, Paul Barker wrote:
> 
>> From: Paul Barker <paul.barker.ct@bp.renesas.com>
>>
>> The GbEth IP supports offloading checksum calculation for VLAN-tagged
>> packets, provided that the EtherType is 0x8100 and only one VLAN tag is
>> present.
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 832132d44fb4..eb7499d42a9b 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>>  
>>  static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
>>  {
>> -	/* TODO: Need to add support for VLAN tag 802.1Q */
>> -	if (skb_vlan_tag_present(skb))
>> +	u16 net_protocol = ntohs(skb->protocol);
>> +
>> +	/* GbEth IP can calculate the checksum if:
>> +	 * - there are zero or one VLAN headers with TPID=0x8100
>> +	 * - the network protocol is IPv4 or IPv6
>> +	 * - the transport protocol is TCP, UDP or ICMP
>> +	 * - the packet is not fragmented
>> +	 */
>> +
>> +	if (skb_vlan_tag_present(skb) &&
>> +	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
> 
>    Not sure I understand this check... Maybe s/==/!=/?

So, after a bit more investigation, I think this was based on a faulty
understanding. I can't find any clear documentation of this so I've gone
wandering through the code.

In vlan_dev_init() in net/8021q/vlan_dev.c, there is a check for
vlan_hw_offload_capable() on the underlying network device. If this is
false (as it will be for the GbEth IP), a set of header_ops is selected
which inserts the vlan tag into the skb data. So, we can ignore
skb->vlan_proto as skb->protocol will always be set to the VLAN protocol
for VLAN tagged packets.

The conclusion is that we can drop this if condition completely and just
keep the following if (net_protocol == ETH_P_8021Q) condition.

Will fix this in v2...

Thanks,

-- 
Paul Barker

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

Posted by Simon Horman 1 month, 4 weeks ago

On Mon, Sep 30, 2024 at 11:36:40PM +0300, Sergey Shtylyov wrote:
> On 9/30/24 19:08, Paul Barker wrote:
> 
> > From: Paul Barker <paul.barker.ct@bp.renesas.com>
> > 
> > The GbEth IP supports offloading checksum calculation for VLAN-tagged
> > packets, provided that the EtherType is 0x8100 and only one VLAN tag is
> > present.
> > 
> > Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> [...]
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 832132d44fb4..eb7499d42a9b 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
> >  
> >  static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
> >  {
> > -	/* TODO: Need to add support for VLAN tag 802.1Q */
> > -	if (skb_vlan_tag_present(skb))
> > +	u16 net_protocol = ntohs(skb->protocol);
> > +
> > +	/* GbEth IP can calculate the checksum if:
> > +	 * - there are zero or one VLAN headers with TPID=0x8100
> > +	 * - the network protocol is IPv4 or IPv6
> > +	 * - the transport protocol is TCP, UDP or ICMP
> > +	 * - the packet is not fragmented
> > +	 */
> > +
> > +	if (skb_vlan_tag_present(skb) &&
> > +	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
> 
>    Not sure I understand this check... Maybe s/==/!=/?

A minor nit if the check stays in some form:
vlan_proto is big endian, while ETH_P_8021Q is host byte order.

> 
> >  		return false;
> >  
> > -	switch (ntohs(skb->protocol)) {
> > +	if (net_protocol == ETH_P_8021Q) {
> > +		struct vlan_hdr vhdr, *vh;
> > +
> > +		vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);
> 
>    Hm, I thought the VLAN header starts at ETH_HLEN - 2, not at ETH_HLEN...
> 
> [...]
> 
> MBR, Sergey
> 
>

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

Posted by Sergey Shtylyov 1 month, 3 weeks ago

On 10/1/24 13:44, Simon Horman wrote:
[...]

>>> From: Paul Barker <paul.barker.ct@bp.renesas.com>
>>>
>>> The GbEth IP supports offloading checksum calculation for VLAN-tagged
>>> packets, provided that the EtherType is 0x8100 and only one VLAN tag is
>>> present.
>>>
>>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 832132d44fb4..eb7499d42a9b 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>>>  
>>>  static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
>>>  {
>>> -	/* TODO: Need to add support for VLAN tag 802.1Q */
>>> -	if (skb_vlan_tag_present(skb))
>>> +	u16 net_protocol = ntohs(skb->protocol);
>>> +
>>> +	/* GbEth IP can calculate the checksum if:
>>> +	 * - there are zero or one VLAN headers with TPID=0x8100
>>> +	 * - the network protocol is IPv4 or IPv6
>>> +	 * - the transport protocol is TCP, UDP or ICMP
>>> +	 * - the packet is not fragmented
>>> +	 */
>>> +
>>> +	if (skb_vlan_tag_present(skb) &&
>>> +	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
>>
>>    Not sure I understand this check... Maybe s/==/!=/?
> 
> A minor nit if the check stays in some form:
> vlan_proto is big endian, while ETH_P_8021Q is host byte order.

   Not minor at all, thanks for spotting!
   Luckily, we also have a kernel test robot which runs sparse. :-)

[...]

MBR, Sergey

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

Posted by Sergey Shtylyov 1 month, 4 weeks ago

On 9/30/24 23:36, Sergey Shtylyov wrote:
[...]

>> From: Paul Barker <paul.barker.ct@bp.renesas.com>
>>
>> The GbEth IP supports offloading checksum calculation for VLAN-tagged
>> packets, provided that the EtherType is 0x8100 and only one VLAN tag is
>> present.
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 832132d44fb4..eb7499d42a9b 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
[...]
>> -	switch (ntohs(skb->protocol)) {
>> +	if (net_protocol == ETH_P_8021Q) {
>> +		struct vlan_hdr vhdr, *vh;
>> +
>> +		vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);
> 
>    Hm, I thought the VLAN header starts at ETH_HLEN - 2, not at ETH_HLEN...

    Wikipedia indeed says that the VLAN header begins with TPID word (0x8100)
and then the TCI word follows -- while in Linux, *struct* vlan_hgr starts with
the TCI word -- hence my confusion... :-)

> [...]

MBR, Sergey