Re: [PATCH net-next v14 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 2 potential
issue(s) to consider:

Critical: 0 · High: 0 (1) · Medium: 2 · Low: 0

- [Medium] priv->flash_reading is documented (kernel-doc for struct
  mxl862xx_priv and the in-function comment in mxl862xx_api_wrap()) as…
- [Medium] In mxl862xx_flash_firmware() the return value of the final,
  purely optional END (0x3cc3) STAT write is propagated as the result of…

Pre-existing issues:
- [High] mxl862xx_remove() stops the periodic stats poll with
  set_bit(MXL862XX_FLAG_WORK_STOPPED) followed by…

--

net: dsa: mxl862xx: add devlink flash_update and info_get

This adds runtime firmware upgrade through "devlink dev flash" and version
reporting through "devlink dev info" for the mxl862xx DSA driver. The flash
path closes the user and conduit interfaces, blocks firmware API commands,
transfers the image over the SB PDI protocol and then schedules a deferred
re-probe.

A few questions about the new code below.

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

[ ... ]

> +	mxl862xx_flash_notify(dl, "Flashing", payload_size, payload_size);
> +
> +	/* Success: the loader has left the receive loop at r_remain == 0 and
> +	 * verified the image, so END(0x3cc3) is a finalise/boot request rather
> +	 * than a byte count. Signal it here -- and only here -- to boot the new
> +	 * image without waiting out the loader's 2 s END timeout.
> +	 */
> +	ret = mxl862xx_smdio_write(priv, MXL862XX_SB_PDI_STAT,
> +				   MXL862XX_SB_PDI_END);
> +	msleep(MXL862XX_FW_REBOOT_DELAY_MS);
> +	return ret;

[Severity: Medium]
Should the status of this last END write really decide the outcome of the
whole flash?

At this point mxl862xx_sb_pdi_flush_last() has already returned 0, which per
its own logic only happens after the loader published
MXL862XX_SB_PDI_VERIFY_OK, so the image is fully transferred, verified and
will be booted. The comment above the write and the file's protocol comment
both say END is optional because the loader finalises on its own 2 s timeout.

mxl862xx_smdio_write() forwards __mdiobus_write()'s return, so it can fail
with -ETIMEDOUT or -EIO from the bus driver. In that case
mxl862xx_devlink_flash_update() takes the failure path: the
mxl862xx_wait_ready() confirmation is skipped, priv->fw_version, asic_id and
asic_rev are zeroed, and a retry is refused with -EBUSY since skip_teardown
is now set. So a successful update is reported as a transfer failure and
"devlink dev info" reports no versions at all until the re-probe runs.

Would it make sense to log the END write failure and still return success
here, given the verdict has already been read?

[ ... ]

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

[ ... ]

> +	set_bit(MXL862XX_FLAG_WORK_STOPPED, &priv->flags);
> +	disable_delayed_work_sync(&priv->stats_work);
> +	cancel_work_sync(&priv->crc_err_work);
> +	for (i = 0; i < ds->num_ports; i++)
> +		cancel_work_sync(&priv->ports[i].host_flood_work);

[Severity: High]
This isn't a bug introduced by this patch, but the same stats_work re-arm
race that disable_delayed_work_sync() closes here is still open in
mxl862xx_remove(), which uses the plain cancel:

drivers/net/dsa/mxl862xx/mxl862xx.c:mxl862xx_remove() {
	set_bit(MXL862XX_FLAG_WORK_STOPPED, &priv->flags);
	cancel_delayed_work_sync(&priv->stats_work);

	dsa_unregister_switch(ds);
	...
}

The reader side tests the flag locklessly:

drivers/net/dsa/mxl862xx/mxl862xx.c:mxl862xx_get_stats64() {
	...
	if (!test_bit(MXL862XX_FLAG_WORK_STOPPED, &priv->flags))
		schedule_delayed_work(&priv->stats_work, 0);
}

Can a get_stats64() caller that already passed the test_bit() re-arm the work
after cancel_delayed_work_sync() has returned? cancel_delayed_work_sync()
only dequeues and waits, it does not make a later schedule_delayed_work() a
no-op, and no further cancel follows, so mxl862xx_stats_work_fn() would then
iterate dsa_switch_for_each_available_port() over ports released by
dsa_unregister_switch() and dereference the devm-allocated priv and ds after
mxl862xx_remove() returns.

The user netdevs are still registered at that point, so the racing reader can
be something as ordinary as "ip -s link" during an unbind. Would converting
remove() to disable_delayed_work_sync() as well be appropriate, either here
or as a separate fix?

> +
> +	ret = mxl862xx_flash_firmware(priv, params->fw, payload_size,
> +				      ds->devlink);
> +	if (ret)
> +		NL_SET_ERR_MSG_MOD(extack, "firmware transfer failed");
> +
> +	if (!ret) {
> +		mutex_lock_nested(&priv->mdiodev->bus->mdio_lock,
> +				  MDIO_MUTEX_NESTED);
> +		/* Keep block_host set so host writes stay blocked, but let the
> +		 * readiness poll below read the freshly booted firmware.
> +		 */
> +		priv->flash_reading = true;
> +		mutex_unlock(&priv->mdiodev->bus->mdio_lock);
> +
> +		/* Refresh the cached versions so the flash update only
> +		 * completes once the new firmware is confirmed running and
> +		 * devlink dev info reports it. Must happen before setting
> +		 * skip_teardown, which discards all firmware API reads.
> +		 */
> +		ret = mxl862xx_wait_ready(ds);

[Severity: Medium]
Does priv->flash_reading actually restrict the bypass to the flash path?

The gate it feeds only looks at the direction of the command:

drivers/net/dsa/mxl862xx/mxl862xx-host.c:mxl862xx_api_wrap() {
	...
	if (priv->block_host && cmd != SYS_MISC_FW_UPDATE &&
	    !(priv->flash_reading && read)) {
		ret = -EBUSY;
		goto out;
	}
	...
}

There is no check of the calling context and no narrowing to
SYS_MISC_FW_VERSION or MXL862XX_COMMON_CFGGET, so while the flag is set every
read-type firmware API command from any context reaches the switch: the
ethtool MAC/RMON/pause stat handlers, mxl862xx_port_fdb_dump(), the
MXL862XX_MAC_TABLEENTRYQUERY half of mxl862xx_port_mdb_add() and
mxl862xx_port_mdb_del(), MXL862XX_COMMON_CFGGET and INT_GPHY_READ.

The window is not short either. mxl862xx_wait_ready() starts with
msleep(2000) and polls up to MXL862XX_READY_TIMEOUT_MS, and rtnl is dropped
by then, while the user netdevs are only closed and detached, still
registered. So "ethtool -S swp0" or "bridge fdb show" in that window issues
admitted reads against the freshly booted firmware.

There is a second effect for read-modify-write sequences: mdb_add() gets its
MXL862XX_MAC_TABLEENTRYQUERY read admitted and then hits -EBUSY on the
following MXL862XX_MAC_TABLEENTRYADD write, so it fails half executed rather
than being refused up front.

Should the bypass be keyed on the flash path itself, for instance on the
specific commands the readiness poll issues?

[ ... ]

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-host.c b/drivers/net/dsa/mxl862xx/mxl862xx-host.c
> index 11759fa6069b1..caa4f74a86cf1 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx-host.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-host.c
> @@ -340,6 +341,21 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
>  
>  	mutex_lock_nested(&priv->mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
>  
> +	if (priv->skip_teardown) {
> +		ret = read ? -ENODEV : 0;
> +		goto out;
> +	}
> +
> +	/* During the post-flash readiness poll block_host stays set, but the
> +	 * flash path's own firmware version reads must reach the new image;
> +	 * host writes stay blocked so stale resource IDs cannot corrupt it.
> +	 */
> +	if (priv->block_host && cmd != SYS_MISC_FW_UPDATE &&
> +	    !(priv->flash_reading && read)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +

The comment here says "the flash path's own firmware version reads", but the
condition admits any read from any caller, as noted above.

[ ... ]

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.h b/drivers/net/dsa/mxl862xx/mxl862xx.h
> index 432a5f3f2e08e..7231ed6c9187c 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx.h
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx.h
> @@ -319,6 +323,12 @@ struct mxl862xx_fw_version {
>   * @evlan_ingress_size: per-port ingress Extended VLAN block size
>   * @evlan_egress_size:  per-port egress Extended VLAN block size
>   * @vf_block_size:      per-port VLAN Filter block size
> + * @block_host:         reject firmware API commands (except FW_UPDATE)
> + *                      during a firmware flash
> + * @flash_reading:      let the flash path's own firmware reads through
> + *                      block_host while polling the freshly booted image

The kernel-doc for @flash_reading documents the same flash-path-only
restriction that the predicate in mxl862xx_api_wrap() does not implement.
Should either the wording or the check be adjusted so the two agree?

[ ... ]

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