Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[David Miller]

From: Madalin Bucur <madalin.bucur@xxxxxxx>
Date: Wed, 2 Nov 2016 22:17:26 +0200

> This introduces the Freescale Data Path Acceleration Architecture
> +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> +{
> +	u8 i;
> +	size_t res = DPAA_BP_RAW_SIZE / 2;

Always order local variable declarations from longest to shortest line,
also know as Reverse Christmas Tree Format.

Please audit your entire submission for this problem, it occurs
everywhere.

> +	/* we do not want shared skbs on TX */
> +	net_dev->priv_flags &= ~IFF_TX_SKB_SHARING;

Why?  By clearing this, you disallow an important fundamental way to do
performane testing, via pktgen.


> +	int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
 ...
> +		cpustats = (u64 *)&percpu_priv->stats;
> +
> +		for (j = 0; j < numstats; j++)
> +			netstats[j] += cpustats[j];

This is a memcpy() on well-typed datastructures which requires no
casting or special handling whatsoever, so use memcpy instead of
needlessly open coding the operation.

> +static int dpaa_change_mtu(struct net_device *net_dev, int new_mtu)
> +{
> +	const int max_mtu = dpaa_get_max_mtu();
> +
> +	/* Make sure we don't exceed the Ethernet controller's MAXFRM */
> +	if (new_mtu < 68 || new_mtu > max_mtu) {
> +		netdev_err(net_dev, "Invalid L3 mtu %d (must be between %d and %d).\n",
> +			   new_mtu, 68, max_mtu);
> +		return -EINVAL;
> +	}
> +	net_dev->mtu = new_mtu;
> +
> +	return 0;
> +}

MTU restrictions are handled in the net-next tree via net_dev->min_mtu and
net_dev->max_mtu.  Use that and do not define this NDO operation as you do
not need it.

> +static int dpaa_set_features(struct net_device *dev, netdev_features_t features)
> +{
> +	/* Not much to do here for now */
> +	dev->features = features;
> +	return 0;
> +}

Do not define unnecessary NDO operations, let the defaults do their job.

> +static netdev_features_t dpaa_fix_features(struct net_device *dev,
> +					   netdev_features_t features)
> +{
> +	netdev_features_t unsupported_features = 0;
> +
> +	/* In theory we should never be requested to enable features that
> +	 * we didn't set in netdev->features and netdev->hw_features at probe
> +	 * time, but double check just to be on the safe side.
> +	 */
> +	unsupported_features |= NETIF_F_RXCSUM;
> +
> +	features &= ~unsupported_features;
> +
> +	return features;
> +}

Unless you can show that your need this, do not "guess" by implement this
NDO operation.  You don't need it.

> +#ifdef CONFIG_FSL_DPAA_ETH_FRIENDLY_IF_NAME
> +static int dpaa_mac_hw_index_get(struct platform_device *pdev)
> +{
> +	struct device *dpaa_dev;
> +	struct dpaa_eth_data *eth_data;
> +
> +	dpaa_dev = &pdev->dev;
> +	eth_data = dpaa_dev->platform_data;
> +
> +	return eth_data->mac_hw_id;
> +}
> +
> +static int dpaa_mac_fman_index_get(struct platform_device *pdev)
> +{
> +	struct device *dpaa_dev;
> +	struct dpaa_eth_data *eth_data;
> +
> +	dpaa_dev = &pdev->dev;
> +	eth_data = dpaa_dev->platform_data;
> +
> +	return eth_data->fman_hw_id;
> +}
> +#endif

Do not play network device naming games like this, use the standard name
assignment done by the kernel and have userspace entities do geographic or
device type specific naming.

I want to see this code completely removed.

> +static int dpaa_set_mac_address(struct net_device *net_dev, void *addr)
> +{
> +	const struct dpaa_priv	*priv;
> +	int err;
> +	struct mac_device *mac_dev;
> +
> +	priv = netdev_priv(net_dev);
> +
> +	err = eth_mac_addr(net_dev, addr);
> +	if (err < 0) {
> +		netif_err(priv, drv, net_dev, "eth_mac_addr() = %d\n", err);
> +		return err;
> +	}
> +
> +	mac_dev = priv->mac_dev;
> +
> +	err = mac_dev->change_addr(mac_dev->fman_mac,
> +				   (enet_addr_t *)net_dev->dev_addr);
> +	if (err < 0) {
> +		netif_err(priv, drv, net_dev, "mac_dev->change_addr() = %d\n",
> +			  err);
> +		return err;
> +	}

You MUST NOT return an error at this point without rewinding the state change
performed by eth_mac_addr().  Otherwise device will be left in an inconsistent
state compared to what the software MAC address has recorded.

This driver is enormous, I don't have the time nor the patience to
review it further for what seems to be many fundamental errors like
the ones I have pointed out so far.

Sorry.