diff mbox series

[net-next] net: stmmac: propagate feature flags to vlan

Message ID 20230411130028.136250-1-vinschen@redhat.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: stmmac: propagate feature flags to vlan | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers warning 7 maintainers not CCed: pabeni@redhat.com kuba@kernel.org edumazet@google.com linux-stm32@st-md-mailman.stormreply.com linux-arm-kernel@lists.infradead.org mcoquelin.stm32@gmail.com davem@davemloft.net
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Corinna Vinschen April 11, 2023, 1 p.m. UTC
stmmac_dev_probe doesn't propagate feature flags to VLANs.  So features
like TX offloading don't correspond with the general features and it's
not possible to manipulate features via ethtool -K to affect VLANs.

Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Simon Horman April 11, 2023, 3:44 p.m. UTC | #1
On Tue, Apr 11, 2023 at 03:00:28PM +0200, Corinna Vinschen wrote:
> stmmac_dev_probe doesn't propagate feature flags to VLANs.  So features
> like TX offloading don't correspond with the general features and it's
> not possible to manipulate features via ethtool -K to affect VLANs.
> 
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Jacob Keller April 11, 2023, 10:36 p.m. UTC | #2
On 4/11/2023 6:00 AM, Corinna Vinschen wrote:
> stmmac_dev_probe doesn't propagate feature flags to VLANs.  So features
> like TX offloading don't correspond with the general features and it's
> not possible to manipulate features via ethtool -K to affect VLANs.
> 
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index e590b6fc4761..308d4ee12d41 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7216,6 +7216,8 @@ int stmmac_dvr_probe(struct device *device,
>  	if (priv->dma_cap.rssen && priv->plat->rss_en)
>  		ndev->features |= NETIF_F_RXHASH;
>  
> +	ndev->vlan_features |= ndev->features;
> +
>  	/* MTU range: 46 - hw-specific max */
>  	ndev->min_mtu = ETH_ZLEN - ETH_HLEN;
>  	if (priv->plat->has_xgmac)

Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@gmail.com>
Corinna Vinschen April 12, 2023, 1:11 p.m. UTC | #3
On Apr 11 15:00, Corinna Vinschen wrote:
> stmmac_dev_probe doesn't propagate feature flags to VLANs.  So features
> like TX offloading don't correspond with the general features and it's
> not possible to manipulate features via ethtool -K to affect VLANs.

On second thought, I wonder if this shouldn't go into net, rather then
net-next?  Does that make sense? And, do I have to re-submit, if so?


Thanks,
Corinna
Jakub Kicinski April 12, 2023, 2:45 p.m. UTC | #4
On Wed, 12 Apr 2023 15:11:44 +0200 Corinna Vinschen wrote:
> On Apr 11 15:00, Corinna Vinschen wrote:
> > stmmac_dev_probe doesn't propagate feature flags to VLANs.  So features
> > like TX offloading don't correspond with the general features and it's
> > not possible to manipulate features via ethtool -K to affect VLANs.  
> 
> On second thought, I wonder if this shouldn't go into net, rather then
> net-next?  Does that make sense? And, do I have to re-submit, if so?

If it's not a regression it should go into net-next.
"never worked, doesn't crash" category.
Corinna Vinschen April 12, 2023, 2:50 p.m. UTC | #5
On Apr 12 07:45, Jakub Kicinski wrote:
> On Wed, 12 Apr 2023 15:11:44 +0200 Corinna Vinschen wrote:
> > On Apr 11 15:00, Corinna Vinschen wrote:
> > > stmmac_dev_probe doesn't propagate feature flags to VLANs.  So features
> > > like TX offloading don't correspond with the general features and it's
> > > not possible to manipulate features via ethtool -K to affect VLANs.  
> > 
> > On second thought, I wonder if this shouldn't go into net, rather then
> > net-next?  Does that make sense? And, do I have to re-submit, if so?
> 
> If it's not a regression it should go into net-next.
> "never worked, doesn't crash" category.

Ok, great, thanks!


Corinna
Jakub Kicinski April 13, 2023, 4:15 a.m. UTC | #6
On Tue, 11 Apr 2023 15:00:28 +0200 Corinna Vinschen wrote:
> stmmac_dev_probe doesn't propagate feature flags to VLANs.  So features
> like TX offloading don't correspond with the general features and it's
> not possible to manipulate features via ethtool -K to affect VLANs.

Actually, could you add to the commit message a sentence or two about
the features that you tested and/or a mention of the manual indicating
that they are supported over vlans? Especially TSO on a quick look.
I just realized now that you didn't explicitly say that those features
work, just that you can manipulate them...
Corinna Vinschen April 13, 2023, 3:25 p.m. UTC | #7
Hi Jakub,

On Apr 12 21:15, Jakub Kicinski wrote:
> On Tue, 11 Apr 2023 15:00:28 +0200 Corinna Vinschen wrote:
> > stmmac_dev_probe doesn't propagate feature flags to VLANs.  So features
> > like TX offloading don't correspond with the general features and it's
> > not possible to manipulate features via ethtool -K to affect VLANs.
> 
> Actually, could you add to the commit message a sentence or two about
> the features that you tested and/or a mention of the manual indicating
> that they are supported over vlans? Especially TSO on a quick look.
> I just realized now that you didn't explicitly say that those features
> work, just that you can manipulate them...

I tested that I can, for instance, set and reset the tx-checksumming
flag with ethtool -K.  As for TSO, I checked the source code, and the
function stmmac_tso_xmit handles VLANs just fine.  While different
NICs supported by stmmac have different offload features, there's no
indication in the driver source that VLANs have less offloading features
than a non-VLAN connection on the same HW.  Admittedly, I never saw
documentation explicitely claiming this.

If that's not sufficient, testing will take another day or two, because
I have to ask for a remote test setup first.


Corinna
Jakub Kicinski April 13, 2023, 4 p.m. UTC | #8
On Thu, 13 Apr 2023 17:25:56 +0200 Corinna Vinschen wrote:
> I tested that I can, for instance, set and reset the tx-checksumming
> flag with ethtool -K.  As for TSO, I checked the source code, and the
> function stmmac_tso_xmit handles VLANs just fine.  While different
> NICs supported by stmmac have different offload features, there's no
> indication in the driver source that VLANs have less offloading features
> than a non-VLAN connection on the same HW.  Admittedly, I never saw
> documentation explicitely claiming this.
> 
> If that's not sufficient, testing will take another day or two, because
> I have to ask for a remote test setup first.

Testing would be great, I think it's worth waiting for that.
Corinna Vinschen April 17, 2023, 7:03 p.m. UTC | #9
Hi Jakub,

On Apr 13 09:00, Jakub Kicinski wrote:
> On Thu, 13 Apr 2023 17:25:56 +0200 Corinna Vinschen wrote:
> > I tested that I can, for instance, set and reset the tx-checksumming
> > flag with ethtool -K.  As for TSO, I checked the source code, and the
> > function stmmac_tso_xmit handles VLANs just fine.  While different
> > NICs supported by stmmac have different offload features, there's no
> > indication in the driver source that VLANs have less offloading features
> > than a non-VLAN connection on the same HW.  Admittedly, I never saw
> > documentation explicitely claiming this.
> > 
> > If that's not sufficient, testing will take another day or two, because
> > I have to ask for a remote test setup first.
> 
> Testing would be great, I think it's worth waiting for that.

Yes, that was a good idea.  Turns out, TSO doesn't really work well
with VLANs.  The speed is... suboptimal.  Here are some results
with iperf, showing only the summary lines.

Base interface with TSO off:

$ ethtool -K enp0s29f1 tso off
$ iperf3 -c 192.168.1.2
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  1015 MBytes   852 Mbits/sec    0             sender
[  5]   0.00-10.04  sec  1012 MBytes   845 Mbits/sec                  receiver

Base interface with TSO on:

$ ethtool -K enp0s29f1 tso on
$ iperf3 -c 192.168.1.2
[...]
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  1.08 GBytes   928 Mbits/sec    0             sender
[  5]   0.00-10.04  sec  1.08 GBytes   921 Mbits/sec                  receiver

VLAN interface with TSO off:

$ ethtool -K enp0s29f1 tso off
$ iperf3 -c 192.168.3.2
[...]
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec   975 MBytes   818 Mbits/sec    0             sender
[  5]   0.00-10.04  sec   973 MBytes   813 Mbits/sec                  receiver

VLAN interface with TSO on:

$ ethtool -K enp0s29f1 tso on
$ iperf3 -c 192.168.3.2
[...]
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  16.0 MBytes  13.4 Mbits/sec  899             sender
[  5]   0.00-10.04  sec  15.9 MBytes  13.3 Mbits/sec                  receiver

Oops.

I'll send a v2 patch which disables TSO on VLANs for the time being.


Corinna
Jakub Kicinski April 17, 2023, 7:11 p.m. UTC | #10
On Mon, 17 Apr 2023 21:03:12 +0200 Corinna Vinschen wrote:
> Yes, that was a good idea.  Turns out, TSO doesn't really work well
> with VLANs.  The speed is... suboptimal.  Here are some results
> with iperf, showing only the summary lines.

Hah, good to find out before users did :)

