Re: [PATCH net-next v18 4/6] net: dsa: mxl862xx: add devlink flash_update and info_get

[netdev-bot+sashiko@kernel.org]

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

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

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] mxl862xx_devlink_flash_update() can run while
  dsa_register_switch() is still creating the user ports, and then touch…
- [Low] mxl862xx_port_mdb_add() and mxl862xx_port_mdb_del() test
  '(priv->block_host || priv->skip_teardown)' with plain loads after…
- [Low] The comment in mxl862xx_devlink_flash_update() says 'The core
  skips the re-probe if the device is unbound or shut down before it…
- [Low] Every devlink flash ends in a full driver re-probe:
  mxl862xx_remove(), then dsa_unregister_switch(), then a fresh probe.

--

net: dsa: mxl862xx: add devlink flash_update and info_get

This adds "devlink dev flash" and "devlink dev info" support to the
mxl862xx DSA driver. A flash validates the image and reboots the switch
into its MCUboot loader. It then streams the image over the SB PDI SMDIO
interface and reinitialises the driver through a deferred re-probe;
.remove() and ->shutdown() take the devlink instance lock first so they
wait for a flash that is already running.

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-fw.c b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> new file mode 100644
> index 0000000000000..9a3db7d43c733
> --- /dev/null
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> @@ -0,0 +1,689 @@

[ ... ]

