Re: [net-next,v23,4/7] net: mtip: Add net_device_ops functions to the L2 switch driver

Re: [net-next,v23,4/7] net: mtip: Add net_device_ops functions to the L2 switch driver

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: mtip: Add net_device_ops functions to the L2 switch driver

This patch provides callbacks for struct net_device_ops for MTIP L2
switch.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> index d209501a99b0..4c64681602d6 100644
> --- a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c

[ ... ]

> @@ -618,6 +741,73 @@ static void mtip_switch_restart(struct net_device *dev, int duplex0,
>  	mtip_config_switch(fep);
>  }
>
> +static void mtip_print_hw_state(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	struct cbd_t *bdp;
> +	bool tx_full;
> +	int i;
> +
> +	spin_lock_bh(&fep->hw_lock);
> +	tx_full = fep->dirty_tx == fep->cur_tx &&
> +		mtip_netif_queues_stopped(fep);
> +
> +	dev_info(&dev->dev, "%s: transmit timed out.\n", dev->name);
> +	dev_info(&dev->dev,
> +		 "Ring data: cur_tx 0x%p%s, dirty_tx 0x%p cur_rx: 0x%p\n",
> +		 fep->cur_tx, tx_full ? " (full)" : "", fep->dirty_tx,
> +		 fep->cur_rx);
> +
> +	bdp = fep->tx_bd_base;
> +	dev_info(&dev->dev, " tx: %u buffers\n", TX_RING_SIZE);
> +	for (i = 0; i < TX_RING_SIZE; i++) {
> +		dev_info(&dev->dev, "  0x%p: %04x %04x %08x\n",
> +			 bdp, bdp->cbd_sc, bdp->cbd_datlen,
> +			 (int)bdp->cbd_bufaddr);
> +		bdp++;
> +	}
> +
> +	bdp = fep->rx_bd_base;
> +	dev_info(&dev->dev, " rx: %lu buffers\n", RX_RING_SIZE);
> +	for (i = 0 ; i < RX_RING_SIZE; i++) {
> +		dev_info(&dev->dev, "  0x%p: %04x %04x %08x\n",
> +			 bdp, bdp->cbd_sc, bdp->cbd_datlen,
> +			 (int)bdp->cbd_bufaddr);
> +		bdp++;
> +	}
> +	spin_unlock_bh(&fep->hw_lock);
> +}
> +
> +static void mtip_timeout(struct net_device *dev, unsigned int txqueue)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +
> +	DO_ONCE(mtip_print_hw_state, dev);
> +
> +	schedule_work(&priv->tx_timeout_work);
              ^^^^

This isn't a regression, but as noted in your v22 response, timeouts are
already counted by the network stack and exposed per-queue in sysfs. Does
this driver need additional counting beyond what the stack provides?

> +}
> +
> +static void mtip_timeout_work(struct work_struct *work)
> +{
> +	struct mtip_ndev_priv *priv =
> +		container_of(work, struct mtip_ndev_priv, tx_timeout_work);
> +	struct switch_enet_private *fep = priv->fep;
> +	struct net_device *dev = priv->dev;
> +
> +	rtnl_lock();
> +	if (netif_device_present(dev) || netif_running(dev)) {
> +		napi_disable(&fep->napi);
> +		netif_tx_lock_bh(dev);
> +		mtip_switch_restart(dev, fep->full_duplex[0],
> +				    fep->full_duplex[1]);
> +		netif_tx_wake_all_queues(dev);
> +		netif_tx_unlock_bh(dev);
> +		napi_enable(&fep->napi);
> +	}
> +	rtnl_unlock();
> +}

[ ... ]

