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

[Andrew Lunn <andrew@lunn.ch>]

> +/**
> + * 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