Re: [PATCH net-next] net: stmmac: Add support to Ethtool get/set ring parameters

[David Miller <davem@davemloft.net>]

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.