[PATCH net-nex 0/2] net: mvneta: improve phylink_speed_up/down usage

[PATCH net-nex 0/2] net: mvneta: improve phylink_speed_up/down usage

[Jisheng Zhang]

patch1 fix the comment
patch2 tries to avoid unnecessary phylink_speed_up() and
phylink_speed_down() during changing the mtu.

Jisheng Zhang (2):
  net: mvneta: fix comment about phylink_speed_down
  net: mvneta: Don't speed down the PHY when changing mtu

 drivers/net/ethernet/marvell/mvneta.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.28.0.rc0

[PATCH net-nex 1/2] net: mvneta: fix comment about phylink_speed_down

[Jisheng Zhang]

mvneta has switched to phylink, so the comment should look
like "We may have called phylink_speed_down before".

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 2c9277e73cef..c9b6b0f85bb0 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3637,7 +3637,7 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
 	phylink_start(pp->phylink);
 
-	/* We may have called phy_speed_down before */
+	/* We may have called phylink_speed_down before */
 	phylink_speed_up(pp->phylink);
 
 	netif_tx_start_all_queues(pp->dev);
-- 
2.28.0.rc0

[PATCH net-nex 2/2] net: mvneta: Don't speed down the PHY when changing mtu

[Jisheng Zhang]

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(-)

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);
-- 
2.28.0.rc0

Re: [PATCH net-nex 2/2] net: mvneta: Don't speed down the PHY when changing mtu

[David Miller]

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.

Re: [PATCH net-nex 2/2] net: mvneta: Don't speed down the PHY when changing mtu

[Jisheng Zhang]

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.