Re: [PATCH net-next v5 3/5] net: dsa: mxl862xx: add devlink flash_update and info_get

[Andrew Lunn]

> The switch leaves MCUboot on its own by booting the new image, but the
> driver has no in-place path back, so it reinitialises with a full
> device_reprobe() scheduled regardless of the transfer outcome -- after a
> failure the switch is still in MCUboot and probe re-detects it. During
> the teardown its API reads return -ENODEV and writes fake success, so it
> neither stalls on the absent firmware nor consumes buffers it never
> filled. The reprobe holds module and device references taken before the
> switch was disturbed and runs from a heap work item, not a kernel
> thread: remove() frees the devm-managed priv, and kthread_create() in
> the devlink caller would return -EINTR on interrupt or trip the
> hung-task watchdog while parked across the minute-long flash.

Just for my understanding....

The reprobe causes all the user point netdevs to be destroyed, and are
then recreated?

That seems like a good solution to the issue of all state information
has been lost in the switch.

> +static int mxl862xx_sb_pdi_poll_stat(struct mxl862xx_priv *priv, u16 expected,
> +				     unsigned long timeout_ms)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +	int ret;
> +
> +	do {
> +		ret = mxl862xx_smdio_read(priv, MXL862XX_SB_PDI_STAT);
> +		if (ret < 0)
> +			return ret;
> +		if ((u16)ret == expected)
> +			return 0;
> +		usleep_range(10000, 11000);
> +	} while (time_before(jiffies, timeout));
> +
> +	return -ETIMEDOUT;

I suggest using iopoll.h. This code has the usual bug when rolling
your own.

> +/* Post-flash reprobe. Runs from a self-contained heap work (not a kthread and
> + * not the devlink caller's context): kthread_create() in the caller context
> + * fails -EINTR if the devlink command was Ctrl-C'd, and schedule_work() cannot.

A English sentence should not finish with the word "cannot".

> +	/* Failures from here on must go through end_magic so MCUboot
> +	 * reboots instead of waiting forever.
> +	 */
> +	ret = mxl862xx_sb_pdi_poll_stat(priv, MXL862XX_SB_PDI_READY,
> +					MXL862XX_FW_READY_TIMEOUT_MS);
> +	if (ret) {
> +		dev_err(&priv->mdiodev->dev,
> +			"flash: bootloader not ready: %pe\n", ERR_PTR(ret));
> +		return ret;

What about the comment above? end_magic?

     	   Andrew