> +int mxl862xx_devlink_flash_update(struct dsa_switch *ds,
> +				  struct devlink_flash_update_params *params,
> +				  struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	rtnl_lock();
> +	dsa_switch_for_each_user_port(dp, ds) {
> +		if (dp->user) {
> +			dev_close(dp->user);
> +			netif_device_detach(dp->user);
> +		}
> +	}

[Severity: High]
Can this loop run while dsa_register_switch() is still creating the user
ports?

dsa_switch_setup() calls dsa_switch_devlink_register() before
dsa_tree_setup() reaches dsa_tree_setup_ports() and
dsa_tree_setup_conduit(). DEVLINK_CMD_FLASH_UPDATE can therefore reach
this function at probe time. dsa_devlink_flash_update() does not check
ds->setup or dst->setup.

dsa_port_setup() only takes devl_lock inside dsa_port_devlink_setup(). A
port that has already passed that call can be in dsa_user_create() at
the same time as this function, and dsa_user_create() sets and clears
port->user without holding rtnl:

net/dsa/user.c:dsa_user_create() {
    ...
	port->user = user_dev;
    ...
	ret = dsa_user_phy_setup(user_dev);
    ...
out_free:
	free_netdev(user_dev);
	port->user = NULL;
}

This loop holds only rtnl. Could it detach a netdev that is not
registered yet? register_netdevice() would then set __LINK_STATE_PRESENT
again, and the port could be reopened during the flash.

Could it also call dev_close() or netif_device_detach() on a user_dev
that the error path above is freeing? The flash itself can drive that
error path: with block_host set, the PHY reads in dsa_user_phy_setup()
return -EBUSY.

The commit message says .remove() and ->shutdown() take the devlink
instance lock first because dsa_unregister_switch() frees the user
netdevs before devlink_unregister(). The dsa_register_switch() failure
unwind in mxl862xx_probe() frees them in the same order, but it has no
such guard:

	err = dsa_register_switch(ds);
	if (err) {
		set_bit(MXL862XX_FLAG_WORK_STOPPED, &priv->flags);
		...

Every flash ends in a re-probe, so this probe-time window opens again
after each flash. This looks unchanged at the end of the series.

> +	dsa_switch_for_each_cpu_port(dp, ds)
> +		dev_close(dp->conduit);

[ ... ]

> +	mutex_lock_nested(&priv->mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +	priv->flash_owner = NULL;
> +	priv->block_host = false;
> +	priv->skip_teardown = true;
> +	mutex_unlock(&priv->mdiodev->bus->mdio_lock);
> +
> +	/* Reinitialise through a deferred re-probe: remove() runs with
> +	 * skip_teardown set, then a fresh probe() starts against whatever
> +	 * the switch now runs. The core skips the re-probe if the device
> +	 * is unbound or shut down before it fires.
> +	 */
> +	err = device_schedule_reprobe(ds->dev, MXL862XX_FW_REPROBE_DELAY_MS);

[Severity: Low]
Is the comment accurate when an unbind is followed by a rebind?

device_reprobe_work_fn() only checks:

	if (!dev->p->dead && dev->driver == rp->drv &&
	    !strcmp(dev->driver->name, rp->drv_name)) {

The device_schedule_reprobe() kernel-doc also says that an unbind
followed by a rebind of the same driver within the delay still gets
the re-probe.

Suppose mxl862xx_remove() is blocked on devl_lock() in
mxl862xx_flash_shutdown() while a flash runs. dev->driver is still set
and shutting_down has not been set yet, so device_schedule_reprobe()
succeeds here.

Now suppose the device is rebound to mxl862xx before the work takes the
device lock, for example by a scripted sysfs unbind/bind. Would the
stale work then detach and re-probe the newly bound instance?

That costs one extra teardown and reset of a healthy switch rather than
corruption, but the comment promises more than the core provides.

> +	if (err)
> +		dev_err(ds->dev,
> +			"flash: re-probe could not be scheduled (%pe); unbind and rebind to reinitialise\n",
> +			ERR_PTR(err));

[ ... ]

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
> index e05ad52cd297e..8f8bd519de987 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx.c

[ ... ]

> @@ -1547,6 +1598,15 @@ static int mxl862xx_port_mdb_add(struct dsa_switch *ds, int port,
>  	qparam.tci = cpu_to_le16(FIELD_PREP(MXL862XX_TCI_VLAN_ID, mdb->vid));
>  
>  	ret = MXL862XX_API_READ(priv, MXL862XX_MAC_TABLEENTRYQUERY, qparam);
> +	/* A flash blocks the API (-EBUSY) and its teardown drops the MAC
> +	 * table (-ENODEV); there is then nothing to program, and the reprobe
> +	 * rebuilds the configuration. The flash moves from block_host to
> +	 * skip_teardown outside this call, so either errno counts under
> +	 * either flag. See mxl862xx_port_mdb_del().
> +	 */

[Severity: Low]
The comment says "the reprobe rebuilds the configuration". Does it?

The re-probe runs mxl862xx_remove() -> dsa_unregister_switch() and then
a fresh probe, which destroys and recreates every user netdev. Anything
userspace configured on those netdevs is lost: bridge membership, VLANs,
addresses and MDB/FDB entries. The new probe only programs defaults for
new, unconfigured ports.

A flash that fails at step 1 goes through the same full teardown. That
is the case where SYS_MISC_FW_UPDATE returns an error and the old
firmware is still running.

The documentation at the end of the series only says the ports "come
back registered but down, so userspace brings them up again". Could the
comment and the documentation say that per-port configuration is lost?

> +	if ((ret == -EBUSY || ret == -ENODEV) &&
> +	    (priv->block_host || priv->skip_teardown))
> +		return 0;

[Severity: Low]
These flag loads run after mxl862xx_api_wrap() has released mdio_lock.
The flash path writes the flags as separate stores:

	priv->flash_owner = NULL;
	priv->block_host = false;
	priv->skip_teardown = true;

The mutex orders those stores only for readers that take the lock.

On a weakly ordered CPU such as arm64, a reader that got -EBUSY from the
block_host gate might see the new block_host == false together with the
old skip_teardown == false. Would it then return -EBUSY to switchdev
anyway?

That is the transition this check is meant to cover.
mxl862xx_port_mdb_del() has the same test. On error,
dsa_port_do_mdb_del() keeps the tracked entry.

These are also plain unannotated racy reads that KCSAN would report. The
next patch in the series uses READ_ONCE() for rescue_mode in the same
expression. This also looks unchanged at the end of the series.

>  	if (ret)
>  		return ret;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1790426599.git.daniel%40makrotopia.org