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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] AB-BA deadlock between the devlink instance lock and the driver-
  core device lock.
- [Low] drivers/net/dsa/mxl862xx/Kconfig does not `select CRC32` although
  the new mxl862xx-fw.c calls crc32_le() twice in…

--

net: dsa: mxl862xx: add devlink flash_update and info_get

This adds devlink dev info version reporting (asic.id, asic.rev and the
running/stored firmware version) and devlink dev flash support for the
MxL862xx switch, transferring a validated image over the SB PDI protocol
after rebooting the switch into its MCUboot loader and then scheduling 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..119792eb7bd58
> --- /dev/null
> +++ b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> @@ -0,0 +1,648 @@

[ ... ]

> +#include <linux/crc32.h>

[ ... ]

> +	if (size1) {
> +		crc = ~crc32_le(~0U, payload, size1);
> +		if (crc != le32_to_cpu(hdr->image_checksum_1)) {
> +			dev_err(&priv->mdiodev->dev,
> +				"flash: image 1 CRC mismatch (got %08x, expected %08x)\n",
> +				crc, le32_to_cpu(hdr->image_checksum_1));
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (size2) {
> +		crc = ~crc32_le(~0U, payload + size1, size2);

[Severity: Low]

Should drivers/net/dsa/mxl862xx/Kconfig also select CRC32 for these two
new crc32_le() users in mxl862xx_flash_validate()?

The driver's Kconfig currently has:

config NET_DSA_MXL862
	tristate "MaxLinear MxL862xx"
	depends on NET_DSA
	select CRC16
	select NET_DSA_TAG_MXL_862XX

CRC16 is selected for the crc16() user in mxl862xx-host.c, but nothing
selects CRC32. In lib/crc/Kconfig, CRC32 is a bare tristate with no prompt
and no default:

config CRC32
	tristate
	select BITREVERSE
	help
	  The CRC32 library functions.  Select this if your module uses any of
	  the functions from <linux/crc32.h> or <linux/crc32c.h>.

and lib/crc/Makefile builds crc32.o under obj-$(CONFIG_CRC32), so with
CONFIG_CRC32=n (or CRC32=m while NET_DSA_MXL862=y) does this fail to link
with an undefined crc32_le? The select is still missing at the end of the
series.

[ ... ]

> +int mxl862xx_devlink_flash_update(struct dsa_switch *ds,
> +				  struct devlink_flash_update_params *params,
> +				  struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	mutex_lock_nested(&priv->mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +	priv->flash_owner = NULL;
> +	priv->block_host = false;
> +	priv->skip_teardown = true;
> +	mutex_unlock(&priv->mdiodev->bus->mdio_lock);
> +
> +	/* Reinitialise through a deferred re-probe: remove() runs with
> +	 * skip_teardown set, then a fresh probe() starts against whatever
> +	 * the switch now runs. The core skips the re-probe if the device
> +	 * is unbound or shut down before it fires.
> +	 */
> +	err = device_schedule_reprobe(ds->dev, MXL862XX_FW_REPROBE_DELAY_MS);

[Severity: High]

Can this call deadlock against a concurrent unbind of the same mdio
device?

.devlink_flash_update runs with the devlink instance lock held by the
netlink core:

devlink_nl_pre_doit()
  __devlink_nl_pre_doit()
    devlink_get_from_attrs_lock()
      devl_dev_lock(devlink, false)   /* devl_lock() only */

and device_schedule_reprobe() then takes device_lock() synchronously in
that context:

drivers/base/dd.c:device_schedule_reprobe() {
	...
	device_lock(dev);
	parent = dev->parent;
	if (!dev->bus || !dev->p || dev->p->dead ||
	    !device_is_registered(dev) || !dev->driver) {
	...
}

So this path is devl_lock -> device_lock.

The opposite order exists in the unbind path, where the driver core holds
device_lock across ->remove():

device_release_driver_internal()   /* holds device_lock(&mdiodev->dev) */
  mxl862xx_remove()
    dsa_unregister_switch()
      dsa_switch_teardown()
        dsa_switch_devlink_unregister()
          devlink_unregister()
            devl_lock()

devlink itself documents the same order in net/devlink/devl_internal.h:

static inline void devl_dev_lock(struct devlink *devlink, bool dev_lock)
{
	if (dev_lock && devlink->dev)
		device_lock(devlink->dev);
	devl_lock(devlink);
}

For DSA, devlink->dev is ds->dev, i.e. &mdiodev->dev, so both locks in
the cycle are the same two objects. If a sysfs unbind or rmmod runs while
devlink dev flash is in progress, does one thread end up holding
device_lock and waiting on devl_lock while the flash thread holds
devl_lock for the whole transfer and then waits on device_lock?

Both are plain mutexes with no trylock or timeout here, and
device_initialize() marks dev->mutex with lockdep_set_novalidate_class(),
so would lockdep report this cycle at all?

Would scheduling the re-probe from a context that does not hold the
devlink instance lock, or a variant of the helper that does not take
device_lock in the caller's context, avoid the inversion?

> +
> +	return ret ? ret : err;
> +}

Please double check this analysis, and thanks for your time reviewing it.

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