Re: [PATCH net-next v9 3/4] net: stmmac: Add PCI glue driver for BCM8958x

Jakub Kicinski
2026-04-07T02:10:11Z

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Date: Thu Apr 2 21:36:28 2026 +0000

net: stmmac: Add PCI glue driver for BCM8958x

Add pci ethernet driver support for Broadcom BCM8958x soc devices used
in automotive applications. This device features a pcie ethernet mac
directly attached to an integrated ethernet switch using an xgmii interface.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-brcm.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-brcm.c
> new file mode 100644
> index 0000000000000..e8c6b4b618acb
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-brcm.c
[ ... ]
> +static const struct software_node *brcm_swnodes[] = {
> + &parent_swnode,
> + &fixed_link_swnode,
> + NULL
> +};

Will this statically allocated global software node array cause issues if
multiple devices are present?

If a second device is probed, software_node_register_node_group might fail
with an error. Additionally, if one device is unbound, unregistering the
group could destroy the global nodes, causing a use-after-free for any other
active instances.

Could these software nodes be dynamically allocated per-device?

[ ... ]
> +static int brcm_drv_init(struct device *dev, void *bsp_priv)
> +{
> + struct brcm_priv_data *brcm_priv = (struct brcm_priv_data *)bsp_priv;
> + struct pci_dev *pdev = to_pci_dev(dev);
> + int vector;
> + int ret;
> +
> + /* This device is directly attached to the switch chip internal to the
> + * SoC using XGMII interface. Since no MDIO is present, register
> + * fixed-link software_node to create phylink.
> + */
> + ret = software_node_register_node_group(brcm_swnodes);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register software_node\n");
> + device_set_node(dev, software_node_fwnode(&parent_swnode));

Does device_set_node overwrite the device's primary firmware node here?

Using device_set_node can destroy existing acpi or device tree companion
nodes. Would it be safer to use device_add_software_node instead to attach
the node without overwriting the primary firmware node?

> +
> + pci_write_config_dword(pdev, XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_LOW,
> + XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_LO_VALUE);
> + pci_write_config_dword(pdev, XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_HIGH,
> + XGMAC_PCIE_CFG_MSIX_ADDR_MATCH_HI_VALUE);
> +
> + misc_iowrite(brcm_priv, XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_LO_OFFSET,
> + XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_LO_VALUE);
> + misc_iowrite(brcm_priv, XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_HI_OFFSET,
> + XGMAC_PCIE_MISC_MSIX_ADDR_MATCH_HI_VALUE);

[ ... ]

> +static void brcm_drv_exit_cleanup(struct device *dev, void *bsp_priv)
> +{
> + struct pci_dev *pdev = to_pci_dev(dev);
> +
> + device_set_node(&pdev->dev, NULL);
> + software_node_unregister_node_group(brcm_swnodes);
> +}

If device_set_node is called with NULL, will it leave the device permanently
mangled without its original firmware node upon unbind?

[ ... ]
> +static int brcm_pci_resume(struct device *dev, void *bsp_priv)
> +{
> + /* Enable Switch Link */
> + misc_iowrite(bsp_priv, XGMAC_PCIE_MISC_MII_CTRL_OFFSET,
> + XGMAC_PCIE_MISC_MII_CTRL_PAUSE_RX |
> + XGMAC_PCIE_MISC_MII_CTRL_PAUSE_TX |
> + XGMAC_PCIE_MISC_MII_CTRL_LINK_UP);
> +
> + return stmmac_pci_plat_resume(dev, bsp_priv);
> +}

Will the hardware state configured in brcm_drv_init be fully restored after
a suspend and resume cycle?

When a pci device resumes from suspend, its internal state is typically reset
to defaults. Since the pci core's pci_save_state does not save arbitrary mmio
registers or custom extended configuration registers, it appears the msi-x
routing and address match configurations programmed during probe might be
lost.

Could this cause the msi-x interrupts to stop working after resume?