Re: [PATCH net-next v2 1/2] net: hsr: Fix PRP duplicate detection

[Paolo Abeni <pabeni@redhat.com>]

On 2/21/25 11:10 AM, Jaakko Karrenpalo wrote:
> From: Jaakko Karrenpalo <jaakko.karrenpalo@fi.abb.com>
> 
> Add PRP specific function for handling duplicate
> packets. This is needed because of potential
> L2 802.1p prioritization done by network switches.

This is IMHO a too terse description of the whys. I don't see how
prioritization should create duplicates. Please expand the commit
message. A cover letter could help
> 
> Signed-off-by: Jaakko Karrenpalo <jaakko.karrenpalo@fi.abb.com>
> Signed-off-by: Jaakko Karrenpalo <jkarrenpalo@gmail.com>

Just one of the 2 ;)

> ---
>  net/hsr/hsr_device.c   |  2 +
>  net/hsr/hsr_forward.c  |  4 +-
>  net/hsr/hsr_framereg.c | 93 ++++++++++++++++++++++++++++++++++++++++--
>  net/hsr/hsr_framereg.h |  8 +++-
>  net/hsr/hsr_main.h     |  2 +
>  5 files changed, 102 insertions(+), 7 deletions(-)
> 
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index b6fb18469439..2c43776b7c4f 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -616,6 +616,7 @@ static struct hsr_proto_ops hsr_ops = {
>  	.drop_frame = hsr_drop_frame,
>  	.fill_frame_info = hsr_fill_frame_info,
>  	.invalid_dan_ingress_frame = hsr_invalid_dan_ingress_frame,
> +	.register_frame_out = hsr_register_frame_out,
>  };
>  
>  static struct hsr_proto_ops prp_ops = {
> @@ -626,6 +627,7 @@ static struct hsr_proto_ops prp_ops = {
>  	.fill_frame_info = prp_fill_frame_info,
>  	.handle_san_frame = prp_handle_san_frame,
>  	.update_san_info = prp_update_san_info,
> +	.register_frame_out = prp_register_frame_out,
>  };
>  
>  void hsr_dev_setup(struct net_device *dev)
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index a4bacf198555..aebeced10ad8 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -536,8 +536,8 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
>  		 * Also for SAN, this shouldn't be done.
>  		 */
>  		if (!frame->is_from_san &&
> -		    hsr_register_frame_out(port, frame->node_src,
> -					   frame->sequence_nr))
> +			hsr->proto_ops->register_frame_out &&
> +		    hsr->proto_ops->register_frame_out(port, frame))

The formatting is quite inconsistent and hard to follow in several
places. I'm surprised checkpatch does not complain. Please align the
condition to the relevant bracket.

[...]
> @@ -482,9 +486,11 @@ void hsr_register_frame_in(struct hsr_node *node, struct hsr_port *port,
>   *	 0 otherwise, or
>   *	 negative error code on error
>   */
> -int hsr_register_frame_out(struct hsr_port *port, struct hsr_node *node,
> -			   u16 sequence_nr)
> +int hsr_register_frame_out(struct hsr_port *port, struct hsr_frame_info *frame)
>  {
> +	struct hsr_node *node = frame->node_src;
> +	u16 sequence_nr = frame->sequence_nr;
> +
>  	spin_lock_bh(&node->seq_out_lock);
>  	if (seq_nr_before_or_eq(sequence_nr, node->seq_out[port->type]) &&
>  	    time_is_after_jiffies(node->time_out[port->type] +
> @@ -499,6 +505,87 @@ int hsr_register_frame_out(struct hsr_port *port, struct hsr_node *node,
>  	return 0;
>  }
>  
> +/* Adaptation of the PRP duplicate discard algorithm described in wireshark
> + * wiki (https://wiki.wireshark.org/PRP)
> + *
> + * A drop window is maintained for both LANs with start sequence set to the
> + * first sequence accepted on the LAN that has not been seen on the other LAN,
> + * and expected sequence set to the latest received sequence number plus one.
> + *
> + * When a frame is received on either LAN it is compared against the received
> + * frames on the other LAN. If it is outside the drop window of the other LAN
> + * the frame is accepted and the drop window is updated.
> + * The drop window for the other LAN is reset.
> + *
> + * 'port' is the outgoing interface
> + * 'frame' is the frame to be sent
> + *
> + * Return:
> + *	 1 if frame can be shown to have been sent recently on this interface,
> + *	 0 otherwise
> + */
> +int prp_register_frame_out(struct hsr_port *port, struct hsr_frame_info *frame)
> +{
> +	enum hsr_port_type other_port;
> +	enum hsr_port_type rcv_port;
> +	struct hsr_node *node;
> +	u16 sequence_nr;
> +
> +	/* out-going frames are always in order
> +	 *and can be checked the same way as for HSR
> +	 */
> +	if (frame->port_rcv->type == HSR_PT_MASTER)
> +		return hsr_register_frame_out(port, frame);
> +
> +	/* for PRP we should only forward frames from the slave ports
> +	 * to the master port
> +	 */
> +	if (port->type != HSR_PT_MASTER)
> +		return 1;
> +
> +	node = frame->node_src;
> +	sequence_nr = frame->sequence_nr;
> +	rcv_port = frame->port_rcv->type;
> +	other_port =
> +		rcv_port == HSR_PT_SLAVE_A ? HSR_PT_SLAVE_B : HSR_PT_SLAVE_A;
> +
> +	spin_lock_bh(&node->seq_out_lock);
> +	if (time_is_before_jiffies(node->time_out[port->type] +
> +	    msecs_to_jiffies(HSR_ENTRY_FORGET_TIME)) ||
> +	    (node->seq_start[rcv_port] == node->seq_expected[rcv_port] &&
> +	    node->seq_start[other_port] == node->seq_expected[other_port])) {
> +		/* the node hasn't been sending for a while
> +		 * or both drop windows are empty, forward the frame
> +		 */
> +		node->seq_start[rcv_port] = sequence_nr;
> +	} else if (seq_nr_before(sequence_nr, node->seq_expected[other_port]) &&
> +	    seq_nr_before_or_eq(node->seq_start[other_port], sequence_nr)) {
> +		/* drop the frame, update the drop window for the other port
> +		 * and reset our drop window
> +		 */
> +		node->seq_start[other_port] = sequence_nr + 1;
> +		node->seq_expected[rcv_port] = sequence_nr + 1;
> +		node->seq_start[rcv_port] = node->seq_expected[rcv_port];
> +		spin_unlock_bh(&node->seq_out_lock);
> +		return 1;
> +	}
> +
> +	/* update the drop window for the port where this frame was received
> +	 * and clear the drop window for the other port
> +	 */
> +	node->seq_start[other_port] = node->seq_expected[other_port];
> +	node->seq_expected[rcv_port] = sequence_nr + 1;
> +	if ((u16)(node->seq_expected[rcv_port] - node->seq_start[rcv_port])
> +	    > PRP_DROP_WINDOW_LEN)
> +		node->seq_start[rcv_port] =
> +			node->seq_expected[rcv_port] - PRP_DROP_WINDOW_LEN;

I could be too sensible, but the above really hurts my eyes;) something
alike:

	u16 seq_exp, seq_diff;

	// [...]

	seq_exp = node->seq_expected[rcv_port]
	seq_diff = seq_exp - node->seq_start[rcv_port];
	if (seq_diff > PRP_DROP_WINDOW_LEN)
		node->seq_start[rcv_port] = seq_exp - PRP_DROP_WINDOW_LEN;

would be better.
	
Cheers,

Paolo