Re: [PATCH v6 net-next 08/15] net: allow ndo_get_stats64 to return an int error code

[Saeed Mahameed <saeed@kernel.org>]

On Sat, 2021-01-09 at 19:26 +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Some drivers need to do special tricks to comply with the new policy
> of
> ndo_get_stats64 being sleepable. For example, the bonding driver,
> which
> derives its stats from its lower interfaces, must recurse with
> dev_get_stats into its lowers with no locks held. But for that to
> work,
> it needs to dynamically allocate some memory for a refcounted copy of
> its array of slave interfaces (because recursing unlocked means that
> the
> original one is subject to disappearing). And since memory allocation
> can fail under pressure, we should not let it go unnoticed, but
> instead
> propagate the error code.
> 
> This patch converts all implementations of .ndo_get_stats64 to return
> int, and propagates that to the dev_get_stats calling site. Error
> checking will be done in further patches.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> Changes in v6:
> Remove the unused "int err" from __bond_release_one and add it in the
> patch it belongs to.
> 
> Changes in v5:
> None.
> 
> Changes in v4:
> Patch is new (Eric's suggestion).
> 

Just Give Eric the proper credit and add:

Suggested-by: Eric .. 


[...]

> @@ -354,4 +356,4 @@ int rmnet_vnd_update_dev_mtu(struct rmnet_port
> *port,
>  	}
>  
>  	return 0;
> -}
> \ No newline at end of file
> +}

Not related? 

[...]
> 
> -void dev_get_stats(struct net_device *dev, struct rtnl_link_stats64
> *storage)
> +int dev_get_stats(struct net_device *dev, struct rtnl_link_stats64
> *storage)
>  {
>  	const struct net_device_ops *ops = dev->netdev_ops;
> +	int err = 0;
>  
>  	if (ops->ndo_get_stats64) {
>  		memset(storage, 0, sizeof(*storage));
> -		ops->ndo_get_stats64(dev, storage);
> +		err = ops->ndo_get_stats64(dev, storage);
>  	} else if (ops->ndo_get_stats) {
>  		netdev_stats_to_stats64(storage, ops-
> >ndo_get_stats(dev));
>  	} else {
> @@ -10418,6 +10419,8 @@ void dev_get_stats(struct net_device *dev,
> struct rtnl_link_stats64 *storage)
>  	storage->rx_dropped += (unsigned long)atomic_long_read(&dev-
> >rx_dropped);
>  	storage->tx_dropped += (unsigned long)atomic_long_read(&dev-
> >tx_dropped);
>  	storage->rx_nohandler += (unsigned long)atomic_long_read(&dev-
> >rx_nohandler);

Must be consistent here, if there was an error you should abort without
touching the caller provided storage, even if you can for some stats.

> +
> +	return err;
>  }
>  EXPORT_SYMBOL(dev_get_stats);
>