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.