Re: [PATCH v7 net-next 4/9] net: dsa: netc: tie setup resources and reset to DSA setup/teardown

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch makes the first netc_setup() skip the reset
  (priv->configured == false) on the grounds that "the probe path…

--

net: dsa: netc: tie setup resources and reset to DSA setup/teardown

This patch adds netc_switch_reset(), which netc_setup() calls first so
each DSA setup starts from a clean hardware state. The first setup skips
the reset and relies on the probe-time FLR. The patch also turns the
setup-path devm_ allocations into explicit alloc/free calls, released in
netc_teardown() and on the error paths.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 77077352c1a57..c291123f53170 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> +static int netc_switch_reset(struct netc_switch *priv)
> +{
> +	struct pci_dev *pdev = priv->pdev;
> +	int err;
> +
> +	if (!priv->configured) {
> +		/* The hardware will be configured after netc_switch_reset()
> +		 * is called, so set priv->configured to true.
> +		 */
> +		priv->configured = true;
> +		return 0;
> +	}

[Severity: Medium]
Can the first netc_setup() start on stale hardware tables if the FLR in
probe failed?

The commit message says:

    The probe path already performs a FLR in netc_switch_pci_init(), so
    the first setup will skip the reset because the priv->configured flag
    is not set.

But netc_switch_pci_init() ignores the pcie_flr() return value:

drivers/net/dsa/netc/netc_main.c:netc_switch_pci_init() {
    ...
	pcie_flr(pdev);
	err = pcim_enable_device(pdev);
    ...
}

pcie_flr() can fail. It returns -ENOTTY if config space is not
accessible or if pci_dev_wait() times out. It can also fail through
pci_dev_reset_iommu_prepare() for an ATS-capable device behind an IOMMU.

Suppose that happens after an unbind/rebind:

netc_switch_probe()
  netc_switch_pci_init()
    pcie_flr()            <- fails, error ignored
  dsa_register_switch()
    netc_setup()
      netc_switch_reset() <- returns 0 early, !priv->configured

The FDB, VLAN filter, IPFT, BPT and ETT entries from the previous binding
would then stay in hardware. netc_setup() builds empty software state on
top of them, and the driver has no record of those entries to remove.

Later setups treat an FLR failure as fatal, so a failed first FLR is
handled differently. Should the probe path check the pcie_flr() result
and fail probe? Alternatively, priv->configured could be set only after
a successful FLR, so that the first setup retries the reset.

> +
> +	/* Block user space config access and other PCI resets, which take
> +	 * pci_cfg_access_lock() via pci_dev_lock(), during the FLR.
> +	 */
> +	pci_cfg_access_lock(pdev);
> +	err = pci_save_state(pdev);
> +	if (err)
> +		goto unlock_cfg_access;
> +
> +	err = pcie_flr(pdev);
> +	if (err)
> +		dev_err(priv->dev, "FLR failed: %pe\n", ERR_PTR(err));
> +
> +	pci_restore_state(pdev);
> +
> +unlock_cfg_access:
> +	pci_cfg_access_unlock(pdev);
> +
> +	return err;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111062753.1397582-1-wei.fang%40oss.nxp.com