Re: [net-next,v20,2/7] net: mtip: The L2 switch driver for imx287

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: The L2 switch driver for imx287

This patch adds support for the More Than IP L2 switch embedded in the
imx287 SoC. It creates a two-port switch driver that can be used
interchangeably with the FEC driver.

[ ... ]

+static void mtip_netif_stop_queues(struct switch_enet_private *fep)
+{
+	for (int i = 0; i < SWITCH_EPORT_NUMBER; i++)
+		netif_stop_queue(fep->ndev[0]);

                                      ^^^

Should this be fep->ndev[i] instead of fep->ndev[0]? The loop iterates
with variable i but always uses index 0, so port 1's queue is never
stopped.

+}
+
+static void mtip_netif_wake_queues(struct switch_enet_private *fep)
+{
+	for (int i = 0; i < SWITCH_EPORT_NUMBER; i++)
+		netif_wake_queue(fep->ndev[0]);

                                      ^^^

Same here - should this be fep->ndev[i]?

+}

[ ... ]

+static int mtip_mii_probe(struct net_device *dev)
+{
+	struct mtip_ndev_priv *priv = netdev_priv(dev);
+	struct switch_enet_private *fep = priv->fep;
+	struct phy_device *phy_dev = NULL;
+	int port_idx = priv->portnum - 1;
+
+	if (fep->phy_np[port_idx]) {
+		phy_dev = of_phy_connect(dev, fep->phy_np[port_idx],
+					 &mtip_adjust_link, 0,
+					 fep->phy_interface[port_idx]);
+		if (!phy_dev) {
+			netdev_err(dev, "Unable to connect to phy\n");
+			return -ENODEV;
+		}
+	}
+
+	phy_set_max_speed(phy_dev, 100);

                          ^^^^^^^

If fep->phy_np[port_idx] is NULL (no phy-handle in DT for this port),
the if block is skipped and phy_dev remains NULL. Can this cause a NULL
pointer dereference in phy_set_max_speed()?

+	fep->phy_dev[port_idx] = phy_dev;

[ ... ]

+static int mtip_parse_of(struct switch_enet_private *fep,
+			 struct device_node *np)
+{
+	struct device_node *p;
+	unsigned int port_num;
+	int ret = 0;
+
+	p = of_get_child_by_name(np, "ethernet-ports");
+
+	for_each_available_child_of_node_scoped(p, port) {
+		if (of_property_read_u32(port, "reg", &port_num))
+			continue;
+
+		if (port_num > SWITCH_EPORT_NUMBER) {

                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This only validates the upper bound. If the device tree contains
reg = <0>, would fep->mac[port_num - 1] access fep->mac[-1]? Perhaps
the check should also verify port_num is at least 1?

+			dev_err(&fep->pdev->dev,
+				"%s: The switch supports up to %d ports!\n",
+				__func__, SWITCH_EPORT_NUMBER);
+			goto of_get_err;
+		}
+
+		fep->n_ports = port_num;
+		ret = of_get_mac_address(port, &fep->mac[port_num - 1][0]);

[ ... ]

+	ret = mtip_mii_init(fep, pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "%s: Cannot init phy bus (%d)!\n", __func__,
+			ret);
+		goto dma_free_coherent_memory;
+	}
+
+	ret = mtip_ndev_init(fep, pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "%s: Failed to create virtual ndev (%d)\n",
+			__func__, ret);
+		goto mdiobus_free_memory;
+	}

[ ... ]

+ mdiobus_free_memory:
+	mdiobus_free(fep->mii_bus);

mtip_mii_init() calls of_mdiobus_register() which registers the bus.
Should mdiobus_unregister() be called here before mdiobus_free()?
Compare with mtip_mii_remove() which does call mdiobus_unregister().

+ dma_free_coherent_memory:
+	dma_free_coherent(&fep->pdev->dev, PAGE_SIZE, fep->rx_bd_base,
+			  fep->bd_dma);
+	fep->rx_bd_base = NULL;
+	fep->tx_bd_base = NULL;
+
+	return ret;
+}
+
+static void mtip_sw_remove(struct platform_device *pdev)
+{
+	struct switch_enet_private *fep = platform_get_drvdata(pdev);
+
+	mtip_ndev_cleanup(fep);
+
+	mtip_mii_remove(fep);
+
+	timer_delete_sync(&fep->timer_mgnt);
+	platform_set_drvdata(pdev, NULL);
+}

mtip_sw_probe() allocates DMA memory with dma_alloc_coherent() in
mtip_switch_dma_init(), but mtip_sw_remove() does not appear to free it.
Is this a memory leak on driver removal?
-- 
pw-bot: cr