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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] mxl862xx.rst says that "In the rare case that the reprobe cannot
  be scheduled at all, ``devlink dev flash`` reports that error and the…
- [Low] The "Rescue lifecycle" overview comment at the top of
  drivers/net/dsa/mxl862xx/mxl862xx-fw.c says "devlink dev info exposes…

--

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

This adds Documentation/networking/devlink/mxl862xx.rst. The new file
describes the devlink info versions and how flash update behaves,
including recovery from MCUboot rescue mode. The file is linked from the
devlink index and added 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
> +============
> +
> +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, ``devlink dev
> +flash`` reports that error and the driver stays bound to a switch it
> +no longer tracks until it is unbound and rebound. A reboot started

[Severity: Low]
Does this hold on every path through mxl862xx_devlink_flash_update()?
The reprobe scheduling error is only returned and logged when the
transfer succeeded:

drivers/net/dsa/mxl862xx/mxl862xx-fw.c:mxl862xx_devlink_flash_update() {
    ...
	priv->skip_teardown = true;
	mutex_unlock(&priv->mdiodev->bus->mdio_lock);
    ...
	err = device_schedule_reprobe(ds->dev, MXL862XX_FW_REPROBE_DELAY_MS);
	if (!ret && err)
		dev_err(ds->dev,
			"flash: new firmware is running but re-probe could not be scheduled (%pe); unbind and rebind to reinitialise\n",
			ERR_PTR(err));

	return ret ? ret : err;
}

Suppose mxl862xx_flash_firmware() or mxl862xx_wait_ready() failed, and
device_schedule_reprobe() then also returned -ENOMEM or -EINVAL. In that
case the reprobe error is not returned, and the dev_err() that tells the
user to rebind is skipped.

skip_teardown has already been set unconditionally. With no reprobe
pending, nothing clears it. Would every later flash then fail here until
a manual rebind?

	if (priv->skip_teardown) {
		NL_SET_ERR_MSG_MOD(extack,
				   "a previous flash awaits its reprobe");
		return -EBUSY;
	}

The document says -EBUSY only means that a rescue recovery is in
progress. Should the code report the reprobe failure on this path too?
Or should the document mention this -EBUSY and the rebind it needs?

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

[ ... ]

> +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: Low]
This paragraph documents the -EIO case. The rescue lifecycle comment at
the top of drivers/net/dsa/mxl862xx/mxl862xx-fw.c was added earlier in
this series, and it still says:

 * devlink dev info exposes the fw version (the "flashable" signal) only once at
 * READY; flash_update returns -EBUSY until then, and reprobes to WSP firmware
 * on success.

However, once rescue_failed is set, mxl862xx_devlink_flash_update()
returns -EIO permanently:

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

rescue_failed is set in three cases:

- mxl862xx_rescue_heal_work_fn(), when the drain fails
- mxl862xx_rescue_heal_work_fn(), when the post-drain reprobe cannot be
  scheduled
- mxl862xx_rescue_mode_detect(), for an interrupted opening handshake

In each of these cases the loader never reaches READY.

Could that comment be updated to mention the -EIO state, so that it
matches both the code and this document?

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

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