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

> 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 <[email protected]>
> 
> 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.

This analysis is wrong, the switch driver will be removed if the conduit interface
is unbound. A device link is created between the switch and the conduit interface.
See dsa_conduit_setup():

if (!netif_is_lag_master(dev)) {
		consumer_link = device_link_add(ds->dev, dev->dev.parent,
						DL_FLAG_AUTOREMOVE_CONSUMER);
		if (!consumer_link)
			netdev_err(dev,
				   "Failed to create a device link to DSA switch %s\n",
				   dev_name(ds->dev));
}

For NETC switch it does not support LAG, so the consumer_link is created
when dsa_conduit_setup() is called. netc_setup() is called only once during
the lifetime of the switch device. The call stack is show below.
netc_switch_probe()
	└──dsa_register_switch()
		 └── dsa_switch_probe()
			   └── dsa_tree_setup()
				     └── dsa_tree_setup_switches()
						   └── dsa_switch_setup()

This is a false positive, please ignore it.

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

NETC switch is PCIe device, what I learned from pci_device_shutdown() is that
it is unnecessary to call pci_clear_master() in netc_switch_shutdown().

See the comment from pci_device_shutdown():
    /*
	 * If this is a kexec reboot, turn off Bus Master bit on the
	 * device to tell it to not continue to do DMA. Don't touch
	 * devices in D3cold or unknown states.
	 * If it is not a kexec reboot, firmware will hit the PCI
	 * devices with big hammer and stop their DMA any way.
	 */

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

I believe this is a technically possible but meaningless edge case, and there
is no need to make any changes to this meaningless situation.

> skipping dsa_unregister_switch() and leaking the DSA state. Is it necessary to
> clear the driver data in the shutdown path?
> > +}