Re: [PATCH net-next v3 3/3] net: txgbe: Set MAC address and register netdev

+/**
+ * txgbe_open - Called when a network interface is made active
+ * @netdev: network interface device structure
+ *
+ * Returns 0 on success, negative value on failure
+ *
+ * The open entry point is called when a network interface is made
+ * active by the system (IFF_UP).
+ **/
+static int txgbe_open(struct net_device *netdev)
+{
+	netif_carrier_off(netdev);

The carrier should already be off, so this should not be needed.

+/**
+ * txgbe_set_mac - Change the Ethernet Address of the NIC
+ * @netdev: network interface device structure
+ * @p: pointer to an address structure
+ *
+ * Returns 0 on success, negative on failure
+ **/
+static int txgbe_set_mac(struct net_device *netdev, void *p)
+{
+	struct txgbe_adapter *adapter = netdev_priv(netdev);
+	struct wx_hw *wxhw = &adapter->hw.wxhw;
+	struct sockaddr *addr = p;
+
+	if (!is_valid_ether_addr(addr->sa_data))
+		return -EADDRNOTAVAIL;

Maybe use eth_prepare_mac_addr_change() ?

+ * txgbe_add_sanmac_netdev - Add the SAN MAC address to the corresponding
+ * netdev->dev_addr_list
+ * @dev: network interface device structure
+ *
+ * Returns non-zero on failure
+ **/
+static int txgbe_add_sanmac_netdev(struct net_device *dev)
+{
+	struct txgbe_adapter *adapter = netdev_priv(dev);
+	struct txgbe_hw *hw = &adapter->hw;
+	int err = 0;
+
+	if (is_valid_ether_addr(hw->mac.san_addr)) {

You have a lot of these checks. Where can the bad MAC address come
from? Can you check this once at a higher level? Generally, if you
don't have a valid MAC address you call eth_hw_addr_random() to create
a valid random MAC address.

+	eth_hw_addr_set(netdev, wxhw->mac.perm_addr);
+
+	if (!is_valid_ether_addr(netdev->dev_addr)) {
+		dev_err(&pdev->dev, "invalid MAC address\n");
+		err = -EIO;
+		goto err_free_mac_table;
+	}

so maybe you should call eth_hw_addr_random() here?

+
+	txgbe_mac_set_default_filter(adapter, wxhw->mac.perm_addr);
+
+	strcpy(netdev->name, "eth%d");

That is not needed. It should already default to that from the call to
alloc_etherdev() or its variants.

+	err = register_netdev(netdev);
+	if (err)
+		goto err_free_mac_table;
+
 	pci_set_drvdata(pdev, adapter);
+	adapter->netdev_registered = true;
+
+	/* carrier off reporting is important to ethtool even BEFORE open */
+	netif_carrier_off(netdev);

It can already be open by the time you get here. As soon as you call
register_netdev(), the device can be used. e.g. NFS root could of
already opened the device and tried to talk to the NFS server before
register_netdev() even returns. The device needs to be 100% ready to
go before you call register_netdev().

 static void txgbe_remove(struct pci_dev *pdev)
 {
+	struct txgbe_adapter *adapter = pci_get_drvdata(pdev);
+	struct net_device *netdev;
+
+	netdev = adapter->netdev;
+
+	/* remove the added san mac */
+	txgbe_del_sanmac_netdev(netdev);
+
+	if (adapter->netdev_registered) {
+		unregister_netdev(netdev);
+		adapter->netdev_registered = false;
+	}

How can remove be called without it being registered? Probe should
either succed and register the netdev, or fail, and hence remove will
never be called.

      Andrew