Re: [PATCH net-next v16 6/6] net: dsa: mxl862xx: document devlink flash and info support

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The new user-facing document and the in-tree driver artifacts
  disagree on the remedy for a failed rescue recovery.

--

net: dsa: mxl862xx: document devlink flash and info support

This adds Documentation/networking/devlink/mxl862xx.rst describing the
devlink info versions and the flash update flow of the mxl862xx driver,
including the MCUboot rescue mode recovery, links it from the devlink
index and adds it to the driver's MAINTAINERS entry.

> diff --git a/Documentation/networking/devlink/mxl862xx.rst b/Documentation/networking/devlink/mxl862xx.rst
> new file mode 100644
> index 0000000000000..793f3521f505f
> --- /dev/null
> +++ b/Documentation/networking/devlink/mxl862xx.rst
> @@ -0,0 +1,91 @@

[ ... ]

> +Flash Update
> +============

[ ... ]

> +If the driver gives up on the recovery, ``devlink dev flash`` returns
> +``-EIO`` and says so in its extack message. The drain runs once and is
> +never resumed, so a failed MDIO transaction ends it as well. A loader
> +that stops answering the drain needs a power cycle; a completed drain
> +whose reprobe could not be scheduled, and a drain a bus error cut
> +short, need only a driver rebind. The driver re-examines the switch
> +when it binds and at no other time, so a power cycle on a board where
> +the switch can be cycled on its own still has to be followed by an
> +unbind and rebind for the recovered switch to be recognised.

[Severity: Medium]

This new text says two of the three rescue_failed causes need only a
driver rebind, but the in-tree kernel-doc and the log message say a power
cycle is needed in all cases.  Which of the three is right?

The kernel-doc for the flag in struct mxl862xx_priv (mxl862xx.h) makes no
distinction:

 * @rescue_failed:      (rescue_mode) the self-heal gave up or cannot run; the
 *                      loader needs a power cycle and no flash can be accepted

and mxl862xx_setup_rescue() in mxl862xx.c prints the same advice for every
cause:

	if (priv->rescue_failed) {
		dev_warn(ds->dev,
			 "switch in MCUboot with an unfinishable download, power cycle it\n");
		return;
	}

mxl862xx_rescue_heal_work_fn() in mxl862xx-fw.c sets the flag both for any
drain error, which includes a plain SMDIO/bus write failure:

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

and after a fully successful drain when only the reprobe hand-off failed:

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

Since rescue_failed is a plain bool in the per-binding priv, an unbind and
rebind starts from false and re-runs mxl862xx_rescue_mode_detect(), which
matches what the .rst says for those two cases.  Only the unfinishable
opening handshake path in mxl862xx_rescue_mode_detect() looks like it
genuinely needs a power cycle.

Should the kernel-doc comment and the dev_warn() text be narrowed the same
way, so an operator reading dmesg is not sent to power cycle (possibly
physically reset) a switch that a rebind would recover?

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