Re: [net-next,v23,4/7] net: mtip: Add net_device_ops functions to the L2 switch driver
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-02-06 02:26:45
Also in:
imx, linux-arm-kernel, linux-devicetree, lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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();
+}[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
+}
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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); }