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

[Simon Horman <horms@kernel.org>]

On Thu, Feb 27, 2025 at 07:09:22AM +0200, Jaakko Karrenpalo wrote:
> Add PRP specific function for handling duplicate
> packets. This is needed because of potential
> L2 802.1p prioritization done by network switches.
> 
> The L2 prioritization can re-order the PRP packets
> from a node causing the existing implementation to
> discard the frame(s) that have been received 'late'
> because the sequence number is before the previous
> received packet. This can happen if the node is
> sending multiple frames back-to-back with different
> priority.
> 
> Signed-off-by: Jaakko Karrenpalo <jkarrenpalo@gmail.com>
> ---
> Changes in v3:
> - Fixed indentation
> - Renamed local variables

Thanks, I see that this addresses Paolo's review of v2
and overall looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

Please find below two minor nits.
I don't think you need to respin because of them.
But do consider addressing them if there is a new
revision for some other reason.

...

> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c

...

> +/* 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_diff;
> +	u16 sequence_exp;
> +	u16 sequence_nr;
> +
> +	/* out-going frames are always in order
> +	 *and can be checked the same way as for HSR

nit: space between '*' and 'and'.

> +	 */
> +	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;
> +	sequence_exp = sequence_nr + 1;
> +	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])) {

nit: the line above should be indented to align with the inside of the
     parentheses on the preceding line.

	    (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_exp;
> +		node->seq_expected[rcv_port] = sequence_exp;
> +		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_exp;
> +	sequence_diff = sequence_exp - node->seq_start[rcv_port];
> +	if (sequence_diff > PRP_DROP_WINDOW_LEN)
> +		node->seq_start[rcv_port] = sequence_exp - PRP_DROP_WINDOW_LEN;
> +
> +	node->time_out[port->type] = jiffies;
> +	node->seq_out[port->type] = sequence_nr;
> +	spin_unlock_bh(&node->seq_out_lock);
> +	return 0;
> +}
> +
>  static struct hsr_port *get_late_port(struct hsr_priv *hsr,
>  				      struct hsr_node *node)
>  {