| Message ID | 20190725231546.23878-1-mcroce@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] mvpp2: document HW checksum behaviour | expand |
Hi Matteo, On Fri, Jul 26, 2019 at 01:15:46AM +0200, Matteo Croce wrote: > The hardware can only offload checksum calculation on first port due to > the Tx FIFO size limitation. Document this in a comment. > > Fixes: 576193f2d579 ("net: mvpp2: jumbo frames support") > Signed-off-by: Matteo Croce <mcroce@redhat.com> Looks good. Please note there's a similar code path in the probe. You could also add a comment there (or move this check/comment in a common place). Thanks! Antoine > --- > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > index d8e5241097a9..2f7286bd203b 100644 > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > @@ -843,7 +843,10 @@ static int mvpp2_bm_update_mtu(struct net_device *dev, int mtu) > /* Add port to new short & long pool */ > mvpp2_swf_bm_pool_init(port); > > - /* Update L4 checksum when jumbo enable/disable on port */ > + /* Update L4 checksum when jumbo enable/disable on port. > + * Only port 0 supports hardware checksum offload due to > + * the Tx FIFO size limitation. > + */ > if (new_long_pool == MVPP2_BM_JUMBO && port->id != 0) { > dev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM); > dev->hw_features &= ~(NETIF_F_IP_CSUM | > -- > 2.21.0 >
On Fri, Jul 26, 2019 at 2:57 PM Antoine Tenart <antoine.tenart@bootlin.com> wrote: > > Hi Matteo, > > On Fri, Jul 26, 2019 at 01:15:46AM +0200, Matteo Croce wrote: > > The hardware can only offload checksum calculation on first port due to > > the Tx FIFO size limitation. Document this in a comment. > > > > Fixes: 576193f2d579 ("net: mvpp2: jumbo frames support") > > Signed-off-by: Matteo Croce <mcroce@redhat.com> > > Looks good. Please note there's a similar code path in the probe. You > could also add a comment there (or move this check/comment in a common > place). > > Thanks! > Antoine > > > --- > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > index d8e5241097a9..2f7286bd203b 100644 > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > @@ -843,7 +843,10 @@ static int mvpp2_bm_update_mtu(struct net_device *dev, int mtu) > > /* Add port to new short & long pool */ > > mvpp2_swf_bm_pool_init(port); > > > > - /* Update L4 checksum when jumbo enable/disable on port */ > > + /* Update L4 checksum when jumbo enable/disable on port. > > + * Only port 0 supports hardware checksum offload due to > > + * the Tx FIFO size limitation. > > + */ > > if (new_long_pool == MVPP2_BM_JUMBO && port->id != 0) { > > dev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM); > > dev->hw_features &= ~(NETIF_F_IP_CSUM | > > -- > > 2.21.0 > > > > -- > Antoine Ténart, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com I see, there is a similar statement in mvpp2_port_probe(). What about adding a static function which sets the flag, and add the comment there instead of duplicating the comment?
From: Matteo Croce <mcroce@redhat.com> Date: Fri, 26 Jul 2019 16:35:59 +0200 > I see, there is a similar statement in mvpp2_port_probe(). > What about adding a static function which sets the flag, and add the > comment there instead of duplicating the comment? That sounds good to me.
On Fri, Jul 26, 2019 at 2:57 PM Antoine Tenart <antoine.tenart@bootlin.com> wrote: > > Hi Matteo, > > On Fri, Jul 26, 2019 at 01:15:46AM +0200, Matteo Croce wrote: > > The hardware can only offload checksum calculation on first port due to > > the Tx FIFO size limitation. Document this in a comment. > > > > Fixes: 576193f2d579 ("net: mvpp2: jumbo frames support") > > Signed-off-by: Matteo Croce <mcroce@redhat.com> > > Looks good. Please note there's a similar code path in the probe. You > could also add a comment there (or move this check/comment in a common > place). > > Thanks! > Antoine > Hi Antoine, I was making a v2, when I looked at the mvpp2_port_probe() which does: --------------------------------%<------------------------------ features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_TSO; if (port->pool_long->id == MVPP2_BM_JUMBO && port->id != 0) { dev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM); dev->hw_features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM); } dev->vlan_features |= features; -------------------------------->%------------------------------ Is it ok to remove NETIF_F_IP*_CSUM from dev->features and dev->hw_features but keep it in dev->vlan_features? Regards,
On Sun, Jul 28, 2019 at 3:36 AM Matteo Croce <mcroce@redhat.com> wrote: > > On Fri, Jul 26, 2019 at 2:57 PM Antoine Tenart > <antoine.tenart@bootlin.com> wrote: > > > > Hi Matteo, > > > > On Fri, Jul 26, 2019 at 01:15:46AM +0200, Matteo Croce wrote: > > > The hardware can only offload checksum calculation on first port > > > due to the Tx FIFO size limitation. Document this in a comment. > > > > > > Fixes: 576193f2d579 ("net: mvpp2: jumbo frames support") > > > Signed-off-by: Matteo Croce <mcroce@redhat.com> > > > > Looks good. Please note there's a similar code path in the probe. > > You could also add a comment there (or move this check/comment in a > > common place). > > > > Thanks! > > Antoine > > > > Hi Antoine, > > I was making a v2, when I looked at the mvpp2_port_probe() which does: > > --------------------------------%<------------------------------ > features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > NETIF_F_TSO; > > if (port->pool_long->id == MVPP2_BM_JUMBO && port->id != 0) { > dev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM); > dev->hw_features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM); > } > > dev->vlan_features |= features; > -------------------------------->%------------------------------ > > Is it ok to remove NETIF_F_IP*_CSUM from dev->features and > dev->hw_features but keep it in dev->vlan_features? > > Regards, > -- > Matteo Croce > per aspera ad upstream Hi all, probably dev->vlan_features is safe to keep the CSUM features to avoid unnecessary calculation in some cases, but I have another question. Does the PP2 hardware support checksumming within any offset? I replaced 'NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM' with NETIF_F_HW_CSUM and then stacked 5 VxLANS on top of a mvpp2 device, to have the last IP header at offset 264: ip link set $dev up ip addr add 192.168.0.$last/24 dev $dev for i in {1..5}; do ip link add vx$i type vxlan id $i dstport 4789 remote 192.168.$((i-1)).$other ip link set vx$i up ip addr add 192.168.$i.$last/24 dev vx$i done 00:51:82:11:22:00 > 3c:fd:fe:9c:60:6c, ethertype IPv4 (0x0800), length 348: 192.168.0.1.33625 > 192.168.0.2.4789: VXLAN, flags [I] (0x08), vni 1 02:25:60:da:87:03 > 92:20:05:45:3d:d3, ethertype IPv4 (0x0800), length 298: 192.168.1.1.33625 > 192.168.1.2.4789: VXLAN, flags [I] (0x08), vni 2 12:20:97:15:8f:aa > 66:08:23:c7:72:ea, ethertype IPv4 (0x0800), length 248: 192.168.2.1.33625 > 192.168.2.2.4789: VXLAN, flags [I] (0x08), vni 3 c6:1c:b9:fd:9d:28 > 22:ca:cb:6a:ea:68, ethertype IPv4 (0x0800), length 198: 192.168.3.1.33625 > 192.168.3.2.4789: VXLAN, flags [I] (0x08), vni 4 02:34:5f:45:a5:9d > d2:4e:d4:d7:42:31, ethertype IPv4 (0x0800), length 148: 192.168.4.1.34504 > 192.168.4.2.4789: VXLAN, flags [I] (0x08), vni 5 a2:99:fd:9c:1b:05 > 5a:81:3b:fc:6a:07, ethertype IPv4 (0x0800), length 98: 192.168.5.1 > 192.168.5.2: ICMP echo request, id 1654, seq 156, length 64 It seems that the HW is capable of doing it, can someone with a datasheet confirm this? Regards,
> Hi all, > > probably dev->vlan_features is safe to keep the CSUM features to avoid > unnecessary calculation in some cases, but I have another question. > Does the PP2 hardware support checksumming within any offset? I replaced > 'NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM' with NETIF_F_HW_CSUM and > then stacked 5 VxLANS on top of a mvpp2 device, to have the last IP header > at offset 264: > > ip link set $dev up > ip addr add 192.168.0.$last/24 dev $dev > > for i in {1..5}; do > ip link add vx$i type vxlan id $i dstport 4789 remote 192.168.$((i- > 1)).$other > ip link set vx$i up > ip addr add 192.168.$i.$last/24 dev vx$i done > > 00:51:82:11:22:00 > 3c:fd:fe:9c:60:6c, ethertype IPv4 (0x0800), length 348: > 192.168.0.1.33625 > 192.168.0.2.4789: VXLAN, flags [I] (0x08), vni 1 > 02:25:60:da:87:03 > 92:20:05:45:3d:d3, ethertype IPv4 (0x0800), length 298: > 192.168.1.1.33625 > 192.168.1.2.4789: VXLAN, flags [I] (0x08), vni 2 > 12:20:97:15:8f:aa > 66:08:23:c7:72:ea, ethertype IPv4 (0x0800), length 248: > 192.168.2.1.33625 > 192.168.2.2.4789: VXLAN, flags [I] (0x08), vni 3 > c6:1c:b9:fd:9d:28 > 22:ca:cb:6a:ea:68, ethertype IPv4 (0x0800), length 198: > 192.168.3.1.33625 > 192.168.3.2.4789: VXLAN, flags [I] (0x08), vni 4 > 02:34:5f:45:a5:9d > d2:4e:d4:d7:42:31, ethertype IPv4 (0x0800), length 148: > 192.168.4.1.34504 > 192.168.4.2.4789: VXLAN, flags [I] (0x08), vni 5 > a2:99:fd:9c:1b:05 > 5a:81:3b:fc:6a:07, ethertype IPv4 (0x0800), length 98: > 192.168.5.1 > 192.168.5.2: ICMP echo request, id 1654, seq 156, length 64 > > It seems that the HW is capable of doing it, can someone with a datasheet > confirm this? L3_offset in TX descriptor has 7 bits, so beginning of Layer3 should be less than 128 Bytes. Stefan, Regards.
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index d8e5241097a9..2f7286bd203b 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -843,7 +843,10 @@ static int mvpp2_bm_update_mtu(struct net_device *dev, int mtu) /* Add port to new short & long pool */ mvpp2_swf_bm_pool_init(port); - /* Update L4 checksum when jumbo enable/disable on port */ + /* Update L4 checksum when jumbo enable/disable on port. + * Only port 0 supports hardware checksum offload due to + * the Tx FIFO size limitation. + */ if (new_long_pool == MVPP2_BM_JUMBO && port->id != 0) { dev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM); dev->hw_features &= ~(NETIF_F_IP_CSUM |
The hardware can only offload checksum calculation on first port due to the Tx FIFO size limitation. Document this in a comment. Fixes: 576193f2d579 ("net: mvpp2: jumbo frames support") Signed-off-by: Matteo Croce <mcroce@redhat.com> --- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)