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

Hi Jakub,

This is an AI-generated review

AI is going to take the "Maintainer's" job :D

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.

quoted

diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c

b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c new file mode
100644 index 000000000000..03c5163d6508

--- /dev/null
+++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c  

[ ... ]

quoted

+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.

Ech.... I will fix it.

quoted

+}
+
+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]?

The same here.

quoted

+}  

[ ... ]

quoted

+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()?

No. The mtip_mii_probe() is only called in mtip_open(), which is called
only when netdevice's DT node is correctly parsed in the
mtip_sw_probe().

quoted

+	fep->phy_dev[port_idx] = phy_dev;  

[ ... ]

quoted

+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?

In the:
./Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml

one can have only ethernet-ports set to [12].

In the current tree - some switches start numbering from 0, other from
1.

I would opt for keeping things as they are now...

quoted

+			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]);  

[ ... ]

quoted

+	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;
+	}  

[ ... ]

quoted

+ 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().

+1

The mdiobus_unregister() is missing.

I will add it.

quoted

+ 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?

Yes. The dma_free_coherent() is missing.

I will add it for v21.

-- 
Best regards,

Łukasz Majewski