Re: [PATCH net-next v2 6/9] hsr: Implement more robust duplicate discard for HSR

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

On 2026-01-29 17:17:04 [+0100], Felix Maurer wrote:
> About the interlink: that's the interface where you attach devices that
> know nothing about HSR, i.e., when we are a RedBox. I consider it very
> similar to the master port, it's our responsibility to de-duplicate what
> we send out there.

Right. It should contain the same "seen" values as the master port.

> I was thinking about exactly this while working on the patch as well and
> I came to each conclusion (A,B are needed vs. are not needed) at least
> once. In the end, I think we will need it. It's right that a well
> behaving node in the ring should not forward "frames for which the node
> is the unique destination" (5.3.2.1). But there could be frames that
> have no unique destination in the ring at all: multicast frames or
> frames addressed to a non-existing MAC address. We should not forward
> such frames either to prevent them from looping forever.

But multicast and non-existing destination frames do one round in the
circle, reach the sender which will remove them from the ring and not
forward.

> Now, such frames should probably only reach back to us, if we sent them
> (either from our stack or from the interlink port). We could track
> sequence numbers sent to master and interlink (for de-duplication) and
> sent by master and interlink (for loop prevention) for more clarity, but
> then we're back at four bitmaps again.

I think we could avoid tracking A/ B. The packets that we receive from
A/ B and forward to the master after de-duplication should be the same
as those that are forwarded to the interlink port.

> I agree that we can and should optimize the HW-offloaded case. I'd
> suggest to do that in a separate patchset, though, partly because I
> don't have access to a hardware with HSR offload at the moment.

Sure.

> > I know you preserve what is already here but what is this even used for?
> > | ip -d link show dev hsr0
> >
> > does not show these numbers. It shows the sequence number of the hsr0
> > interface which I understand.
> > But then it is also possible to show the last received sequence number
> > of any node on either of the two interfaces?
> 
> This is only exposed through the HSR_C_GET_NODE_STATUS netlink command
> for the "HSR" (sic!) netlink family. I'm not aware of a userspace tool
> that actually shows this data. It's unclear if it is of any real use.

hehe. Great.

> I assume that the two sequence numbers are reported because they would
> in theory allow to detect a broken ring: if one of the numbers stops
> increasing or the numbers generally diverge, some link in the ring is
> broken. If we have traffic from multiple nodes, we could even detect
> where the ring is broken. That's likely also the reason for the weird
> cross-assinment of B->1, A->2. What we send out on B has to have arrived
> on port A resp. 1 before.
> 
> We can discuss removing this stuff in the future, but I'd prefer not to
> touch userspace at this time.

Sure. I was just looking who is using this and this netlink interface
was the only user. It looks like debug code.

> > > +}
> > > +
> > >  int hsr_get_node_data(struct hsr_priv *hsr,
> > >  		      const unsigned char *addr,
> > >  		      unsigned char addr_b[ETH_ALEN],
> > …
> > > --- a/net/hsr/hsr_framereg.h
> > > +++ b/net/hsr/hsr_framereg.h
> > > @@ -82,15 +82,18 @@ int prp_register_frame_out(struct hsr_port *port, struct hsr_frame_info *frame);
> > >  #define hsr_seq_block_index(sequence_nr) ((sequence_nr) >> HSR_SEQ_BLOCK_SHIFT)
> > >  #define hsr_seq_block_bit(sequence_nr) ((sequence_nr) & HSR_SEQ_BLOCK_MASK)
> > >
> > > +#define DECLARE_BITMAP_FLEX_ARRAY(name, bits) \
> > > +	unsigned long name[][BITS_TO_LONGS(bits)]
> > > +
> > >  struct hsr_seq_block {
> > >  	unsigned long		time;
> > >  	u16			block_idx;
> > > -	DECLARE_BITMAP(seq_nrs, HSR_SEQ_BLOCK_SIZE);
> > > +	DECLARE_BITMAP_FLEX_ARRAY(seq_nrs, HSR_SEQ_BLOCK_SIZE);
> >
> > is there a story behind DECLARE_BITMAP_FLEX_ARRAY()? We have just this
> > one user.
> 
> I've added it this way to make it obvious that almost all of it is the
> same as DECLARE_BITMAP(). But probably it's better to get rid of the
> macro, add the definition directly to the struct, and maybe add a
> comment that it's supposed to be similar to what DECLARE_BITMAP()
> produces. What do you think?

I would get rid of it. There the "official" DECLARE_BITMAP(). If this
doesn't work just open code it avoiding the macro.

> Thanks for your comments,
>    Felix

Sebastian