Re: [PATCH net v7 2/4] net: hsr: preserve submission order without a forwarding lock

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2026-10-09 22:13:22 [+0200], Xin Xie wrote:
> Holding seqnr_lock across dev_queue_xmit() serializes numbering and
> submission, but creates transmit-lock dependencies that can deadlock
> stacked HSR devices. Merely shrinking the lock to the counter lets
> one CPU send frame N after another has sent N+1; old HSR peers discard
> the late N as stale.

What are stacked HSR devices? Looking from the setup in the referenced
report this is a bridge device attached to a HSR ring. 
An announce frame is sent and this goes to hsr_dev_xmit(). This does not
look like a normal setup. Is this something you use or is this something
the bot came up with?

> Use one consumer for master TX, interlink RX and internally generated
> supervision frames. An idle caller processes its own input inline;
> contended inputs wait in a FIFO drained by one BH work item. Allocate
> sequence numbers only when the consumer executes a frame, and submit
> it to every lower before numbering the next frame. The short counter
> lock is released before forwarding.
> 
> Limit queued inputs to 1024 jobs and 8 MiB, reserving capacity for
> internal supervision. Carry transmit recursion depth across deferred
> execution, cancel the worker and purge the queue on teardown, and count
> queue drops once through core per-CPU statistics. LAN A/B reception
> stays synchronous and tagged frames retain their received numbers.
> The ordering guarantee is per-lower submission, not physical wire
> order across multiple TX queues.
> 
> On PREEMPT_RT, high-priority load preempted both the inline consumer
> and BH worker while another CPU continued to enqueue inputs. All
> 2118 inputs completed without drops; the finite data workload
> completed 4.07 seconds after the load ended.

This looks very complex and I am not a big fan of it. I would prefer to
break lock chain early assuming the setup is valid/ sane (see above).

The ID by itself can be assigned with an atomic_inc() but then you don't
have the transmit ordering. So you introduce the workqueue for the
ordering which is ugh. I'm not sure how this works if you get
submissions from two CPUs given the lock is gone and the workqueue is
per-CPU.

> Reported-by: syzbot+fbf74291c3b7e753b481@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=fbf74291c3b7e753b481
> Fixes: 06afd2c31d33 ("hsr: Synchronize sending frames to have always incremented outgoing seq nr.")
> Fixes: 430d67bdcb04 ("net: hsr: Use the seqnr lock for frames received via interlink port.")
> Signed-off-by: Xin Xie <xiexinet@gmail.com>

Sebastian