Re: [PATCH net-next v19 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 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] The new devlink document gives two different answers for what
  `devlink dev flash` returns after a flash whose reprobe could not be…

--

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

This commit adds Documentation/networking/devlink/mxl862xx.rst. The new
file describes the devlink info versions and the flash update behaviour
of the mxl862xx DSA driver, including recovery from MCUboot rescue mode.
It 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..6ec2c793fd0f3
> --- /dev/null
> +++ b/Documentation/networking/devlink/mxl862xx.rst
> @@ -0,0 +1,118 @@

[ ... ]

> +   * - ``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 until the reprobe it schedules, and in MCUboot rescue mode
> +       while an interrupted download is still being recovered in the
> +       background, once that recovery has failed, or while the loader
> +       waits in an opening handshake nobody can finish. 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. A missing version on its own does not say
> +       why; ``devlink dev flash``, given an image file that passes the
> +       driver's validation, answers ``-EBUSY`` while the switch is still
> +       recovering and ``-EIO`` once it cannot be flashed from this
> +       binding, see below.

[Severity: Low]
Does this rule still hold after a flash whose reprobe could not be
scheduled?

Every flash in mxl862xx_devlink_flash_update() ends by setting
skip_teardown and scheduling a reprobe. If scheduling fails, the driver
only logs it:

	priv->skip_teardown = true;
	...
	err = device_schedule_reprobe(ds->dev, MXL862XX_FW_REPROBE_DELAY_MS);
	/* -EINVAL is a device being deleted, whose teardown takes over. */
	if (err == -EINVAL)
		err = 0;

Here device_schedule_reprobe() can return -ENOMEM or -EBUSY, and
rescue_failed is not set. The rescue path in
mxl862xx_rescue_heal_work_fn() does set it:

	ret = device_schedule_reprobe(dev, MXL862XX_FW_REPROBE_DELAY_MS);
	if (ret && ret != -EINVAL) {
		dev_err(dev, ...);
		WRITE_ONCE(priv->rescue_failed, true);
	}

>From then on, every later flash returns early. This happens before
mxl862xx_flash_validate() and before the rescue_failed -EIO check:

	if (priv->skip_teardown) {
		NL_SET_ERR_MSG_MOD(extack,
				   "a previous flash left the driver awaiting reinitialisation");
		return -EBUSY;
	}

If the transfer had also failed, fw_version was zeroed. In that case
devlink dev info shows no fw version, and devlink dev flash returns
-EBUSY until an unbind and rebind.

Under the rule in this row, that looks like "still recovering". In fact
the switch can no longer be flashed from this binding, and this row says
that case should give -EIO.

The Flash Update section below describes this case correctly as a
permanent -EBUSY ("further updates refused with -EBUSY, until it is
unbound and rebound").

The later paragraph, however, starts with "If the switch cannot be
flashed from its binding, devlink dev flash returns -EIO". It also lists
"a reprobe that could not be scheduled" as one of the causes, but that
only applies to the reprobe after the rescue drain.

Should the fw row and the -EIO paragraph make an exception for a failed
post-flash reprobe? Or should the flash path also move to the -EIO
state?

[ ... ]

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