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

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

- [Medium] Rescue mode leaves priv->fw_version at 0.0.0, so
  mxl862xx_phylink_get_caps() (drivers/net/dsa/mxl862xx/mxl862xx-…
- [Medium] mxl862xx_setup() now calls mxl862xx_rescue_mode_detect() as
  the very first switch access and returns its errno verbatim, so…
- [Medium] mxl862xx_rescue_drain_finish() orders its checks so that `if
  (!chunk)` pre-empts the 'a firmware is answering ... an image survived…
- [Medium] mxl862xx_setup_rescue() queues the multi-minute (up to hours)
  drain with queue_work(system_long_wq, &priv->rescue_heal_work).
- [Low] struct mxl862xx_priv's kernel-doc documents @rescue_failed as
  '(rescue_mode) the self-heal gave up or cannot run; the loader needs a…

--

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

This adds probe-time detection of the MCUboot loader over the clause-22
SB PDI mailbox, registers the switch in a reduced "rescue mode" so
devlink stays usable, and adds a background work item that drains a
download an interrupted host left wedged in the loader's receive loop.

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-fw.c b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> index 139c14adf2d98..d001a884e6fc6 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c

[ ... ]

> @@ -219,6 +269,379 @@ 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. Before the first chunk it may also still be erasing
> + * for the session that died, which no verify window covers. Wait that out --
> + * STAT cannot tell the cases 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 window
> + * expires, -ECANCELED on teardown, 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;
> +
> +	if (chunk)
> +		stat = mxl862xx_sb_pdi_poll_drain(priv, MXL862XX_SB_PDI_POLL_US,
> +						  MXL862XX_SB_PDI_VERIFY_MS);
> +	else
> +		stat = mxl862xx_sb_pdi_poll_drain(priv,
> +						  MXL862XX_SB_PDI_SLOW_POLL_US,
> +						  MXL862XX_FW_ERASE_TIMEOUT_MS);
> +	if (stat < 0)
> +		return stat;
> +	if (stat == MXL862XX_SB_PDI_READY)
> +		return 0;
> +	if (stat == MXL862XX_SB_PDI_VERIFY_BAD) {
> +		dev_err(dev,
> +			"flash: loader stuck after %u chunks, power cycle it\n",
> +			chunk);
> +		return -EIO;
> +	}
> +	if (stat) {
> +		if (!chunk) {
> +			dev_err(dev,
> +				"flash: loader still busy after the erase window\n");
> +			return -EIO;
> +		}
> +		/* A firmware is answering, not the loader: an image survived
> +		 * in flash and booted.
> +		 */
> +		dev_info(dev, "flash: firmware booted while draining\n");
> +		return 0;
> +	}

[Severity: Medium]

Is the ordering of the chunk == 0 test against the "firmware booted"
case here intentional?  The same non-zero, non-READY STAT value returns
-EIO when chunk == 0 and 0 ("firmware booted while draining") when
chunk > 0.

There seems to be a path that reaches the chunk == 0 arm with a real
firmware status word in STAT.  mxl862xx_rescue_mode_detect() already
feeds one advance byte in its STAT == 0 branch:

	ret = mxl862xx_smdio_write(priv, MXL862XX_SB_PDI_STAT,
				   MXL862XX_DRAIN_CHUNK_BYTES);
	...
	rb = mxl862xx_sb_pdi_poll_drain(priv, MXL862XX_SB_PDI_POLL_US,
					MXL862XX_SB_PDI_STEP_MS);

If that byte was the last one outstanding and the loader's verify plus
reset outlasts MXL862XX_SB_PDI_STEP_MS, detection returns
MXL862XX_IN_RESCUE with rescue_ready still false.  The heal work then
starts its drain, its first mxl862xx_sb_pdi_poll_drain() sees the
lingering count, and mxl862xx_rescue_drain_finish(priv, 0) waits up to
MXL862XX_FW_ERASE_TIMEOUT_MS -- during which the freshly booted
firmware publishes its own status word, which lands in the chunk == 0
arm and returns -EIO.

mxl862xx_rescue_heal_work_fn() then latches rescue_failed and skips the
reprobe, so priv->rescue_mode stays set for the life of the binding:
mxl862xx_api_wrap() keeps returning -ENODEV, devlink dev info reports
nothing and devlink dev flash returns -EIO, on a switch that is running
firmware.

Does that conflict with the commit message?

  "a valid image that a last-moment interruption left bootable comes up
   as running firmware, with no second-guessing in the drain path"

Would moving the chunk == 0 check after the "firmware booted"
classification, or distinguishing the two by the STAT value rather than
by chunk, cover this?

[ ... ]

> +void mxl862xx_rescue_heal_work_fn(struct work_struct *work)
> +{
> +	struct mxl862xx_priv *priv =
> +		container_of(work, struct mxl862xx_priv, rescue_heal_work);
> +	struct device *dev = &priv->mdiodev->dev;
> +	int ret;
> +
> +	ret = mxl862xx_rescue_drain(priv);
> +	if (ret == -ECANCELED)
> +		return;
> +	if (ret) {
> +		/* Nothing retries this, so say so: rescue_ready stays clear
> +		 * and devlink dev flash reports why it refuses.
> +		 */
> +		dev_err(dev, "flash: download recovery failed: %pe\n",
> +			ERR_PTR(ret));
> +		WRITE_ONCE(priv->rescue_failed, true);
> +		return;
> +	}

[ ... ]

> +	if (test_bit(MXL862XX_FLAG_WORK_STOPPED, &priv->flags))
> +		return;
> +
> +	if (device_schedule_reprobe(dev, MXL862XX_FW_REPROBE_DELAY_MS))
> +		WRITE_ONCE(priv->rescue_failed, true);
> +}

[ ... ]

> +int mxl862xx_rescue_mode_detect(struct mxl862xx_priv *priv, bool settle)
> +{
> +	int stat, dat, ret, rb, a, d;
> +
> +	/* rescue_ready gates flashing; a wedged loader needs the drain first. */
> +	WRITE_ONCE(priv->rescue_ready, false);
> +
> +	ret = mxl862xx_sb_pdi_reset(priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Presence: a live chip latches the scratch write, an absent one floats. */
> +	a = mxl862xx_smdio_write(priv, MXL862XX_SB_PDI_ADDR,
> +				 MXL862XX_SB_PDI_PROBE_A);
> +	if (a < 0)
> +		return a;
> +	d = mxl862xx_smdio_write(priv, MXL862XX_SB_PDI_DATA,
> +				 MXL862XX_SB_PDI_PROBE_D);
> +	if (d < 0)
> +		return d;
> +	a = mxl862xx_smdio_read(priv, MXL862XX_SB_PDI_ADDR);
> +	if (a < 0)
> +		return a;
> +	d = mxl862xx_smdio_read(priv, MXL862XX_SB_PDI_DATA);
> +	if (d < 0)
> +		return d;
> +	if ((u16)a != MXL862XX_SB_PDI_PROBE_A ||
> +	    (u16)d != MXL862XX_SB_PDI_PROBE_D)
> +		return -ENODEV;

[Severity: Medium]

Should a failure of the SB PDI probe be fatal to binding?  The rescue
channel is an addition, but mxl862xx_setup() now runs this first and
propagates the errno:

	rescue = mxl862xx_rescue_mode_detect(priv, false);
	if (rescue < 0) {
		dev_err(ds->dev, "switch state detection failed: %pe\n",
			ERR_PTR(rescue));
		return rescue;
	}

so a part whose clause-45 mailbox works fine, but which does not latch
0x5a5a/0xa5a5 at MXL862XX_SB_PDI_ADDR/DATA, now fails probe with -ENODEV
and exposes no interfaces at all.  The file header states the offsets are
not guaranteed:

  * The SMDIO PHY address comes from the device tree; the 0xe1xx register
  *   offsets are the OTP reset defaults and the only layout supported here.

Would treating a detection failure as "no rescue support available" and
falling through to mxl862xx_reset()/mxl862xx_wait_ready() be safer than
refusing to bind?

The STAT classification rests on the same single sample.  The commit
message says a running firmware "answers with its own non-zero status
word (0x0003 on the firmware tested)", and the STAT == 0 branch below
calls 0 "the one value a running firmware never holds".  A firmware
whose status word reads 0 gets the destructive CTRL=WR / DATA=0 / STAT=1
poke and is then registered in rescue mode with every user port
re-registered as unused.  Is that assumption documented anywhere outside
the tested image?

[ ... ]

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
> index 5668aac9ef7a1..d5e29fd74adbc 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx.c
> @@ -667,27 +667,85 @@ static void mxl862xx_free_bridge(struct dsa_switch *ds,
>  	priv->bridges[bridge->num] = 0;
>  }
>  
> +static void mxl862xx_setup_rescue(struct dsa_switch *ds)
> +{
> +	struct mxl862xx_priv *priv = ds->priv;
> +
> +	if (priv->rescue_ready) {
> +		dev_warn(ds->dev,
> +			 "switch in MCUboot rescue mode, use devlink to flash new firmware\n");
> +		return;
> +	}
> +
> +	if (priv->rescue_failed) {
> +		dev_warn(ds->dev,
> +			 "switch in MCUboot with an unfinishable download, power cycle it\n");
> +		return;
> +	}
> +
> +	/* Drain the wedged download in the background so it never holds the
> +	 * devlink lock; info and flash become available once ready.
> +	 */
> +	dev_warn(ds->dev,
> +		 "switch in MCUboot with an interrupted download, recovering in background\n");
> +	queue_work(system_long_wq, &priv->rescue_heal_work);
> +}

[Severity: Medium]

What happens to this work item across a system suspend?  system_long_wq
is not WQ_FREEZABLE, and the only abort condition inside
mxl862xx_rescue_drain() and mxl862xx_sb_pdi_poll_drain() is
MXL862XX_FLAG_WORK_STOPPED, which is set from the probe error path,
remove(), shutdown() and the flash path, but not from any PM transition.

The commit message says the drain "takes tens of minutes for a large
image", so a suspend landing inside it does not look exotic.  The worker
would keep issuing mxl862xx_smdio_read()/mxl862xx_smdio_write() while the
MDIO controller or conduit driver suspends, and a negative return from
those propagates out of mxl862xx_rescue_drain() into
mxl862xx_rescue_heal_work_fn():

	if (ret) {
		dev_err(dev, "flash: download recovery failed: %pe\n",
			ERR_PTR(ret));
		WRITE_ONCE(priv->rescue_failed, true);
		return;
	}

which then refuses devlink dev flash with -EIO for the rest of the
binding, even though the loader is still drainable.  Would a freezable
workqueue, or a PM notifier that sets MXL862XX_FLAG_WORK_STOPPED and
requeues afterwards, fit better here?  device_schedule_reprobe() already
uses system_freezable_wq for the same reason.

>  static int mxl862xx_setup(struct dsa_switch *ds)
>  {

[ ... ]

> +	priv->rescue_mode = rescue;
>  
> +	/* Software-only SerDes state, needed before anything can reach phylink,
> +	 * including a rescue-mode flash clearing rescue_mode ahead of reprobe.
> +	 */
>  	mutex_init(&priv->serdes_lock);
>  	for (i = 0; i < ARRAY_SIZE(priv->serdes_ports); i++)
>  		mxl862xx_setup_pcs(priv, &priv->serdes_ports[i],
>  				   i + MXL862XX_FIRST_SERDES_PORT);
>  
> +	if (priv->rescue_mode) {
> +		mxl862xx_setup_rescue(ds);
> +		return 0;
> +	}
> +

[Severity: Medium]

This path returns success with priv->fw_version still all zeros, since
mxl862xx_wait_ready() -- the only writer of fw_version -- never runs in
rescue mode.  Does that break phylink creation for the CPU port on some
boards?

mxl862xx_phylink_get_caps() gates the SerDes interfaces on the cached
version:

	case 10 ... 12:
	case 14 ... 16:
		if (!MXL862XX_FW_VER_MIN(priv, 1, 0, 84))
			break;
		__set_bit(PHY_INTERFACE_MODE_QSGMII, config->supported_interfaces);

With 0.0.0 the gate is always false, so ports 10-12 and 14-16 end up with
no supported_interfaces bit set at all, and ports 9/13 lose QSGMII and
10G_QXGMII.  phylink_create() rejects that outright:

net/phy/phylink.c:phylink_create() {
	if (phy_interface_empty(config->supported_interfaces)) {
		dev_err(config->dev,
			"phylink: error: empty supported_interfaces\n");
		return ERR_PTR(-EINVAL);
	...
}

and CPU ports have no unused-port fallback:

net/dsa/dsa.c:dsa_tree_setup_ports() {
		if (dsa_port_is_dsa(dp) || dsa_port_is_cpu(dp)) {
			err = dsa_port_setup(dp);
			if (err)
				goto teardown;
	...
}

So on a board whose CPU port is one of 10-12/14-16, or is 9/13 with
phy-mode qsgmii or 10g-qxgmii, probe would fail with -EINVAL and rescue
mode could never be entered.  The patch adds the rescue-mode bypass in
mxl862xx_phylink_mac_select_pcs() -- should mxl862xx_phylink_get_caps()
get the same treatment for its firmware-version gate?

[ ... ]

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.h b/drivers/net/dsa/mxl862xx/mxl862xx.h
> index 2d38885ecfdab..4ed2fd5cbca7c 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx.h
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx.h
> @@ -332,6 +338,16 @@ struct mxl862xx_fw_version {

[ ... ]

> + * @rescue_ready:       (rescue_mode) loader is at a clean READY and will accept
> + *                      a flash; false while rescue_heal_work is draining
> + * @rescue_failed:      (rescue_mode) the self-heal gave up or cannot run; the
> + *                      loader needs a power cycle and no flash can be accepted

[Severity: Low]

This isn't a bug, but does "the loader needs a power cycle" hold for
every producer of this flag?  mxl862xx_rescue_heal_work_fn() also sets it
after a fully successful drain, purely because the hand-off failed:

	if (device_schedule_reprobe(dev, MXL862XX_FW_REPROBE_DELAY_MS))
		WRITE_ONCE(priv->rescue_failed, true);

In that case the loader sits at a clean READY and only a driver rebind is
needed, which is what the devlink documentation added later in this
series says ("a completed drain whose reprobe could not be scheduled ...
need only a driver rebind").

A third producer is the START/ACK branch of
mxl862xx_rescue_mode_detect(), where rescue_heal_work is never queued at
all, so "the self-heal gave up or cannot run" does not describe it
either.  All three collapse into the same -EIO and "download recovery
failed" extack in mxl862xx_devlink_flash_update().  Could the kernel-doc
just say the loader cannot accept a flash, and leave the per-cause
remedies to the documentation?

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