| Message ID | 20200727195314.704dfaed@xhacker.debian |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: mvneta: improve phylink_speed_up/down usage | expand |
From: Jisheng Zhang <Jisheng.Zhang@synaptics.com> Date: Mon, 27 Jul 2020 19:53:14 +0800 > @@ -3651,7 +3651,8 @@ static void mvneta_stop_dev(struct mvneta_port *pp) > > set_bit(__MVNETA_DOWN, &pp->state); > > - if (device_may_wakeup(&pp->dev->dev)) > + if (device_may_wakeup(&pp->dev->dev) && > + pp->pkt_size == MVNETA_RX_PKT_SIZE(pp->dev->mtu)) > phylink_speed_down(pp->phylink, false); > This is too much for me. You shouldn't have to shut down the entire device and take it back up again just to change the MTU. Unfortunately, this is a common pattern in many drivers and it is very dangerous to take this lazy path of just doing "stop/start" around the MTU change. It means you can't recover from partial failures properly, f.e. recovering from an inability to allocate queue resources for the new MTU. To solve this properly, you must restructure the MTU change such that is specifically stops the necessary and only the units of the chip necessary to change the MTU. It should next try to allocate the necessary resources to satisfy the MTU change, keeping the existing resources allocated in case of failure. Then, only is all resources are successfully allocated, it should commit the MTU change fully and without errors. Then none of these link flapping issues are even possible.
Hi David, On Tue, 28 Jul 2020 17:52:02 -0700 (PDT) David Miller wrote: > > > > @@ -3651,7 +3651,8 @@ static void mvneta_stop_dev(struct mvneta_port *pp) > > > > set_bit(__MVNETA_DOWN, &pp->state); > > > > - if (device_may_wakeup(&pp->dev->dev)) > > + if (device_may_wakeup(&pp->dev->dev) && > > + pp->pkt_size == MVNETA_RX_PKT_SIZE(pp->dev->mtu)) > > phylink_speed_down(pp->phylink, false); > > > > This is too much for me. > > You shouldn't have to shut down the entire device and take it back up > again just to change the MTU. > > Unfortunately, this is a common pattern in many drivers and it is very > dangerous to take this lazy path of just doing "stop/start" around > the MTU change. > > It means you can't recover from partial failures properly, > f.e. recovering from an inability to allocate queue resources for the > new MTU. > > To solve this properly, you must restructure the MTU change such that > is specifically stops the necessary and only the units of the chip > necessary to change the MTU. > > It should next try to allocate the necessary resources to satisfy the > MTU change, keeping the existing resources allocated in case of > failure. > > Then, only is all resources are successfully allocated, it should > commit the MTU change fully and without errors. > > Then none of these link flapping issues are even possible. Thanks a lot for pointing out the correct direction. Refactoring change mtu method needs more time(maybe for linux-5.10 is reasonable), so I just drop patch2 in v2.
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index c9b6b0f85bb0..9cdbb05277eb 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -3651,7 +3651,8 @@ static void mvneta_stop_dev(struct mvneta_port *pp) set_bit(__MVNETA_DOWN, &pp->state); - if (device_may_wakeup(&pp->dev->dev)) + if (device_may_wakeup(&pp->dev->dev) && + pp->pkt_size == MVNETA_RX_PKT_SIZE(pp->dev->mtu)) phylink_speed_down(pp->phylink, false); phylink_stop(pp->phylink);
We found a case where the phy link speed is changed to 10Mbps then back to 1000Mbps when changing the mtu: ethtool -s eth0 wol g ip link set eth0 mtu 1400 Add a simple check to avoid unnecessary phylink_speed_down() when changing the mtu. Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> --- drivers/net/ethernet/marvell/mvneta.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)