Re: [PATCH v4 net-next 10/14] net: dsa: netc: introduce NXP NETC switch driver for i.MX94

[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.
---
net: dsa: netc: introduce NXP NETC switch driver for i.MX94
    
This commit introduces the initial NXP NETC switch driver for the i.MX94
series. It supports basic probe and remove functionality for the integrated
802.1Q Ethernet switch. The switch includes 3 user ports and 1 CPU port
connected to an internal ENETC.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> new file mode 100644
> index 000000000000..5828fd3e342e
> --- /dev/null
> +++ b/drivers/net/dsa/netc/netc_main.c
[ ... ]
> +static int netc_port_create_emdio_bus(struct netc_port *np,
> +				      struct device_node *node)
> +{
[ ... ]
> +	err = devm_of_mdiobus_register(dev, bus, node);
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "Cannot register EMDIO bus\n");
> +
> +	np->emdio = bus;
> +
> +	return 0;
> +}
[ ... ]
> +static int netc_init_switch_id(struct netc_switch *priv)
> +{
> +	struct netc_switch_regs *regs = &priv->regs;
> +	struct dsa_switch *ds = priv->ds;
> +
> +	/* The value of 0 is reserved for the VEPA switch and cannot
> +	 * be used.
> +	 */
> +	if (ds->index > SWCR_SWID || !ds->index) {
> +		dev_err(priv->dev, "Switch index %d out of range\n",
> +			ds->index);
> +		return -ERANGE;
> +	}

Does this check cause the probe to fail unconditionally for standard
single-switch topologies?

The DSA core typically assigns ds->index = 0 by default for the first switch.
Because !ds->index evaluates to true for index 0, this setup function will
always fail unless the dsa,member property is explicitly overridden in the
device tree.

Could the driver translate the hardware ID internally, for example by writing
ds->index + 1 to NETC_SWCR, rather than forcing this hardware-specific
restriction onto the software DSA index?

> +
> +	netc_base_wr(regs, NETC_SWCR, ds->index);
> +
> +	return 0;
> +}
[ ... ]
> +static bool netc_port_is_emdio_consumer(struct device_node *node)
> +{
> +	struct device_node *mdio_node;
> +
> +	/* If the port node has phy-handle property and it does
> +	 * not contain a mdio child node, then the port is the
> +	 * EMDIO consumer.
> +	 */
> +	mdio_node = of_get_child_by_name(node, "mdio");
> +	if (!mdio_node)
> +		return true;
> +
> +	of_node_put(mdio_node);
> +
> +	return false;
> +}
> +
> +/* Currently, phylink_of_phy_connect() is called by dsa_user_create(),
> + * so if the switch uses the external MDIO controller (like the EMDIO
> + * function) to manage the external PHYs. The MDIO bus may not be
> + * created when phylink_of_phy_connect() is called, so it will return
> + * an error and cause the switch driver to fail to probe.
> + * This workaround can be removed when DSA phylink_of_phy_connect()
> + * calls are moved from probe() to ndo_open().
> + */
> +static int netc_switch_check_emdio_is_ready(struct device *dev)
> +{
[ ... ]
> +	for_each_available_child_of_node_scoped(ports, child) {
> +		/* If the node does not have phy-handle property, then
> +		 * the port does not connect to a PHY, so the port is
> +		 * not the EMDIO consumer.
> +		 */
> +		phy_node = of_parse_phandle(child, "phy-handle", 0);
> +		if (!phy_node)
> +			continue;
> +
> +		if (!netc_port_is_emdio_consumer(child)) {
> +			of_node_put(phy_node);
> +			continue;
> +		}
> +
> +		phydev = of_phy_find_device(phy_node);
> +		of_node_put(phy_node);
> +		if (!phydev) {
> +			err = -EPROBE_DEFER;
> +			goto out;
> +		}

Could this logic cause an infinite probe deferral loop for ports sharing an
internal MDIO bus?

The netc_port_is_emdio_consumer() function assumes any port node without an
mdio child is an EMDIO consumer waiting for an external controller. However,
if multiple sibling ports on the same switch share a single internal MDIO bus
defined under one port, the other ports will lack an mdio child node.

This would cause those ports to be mistakenly treated as external EMDIO
consumers, calling of_phy_find_device(). Because the internal MDIO bus is not
registered until later in netc_setup(), the PHY device cannot be found, and
the driver returns -EPROBE_DEFER.

Since netc_setup() is never reached, the MDIO bus is never registered,
causing an infinite deferral loop. Could the driver more accurately identify
whether the PHY belongs to its own switch before deferring?

> +
> +		put_device(&phydev->mdio.dev);
> +	}
> +
> +out:
> +	of_node_put(ports);
> +
> +	return err;
> +}
> +
> +static int netc_switch_pci_init(struct pci_dev *pdev)
> +{
[ ... ]
> +	regs->base = pci_ioremap_bar(pdev, NETC_REGS_BAR);
> +	if (!regs->base) {
> +		err = -ENXIO;
> +		dev_err(dev, "pci_ioremap_bar() failed\n");
> +		goto release_mem_regions;
> +	}
[ ... ]
> +static void netc_switch_pci_destroy(struct pci_dev *pdev)
> +{
> +	struct netc_switch *priv = pci_get_drvdata(pdev);
> +
> +	iounmap(priv->regs.base);
> +	pci_release_mem_regions(pdev);
> +	pci_disable_device(pdev);
> +}

Does unmapping the memory here introduce a use-after-unmap issue when the
MDIO bus is unregistered?

The driver registers the MDIO bus using devm_of_mdiobus_register(), meaning
devres cleans it up asynchronously after the remove function returns or when
probe fails.

However, in netc_switch_pci_destroy(), the driver explicitly unmaps the PCI
I/O memory via iounmap(). Since netc_switch_pci_destroy() is called
synchronously inside the remove function and probe error paths, the memory is
unmapped before the MDIO bus is unregistered by devres.

When devres eventually unregisters the MDIO bus, it detaches PHY devices,
which often perform MDIO transactions during removal. Because the underlying
memory priv->regs.base is already unmapped, these MDIO read/write callbacks
could trigger a synchronous external abort or kernel panic.

Would it be better to use pcim_iomap() to tie the memory mapping lifecycle to
devres, ensuring it outlives the MDIO bus?