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

[netdev-bot+sashiko]

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

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

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_setup() now runs mxl862xx_rescue_mode_detect(priv,
  false) on every probe, before any clause-45 access.
- [Medium] mxl862xx_rescue_mode_detect(priv, true) can never return
  MXL862XX_NOT_RESCUE.
- [Medium] mxl862xx_rescue_drain() starts chunk at 0 on every run (line
  379).
- [Medium] In rescue mode the CPU port and conduit are set up, so DSA's
  conduit ethtool override stays installed.
- [Low] At probe time, rescue_failed can only be set by the START/ACK
  branch of mxl862xx_rescue_mode_detect().
- [Low] The patch made mxl862xx_port_mdb_add() also return 0 on -ENODEV
  when READ_ONCE(priv->rescue_mode) is set.
- [Low] The @flags kernel-doc in struct mxl862xx_priv still says
  MXL862XX_FLAG_WORK_STOPPED "is set before cancelling stats_work to…
- [Low] The file header says the loader leaves the host's byte count in
  STAT while it programs a chunk.
- [Low] In rescue mode, mxl862xx_devlink_info_get() reports the zeroed
  fw_version cache ("0.0.0") with devlink_info_version_stored_put() as…
- [Low] mxl862xx_setup_rescue() queues rescue_heal_work on
  system_long_wq, which is not freezable.

--

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

At setup, this patch detects the MCUboot loader over clause-22 SB PDI,
before any clause-45 access. If the loader is found, the switch is
registered with only its CPU port so devlink can reflash it, and devlink
dev info reports 0.0.0. A background work item drains an interrupted
download one byte at a time and then schedules a re-probe.

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-fw.c b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> index 7a506d110c8da..0761132120b1a 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c

[ ... ]

