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

[Jakub Kicinski <kuba@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.
---
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?