IIRC there is something in the TCP stack which retries sending 
as non-TSO if TSO keeps failing so "TSO is very slow" may in fact 
mean TSO is completely broken but TCP is managing to deliver a little
bit with just retransmissions or super tiny cwnd or some such.
Oleksij Rempel June 22, 2023, 8:20 a.m. UTC | #11
Hi Corinna,

this patch breaks netboot on PRTT1C. Reverting this patch make it works again.
Here is a boot log with reverted patch:
root@prtt1c:~ dmesg | grep -e "\(ether\|sja\)"
[    2.523153] sja1105 spi0.0: Probed switch chip: SJA1105Q
[    2.534541] stm32-dwmac 5800a000.ethernet: IRQ eth_wake_irq not found
[    2.539678] stm32-dwmac 5800a000.ethernet: IRQ eth_lpi not found
[    2.564079] stm32-dwmac 5800a000.ethernet: User ID: 0x40, Synopsys ID: 0x42
[    2.569726] stm32-dwmac 5800a000.ethernet:   DWMAC4/5
[    2.582496] stm32-dwmac 5800a000.ethernet: DMA HW capability register supported
[    2.588415] stm32-dwmac 5800a000.ethernet: RX Checksum Offload Engine supported
[    2.612482] stm32-dwmac 5800a000.ethernet: TX Checksum insertion supported
[    2.617992] stm32-dwmac 5800a000.ethernet: Wake-Up On Lan supported
[    2.632481] stm32-dwmac 5800a000.ethernet: TSO supported
[    2.636369] stm32-dwmac 5800a000.ethernet: Enable RX Mitigation via HW Watchdog Timer
[    2.662493] stm32-dwmac 5800a000.ethernet: Enabled L3L4 Flow TC (entries=2)
[    2.668109] stm32-dwmac 5800a000.ethernet: Enabled RFS Flow TC (entries=10)
[    2.682494] stm32-dwmac 5800a000.ethernet: TSO feature enabled
[    2.686993] stm32-dwmac 5800a000.ethernet: Using 32/32 bits DMA host/device width
[    3.513207] sja1105 spi0.0: Probed switch chip: SJA1105Q
[    3.536541] stm32-dwmac 5800a000.ethernet eth0: Only single VLAN ID supported
[    3.544291] stm32-dwmac 5800a000.ethernet eth0: Only single VLAN ID supported
[    3.551823] stm32-dwmac 5800a000.ethernet eth0: Only single VLAN ID supported
[    3.559610] sja1105 spi0.0: configuring for fixed/rmii link mode
[    3.565608] sja1105 spi0.0: Link is Up - 100Mbps/Full - flow control off
[    3.575253] sja1105 spi0.0 t1l0 (uninitialized): PHY [gpio-0:06] driver [TI DP83TD510E] (irq=31)
[    3.594168] sja1105 spi0.0 t1l1 (uninitialized): PHY [gpio-0:07] driver [TI DP83TD510E] (irq=32)
[    3.612244] sja1105 spi0.0 t1l2 (uninitialized): PHY [gpio-0:0a] driver [TI DP83TD510E] (irq=33)
[    3.715610] sja1105 spi0.0 rj45 (uninitialized): PHY [gpio-0:02] driver [Micrel KSZ9031 Gigabit PHY] (irq=34)
[    3.729040] stm32-dwmac 5800a000.ethernet eth0: entered promiscuous mode
[    3.860011] stm32-dwmac 5800a000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
[    3.876544] stm32-dwmac 5800a000.ethernet eth0: No Safety Features support found
[    4.084884] stm32-dwmac 5800a000.ethernet eth0: IEEE 1588-2008 Advanced Timestamp supported
[    4.092530] stm32-dwmac 5800a000.ethernet eth0: registered PTP clock
[    4.099997] stm32-dwmac 5800a000.ethernet eth0: configuring for fixed/rmii link mode
[    4.106645] stm32-dwmac 5800a000.ethernet eth0: Link is Up - 100Mbps/Full - flow control off
[    4.120533] stm32-dwmac 5800a000.ethernet eth0: Adding VLAN ID 0 is not supported
[    4.129557] sja1105 spi0.0 t1l0: configuring for phy/rmii link mode
[    4.143546] sja1105 spi0.0 t1l1: configuring for phy/rmii link mode
[    4.161097] sja1105 spi0.0 t1l2: configuring for phy/rmii link mode
[    4.167342] sja1105 spi0.0 rj45: configuring for phy/rgmii-id link mode
[    9.809396] sja1105 spi0.0 rj45: Link is Up - 1Gbps/Full - flow control off
[   66.587255] sja1105 spi0.0 t1l0: entered allmulticast mode
[   66.605179] stm32-dwmac 5800a000.ethernet eth0: entered allmulticast mode
[   66.624612] sja1105 spi0.0 t1l0: entered promiscuous mode
[   66.676943] sja1105 spi0.0 t1l1: entered allmulticast mode
[   66.700590] sja1105 spi0.0 t1l1: entered promiscuous mode
[   66.762670] sja1105 spi0.0 t1l2: entered allmulticast mode
[   66.782927] sja1105 spi0.0 t1l2: entered promiscuous mode
[   66.921089] sja1105 spi0.0 t1l0: configuring for phy/rmii link mode
[   66.951074] sja1105 spi0.0 t1l1: configuring for phy/rmii link mode
[   67.008160] sja1105 spi0.0 t1l2: configuring for phy/rmii link mode
[   68.557994] sja1105 spi0.0 t1l1: Link is Up - 10Mbps/Full - flow control off
[   93.820549] sja1105 spi0.0 t1l1: Link is Down
[   94.412697] sja1105 spi0.0 t1l1: Link is Up - 10Mbps/Full - flow control off
[   94.445096] stm32-dwmac 5800a000.ethernet eth0: Couldn't decode source port

Please tell me if you'll need help to debug this issue.

Regards,
Oleksij
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e590b6fc4761..308d4ee12d41 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7216,6 +7216,8 @@  int stmmac_dvr_probe(struct device *device,
 	if (priv->dma_cap.rssen && priv->plat->rss_en)
 		ndev->features |= NETIF_F_RXHASH;
 
+	ndev->vlan_features |= ndev->features;
+
 	/* MTU range: 46 - hw-specific max */
 	ndev->min_mtu = ETH_ZLEN - ETH_HLEN;
 	if (priv->plat->has_xgmac)