From: Madalin Bucur <madalin.bu...@nxp.com>
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.
