Re: [PATCH v5 05/13] net: ethernet: aquantia: Support for NIC-specific code

On 01/12/2017 09:02 PM, Alexander Loktionov wrote:

Inline the defines directly into that file, and these typically go at
the end of the file.

+
+static struct aq_hw_ops *aq_pci_probe_get_hw_ops_by_id(struct pci_dev *pdev)
+{
+	struct aq_hw_ops *ops = NULL;
+	int err = 0;
+
+	ops = hw_atl_a0_get_ops_by_id(pdev);
+	if (ops) {
+		err = 0;
+		goto err_exit;
+	}

Should not that come from the pci_device_id .driver_data structure?

+
+	ops = hw_atl_b0_get_ops_by_id(pdev);
+	if (ops) {
+		err = 0;
+		goto err_exit;
+	}
+
+/* the H/W was not recognized */
+	err = -EFAULT;
+
+err_exit:
+	return ops;
+}
+
+static int aq_ndev_open(struct net_device *ndev)
+{
+	struct aq_nic_s *aq_nic = NULL;
+	int err = 0;
+
+	aq_nic = aq_nic_alloc_hot(ndev);
+	if (!aq_nic) {
+		err = -ENOMEM;
+		goto err_exit;
+	}
+	err = aq_nic_init(aq_nic);
+	if (err < 0)
+		goto err_exit;
+	err = aq_nic_start(aq_nic);
+	if (err < 0)
+		goto err_exit;
+
+err_exit:
+	if (err < 0) {

Just make the error label be taken only if there is an error instead of
checking here again for a negative error code.

+		if (aq_nic)
+			aq_nic_deinit(aq_nic);
+	}
+	return err;
+}
+
+static int aq_ndev_close(struct net_device *ndev)
+{
+	struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);

No casting needed.

+
+	aq_nic_stop(aq_nic);
+	aq_nic_deinit(aq_nic);
+	aq_nic_free_hot_resources(aq_nic);
+
+	return 0;
+}
+
+static int aq_ndev_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
+	int err = 0;
+
+	err = aq_nic_xmit(aq_nic, skb);
+	if (err < 0)
+		goto err_exit;
+
+err_exit:
+	return err;

Just inline the contents of aq_nic_xmit(), this function serves nothing
useful here.

+}
+
+static int aq_ndev_change_mtu(struct net_device *ndev, int new_mtu)
+{
+	struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
+	int err = 0;
+
+	if (new_mtu == ndev->mtu) {
+		err = 0;
+		goto err_exit;
+	}
+	if (new_mtu < 68) {
+		err = -EINVAL;
+		goto err_exit;
+	}

What's so special about 68 here?

+	err = aq_nic_set_mtu(aq_nic, new_mtu + ETH_HLEN);

Why do not need to include ETH_HLEN here?

+	if (err < 0)
+		goto err_exit;
+	ndev->mtu = new_mtu;
+
+	if (netif_running(ndev)) {
+		aq_ndev_close(ndev);
+		aq_ndev_open(ndev);
+	}
+
+err_exit:
+	return err;
+}
+
+static int aq_ndev_set_features(struct net_device *ndev,
+				netdev_features_t features)
+{
+	struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
+	struct aq_nic_cfg_s *aq_cfg = aq_nic_get_cfg(aq_nic);
+	bool is_lro = false;
+
+	if (aq_cfg->hw_features & NETIF_F_LRO) {
+		is_lro = features & NETIF_F_LRO;
+
+		if (aq_cfg->is_lro != is_lro) {
+			aq_cfg->is_lro = is_lro;
+
+			if (netif_running(ndev)) {
+				aq_ndev_close(ndev);
+				aq_ndev_open(ndev);
+			}

Can this be made less disruptive to the network interface?

+		}
+	}
+
+	return 0;
+}
+
+static int aq_ndev_set_mac_address(struct net_device *ndev, void *addr)
+{
+	struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
+	int err = 0;
+
+	err = eth_mac_addr(ndev, addr);
+	if (err < 0)
+		goto err_exit;
+	err = aq_nic_set_mac(aq_nic, ndev);
+	if (err < 0)
+		goto err_exit;
+
+err_exit:
+	return err;
+}
+
+static void aq_ndev_set_multicast_settings(struct net_device *ndev)
+{
+	struct aq_nic_s *aq_nic = (struct aq_nic_s *)netdev_priv(ndev);
+	int err = 0;
+
+	err = aq_nic_set_packet_filter(aq_nic, ndev->flags);
+	if (err < 0)
+		goto err_exit;
+
+	if (netdev_mc_count(ndev)) {
+		err = aq_nic_set_multicast_list(aq_nic, ndev);
+		if (err < 0)
+			goto err_exit;
+	}

That's not enough, you need to deal with Unicast address filtering,
promiscuous mode, and if you run out of space in your multicast address
filter.
-- 
Florian