> @@ -218,6 +268,379 @@ static void mxl862xx_flash_notify(struct devlink *dl, const char *status,

[ ... ]

> +static int mxl862xx_sb_pdi_poll_drain(struct mxl862xx_priv *priv,
> +				      unsigned long sleep_us,
> +				      unsigned long timeout_ms)
> +{
> +	int val;
> +
> +	read_poll_timeout(mxl862xx_smdio_read, val,
> +			  val < 0 || (u16)val == MXL862XX_SB_PDI_READY ||
> +			  (u16)val == 0 ||
> +			  test_bit(MXL862XX_FLAG_WORK_STOPPED, &priv->flags),
> +			  sleep_us, timeout_ms * 1000, false,
> +			  priv, MXL862XX_SB_PDI_STAT);
> +	if (val < 0)
> +		return val;
> +	if (test_bit(MXL862XX_FLAG_WORK_STOPPED, &priv->flags))
> +		return -ECANCELED;
> +	return (u16)val;
> +}

[Severity: Low]
This isn't a bug, but the @flags kernel-doc in struct mxl862xx_priv still
says MXL862XX_FLAG_WORK_STOPPED "is set before cancelling stats_work to
prevent rescheduling during teardown".

The flag now has a second job. It cancels the drain here and in
mxl862xx_rescue_drain(), and it also skips the reprobe hand-off in
mxl862xx_rescue_heal_work_fn(). Unbind and shutdown rely on it to stop a
long drain.

Should the kernel-doc mention this as well?

[ ... ]

> +static int mxl862xx_rescue_drain_finish(struct mxl862xx_priv *priv, u32 chunk)
> +{

[ ... ]

> +	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]
Can the "firmware booted while draining" branch be reached when
chunk == 0?

mxl862xx_rescue_drain() starts with chunk = 0 on every run. It does not
count the 1-byte slice-advance that mxl862xx_rescue_mode_detect() already
sent, or the settle wait.

Suppose WSP firmware boots before the worker feeds its first byte. That
can happen if detect's advance completed the image, or on the settle path
after mxl862xx_wait_ready() failed. Wouldn't this then poll for 300 s,
return -EIO and set rescue_failed?

After that there is no reprobe, and rescue_mode stays true, so
mxl862xx_api_wrap() returns -ENODEV for every command. In rescue mode,
mxl862xx_port_setup() also returned before mxl862xx_port_state(false) and
the single-port bridge setup.

Would the running firmware then forward traffic with its default setup
while the driver treats the switch as an inert loader, until someone
rebinds it by hand?

[ ... ]

> +	stat = mxl862xx_smdio_read(priv, MXL862XX_SB_PDI_STAT);
> +	if (stat < 0)
> +		return stat;
> +
> +	/* Flashless-download loop (MxL86281S tier): this driver does not
> +	 * support it -- the console flash path expects READY. Treat it as an
> +	 * unusable configuration, like any other unsupported state.
> +	 */
> +	if ((u16)stat == MXL862XX_SB_PDI_DL_READY)
> +		return -EOPNOTSUPP;

[Severity: Low]
The file header says the loader keeps the host's byte count in STAT while
it programs a chunk. For the final slice, mxl862xx_sb_pdi_flush_last()
writes data_written, which can be any value from 1 to 65520. The values
0xc33c, 0xc55c, 0xf48f and 0xf490 all fall in that range.

Can detection misread STAT if probe runs while an interrupted host's
final chunk of one of these sizes is still being programmed or verified?
These checks run before any settle wait:

  0xc33c: probe fails with -EOPNOTSUPP
  0xc55c: the RDREG challenge goes to a loader outside its console loop,
          and probe fails with -ENXIO
  0xf48f/0xf490: rescue_failed is set, although the loader would finish
                 on its own

[ ... ]

> +	if ((u16)stat == MXL862XX_SB_PDI_START ||
> +	    (u16)stat == MXL862XX_SB_PDI_START + 1) {
> +		WRITE_ONCE(priv->rescue_failed, true);
> +		return MXL862XX_IN_RESCUE;
> +	}

[Severity: Low]
At probe time this is the only place that sets rescue_failed, and no
drain has run yet. rescue_heal_work is only queued after the
rescue_failed check in mxl862xx_setup_rescue().

Even so, mxl862xx_setup_rescue() logs "download recovery gave up", and
mxl862xx_devlink_flash_update() returns the extack "download recovery
failed".

Could these messages say that the download handshake is stuck, rather
than pointing at a recovery that never ran?

[ ... ]

> @@ -486,6 +911,25 @@ int mxl862xx_devlink_info_get(struct dsa_switch *ds,
>  	char buf[16];
>  	int ret;
>  
> +	/* No chip-id/revision in MCUboot (needs the firmware MMD mailbox). The
> +	 * fw version doubles as the "ready to flash" signal: report it only
> +	 * once the loader is at a clean READY, nothing while still draining.
> +	 */
> +	if (READ_ONCE(priv->rescue_mode)) {
> +		if (!READ_ONCE(priv->rescue_ready))
> +			return 0;
> +
> +		snprintf(buf, sizeof(buf), "%u.%u.%u",
> +			 priv->fw_version.major, priv->fw_version.minor,
> +			 priv->fw_version.revision);
> +		ret = devlink_info_version_running_put(req,
> +				DEVLINK_INFO_VERSION_GENERIC_FW, buf);
> +		if (ret)
> +			return ret;
> +		return devlink_info_version_stored_put(req,
> +				DEVLINK_INFO_VERSION_GENERIC_FW, buf);
> +	}

[Severity: Low]
Documentation/networking/devlink/devlink-info.rst defines the stored group
as the versions in device flash. It also says a device that cannot keep
those versions accurate must not report them.

The file header says MCUboot is also entered through the sticky rescue
bit, so the flash may still hold a valid image with a real version.

Is it intended to report stored 0.0.0 here? That value means "ready to
flash" rather than describing what is in flash.

[ ... ]

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-host.c b/drivers/net/dsa/mxl862xx/mxl862xx-host.c
> index 4b3956a518cfe..694c22d2dd094 100644
> --- a/drivers/net/dsa/mxl862xx/mxl862xx-host.c
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-host.c

[ ... ]

> @@ -347,6 +348,11 @@ int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
>  		goto out;
>  	}
>  
> +	if (priv->rescue_mode) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +

[Severity: Medium]
In rescue mode the CPU port and the conduit are still set up, so DSA's
conduit ethtool override stays in place. ETHTOOL_GSTATS needs no
CAP_NET_ADMIN, so any local user can reach this path:

ethtool -S <conduit>
  dsa_conduit_get_ethtool_stats()
    dsa_conduit_append_port_stats()
      mxl862xx_get_ethtool_stats(ds, cpu_port)
        mxl862xx_read_rmon()
          mxl862xx_api_wrap() returns -ENODEV here

mxl862xx_get_ethtool_stats() then does this, with no ratelimit:

	ret = mxl862xx_read_rmon(ds, port, &cnt);
	if (ret) {
		dev_err(ds->dev, "failed to read RMON stats on port %d\n", port);
		return;
	}

Rescue mode can last indefinitely: a stuck handshake, a long drain, or a
loader waiting for a flash. Can an unprivileged user flood the kernel log
this way?

port_enable/disable, stp_state_set and mdb_add/del now have rescue_mode
guards. Should the stats callbacks get one too?

[ ... ]

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx.c b/drivers/net/dsa/mxl862xx/mxl862xx.c
> index 33a7cdb8edd31..911114579f100 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)
> +{

[ ... ]

> +	/* 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: Low]
system_long_wq is not freezable. The driver also has no PM callback that
sets MXL862XX_FLAG_WORK_STOPPED or cancels this work.

Can a drain that runs for tens of minutes keep accessing the MDIO bus
while the bus controller suspends? Any bus error in that window makes
mxl862xx_rescue_heal_work_fn() set rescue_failed with no retry. After
that, devlink flash returns -EIO until the driver is rebound.

device_schedule_reprobe() uses system_freezable_wq. Would a freezable
workqueue fit better here as well?

>  static int mxl862xx_setup(struct dsa_switch *ds)
>  {
>  	struct mxl862xx_priv *priv = ds->priv;
>  	int n_user_ports = 0, max_vlans;
>  	int ingress_finals, vid_rules;
>  	struct dsa_port *dp;
> -	int ret, i;
> +	int ret, i, rescue;
>  
> -	ret = mxl862xx_reset(priv);
> -	if (ret)
> -		return ret;
> +	/* Detect the loader over SB PDI first: it needs no firmware, unlike the
> +	 * C45 API (mxl862xx_reset/wait_ready), which spends its whole 10 s
> +	 * window on a mailbox nobody answers. Touch C45 only once rescue is
> +	 * ruled out.
> +	 */
> +	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;
> +	}

