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

[Wei Fang <wei.fang@nxp.com>]

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

As I mentioned in another mail, we added the 'dsa,member' property to the
netc switch DT-binding doc, specifying that the 'member' (switch index) value
cannot be 0. And 'dsa,member' is a required property for netc switch.

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

The current approach is based on Vladimir's suggestion. I need to confirm with
Vladimir which approach is better.

Hi Vladimir,

What are your thoughts on this suggestion? Is this approach better?

> 
> [ ... ]
> > +	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?

This case will not be true, from the hardware perspective, the switch ports
do not support share a single internal MDIO bus defined under one port.
The internal MDIO bus of a port can only access its own external PHY. So
a switch port must either use EMDIO to access its PHY or its internal MDIO
bus to access its PHY.

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

Great, I will pcim_iomap().