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

On Thu, Jul 30, 2026 at 11:29:47PM +0200, Andrew Lunn wrote:
>> 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
[...]
>
> 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.

Yes, exactly. device_reprobe() runs remove(), which unregisters the
switch and with it all user netdevs, and the fresh probe registers
them anew. They come back administratively down and userspace brings
them up again. Everything is rebuilt from scratch rather than
restored, which is the point given the switch itself lost all state
too.

>> +static int mxl862xx_sb_pdi_poll_stat(struct mxl862xx_priv *priv, u16 expected,
[...]
>> +		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.

Agreed. Since v6 all SB PDI polling uses read_poll_timeout(), which
also does the one more read after the deadline that the open-coded
loop missed.

>> +/* 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".

That comment, and the kthread it talked about, are gone since v7; the
reprobe is a plain delayed work item now.

>> +	/* 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) {
[...]
>> +		return ret;
>
> What about the comment above? end_magic?

You are right, in v5 the comment and the code disagreed. It has moved
on since: it turned out that END written while the loader still sits
in its receive loop is consumed as a byte count and can underflow the
receive counter, so since v7 failures deliberately do not send END
(the loader finalises on its own 2s timeout), and v8 renamed the
label to no_end with a comment that now matches the code.