Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 28 Jul 2020 17:52:02 -0700 (PDT)
Message-Id: <20200728.175202.598794850221205861.davem@davemloft.net>
To:     Jisheng.Zhang@synaptics.com
Cc:     thomas.petazzoni@bootlin.com, kuba@kernel.org,
        linux@armlinux.org.uk, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-nex 2/2] net: mvneta: Don't speed down the PHY when
 changing mtu
From:   David Miller <davem@davemloft.net>
In-Reply-To: <20200727195314.704dfaed@xhacker.debian>
References: <20200727195012.4bcd069d@xhacker.debian>
        <20200727195314.704dfaed@xhacker.debian>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.