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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 29 Jul 2020 17:53:12 +0800
From:   Jisheng Zhang <Jisheng.Zhang@...aptics.com>
To:     David Miller <davem@...emloft.net>
Cc:     thomas.petazzoni@...tlin.com, kuba@...nel.org,
        linux@...linux.org.uk, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-nex 2/2] net: mvneta: Don't speed down the PHY when
 changing mtu

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.