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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] devres-managed functions are incorrectly used inside the DSA `.setup` callback, leading to memory leaks and MDIO bus registration failures if the DSA tree is unbound and rebound.
- [High] The driver's PCI shutdown callback fails to stop hardware DMA rings and leaves PCI bus mastering enabled.
- [Low] Clearing the PCI driver data via pci_set_drvdata(pdev, NULL) in the shutdown callback breaks potential subsequent unbind operations.
--

commit bee0164d1f3476eab41d58efbe3baf540bb03cda
Author: Wei Fang <wei.fang@nxp.com>

net: dsa: netc: introduce NXP NETC switch driver for i.MX94

This commit introduces the initial NXP NETC Ethernet switch driver for
the i.MX94 series. It provides basic probe and remove functionality, as
well as port capabilities and basic network setup.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 0000000000000..8e3a3230226c3 100644
> --- /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)
> +{
> +	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));

[ ... ]

> +	err = devm_of_mdiobus_register(dev, bus, node);

[ ... ]

> +static int netc_init_all_ports(struct netc_switch *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct netc_port *np;
> +	struct dsa_port *dp;
> +	int err;
> +
> +	priv->ports = devm_kcalloc(dev, priv->info->num_ports,
> +				   sizeof(struct netc_port *),
> +				   GFP_KERNEL);

[ ... ]

> +	for (int i = 0; i < priv->info->num_ports; i++) {
> +		np = devm_kzalloc(dev, sizeof(*np), GFP_KERNEL);

[ ... ]

> +static int netc_init_all_cbdrs(struct netc_switch *priv)
> +{
> +	struct netc_switch_regs *regs = &priv->regs;
> +	struct ntmp_user *ntmp = &priv->ntmp;
> +	int i, err;
> +
> +	ntmp->cbdr_num = NETC_CBDR_NUM;
> +	ntmp->dev = priv->dev;
> +	ntmp->ring = devm_kcalloc(ntmp->dev, ntmp->cbdr_num,
> +				  sizeof(struct netc_cbdr),
> +				  GFP_KERNEL);

[ ... ]

> +static int netc_setup(struct dsa_switch *ds)
> +{
> +	struct netc_switch *priv = ds->priv;
> +	struct dsa_port *dp;
> +	int err;
> +
> +	err = netc_init_switch_id(priv);
> +	if (err)
> +		return err;
> +
> +	err = netc_init_all_ports(priv);
> +	if (err)
> +		return err;
> +
> +	err = netc_init_ntmp_user(priv);

Can these devres-managed allocations and registrations cause issues when the
DSA tree unbinds and rebounds?

In the DSA subsystem, the .setup callback is invoked when the conduit
interface becomes available. If the conduit interface is unbound and rebound,
the DSA tree goes through a teardown and setup cycle, meaning netc_setup()
can be called multiple times during the lifetime of the underlying PCI device.

Because netc_init_all_ports() and netc_init_ntmp_user() use devres functions
(devm_kzalloc(), devm_kcalloc(), and devm_of_mdiobus_register()) tied to the
PCI device, their resources will not be freed when .teardown is called. When
the tree binds again, netc_setup() will execute anew, leaking the previously
allocated memory. Also, devm_of_mdiobus_register() will attempt to register an
MDIO bus with an ID that is already in use by the previous unfreed
registration, potentially causing .setup to fail.

Would it be better to either move these allocations to probe(), or manage them
without devm_ helpers and explicitly clean them up in .teardown()?

[ ... ]

> +static void netc_switch_shutdown(struct pci_dev *pdev)
> +{
> +	struct netc_switch *priv = pci_get_drvdata(pdev);
> +
> +	if (!priv)
> +		return;
> +
> +	dsa_switch_shutdown(priv->ds);

Does the hardware continue to perform DMA after this shutdown callback runs?

The netc_switch_pci_init() function enables PCI bus mastering and sets up a
64-bit DMA mask, and netc_init_all_cbdrs() initializes the Control BD rings.
However, dsa_switch_shutdown() only closes the logical network ports without
invoking the DSA teardown callback, meaning the hardware BD rings are never
disabled.

Leaving the hardware rings active with bus mastering enabled during a system
reboot or a transition to a kexec/kdump kernel might allow the device to
continue performing DMA into memory that may have been reassigned, potentially
leading to silent memory corruption or IOMMU faults. Should the shutdown
callback explicitly disable the rings or call pci_clear_master() here?

> +	pci_set_drvdata(pdev, NULL);

Could clearing the driver data here break subsequent unbind operations?

If a user manually unbinds the driver via sysfs shortly after a shutdown
event, netc_switch_remove() will read a NULL priv pointer and return early,
skipping dsa_unregister_switch() and leaking the DSA state. Is it necessary to
clear the driver data in the shutdown path?
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509102954.4116624-1-wei.fang@nxp.com?part=10