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

[Simon Horman <horms@kernel.org>]

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: dsa: netc: introduce NXP NETC switch driver for i.MX94

This commit introduces the initial NETC switch driver for the i.MX94
series. The driver provides basic probe and remove functionality for
the NXP NETC Ethernet switch which has 3 user ports and 1 CPU port.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> --- /dev/null
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> @@ -0,0 +1,698 @@
> +static int netc_port_create_emdio_bus(struct netc_port *np,
> +				      struct device_node *node)
> +{
> +	struct netc_switch *priv = np->switch_priv;
> +	struct enetc_mdio_priv *mdio_priv;
> +	struct device *dev = priv->dev;
> +	struct enetc_hw *hw;
> +	struct mii_bus *bus;
> +	int err;
> +
> +	hw = enetc_hw_alloc(dev, np->iobase);
> +	if (IS_ERR(hw))
> +		return dev_err_probe(dev, PTR_ERR(hw),
> +				     "Failed to allocate enetc_hw\n");
> +
> +	bus = devm_mdiobus_alloc_size(dev, sizeof(*mdio_priv));
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "NXP NETC switch external MDIO Bus";
> +	bus->read = enetc_mdio_read_c22;
> +	bus->write = enetc_mdio_write_c22;
> +	bus->read_c45 = enetc_mdio_read_c45;
> +	bus->write_c45 = enetc_mdio_write_c45;
> +	bus->parent = dev;
> +	mdio_priv = bus->priv;
> +	mdio_priv->hw = hw;
> +	mdio_priv->mdio_base = NETC_EMDIO_BASE;
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-p%d-emdio",
> +		 dev_name(dev), np->dp->index);
> +
> +	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_port_create_mdio_bus(struct netc_port *np,
> +				     struct device_node *node)
> +{
> +	struct device_node *mdio_node;
> +	int err;
> +
> +	mdio_node = of_get_child_by_name(node, "mdio");
> +	if (mdio_node) {
> +		err = netc_port_create_emdio_bus(np, mdio_node);
> +		of_node_put(mdio_node);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static void netc_port_free_mdio_bus(struct netc_port *np)
> +{
> +	if (np->emdio)
> +		mdiobus_unregister(np->emdio);
                         ^^^^

Does this cause a double unregister? The MDIO bus is registered with
devm_of_mdiobus_register() in netc_port_create_emdio_bus(), which
installs a devres action that automatically calls mdiobus_unregister()
during device removal.

However, netc_port_free_mdio_bus() also explicitly calls
mdiobus_unregister(np->emdio).

During device removal:

  netc_switch_remove()
    -> dsa_unregister_switch()
      -> dsa_switch_teardown()
        -> netc_teardown()
          -> netc_free_mdio_bus()
            -> netc_port_free_mdio_bus()
              -> mdiobus_unregister()  // first call

Then devres cleanup fires:

  devm_mdiobus_unregister()
    -> mdiobus_unregister()  // second call
      -> WARN_ON_ONCE(bus->state != MDIOBUS_REGISTERED)

The second call triggers a warning at drivers/net/phy/mdio_bus_provider.c:675
because the bus state is already MDIOBUS_UNREGISTERED. With
panic_on_warn=1, this causes a kernel panic.

The same double-unregister occurs on the error path of
netc_init_all_ports(), where netc_free_mdio_bus() is called, but devres
will also fire during device teardown.

> +}

[ ... ]

> +static int netc_switch_pci_init(struct pci_dev *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct netc_switch_regs *regs;
> +	struct netc_switch *priv;
> +	int err;
> +
> +	pcie_flr(pdev);
> +	err = pci_enable_device_mem(pdev);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to enable device\n");
> +
> +	/* The command BD rings and NTMP tables need DMA */
> +	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
                                       ^^^^

Is the return value being checked? The comment states "command BD rings
and NTMP tables need DMA", confirming DMA is essential for this driver.

The function can fail with -EIO when dma_supported() returns false for
the 64-bit mask (from dma_set_mask() in kernel/dma/mapping.c:902).

The sibling enetc driver from the same NXP NETC subsystem checks this
return value in enetc.c:

  err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
  if (err) {
      ...
      goto err_dma;
  }

If the 64-bit DMA mask setup fails, the DMA mask remains at the default,
and subsequent DMA operations for NTMP table management (added in later
commits) may silently fail or produce incorrect results on systems with
memory above 4GB.

> +	err = pci_request_mem_regions(pdev, KBUILD_MODNAME);
> +	if (err) {
> +		dev_err(dev, "Failed to request memory regions, err: %pe\n",
> +			ERR_PTR(err));
> +		goto disable_pci_device;
> +	}
> +
> +	pci_set_master(pdev);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		err = -ENOMEM;
> +		goto release_mem_regions;
> +	}
> +
> +	priv->pdev = pdev;
> +	priv->dev = dev;
> +
> +	regs = &priv->regs;
> +	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;
> +	}
> +
> +	regs->port = regs->base + NETC_REGS_PORT_BASE;
> +	regs->global = regs->base + NETC_REGS_GLOBAL_BASE;
> +	pci_set_drvdata(pdev, priv);
> +
> +	return 0;
> +
> +release_mem_regions:
> +	pci_release_mem_regions(pdev);
> +disable_pci_device:
> +	pci_disable_device(pdev);
> +
> +	return err;
> +}

[ ... ]