| Message ID | 20240930160845.8520-12-paul@pbarker.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Extend GbEth checksum offload support to VLAN/IPv6 packets | expand |
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
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
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 > >
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
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
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,
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)