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

[Paul Barker]

On 30/09/2024 21:36, Sergey Shtylyov wrote:
> On 9/30/24 19:08, Paul Barker wrote:
> 
>> From: Paul Barker <paul.barker.ct@xxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxx>
> [...]
> 
>> 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

Attachment: OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature