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

[Wei Fang <wei.fang@nxp.com>]

There are some comments from Sashiko, and see my response inline.

> +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);
> +	if (!priv->ports)
> +		return -ENOMEM;
> +
> +	/* Some DSA interfaces may set the port even it is disabled, such
> +	 * as .port_disable(), .port_stp_state_set() and so on. To avoid
> +	 * crash caused by accessing NULL port pointer, each port is
> +	 * allocated its own memory. Otherwise, we need to check whether
> +	 * the port pointer is NULL in these interfaces. The latter is
> +	 * difficult for us to cover.
> +	 */
> +	for (int i = 0; i < priv->info->num_ports; i++) {
> +		np = devm_kzalloc(dev, sizeof(*np), GFP_KERNEL);

These devm_* allocations are tied to the PCI device (dev), but happen in a
call chain invoked by the DSA setup() callback:
netc_setup()
  netc_init_all_ports()
    devm_kcalloc(...)
    devm_kzalloc(...)
    netc_port_create_mdio_bus()
      netc_port_create_emdio_bus()
        devm_mdiobus_alloc_size(...)
        devm_of_mdiobus_register(...)
Because the DSA setup() and teardown() callbacks can execute multiple times
over the device's lifetime (e.g., when the DSA conduit interface is unbound
and rebound), these devm_* allocations won't be freed during teardown().

Will this cause memory leaks and MDIO bus registration failures due to sysfs
collisions on subsequent setup() calls?

I checked the DSA core driver with Cody help, the .teardown() is only called
when .setup() fails or the switch device is removed. So .setup() and .teardown()
are not called multiple time over the switch's lifetime, this should be a false
positive.

[ ... ]
> +static int netc_switch_pci_init(struct pci_dev *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct netc_switch_regs *regs;
> +	struct netc_switch *priv;
> +	void __iomem *base;
> +	int err;
> +
> +	pcie_flr(pdev);

Will this pcie_flr() call wipe the PCI configuration space and BARs?

Since pcie_flr() is called without first saving and later restoring the state
with pci_save_state() and pci_restore_state(), the BARs might remain zeroed.
Could subsequent calls to pcim_iomap() and MMIO accesses then fail to decode
at the hardware level, resulting in target aborts?

This is a false positive, the pcie_flr() is called when initializing the switch
to clear the configuration from U-boot or the last binding.