Re: [PATCH v5 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 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · 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).

- [High] netc_setup() now calls pci_reset_function(A->pdev) whenever
  A->in_probe is false.
- [Medium] priv->in_probe is a plain bool.
- [Medium] Moving the initial reset from before pcim_enable_device() into
  netc_setup() means pci_set_master() (netc_switch_pci_init(), line…

--

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

This patch replaces the one-time pcie_flr() in probe with a
netc_switch_reset() call at the start of netc_setup(). The reset uses
pci_reset_function_locked() during probe and pci_reset_function()
otherwise. It also converts the devm_ allocations on the setup path to
explicit alloc/free, so their lifetime matches DSA setup and teardown.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 77077352c1a57..c639ae67c9b71 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)
> +{
> +	if (priv->in_probe)
> +		return pci_reset_function_locked(priv->pdev);
> +
> +	return pci_reset_function(priv->pdev);
>  }

[Severity: High]
Can the pci_reset_function() branch deadlock against unbind in the
multi-switch case this patch is preparing for?

In a multi-switch tree, ds->ops->setup() runs under dsa2_mutex for every
switch in the tree, including switches that are already bound:

dsa_register_switch(B)
  mutex_lock(&dsa2_mutex)
  dsa_switch_probe()
    dsa_tree_setup()
      ...
        netc_setup(A)
          netc_switch_reset(A)
            pci_reset_function(A->pdev)
              pci_dev_lock(A->pdev)    /* device_lock(A) */

The remove path takes the same two locks in the opposite order, because
the driver core already holds device_lock(A) when it calls the driver:

device_release_driver(A)           /* holds device_lock(A) */
  netc_switch_remove()
    dsa_unregister_switch()
      mutex_lock(&dsa2_mutex)

Shutdown has the same ordering through
netc_switch_shutdown()->dsa_switch_shutdown().

If CPU0 is probing switch B while CPU1 is unbinding switch A, would both
threads hang forever? Re-probing A while B is being unbound looks like
the same inversion with the roles swapped.

device->mutex uses lockdep_set_novalidate_class(), so lockdep would not
report this.

This code is still the same at the end of the series. Should this
branch use a different approach, rather than resetting another switch
under its device lock from inside .setup while dsa2_mutex is held?

>  static int netc_setup(struct dsa_switch *ds)
> @@ -875,6 +957,10 @@ static int netc_setup(struct dsa_switch *ds)
>  	struct dsa_port *dp;
>  	int err;
>  
> +	err = netc_switch_reset(priv);
> +	if (err)
> +		return err;
> +
>  	err = netc_init_switch_id(priv);
>  	if (err)
>  		return err;

[ ... ]

> @@ -1045,7 +1136,6 @@ static int netc_switch_pci_init(struct pci_dev *pdev)
>  	void __iomem *base;
>  	int err;
>  
> -	pcie_flr(pdev);
>  	err = pcim_enable_device(pdev);
>  	if (err)
>  		return dev_err_probe(dev, err, "Failed to enable device\n");

[Severity: Medium]
Now that pcie_flr() is gone from here, netc_switch_pci_init() calls
pci_set_master() before the switch has been reset. The first reset now
comes much later. It happens after netc_switch_get_ip_revision() and
netc_switch_platform_probe(), once dsa_register_switch() reaches
netc_setup().

Is that ordering safe after a kexec? pci_device_shutdown() only clears
bus mastering:

	if (kexec_in_progress && (pci_dev->current_state <= PCI_D3hot))
		pci_clear_master(pci_dev);

netc_switch_shutdown()->dsa_switch_shutdown() does not call
netc_teardown() or ntmp_free_cbdr(). Also, netc_xmit_ntmp_cmd() can
return on a read_poll_timeout() expiry after it has already advanced the
producer index.

The switch could then keep enabled command BD rings with a pending
command that points at the previous kernel's DMA addresses. If the
hardware resumes fetching as soon as bus mastering is set again, could
it access memory the new kernel has reused? With an IOMMU, would it
cause faults instead?

Also, if probe fails between pci_set_master() and netc_setup(), for
example in netc_switch_platform_probe() or dsa_register_switch(), the
device is never reset before devres disables it.

Would it work to keep a probe-time reset in netc_switch_pci_init()
before pci_set_master()?

[ ... ]

> @@ -2475,12 +2565,15 @@ static int netc_switch_probe(struct pci_dev *pdev,
>  	ds->ageing_time_max = U32_MAX;
>  	ds->priv = priv;
>  	priv->ds = ds;
> +	priv->in_probe = true;
>  
>  	err = dsa_register_switch(ds);
>  	if (err)
>  		return dev_err_probe(dev, err,
>  				     "Failed to register DSA switch\n");
>  
> +	priv->in_probe = false;
> +
>  	return 0;
>  }

[Severity: Medium]
netc_switch_reset() reads priv->in_probe to decide whether the current
thread holds A's device lock. The flag only records that A's probe is in
progress somewhere, not that the current thread owns the lock.

dsa_register_switch() drops dsa2_mutex before it returns, and it returns
0 when the tree is still incomplete. Consider this sequence:

CPU0 (probe of A)
  priv->in_probe = true;
  dsa_register_switch(A)
    mutex_unlock(&dsa2_mutex);
  <window>
  priv->in_probe = false;

CPU1 (probe of B, async probe or deferred probe)
  dsa_register_switch(B)
    dsa_tree_setup()
      netc_setup(A)
        netc_switch_reset(A)    /* sees in_probe == true */
          pci_reset_function_locked(A->pdev)

In this case pci_reset_function_locked() runs on B's thread, which does
not hold A's device lock. Wouldn't device_lock_assert() fire under
lockdep?

The FLR includes msleep(100) and a readiness poll. Could it still be
running after A's probe returns and the driver core releases
device_lock(A)? If so, it would not be serialized against other
device-locked operations such as a sysfs reset.

The cross-CPU write and read of in_probe is also unsynchronized.

This code is still the same at the end of the series. Could the caller
context decide whether the lock is held, for example through a parameter
or by doing the probe-time reset directly in probe, instead of a shared
per-device flag?

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