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

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

[netdev-bot+sashiko]

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

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

- [Medium] Documentation vs implementation mismatch on the `fw` info
  version.
- [Low] The Flash update section says "All ports of the switch are closed
  for the duration of the update and the driver reprobes the switch…
- [Low] Every `rescue_failed` cause is reported by
  mxl862xx_devlink_flash_update() (mxl862xx-fw.c:998-1001) with the…

--

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 behaviour of the mxl862xx
driver, links it from the devlink index and adds the file to the driver's
MAINTAINERS entry.

A few questions below about places where the new document and the driver
code seem to describe different behaviour.

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

[ ... ]

> +   * - ``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. 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: Medium]
Does the "omitted ... after a failed flash" wording hold for a flash that
was started from rescue mode?

mxl862xx_devlink_info_get() returns early on the rescue path and publishes
the cached version without the all-zero test that guards the normal path:

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

On failure mxl862xx_devlink_flash_update() zeroes the cache but only the
success path clears rescue_mode:

	if (ret) {
		memset(&priv->fw_version, 0, sizeof(priv->fw_version));
		priv->asic_id = 0;
		priv->asic_rev = 0;
	}

so rescue_mode and rescue_ready both stay set and devlink dev info keeps
reporting fw running/stored "0.0.0" - which this document defines as "the
loader is ready to accept a new image" - while the loader was actually
left mid transfer by the no_end path and devlink dev flash answers -EBUSY
("device is reinitializing, retry later") because skip_teardown is set.

Normally this window lasts until the deferred reprobe fires, but if
device_schedule_reprobe() fails with -ENOMEM nothing else re-classifies
the switch, so it persists. Should either the rescue branch also suppress
an all-zero version, or the document describe this case?

> +
> +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. A
> +complete flash and reprobe cycle takes about one minute.

[Severity: Low]
This isn't a bug, but "closed for the duration of the update" reads as if
the pre-flash administrative state comes back once the cycle completes.

mxl862xx_devlink_flash_update() closes and detaches the user ports and
closes the conduit:

	dsa_switch_for_each_user_port(dp, ds) {
		if (dp->user) {
			dev_close(dp->user);
			netif_device_detach(dp->user);
		}
	}
	dsa_switch_for_each_cpu_port(dp, ds)
		dev_close(dp->conduit);

and the rest of the function never records the previous state nor reopens
anything before scheduling the reprobe, so the interfaces come back
registered but down and userspace has to bring them up again. The commit
message of "net: dsa: mxl862xx: add devlink flash_update and info_get"
does say this ("The closed user ports and conduit are not returned to
their pre-flash administrative state across the reprobe; userspace brings
them back up"). Could that sentence be mentioned here as well?

[ ... ]

> +If the driver gives up on the recovery, ``devlink dev flash`` returns
> +``-EIO`` and says so in its extack message. The switch then needs a
> +power cycle, a driver rebind, or both: the driver only re-examines the
> +switch when it binds, so on a board where the switch can be power
> +cycled on its own, unbind and rebind the driver afterwards to have the
> +recovered switch recognised.

[Severity: Low]
This paragraph and the extack text in the driver seem to disagree about
the remedy.

mxl862xx_devlink_flash_update() always advises a power cycle:

	if (READ_ONCE(priv->rescue_failed)) {
		NL_SET_ERR_MSG_MOD(extack,
				   "download recovery failed, power cycle the switch");
		return -EIO;
	}

but mxl862xx_rescue_heal_work_fn() sets rescue_failed for two different
situations. One is a drain error, where a power cycle is the right advice.
The other is a successful drain whose hand-off failed:

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

In that second case the loader is already sitting at a clean READY and
only the driver needs to re-examine it, yet flashing stays refused with
-EIO for the lifetime of the binding and a power cycle alone does not
clear the flag. Would it make sense for the extack to mention the rebind
too, so it matches what this document says?

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

[PATCH net-next v15 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>
---
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 | 81 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 3 files changed, 83 insertions(+)
 create mode 100644 Documentation/networking/devlink/mxl862xx.rst

diff --git a/Documentation/networking/devlink/index.rst b/Documentation/networking/devlink/index.rst
index d4a83fdcff7f..145cb8a4becf 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..7cbd538a5f3d
--- /dev/null
+++ b/Documentation/networking/devlink/mxl862xx.rst
@@ -0,0 +1,81 @@
+.. 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. 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. A
+complete flash and reprobe cycle takes about one minute.
+
+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 switch then needs a
+power cycle, a driver rebind, or both: the driver only re-examines the
+switch when it binds, so on a board where the switch can be power
+cycled on its own, unbind and rebind the driver afterwards to have the
+recovered switch 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 b23fb6f2f4ef..c97e710dda0c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16246,6 +16246,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