Re: [PATCH net-next v19 4/6] net: dsa: mxl862xx: add devlink flash_update and info_get

[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] In this patch, mxl862xx_devlink_flash_update() never checks that
  the host MDIO bus can do clause-22 transactions before sending the…

--

net: dsa: mxl862xx: add devlink flash_update and info_get

This patch adds devlink dev info, which reports the asic.id, asic.rev
and firmware version. It also adds devlink dev flash, which reboots the
switch into its MCUboot loader with SYS_MISC_FW_UPDATE and sends the
image over the clause-22 SB PDI SMDIO protocol. After the flash, the
driver reinitialises through a deferred re-probe.

> diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-fw.c b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> new file mode 100644
> index 0000000000000..eae7048707870
> --- /dev/null
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c

[ ... ]

> +static int mxl862xx_flash_firmware(struct mxl862xx_priv *priv,
> +				   const struct firmware *fw,
> +				   u32 payload_size, struct devlink *dl)
> +{
> +	const u8 *payload = fw->data + MXL862XX_FW_HDR_SIZE;
> +	u32 word_idx = 0, data_written = 0, idx = 0;
> +	unsigned long next_notify = jiffies - 1;
> +	u16 word, fdata;
> +	int ret, val, i;
> +
> +	/* Step 1: reboot the firmware into MCUboot rescue mode */
> +	ret = mxl862xx_api_wrap(priv, SYS_MISC_FW_UPDATE, NULL, 0,
> +				false, false);
> +	if (ret) {
> +		dev_err(&priv->mdiodev->dev,
> +			"flash: FW_UPDATE command failed: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	/* Step 2: wait for bootloader ready */
> +	mxl862xx_flash_notify(dl, "Waiting for bootloader", 0, 0);
> +	ret = mxl862xx_sb_pdi_reset(priv);
> +	if (ret < 0)
> +		goto write_err;

[Severity: Low]
What happens here if the host MDIO bus only supports clause-45?

The normal firmware command path uses only clause-45 accessors:

drivers/net/dsa/mxl862xx/mxl862xx-host.c:mxl862xx_reg_read() {
	return __mdiodev_c45_read(priv->mdiodev, MDIO_MMD_VEND1, addr);
}

So in this patch the driver can bind and run on a bus that has no
bus->read or bus->write. The SB PDI path goes through clause-22 instead:

mxl862xx_sb_pdi_reset()
  mxl862xx_smdio_write()
    __mdiobus_write()

and __mdiobus_write() does:

	if (bus->write)
		err = bus->write(bus, addr, regnum, val);
	else
		err = -EOPNOTSUPP;

On such a bus, the first SB PDI access fails with -EOPNOTSUPP. By then
SYS_MISC_FW_UPDATE has already set the sticky rescue bit and rebooted
the switch into MCUboot. MCUboot only offers the clause-22 SB PDI
window, so the driver has no way to reach the switch again.

Could the clause-22 capability be checked before the irreversible
SYS_MISC_FW_UPDATE is sent, for example in
mxl862xx_devlink_flash_update() before any ports are closed?

The next patch in the series, "net: dsa: mxl862xx: recover switch stuck
in MCUboot rescue mode", appears to make this unreachable.
mxl862xx_setup() there calls mxl862xx_rescue_mode_detect() over SMDIO
before any clause-45 access. On a clause-45-only bus that fails, setup
returns an error ("switch in an unusable state"), and the driver never
binds. So the problem only shows up when bisecting to this patch.

[ ... ]

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