[Severity: High]
Can this break probe on switches that worked before this patch?

Previously the driver only used clause-45 accesses, so a switch with
working firmware probed even when SB PDI was unusable. Now any negative
return from mxl862xx_rescue_mode_detect() fails setup before
mxl862xx_reset() or mxl862xx_wait_ready() is tried.

These cases return an error here:

- The host MDIO bus has no clause-22 ops. In that case
  mxl862xx_smdio_write() -> __mdiobus_write() returns -EOPNOTSUPP because
  bus->write is NULL. For example, mvmdio in BUS_TYPE_XSMI mode sets only
  read_c45/write_c45.

- The ADDR/DATA scratch readback does not match. The kernel-doc says this
  includes "one whose SB PDI window is not at the offsets above". The file
  header says the 0xe1xx offsets are only the OTP reset defaults.

- STAT reads 0xc33c (-EOPNOTSUPP), or it reads 0xc55c and the marker is
  left untouched (-ENXIO).

Every normal boot also writes to the SB PDI mailbox now. If STAT reads 0,
detect sends a 1-byte download chunk and returns MXL862XX_IN_RESCUE
whatever follows. That relies on 0 being "the one value a running
firmware never holds". Is that guaranteed?

Would it be safer to fall back to the clause-45 path when detection
fails, instead of failing probe?

The commit message only says "Detect the loader over SB PDI at setup,
ahead of any clause-45 access". The v10 and v16 changelog notes say the
message was extended to cover the scratch writes to a running firmware,
the -ENODEV/-ENXIO outcomes and the handshake outcome. That text is not
in the final commit message. Could it be added back?

> -	ret = mxl862xx_wait_ready(ds);
> -	if (ret)
> -		return ret;
> +	if (rescue == MXL862XX_NOT_RESCUE) {
> +		ret = mxl862xx_reset(priv);
> +		if (ret)
> +			return ret;
> +
> +		ret = mxl862xx_wait_ready(ds);
> +		if (ret) {
> +			/* the reset may only now have triggered rescue mode */
> +			rescue = mxl862xx_rescue_mode_detect(priv, true);
> +			if (rescue < 0) {
> +				dev_err(ds->dev,
> +					"switch not responding after reset: %pe\n",
> +					ERR_PTR(rescue));
> +				return rescue;
> +			}
> +			if (rescue == MXL862XX_NOT_RESCUE)
> +				return ret;

[Severity: Medium]
Can mxl862xx_rescue_mode_detect(priv, true) ever return
MXL862XX_NOT_RESCUE? With settle set, each branch returns either an error
or MXL862XX_IN_RESCUE:

  DL_READY: -EOPNOTSUPP
  READY: IN_RESCUE or -ENXIO
  START/ACK: IN_RESCUE
  other non-zero: the !settle return is skipped. The poll then returns
                  IN_RESCUE for READY or non-zero, or falls into the
                  STAT == 0 path, which also returns IN_RESCUE

If so, this check is dead code, and the -ETIMEDOUT from
mxl862xx_wait_ready() is always dropped.

Take a firmware that is present but fails mxl862xx_wait_ready(): for
example, it boots slower than the timeout, reports iv_major == 0, or
fails CFGGET. It holds a stable non-zero STAT word. Wouldn't it be
classified as rescue, bound with all user ports unused, and have
rescue_heal_work queued?

The drain then calls mxl862xx_rescue_drain_finish(priv, 0), waits 300 s,
returns -EIO and sets rescue_failed. Before this patch, probe simply
failed with -ETIMEDOUT.

> +		}
> +	}

[ ... ]

> @@ -1603,7 +1677,8 @@ static int mxl862xx_port_mdb_add(struct dsa_switch *ds, int port,
>  	 * rebuilds the configuration. See mxl862xx_port_mdb_del().
>  	 */
>  	if ((ret == -EBUSY && priv->block_host) ||
> -	    (ret == -ENODEV && priv->skip_teardown))
> +	    (ret == -ENODEV &&
> +	     (priv->skip_teardown || READ_ONCE(priv->rescue_mode))))
>  		return 0;
>  	if (ret)
>  		return ret;

[Severity: Low]
This isn't a bug, but the comment above still mentions only the flash
cases ("A flash blocks the API (-EBUSY) and its teardown drops the MAC
table (-ENODEV)"). The comment in mxl862xx_port_mdb_del() was updated to
say "its teardown or MCUboot drops the MAC table". Should this comment
be updated to match?

[ ... ]

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