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

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

[netdev-bot+sashiko]

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

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

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).

- [Medium] The new Documentation/networking/devlink/mxl862xx.rst (lines
  82-87) gives different remedies for different rescue-recovery failures.
- [Low] In the fw row of Documentation/networking/devlink/mxl862xx.rst
  (lines 47-49), the sentence "Use ``devlink dev flash`` to tell a…

--

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

This commit adds Documentation/networking/devlink/mxl862xx.rst. The file
describes the mxl862xx devlink info versions and the flash update
behaviour, including MCUboot rescue mode recovery. It also links the file
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..c2dfc72ef9a40
> --- /dev/null
> +++ b/Documentation/networking/devlink/mxl862xx.rst

[ ... ]

> +   * - ``fw``
> +     - running, stored
> +     - 1.0.70
> +     - Version of the firmware running on the switch, reported as both
> +       running and stored since the switch boots it from its own flash.

[ ... ]

> +       running only, since the driver cannot tell what the flash holds
> +       while the loader runs. Use ``devlink dev
> +       flash`` to tell a recovering switch from a ready one, see below;
> +       a missing version on its own does not say why.

[Severity: Low]

Is this the right pair of states? mxl862xx_devlink_info_get() already
shows whether a rescue-mode switch is ready, without changing anything:

	if (READ_ONCE(priv->rescue_mode)) {
		if (!READ_ONCE(priv->rescue_ready))
			return 0;
		...
		return devlink_info_version_running_put(req,
				DEVLINK_INFO_VERSION_GENERIC_FW, buf);
	}

On a ready switch, devlink dev flash does not report a state. It closes
the ports and flashes the image.

Two states omit the version: a switch that is still recovering, and one
whose recovery failed. Those are the two that flash tells apart, with
-EBUSY "switch is recovering an interrupted download" and -EIO "loader
cannot accept a download".

