Re: [PATCH net-next v14 5/6] net: dsa: mxl862xx: recover switch stuck in MCUboot rescue mode

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The new `rescue_heal_work` drain can block driver unbind and
  system shutdown for ~17 s: `mxl862xx_rescue_drain()` tests…
- [Medium] `mxl862xx_rescue_mode_detect()` recognises only READY
  (0xc55c), DL_READY (0xc33c) and 0; every other non-zero STAT is…
- [Low] `priv->rescue_mode` is cleared as a plain store under
  `priv->mdiodev->bus->mdio_lock` in `mxl862xx_devlink_flash_update()`,…

--

net: dsa: mxl862xx: recover switch stuck in MCUboot rescue mode

This adds probe-time detection of a switch left in its MCUboot loader over
the clause-22 SMDIO mailbox, registers the switch in a reduced "rescue
mode" so devlink stays usable, and drains an interrupted download back to a
clean loader state from a background work item.

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-fw.c b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> index e8913954b2bae..042a4eee6af81 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c

[ ... ]

> @@ -218,6 +265,338 @@ static void mxl862xx_flash_notify(struct devlink *dl, const char *status,
>  	devlink_flash_update_status_notify(dl, status, NULL, done, total);
>  }
>  

[ ... ]

> +/* The loader is not asking for a chunk: it may still be programming the last
> + * one, or the counter has reached zero and it is verifying the image and
> + * resetting into READY. Wait that out -- STAT cannot tell the two apart, and
> + * guessing would mean writing END into a live receive loop.
> + *
> + * Return: 0 once the loader has left the loop, -EAGAIN if it asks for another
> + * chunk after all, -EIO for a loader still holding the count when the verify
> + * window expires, or an SMDIO bus error.
> + */
> +static int mxl862xx_rescue_drain_finish(struct mxl862xx_priv *priv, u32 chunk)
> +{
> +	struct device *dev = &priv->mdiodev->dev;
> +	int stat;
> +
> +	stat = mxl862xx_sb_pdi_poll_drain(priv, MXL862XX_SB_PDI_VERIFY_MS);

[ ... ]

> +static int mxl862xx_rescue_drain(struct mxl862xx_priv *priv)
> +{
> +	struct device *dev = &priv->mdiodev->dev;
> +	/* Bound: twice the loader's 16 MiB image cap, one byte per chunk. */
> +	u32 max_chunks = 2u * (16u << 20) / MXL862XX_DRAIN_CHUNK_BYTES;
> +	u32 chunk = 0;
> +	int ret, stat;
> +
> +	while (chunk < max_chunks) {
> +		/* Teardown can interrupt this long drain. */
> +		if (test_bit(MXL862XX_FLAG_WORK_STOPPED, &priv->flags))
> +			return -ECANCELED;
> +
> +		stat = mxl862xx_sb_pdi_poll_drain(priv, MXL862XX_SB_PDI_STEP_MS);
> +		if (stat < 0)
> +			return stat;
> +		if (stat == MXL862XX_SB_PDI_READY)
> +			return 0;
> +
> +		if (stat) {
> +			ret = mxl862xx_rescue_drain_finish(priv, chunk);
> +			if (ret != -EAGAIN)
> +				return ret;
> +		}

[Severity: Medium]
The MXL862XX_FLAG_WORK_STOPPED test only runs at the top of each chunk
iteration, but neither poll that follows it re-tests the flag.

mxl862xx_sb_pdi_poll_drain() sleeps up to MXL862XX_SB_PDI_STEP_MS (2000 ms)
and, when STAT still holds a count, mxl862xx_rescue_drain_finish() sleeps up
to MXL862XX_SB_PDI_VERIFY_MS (15000 ms) on top of that, with the stop flag
in neither poll condition.

The join point is synchronous:

drivers/net/dsa/mxl862xx/mxl862xx-host.c:mxl862xx_host_shutdown() {
	cancel_work_sync(&priv->crc_err_work);
	cancel_work_sync(&priv->rescue_heal_work);
}

Can this stall driver unbind and, via mxl862xx_shutdown(), reboot and
poweroff for roughly 17 s whenever the switch is in the wedged-download
state that rescue_heal_work exists to handle?

Would it be reasonable to fold the stop flag into the poll conditions of
mxl862xx_sb_pdi_poll_drain() so the wait aborts as soon as teardown asks
for it?

[ ... ]

> +/* Detect MCUboot rescue mode over clause-22 SMDIO alone, so the caller can rule
> + * the loader out before any C45 API request (which spews CRC errors when no WSP
> + * firmware answers). A scratch write to ADDR/DATA must latch or the chip is
> + * absent (-ENODEV); the mailbox is reset first, or a transfer interrupted with
> + * CTRL=WR would take that write as a payload word instead of latching it. STAT
> + * then classifies the state, poked destructively only when 0, the one value a
> + * running firmware never holds:
> + *
> + *  - 0xc33c: flashless loop; recognised but not supported here.
> + *  - 0xc55c: console loop, if the register-read challenge is serviced.
> + *  - other non-zero: running firmware, left unpoked.
> + *  - 0: wedged receive loop; the 1-byte slice-advance then says whether it
> + *    still needs draining or has just finished.

[ ... ]

> +int mxl862xx_rescue_mode_detect(struct mxl862xx_priv *priv)
> +{

[ ... ]

> +	/* Any other non-zero value is a running firmware, not a loader. */
> +	if (stat)
> +		return MXL862XX_NOT_RESCUE;

[Severity: Medium]
STAT is also where the download handshake lives, and it is host-writable.
Are the loader's handshake wait states covered by this catch-all?

mxl862xx_sb_pdi_reset() writes CTRL, ADDR and DATA only, so a value already
in STAT survives it:

drivers/net/dsa/mxl862xx/mxl862xx-fw.c:mxl862xx_sb_pdi_reset() {
	ret = mxl862xx_smdio_write(priv, MXL862XX_SB_PDI_CTRL,
				   MXL862XX_SB_PDI_CTRL_RST);
	...
	ret = mxl862xx_smdio_write(priv, MXL862XX_SB_PDI_ADDR,
				   MXL862XX_SB_PDI_CTRL_RST);
	...
	return mxl862xx_smdio_write(priv, MXL862XX_SB_PDI_DATA,
				    MXL862XX_SB_PDI_CTRL_RST);
}

In mxl862xx_flash_firmware() there is a window between the START handshake
and the header byte count where STAT holds 0xf48f (written by the host) or
0xf490 (published by the loader while it waits for the header words and
their count):

	ret = mxl862xx_sb_pdi_poll_stat(priv, MXL862XX_SB_PDI_START + 1,
					MXL862XX_FW_ACK_TIMEOUT_MS);
	...
	for (i = 0; i < MXL862XX_FW_HDR_SIZE / 2; i++) {
		...
		if (ret < 0)
			goto write_err;
	}

Any SMDIO error there takes the write_err/no_end paths, which return without
advancing or aborting the loader; a host crash in the same burst has the same
effect. On the next probe, detection sees 0xf48f or 0xf490 and returns
MXL862XX_NOT_RESCUE, so mxl862xx_setup() runs mxl862xx_reset() and
mxl862xx_wait_ready() over clause-45 against a switch with no firmware,
re-runs detection, gets NOT_RESCUE again and fails probe with -ETIMEDOUT.

That is the CRC-error storm plus probe abort this patch sets out to avoid,
and it also conflicts with the commit message:

    after a failed one the switch is still in MCUboot, rescue mode
    is detected again, and the user can retry.

Should the handshake magics (START, ACK, and a lingering host-written count)
be classified as a loader rather than as running firmware, or should STAT be
reset out of the handshake before the flash path returns an error?

[ ... ]

> @@ -602,6 +1021,7 @@ int mxl862xx_devlink_flash_update(struct dsa_switch *ds,
>  		 * readiness poll below read the freshly booted firmware.
>  		 */
>  		priv->flash_reading = true;
> +		priv->rescue_mode = false;
>  		mutex_unlock(&priv->mdiodev->bus->mdio_lock);

[Severity: Low]
This is the only post-registration writer of rescue_mode, and it is a plain
store under mdio_lock. mxl862xx_api_wrap() reads it under the same lock, but
the readers this patch adds run in a different lock domain (rtnl_lock, or no
lock at all) and use neither mdio_lock nor READ_ONCE():

mxl862xx_port_enable()
mxl862xx_port_disable()
mxl862xx_port_stp_state_set()
mxl862xx_port_mdb_del()
mxl862xx_phylink_mac_select_pcs()

rtnl_lock and mdio_lock do not order these against the store, so a reader can
observe a stale value indefinitely. Would WRITE_ONCE()/READ_ONCE() pairing be
worth adding here, matching what the same patch already does for the sibling
rescue_ready and rescue_failed flags?

mxl862xx_port_mdb_del() has a second ordering detail: the flag is read after
mxl862xx_api_wrap() has already dropped mdio_lock, so the -ENODEV
classification can use a value that changed in between:

	ret = MXL862XX_API_READ(priv, MXL862XX_MAC_TABLEENTRYQUERY, qparam);
	...
	if (ret == -ENODEV && (priv->skip_teardown || priv->rescue_mode))
		return 0;

[ ... ]

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-host.c b/drivers/net/dsa/mxl862xx/mxl862xx-host.c
> index caa4f74a86cf1..23e5dfe81e364 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx-host.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-host.c

[ ... ]

> @@ -554,9 +560,11 @@ int mxl862xx_smdio_write(struct mxl862xx_priv *priv, u32 addr, u16 val)
>  void mxl862xx_host_init(struct mxl862xx_priv *priv)
>  {
>  	INIT_WORK(&priv->crc_err_work, mxl862xx_crc_err_work_fn);
> +	INIT_WORK(&priv->rescue_heal_work, mxl862xx_rescue_heal_work_fn);
>  }
>  
>  void mxl862xx_host_shutdown(struct mxl862xx_priv *priv)
>  {
>  	cancel_work_sync(&priv->crc_err_work);
> +	cancel_work_sync(&priv->rescue_heal_work);
>  }

[ ... ]

Thanks for the detailed protocol comments in mxl862xx-fw.c, they made the
state machine much easier to follow while reading this.

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