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:   Wed, 16 Sep 2020 15:25:50 -0700 (PDT)
Message-Id: <20200916.152550.1833517348137875378.davem@davemloft.net>
To:     vee.khee.wong@intel.com
Cc:     peppe.cavallaro@st.com, alexandre.torgue@st.com,
        joabreu@synopsys.com, kuba@kernel.org, mcoquelin.stm32@gmail.com,
        linux@armlinux.org.uk, netdev@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        boon.leong.ong@intel.com, weifeng.voon@intel.com,
        yoong.siang.song@intel.com, sadhishkhanna.vijaya.balan@intel.com,
        chen.yong.seow@intel.com
Subject: Re: [PATCH net-next] net: stmmac: Add support to Ethtool get/set
 ring parameters
From:   David Miller <davem@davemloft.net>
In-Reply-To: <20200916074020.25491-1-vee.khee.wong@intel.com>
References: <20200916074020.25491-1-vee.khee.wong@intel.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Precedence: bulk

From: Wong Vee Khee <vee.khee.wong@intel.com>
Date: Wed, 16 Sep 2020 15:40:20 +0800

> +int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size)
> +{
> +	struct stmmac_priv *priv = netdev_priv(dev);
> +	int ret = 0;
> +
> +	if (netif_running(dev))
> +		stmmac_release(dev);
 ...
> +	if (netif_running(dev))
> +		ret = stmmac_open(dev);
> +

I've applied this patch but this approach is so fragile, but everyone
does it initially because it is so easy.

The problem here is that for so many reasons the stmmac_open() can
fail, and instead of just the ringparam() operation failing, the
interface becomes down and unusable.

Can you please eventually implement this properly?  Allocate the new
ring resources, and only commit the new configuration if it is
guaranteed to succeed.  Otherwise, backout the ringparam change
and keep the old configuration.

This way the device stays up regardless of whether the resources
(memory, DMA mappings, etc.) can be allocated fully.

Right now, if you do a ringparam under hard memory pressure, this will
take the inteface down as stmmac_open() fails.