Also, mxl862xx_devlink_flash_update() validates the image before either
of those checks:

	ret = mxl862xx_flash_validate(priv, params->fw, &payload_size);
	if (ret) {
		NL_SET_ERR_MSG_MOD(extack, "firmware image validation failed");
		return ret;
	}

	/* Refuse to flash while the background self-heal is still draining, and
	 * for good once it has given up on the loader.
	 */
	if (READ_ONCE(priv->rescue_failed)) {
	...
	if (READ_ONCE(priv->rescue_mode) && !READ_ONCE(priv->rescue_ready)) {

So the documented -EBUSY and -EIO only appear when a valid signed image is
supplied. A placeholder file gets the validation error instead. Could the
document mention that?

[ ... ]

> +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

[Severity: Medium]

How can a user tell these cases apart? Take the case of a completed drain
whose reprobe could not be scheduled. mxl862xx_rescue_heal_work_fn() only
sets the flag and logs nothing:

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

The drain error path just above it logs "flash: download recovery failed:
%pe". The flash path's own reprobe failure in
mxl862xx_devlink_flash_update() also logs a message:

	err = device_schedule_reprobe(ds->dev, MXL862XX_FW_REPROBE_DELAY_MS);
	if (err)
		dev_err(ds->dev,
			"flash: re-probe could not be scheduled (%pe); unbind and rebind to reinitialise\n",
			ERR_PTR(err));

The heal path seems to be the only one that says nothing.

After that, mxl862xx_devlink_flash_update() returns the same -EIO and
extack for every rescue_failed cause:

	if (READ_ONCE(priv->rescue_failed)) {
		NL_SET_ERR_MSG_MOD(extack, "loader cannot accept a download");
		return -EIO;
	}

In the completed-drain case the extack is also wrong. The drain finished,
so the loader is at READY or a firmware has booted.

The v18 changelog says "a re-probe that cannot be scheduled is logged on
every path". That doesn't seem to hold for the heal path.

The errno and extack are the same and the log is empty. Is there a way
for a user to choose between the rebind and the power cycle described
here? The @rescue_failed kernel-doc in mxl862xx.h also sends readers to
this document, saying "the remedy depends on the cause".

The list of causes also leaves out two drain failures from
mxl862xx_rescue_drain() and mxl862xx_rescue_drain_finish(): -ETIMEDOUT
"did not drain after %u chunks" and -EIO "loader still busy after the
erase window". Which remedy applies to those?

> +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.

[ ... ]

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

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

[Daniel Golle]

Describe the devlink info versions and the flash update behaviour,
including the MCUboot rescue mode recovery, in a dedicated file under
Documentation/networking/devlink/ and link it from the index. Add the
new file to the driver's MAINTAINERS entry.

Assisted-by: LLM
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v18:
 - the rescue-mode firmware version is reported as running only, and a
   re-probe that cannot be scheduled is logged on every path and leaves
   later flashes refused with -EBUSY until a rebind (found by Sashiko AI
   review)
v17: no changes
v16:
 - document that a reboot waits for a running flash and that one
   requested afterwards is refused, and that a bus error ends the
   download recovery for good
 - title-case the "Flash Update" heading, as the other devlink driver
   documents do
 - say that the ports come back down from a flash, that a reprobe which
   cannot be scheduled needs a rebind, and which of the two recovery
   failures needs a power cycle and which a rebind (found by Sashiko AI
   review)
v15:
 - asic.rev is read from the CHIP ID registers as well, not from one
   register word shared with asic.id (found by Sashiko AI review)
 - -EIO says the driver gave up on the recovery, which may need a driver
   rebind rather than a power cycle, and an interrupted opening
   handshake is reported the same way (found by Sashiko AI review)
v14: no changes
v13: no changes
v12: no changes
v11: no changes
v10: document that a switch power cycled on its own needs the driver
     unbound and rebound before a failed recovery is re-examined
v9: no changes, picked up Andrew's v5 Reviewed-by
v8:
 - asic.id and asic.rev are omitted whenever the part number reads
   zero, not only in MCUboot rescue mode (found by Sashiko AI review)
 - drop the claim that "0.0.0" marks a switch that never ran firmware;
   rescue mode always reports it (found by Sashiko AI review)
 - document devlink dev flash as the signal that says whether a
   recovery is still running, including the -EIO it returns once the
   recovery has failed (found by Sashiko AI review)
v7: no changes
v6: no changes
v5: new patch, splitting the devlink documentation out of the flash
    update and rescue mode recovery patches so each keeps to code
    (Jakub Kicinski asked for the documentation)

 Documentation/networking/devlink/index.rst    |  1 +
 Documentation/networking/devlink/mxl862xx.rst | 95 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 3 files changed, 97 insertions(+)
 create mode 100644 Documentation/networking/devlink/mxl862xx.rst

diff --git a/Documentation/networking/devlink/index.rst b/Documentation/networking/devlink/index.rst
index 1af780c811ee..53d3ef16d13f 100644
--- a/Documentation/networking/devlink/index.rst
+++ b/Documentation/networking/devlink/index.rst
@@ -95,6 +95,7 @@ parameters, info versions, and other features it supports.
    mlx5
    mlxsw
    mv88e6xxx
+   mxl862xx
    netdevsim
    nfp
    octeontx2
diff --git a/Documentation/networking/devlink/mxl862xx.rst b/Documentation/networking/devlink/mxl862xx.rst
new file mode 100644
index 000000000000..c2dfc72ef9a4
--- /dev/null
+++ b/Documentation/networking/devlink/mxl862xx.rst
@@ -0,0 +1,95 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+========================
+mxl862xx devlink support
+========================
+
+This document describes the devlink features implemented by the
+``mxl862xx`` device driver.
+
+Info versions
+=============
+
+The ``mxl862xx`` driver reports the following versions
+
+.. list-table:: devlink info versions implemented
+   :widths: 5 5 5 85
+
+   * - Name
+     - Type
+     - Example
+     - Description
+   * - ``asic.id``
+     - fixed
+     - 8628
+     - The chip part number read from the CHIP ID registers. Omitted
+       when the part number reads as zero, which happens for a switch
+       sitting in MCUboot rescue mode (the registers need a running
+       firmware), for an unfused part, and after a failed flash.
+   * - ``asic.rev``
+     - fixed
+     - 0
+     - The chip version, read from the CHIP ID registers as well. Both
+       values are published behind the same check, so it is omitted
+       whenever ``asic.id`` is.
+   * - ``fw``
+     - running, stored
+     - 1.0.70
+     - Version of the firmware running on the switch, reported as both
+       running and stored since the switch boots it from its own flash.
+       It is omitted while no firmware version is known: after a failed
+       flash, and in MCUboot rescue mode while an interrupted download
+       is still being recovered in the background. Once the loader is
+       ready to accept a new image the version appears as "0.0.0",
+       which no released firmware reports, so version-comparing tools
+       offer any available release as an upgrade; it is reported as
+       running only, since the driver cannot tell what the flash holds
+       while the loader runs. Use ``devlink dev
+       flash`` to tell a recovering switch from a ready one, see below;
+       a missing version on its own does not say why.
+
+Flash Update
+============
+
+The ``mxl862xx`` driver implements support for ``devlink dev flash``.
+The signed firmware image is transferred to the switch over the same
+MDIO bus which is also used to manage the switch, then verified and
+installed by the MCUboot bootloader running on the switch. All ports
+of the switch are closed for the duration of the update and the driver
+reprobes the switch after it has rebooted into the new firmware; they
+come back registered but down, so userspace brings them up again. A
+complete flash and reprobe cycle takes about one minute. In the rare
+case that the reprobe cannot be scheduled at all, the kernel log says
+so, ``devlink dev flash`` reports that error or the transfer's own if
+the transfer failed as well, and the driver stays bound to a switch it
+no longer tracks, refusing further updates with ``-EBUSY``, until it is
+unbound and rebound. A reboot started
+while an update is running waits for the transfer to finish, and an
+update requested after the system has begun shutting down is refused
+with ``-ENODEV``.
+
+A switch stuck in MCUboot rescue mode, e.g. after an interrupted
+update, is registered without user ports. If the previous download was
+interrupted mid-transfer the loader is wedged; the driver drains it
+back to a clean ready state in the background, one byte at a time,
+which takes tens of minutes for a large image and is reported through
+the kernel log as it progresses. During that recovery ``devlink dev
+flash`` returns ``-EBUSY`` with an extack message saying so, and
+``devlink dev info`` reports no firmware version. Once the loader is
+ready the firmware version appears and flashing a firmware image
+through the regular update flow recovers the switch.
+
+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.
+
+A download interrupted during its opening handshake, before the image
+header reached the loader, is reported the same way. The loader waits
+for a header that no later session can supply, so that state needs a
+power cycle.
diff --git a/MAINTAINERS b/MAINTAINERS
index 6de1ff058db6..cb1dad080f63 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16250,6 +16250,7 @@ M:	Daniel Golle <daniel@makrotopia.org>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/net/dsa/maxlinear,mxl862xx.yaml
+F:	Documentation/networking/devlink/mxl862xx.rst
 F:	drivers/net/dsa/mxl862xx/
 F:	net/dsa/tag_mxl862xx.c
 
-- 
2.55.0