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