> @@ -1096,6 +1286,92 @@ static int mtip_close(struct net_device *dev)
>  	return 0;
>  }
>
> +#define FEC_HASH_BITS	6		/* #bits in hash */
> +static void mtip_set_multicast_list(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	unsigned int hash_high = 0, hash_low = 0, crc;
> +	struct switch_enet_private *fep = priv->fep;
> +	void __iomem *enet_addr = fep->enet_addr;
> +	struct netdev_hw_addr *ha;
> +	unsigned char hash;
> +
> +	if (priv->portnum == 2)
> +		enet_addr += MCF_ESW_ENET_PORT_OFFSET;
> +
> +	if (dev->flags & IFF_PROMISC) {
> +		/* Promisc mode is required for switch - it is
> +		 * already enabled during driver's probe.
> +		 */
> +		dev_dbg(&dev->dev, "%s: IFF_PROMISC\n", __func__);
> +		return;
> +	}
> +
> +	if (dev->flags & IFF_ALLMULTI) {
> +		dev_dbg(&dev->dev, "%s: IFF_ALLMULTI\n", __func__);
> +
> +		/* Allow all multicast addresses */
> +		writel(0xFFFFFFFF, enet_addr + MCF_FEC_GRP_HASH_TABLE_HIGH);
> +		writel(0xFFFFFFFF, enet_addr + MCF_FEC_GRP_HASH_TABLE_LOW);
> +
> +		return;
> +	}
> +
> +	netdev_for_each_mc_addr(ha, dev) {
> +		/* Calculate crc32 value of mac address */
> +		crc = ether_crc_le(dev->addr_len, ha->addr);
> +
> +		/* Only upper 6 bits (FEC_HASH_BITS) are used
> +		 * which point to specific bit in the hash registers
> +		 */
> +		hash = (crc >> (32 - FEC_HASH_BITS)) & 0x3F;
> +
> +		if (hash > 31)
> +			hash_high |= 1 << (hash - 32);
> +		else
> +			hash_low |= 1 << hash;
> +	}
> +
> +	writel(hash_high, enet_addr + MCF_FEC_GRP_HASH_TABLE_HIGH);
> +	writel(hash_low, enet_addr + MCF_FEC_GRP_HASH_TABLE_LOW);
> +}
> +
> +static int mtip_set_mac_address(struct net_device *dev, void *p)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	void __iomem *enet_addr = fep->enet_addr;
> +	struct sockaddr *addr = p;
> +
> +	if (!is_valid_ether_addr(addr->sa_data))
> +		return -EADDRNOTAVAIL;
> +	eth_hw_addr_set(dev, addr->sa_data);
> +
> +	if (priv->portnum == 2)
> +		enet_addr += MCF_ESW_ENET_PORT_OFFSET;
> +
> +	writel(dev->dev_addr[3] | (dev->dev_addr[2] << 8) |
> +	       (dev->dev_addr[1] << 16) | (dev->dev_addr[0] << 24),
> +	       enet_addr + MCF_FEC_PALR);
> +	writel((dev->dev_addr[5] << 16) | (dev->dev_addr[4] << 24),
> +	       enet_addr + MCF_FEC_PAUR);
> +
> +	return mtip_update_atable_static((unsigned char *)dev->dev_addr,
> +					 7, 7, fep);
                                         ^^^^

If mtip_update_atable_static() returns an error (like -ENOSPC when the
address table is full), the device MAC address has already been updated
by eth_hw_addr_set() and the hardware registers have been written.

This leaves the system with a MAC address in dev->dev_addr and the
hardware registers that isn't in the switch's address table. Should the
MAC address change be rolled back on error, or should the address table
be updated first before committing the MAC change?

> +}

[ ... ]

> @@ -1210,6 +1492,8 @@ static int mtip_ndev_init(struct switch_enet_private *fep,
>  			goto cleanup_created_ndev;
>  		}
>
> +		INIT_WORK(&priv->tx_timeout_work, mtip_timeout_work);
                 ^^^^

Could there be a race if a TX timeout occurs between register_netdev()
(which happens a few lines above at line 1488) and INIT_WORK() here?

At line 1478, netdev_ops is set to &mtip_netdev_ops which includes
.ndo_tx_timeout = mtip_timeout. Then register_netdev() is called at line
1488, making the device active. If a TX timeout occurs before
INIT_WORK() initializes the work struct, mtip_timeout() would call
schedule_work() on an uninitialized work struct.

Should INIT_WORK() be moved before register_netdev()?

> +
>  		dev_dbg(&fep->ndev[i]->dev, "%s: MTIP eth L2 switch %pM\n",
>  			fep->ndev[i]->name, fep->ndev[i]->dev_addr);
>  	}