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

Re: [PATCH net-next v14 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: 0 · Low: 2

- [Low] Documentation/networking/devlink/mxl862xx.rst (new "Flash update"
  section, lines 70-74) states unconditionally that `-EIO` from `devlink…
- [Low] Documentation/networking/devlink/mxl862xx.rst describes
  ``asic.rev`` as "The chip version read from the same register word" as…

--

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

Adds a new Documentation/networking/devlink/mxl862xx.rst describing the
devlink info versions and the flash update flow of the mxl862xx DSA
driver, including the MCUboot rescue mode recovery. The file is linked
from the devlink index and added to the driver's MAINTAINERS entry.

Two questions about the wording of the new document below.

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

[ ... ]

> +   * - ``asic.rev``
> +     - fixed
> +     - 0
> +     - The chip version read from the same register word, so it is
> +       omitted whenever ``asic.id`` is.

[Severity: Low]
Is "the same register word" accurate here?  mxl862xx_read_chip_id() in
mxl862xx.c reads two separate words and builds the part number from both:

	reg.addr = cpu_to_le32(MXL862XX_CHIPID_L);
	...
	reg.addr = cpu_to_le32(MXL862XX_CHIPID_M);
	...
	priv->asic_id = FIELD_GET(MXL862XX_CHIPID_L_PNUML, chipid_l) |
			FIELD_GET(MXL862XX_CHIPID_M_PNUMM, chipid_m) << 4;
	priv->asic_rev = FIELD_GET(MXL862XX_CHIPID_M_VERSION, chipid_m);

So the revision comes only from MXL862XX_CHIPID_M, while the part number
spans MXL862XX_CHIPID_L and MXL862XX_CHIPID_M.  The adjacent asic.id row
already says "CHIP ID registers" in the plural.

The row's conclusion still holds, but for a different reason: both puts in
mxl862xx_devlink_info_get() sit behind one gate,

	if (priv->asic_id) {

and a failed read of either word leaves both cached values at zero.  Would
it be clearer to say the revision comes from the CHIP ID registers as well
and that both are omitted because they share that gate?

[ ... ]

> +If the recovery fails, the loader needs a power cycle: ``devlink dev
> +flash`` then returns ``-EIO`` and says so in its extack message. 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]
Does -EIO always mean the loader needs a power cycle?  The only site
producing that errno is mxl862xx_devlink_flash_update() in mxl862xx-fw.c:

	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() also sets that flag after a drain that
succeeded:

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

device_schedule_reprobe() in drivers/base/dd.c returns -ENOMEM when its
kzalloc_obj() fails, and -EINVAL when the device raced an unbind.  In that
case the loader is back at READY and only the reprobe hand-off was lost, so
what is actually needed is an unbind/rebind rather than a power cycle -
rescue_ready is only ever set by mxl862xx_rescue_mode_detect() at probe.

The flag is also set when mxl862xx_rescue_drain() aborts on a transient
SMDIO error or -ETIMEDOUT, not just on the loader-stuck verdict from
mxl862xx_rescue_drain_finish().

Could the sentence be softened to say -EIO means the driver gave up on the
recovery and the switch needs a power cycle and/or a driver rebind before it
is re-examined?

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

[PATCH net-next v14 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.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
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 | 74 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 3 files changed, 76 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..5b24d8cb57bc
--- /dev/null
+++ b/Documentation/networking/devlink/mxl862xx.rst
@@ -0,0 +1,74 @@
+.. 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 same register word, 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 recovery fails, the loader needs a power cycle: ``devlink dev
+flash`` then returns ``-EIO`` and says so in its extack message. 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.
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