Re: [PATCH net-next v3 3/3] net: ti: icssm-prueth: Support duplicate HW offload feature for HSR and PRP

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
--- Patch [3]: [PATCH net-next v3 3/3] net: ti: icssm-prueth: Support duplicate HW offload feature for HSR and PRP ---
commit e6e3942b25c7905f76512f3ac3b7e21bb0915c5e
Author: Roger Quadros <rogerq@ti.com>

net: ti: icssm-prueth: Support duplicate HW offload feature for HSR and PRP

This adds NETIF_F_HW_HSR_DUP support by moving the two per-port LRE
transmit queues onto one shared BD ring, so a frame is copied into OCMC RAM
once and both PRU ports transmit it.  The TX path inspects the HSR tag or
the PRP RCT trailer to decide whether to set the LAN A/LAN B duplication
bits, and the receive path switches from the BD Port field to a new HostRecv
bit.

A few questions about the new code below.

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
> index 7031b78d2cc2f..43862946d4f14 100644
> --- a/drivers/net/ethernet/ti/icssm/icssm_prueth.c
> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth.c

[ ... ]

> @@ -567,16 +615,19 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac *emac,
>  	u16 update_wr_ptr;
>  	u32 wr_buf_desc;
>  	void *ocmc_ram;
> +	__be16 proto;
> +	u8 *hdr;
> +
> +	other_emac = emac->prueth->emac[(emac->port_id == PRUETH_PORT_MII0) ?
> +				PRUETH_PORT_MII1 - 1 : PRUETH_PORT_MII0 - 1];
> +
> +	if (prueth_is_lre(prueth) && (emac->link || other_emac->link))
> +		link_up = true;

[Severity: Medium]
[Finding: main-5-19]
[Sources: opus-5]
Is emac->link safe to read here without emac->lock?

icssm_emac_adjust_link() mutates it under the lock:

	spin_lock_irqsave(&emac->lock, flags);
	...
	} else if (emac->link) {
		new_state = true;
		emac->link = 0;
	}

The transmit path holds only prueth->lre_host_queue_lock[qid - 2], so the
lockset intersection with adjust_link is empty.

Since link_up decides whether the second write pointer of the shared queue
is advanced, can a sample that races with adjust_link leave the two write
pointers for one ring permanently out of step?

>  	if (!PRUETH_IS_EMAC(prueth))
>  		dram = prueth->mem[PRUETH_MEM_DRAM1].va;
>  	else
>  		dram = emac->prueth->mem[emac->dram].va;
> -	if (eth_skb_pad(skb)) {
> -		if (netif_msg_tx_err(emac) && net_ratelimit())
> -			netdev_err(ndev, "packet pad failed\n");
> -		return -ENOMEM;
> -	}
>  
>  	/* which port to tx: MII0 or MII1 */
>  	txport = emac->tx_port_queue;

[ ... ]

> @@ -607,6 +661,29 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac *emac,
>  		free_blocks = buffer_desc_count;
>  	}
>  
> +	/* Fetch queue state for the second LRE port */
> +	if (prueth_is_lre(prueth) && link_up) {
> +		queue_desc_other_port = emac->tx_queue_descs_other_port +
> +					queue_id;
> +		bd_rd_ptr_other_port = readw(&queue_desc_other_port->rd_ptr);
> +
> +		read_block_other_port = (bd_rd_ptr_other_port -
> +					 txqueue->buffer_desc_offset) / BD_SIZE;

[Severity: High]
[Finding: main-3-13]
[Sources: opus-5]
Should this be conditional on the peer port actually having been
configured, i.e. prueth->emac_configured & BIT(other_port)?

link_up is true whenever either port's phy link is up, so with only one LRE
slave netdev opened this reads a descriptor that was never initialised.
icssm_prueth_hostinit() has memset_io()'d DRAM1:

	if (prueth->eth_node[PRUETH_MAC1])
		icssm_prueth_clearmem(prueth, PRUETH_MEM_DRAM1);

so bd_rd_ptr_other_port reads back 0 and, with both operands promoted to
int:

	read_block_other_port = (0 - P1_Q3_TXOPT_BD_OFFSET) / BD_SIZE
			      = (0 - 4128) / 4 = -1032

	free_blocks_other_port = 194 - 0 + (-1032) = -838

> +
> +		if (write_block > read_block_other_port) {
> +			free_blocks_other_port = buffer_desc_count -
> +						 write_block;
> +			free_blocks_other_port += read_block_other_port;
> +		} else if (write_block < read_block_other_port) {
> +			free_blocks_other_port = read_block_other_port -
> +						 write_block;
> +		} else {
> +			free_blocks_other_port = buffer_desc_count;
> +		}
> +
> +		if (free_blocks_other_port < free_blocks)
> +			free_blocks = free_blocks_other_port;
> +	}
>  	pkt_block_size = DIV_ROUND_UP(pktlen, ICSS_BLOCK_SIZE);
>  	if (pkt_block_size >= free_blocks) /* out of queue space */
>  		return -ENOBUFS;

With free_blocks negative, does pkt_block_size >= free_blocks not stay true
forever, so every xmit returns -ENOBUFS and icssm_emac_ndo_start_xmit()
loops on netif_stop_queue() plus the 100us hrtimer retry, stalling TX on
the port that does have a link?

> @@ -656,6 +733,57 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac *emac,
>  	if (PRUETH_IS_HSR(prueth))
>  		wr_buf_desc |= BIT(PRUETH_BD_HSR_FRAME_SHIFT);
>  
> +	if (prueth_is_lre(prueth)) {
> +		ethhdr = (struct ethhdr *)skb_mac_header(skb);
> +		proto = ethhdr->h_proto;
> +
> +		if (proto == htons(ETH_P_8021Q)) {
> +			vlan_hdr = (struct vlan_ethhdr *)ethhdr;
> +			proto = vlan_hdr->h_vlan_encapsulated_proto;
> +			is_vlan = true;
> +		}
> +
> +		/* Extract HSR sequence number and LAN ID
> +		 * from the tag for the Buffer Descriptor
> +		 */
> +		if (proto == htons(ETH_P_HSR)) {
> +			hdr = skb_mac_header(skb);
> +
> +			if (is_vlan) {
> +				hsr_ethhdr =
> +					(struct hsr_txopt_ethhdr *)(hdr +
> +								    VLAN_HLEN);
> +			} else {
> +				hsr_ethhdr = (struct hsr_txopt_ethhdr *)hdr;
> +			}
> +
> +			/* PTP frames (ETH_P_1588) carry no LAN ID
> +			 * in the HSR tag
> +			 */
> +			if (hsr_ethhdr->hsr_tag.encap_proto !=
> +			    htons(ETH_P_1588)) {
> +				wr_buf_desc |= PRUETH_BD_LAN_INFO_MASK;
> +			} else {
> +				wr_buf_desc |= (txport <<
> +						PRUETH_BD_LAN_A_SHIFT);
> +			}
> +			wr_buf_desc |= PRUETH_BD_RED_PKT_MASK;
> +		} else {
> +			/* Read PRP RCT to extract sequence number and LAN ID */
> +			struct prp_txopt_rct *rct =
> +				(struct prp_txopt_rct *)(skb_tail_pointer(skb) -
> +							 ICSSM_LRE_TAG_SIZE);
> +
> +			if (rct->prp_suffix == htons(ETH_P_PRP)) {
> +				wr_buf_desc |= PRUETH_BD_LAN_INFO_MASK;
> +				wr_buf_desc |= PRUETH_BD_RED_PKT_MASK;
> +			} else {
> +				wr_buf_desc |= (txport <<
> +						PRUETH_BD_LAN_A_SHIFT);
> +			}
> +		}
> +	}

[Severity: Medium]
[Finding: main-1-4]
[Sources: opus-5]
Should the RCT branch be gated on PRUETH_IS_PRP(prueth)?

As written the else arm also runs in HSR mode, for every frame whose
(VLAN-inner) ethertype is not ETH_P_HSR, for example an untagged PTP, ARP or
IPv6 DAD frame sent directly on the slave netdev.  The last six bytes of
such a frame are then reinterpreted as struct prp_txopt_rct, and if the
final two payload bytes happen to be 0x88FB the frame gets

	wr_buf_desc |= PRUETH_BD_LAN_INFO_MASK;
	wr_buf_desc |= PRUETH_BD_RED_PKT_MASK;

and is duplicated onto both LANs.  With a raw socket those two bytes are
fully caller controlled.  The commit message says "Frames without an RCT
are sent on the originating port only" - is that still what happens here in
HSR mode?

Also, the two comments say the sequence number and LAN ID are extracted
from the HSR tag and from the RCT, but neither sequence_nr nor
lan_id_and_lsdu_size is ever read.  Could the comments be reworded?

> @@ -668,6 +796,10 @@ static int icssm_prueth_tx_enqueue(struct prueth_emac *emac,
>  	update_wr_ptr = txqueue->buffer_desc_offset + (update_block * BD_SIZE);
>  	writew(update_wr_ptr, &queue_desc->wr_ptr);
>  
> +	/* update the write pointer in queue descriptor of other port */
> +	if (prueth_is_lre(prueth) && link_up)
> +		writew(update_wr_ptr, &queue_desc_other_port->wr_ptr);
> +
>  	return 0;
>  }

[Severity: High]
[Finding: main-1-5]
[Sources: opus-5]
The BD write and the OCMC payload copy above are unconditional, but the peer
write pointer is only mirrored when link_up is true.  Can the two
descriptors that describe the same ring diverge permanently?

icssm_emac_ndo_open() does not stop the TX queue and emac->link stays 0
until the first icssm_emac_adjust_link() callback, so a frame sent right
after "ip link set ethX up" (with the peer also down) takes this path:

	writel(wr_buf_desc, sram + readw(&queue_desc->wr_ptr));
	...
	writew(update_wr_ptr, &queue_desc->wr_ptr);
	/* peer wr_ptr not advanced */

On the next transmit with link_up true, update_wr_ptr is computed from the
local descriptor and written into the peer descriptor, so does the peer PRU
jump over BDs it never saw as pending and transmit whatever ring content is
there now?

Separately, because the condition is an OR of both links, a peer whose link
is down still clamps free_blocks via free_blocks_other_port.  Should the
peer's rd_ptr only be consulted when that peer is actually draining the
ring?

> @@ -680,8 +812,10 @@ void icssm_parse_packet_info(struct prueth *prueth, u32 buffer_descriptor,
>  	else
>  		pkt_info->start_offset = false;
>  
> -	pkt_info->port = (buffer_descriptor & PRUETH_BD_PORT_MASK) >>
> -			 PRUETH_BD_PORT_SHIFT;
> +	/* Flag from BD to indicate packet is valid for HOST or not. */
> +	pkt_info->host_recv_flag = !!(buffer_descriptor &
> +				      PRUETH_BD_HOST_RECV_MASK);
> +

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5]
The commit message describes transmit-side duplicate offload only ("The
driver writes the frame once into OCMC RAM", HSR tag and RCT inspection),
but the patch also changes the receive path for every eth_type:

- icssm_parse_packet_info() stops decoding the BD Port field and decodes
  the new HostRecv bit for all modes
- icssm_emac_rx_packet() silently returns 0 for HSR frames without the
  HostRecv bit, with no drop counter, while the caller still counts the
  descriptor as used++
- software HSR tag removal excises six bytes from the middle of the frame
- the OCMC to skb copy is restructured into MAC header, optional VLAN and
  body stages for EMAC, switch, HSR and PRP alike

Could the changelog mention these receive-side changes?

>  	pkt_info->length = (buffer_descriptor & PRUETH_BD_LENGTH_MASK) >>
>  			   PRUETH_BD_LENGTH_SHIFT;
>  	pkt_info->broadcast = !!(buffer_descriptor & PRUETH_BD_BROADCAST_MASK);

[ ... ]

> @@ -744,9 +880,19 @@ int icssm_emac_rx_packet(struct prueth_emac *emac, u16 *bd_rd_ptr,
>  	/* calculate new pointer in ram */
>  	*bd_rd_ptr = rxqueue->buffer_desc_offset + (update_block * BD_SIZE);
>  
> +	if (PRUETH_IS_HSR(emac->prueth)) {
> +		if (!pkt_info->host_recv_flag)
> +			return 0;
> +	}
> +

[Severity: Low]
[Finding: main-2-6]
[Sources: opus-5]
The new bit is documented in icssm_switch.h without any protocol
qualification:

 * 10		HostRecv	Set if the frame is destined for the host port

and icssm_parse_packet_info() fills host_recv_flag for every eth_type, but
the only consumer is inside if (PRUETH_IS_HSR(...)).  In PRP mode the flag
is parsed and then ignored, so BDs the firmware marks as not for the host
are still copied into an skb and passed to netif_receive_skb().  Is that
intentional, and if so should the header say the bit is HSR only?

>  	/* Exclude the HSR tag bytes already stripped by firmware, if any. */
>  	actual_pkt_len = pkt_info->length - start_offset;
>  
> +	if (PRUETH_IS_HSR(emac->prueth)) {
> +		if (!start_offset && !pkt_info->timestamp)
> +			actual_pkt_len -= ICSSM_LRE_TAG_SIZE;
> +	}
> +

[Severity: Medium]
[Finding: main-1-3]
[Sources: opus-5]
This removes six bytes from every HSR-mode frame whose BD has neither the
start-offset bit nor the timestamp bit, without ever looking at the frame's
ethertype.  Can that corrupt untagged frames?

The same new code already reads the ethertype a few lines below for the VLAN
test:

	type = get_unaligned_be16(src_addr);

and the transmit side of this patch does discriminate on
proto == htons(ETH_P_HSR), and sends non-HSR protocols untagged on the
directed port.  So untagged frames do appear on the ring.

For such a frame, actual_pkt_len is reduced by six, src_addr is advanced by
six past ordinary payload, and the wrapped-copy path applies the same
adjustment to bytes.  Would gating the strip on the frame actually carrying
an HSR tag avoid delivering the frame six bytes short with a hole after the
MAC (and optional VLAN) header?

>  	/* Allocate a socket buffer for this packet */
>  	skb = netdev_alloc_skb_ip_align(ndev, actual_pkt_len);
>  	if (!skb) {

[ ... ]

> @@ -767,6 +913,29 @@ int icssm_emac_rx_packet(struct prueth_emac *emac, u16 *bd_rd_ptr,
>  		   (read_block * ICSS_BLOCK_SIZE);
>  	src_addr += start_offset;
>  
> +	/* Copy destination and source MAC address */
> +	memcpy(dst_addr, src_addr, PRUETH_ETHER_TYPE_OFFSET);
> +	src_addr += PRUETH_ETHER_TYPE_OFFSET;
> +	dst_addr += PRUETH_ETHER_TYPE_OFFSET;
> +
> +	adjust_for_hsr_tag += PRUETH_ETHER_TYPE_OFFSET;
> +
> +	/* Check for VLAN tag */
> +	type = get_unaligned_be16(src_addr);
> +
> +	if (type == ETH_P_8021Q) {
> +		memcpy(dst_addr, src_addr, VLAN_HLEN);
> +		src_addr += VLAN_HLEN;
> +		dst_addr += VLAN_HLEN;
> +		adjust_for_hsr_tag += VLAN_HLEN;
> +	}
> +
> +	/* HSR tag removal handling */
> +	if (PRUETH_IS_HSR(emac->prueth)) {
> +		if (!start_offset && !pkt_info->timestamp)
> +			src_addr += ICSSM_LRE_TAG_SIZE;
> +	}
> +

[ ... ]

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.h b/drivers/net/ethernet/ti/icssm/icssm_prueth.h
> index 9af19617967ee..2447fb8b99e79 100644
> --- a/drivers/net/ethernet/ti/icssm/icssm_prueth.h
> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth.h

[ ... ]

> @@ -314,9 +317,13 @@ struct prueth {
>  	u8 emac_configured;
>  	u8 hsr_members;
>  	u8 br_members;
> +
> +	/* Per-queue TX lock - LRE uses only the two high-priority queues */
> +	spinlock_t lre_host_queue_lock[NUM_QUEUES / 2];
>  };

[Severity: Low]
[Finding: main-2-9]
[Sources: opus-5]
This isn't a bug, but the comment says "the two high-priority queues" while
the queues the LRE transmit path can reach are PRUETH_QUEUE3 and
PRUETH_QUEUE4:

	emac_pcp_tx_priority_queue_map[0..3] == {QUEUE4, QUEUE4, QUEUE3, QUEUE3}

and this same patch labels them in icssm_switch.h as

	#define QUEUE_3_TXOPT_SIZE	194	/* Protocol specific - High Priority */
	#define QUEUE_4_TXOPT_SIZE	194	/* NRT(IP,ARP, ICMP) - Low Priority*/

which is one high and one low priority queue.

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c b/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c
> index 43140add81725..a99cdbb79bbf0 100644
> --- a/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c
> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth_common.c
> @@ -137,16 +137,13 @@ static int icssm_prueth_common_emac_rx_packets(struct prueth_emac *emac,
>  			used++;
>  		}
>  
> -		/* Zero the BD after consuming it, a misaligned rd_ptr
> -		 * would otherwise mistake stale data for a valid incoming
> -		 * frame.
> +		/* Leave the BD intact after reading. Firmware reuses it to
> +		 * forward the frame to the second LRE port.
>  		 */
>  		if (port == 0) {
> -			writel(0, shared_ram + bd_rd_ptr);
>  			writew(update_rd_ptr, &queue_desc->rd_ptr);
>  			bd_rd_ptr = update_rd_ptr;
>  		} else {
> -			writel(0, shared_ram + bd_rd_ptr_o);
>  			writew(update_rd_ptr, &queue_desc_o->rd_ptr);
>  			bd_rd_ptr_o = update_rd_ptr;
>  		}

[Severity: Medium]
[Finding: main-1-2]
[Sources: opus-5]
Should this removal be gated on prueth_is_lre()?

The justification is LRE specific, but this is the common host receive
routine: icssm_prueth_lre_napi_poll_hpq()/lpq() are registered and enabled
whenever PRUETH_IS_SWITCH(prueth) || prueth_is_lre(prueth), so RSTP switch
mode loses the invalidation too.

The immediately preceding patch in this series added the zeroing with the
opposite reasoning, and the single-port path still does it:

drivers/net/ethernet/ti/icssm/icssm_prueth.c:icssm_emac_rx_packets() {
	...
			/* after reading the buffer descriptor we clear it
			 * to prevent improperly moved read pointer errors
			 * from simply looking like old packets.
			 */
			writel(0, shared_ram + bd_rd_ptr);
	...
}

In switch mode, after any rd_ptr skew, including the two length-error paths
in this same loop that force update_rd_ptr = *bd_wr_ptr_p, can a stale
non-zero BD word be re-parsed as a valid length and flags descriptor and up
to pkt_info->length bytes of old OCMC content be pushed to
netif_receive_skb()?

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth_switch.c b/drivers/net/ethernet/ti/icssm/icssm_prueth_switch.c
> index 9d67fc7c23acc..bda2fac13d749 100644
> --- a/drivers/net/ethernet/ti/icssm/icssm_prueth_switch.c
> +++ b/drivers/net/ethernet/ti/icssm/icssm_prueth_switch.c

[ ... ]

> @@ -983,6 +1177,109 @@ static int icssm_prueth_sw_port_config(struct prueth *prueth,
>  	return 0;
>  }
>  
> +/* Configure TX/RX queue contexts and buffer descriptor tables for LRE port */
> +static int icssm_prueth_lre_port_config(struct prueth *prueth,
> +					enum prueth_port port_id)
> +{

[ ... ]

> +	/* queue size lookup table */
> +	dram = dram_base + QUEUE_SIZE_ADDR +
> +		port_id * NUM_QUEUES * sizeof(u16);
> +	writew(HOST_QUEUE_1_SIZE, dram);
> +	writew(HOST_QUEUE_2_SIZE, dram + 2);
> +	writew(QUEUE_3_TXOPT_SIZE, dram + 4);
> +	writew(QUEUE_4_TXOPT_SIZE, dram + 6);

[Severity: Low]
[Finding: main-2-10]
[Sources: opus-5]
For PRUETH_PORT_QUEUE_MII0 the entries QUEUE1 and QUEUE2 of
lre_queue_infos describe host queues 3 and 4:

	[PRUETH_QUEUE1] = {
		P0_Q3_BUFFER_OFFSET,
		...
		P0_Q3_BD_OFFSET + ((HOST_QUEUE_3_SIZE - 1) * BD_SIZE),
	},

so the firmware is told HOST_QUEUE_1_SIZE / HOST_QUEUE_2_SIZE for rings
whose buffer_desc_end the driver derives from HOST_QUEUE_3_SIZE /
HOST_QUEUE_4_SIZE.  All four constants are 194 today, so nothing breaks,
but should the sizes be derived from the same constants the queue info
table uses?

> +
> +	/* queue table */
> +	memcpy_toio(dram_base + queue_desc_ofs,
> +		    &hsr_prp_txopt_queue_descs[port_id][0],
> +		    4 * sizeof(hsr_prp_txopt_queue_descs[port_id][0]));

[Severity: High]
[Finding: main-3-13]
[Sources: opus-5]
Is it safe to reset these descriptors while the peer port is live?

hsr_prp_txopt_queue_descs[MII0][2..3] and [MII1][2..3] both point at
P1_Q3_TXOPT_BD_OFFSET / P2_Q1_TXOPT_BD_OFFSET, so the two per-port
descriptor sets describe the same BD ring and the same OCMC buffer.  This
memcpy_toio() rewinds rd_ptr and wr_ptr of that shared ring to its base.

The path is reachable from ndo_open of the second slave, and again on
"ip link set ethX down; ip link set ethX up" while the other slave is up,
because icssm_prueth_sw_shutdown_prus() returns early while
emac_configured is non-zero and icssm_emac_ndo_stop() only clears the
port's bit, so both PRUs keep running.

Also, this runs with no lock held, while the peer's
icssm_emac_ndo_start_xmit() holds only prueth->lre_host_queue_lock[qid - 2]
when it reads queue_desc_other_port->rd_ptr and writes
queue_desc_other_port->wr_ptr.  Should the configuration path take the same
lock?

> +
> +	/* In HSR/PRP mode both slave ports share the host receive queue
> +	 * descriptor region (P0_QUEUE_DESC_OFFSET). The firmware arbitrates
> +	 * ownership; the driver always reads from the same host-side descriptor
> +	 * base regardless of which physical port the frame arrived on.
> +	 */
> +	emac->rx_queue_descs = dram_base + P0_QUEUE_DESC_OFFSET;
> +	emac->tx_queue_descs = dram_base +
> +		lre_rx_queue_infos[port_id][PRUETH_QUEUE1].queue_desc_offset;
> +
> +	if (port_id == PRUETH_PORT_MII0) {
> +		emac->tx_queue_descs_other_port = dram_base +
> +			lre_rx_queue_infos
> +			[port_id + 1][PRUETH_QUEUE1].queue_desc_offset;
> +	} else if (port_id == PRUETH_PORT_MII1) {
> +		emac->tx_queue_descs_other_port = dram_base +
> +			lre_rx_queue_infos
> +			[port_id - 1][PRUETH_QUEUE1].queue_desc_offset;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_switch.h b/drivers/net/ethernet/ti/icssm/icssm_switch.h
> index 13b1b93ef4c27..b0d623b4979b0 100644
> --- a/drivers/net/ethernet/ti/icssm/icssm_switch.h
> +++ b/drivers/net/ethernet/ti/icssm/icssm_switch.h

[ ... ]

> @@ -49,20 +52,18 @@
>   *				For RED, NodeTable lookup was successful.
>   * 7		Flood		Packet should be flooded (destination MAC
>   *				address found in FDB). For switch only.
> - * 8..12	Block_length	number of valid bytes in this specific block.
> - *				Will be <=32 bytes on last block of packet
> + * 8		RED_INFO	Set if the frame carries an HSR or PRP
> + *				redundancy tag
> + * 10		HostRecv	Set if the frame is destined for the host port
>   * 13		More		"More" bit indicating that there are more blocks
>   * 14		Shadow		indicates that "index" is pointing into shadow
>   *				buffer
>   * 15		TimeStamp	indicates that this packet has time stamp in
>   *				separate buffer - only needed if PTP runs on
>   *				host
> - * 16..17	Port		different meaning for ingress and egress,
> - *				Ingress: Port = 0 indicates phy port 1 and
> - *				Port = 1 indicates phy port 2.
> - *				Egress: 0 sends on phy port 1 and 1 sends on
> - *				phy port 2. Port = 2 goes over MAC table
> - *				look-up
> + * 16..17	LAN		Destination LAN for transmission:
> + *				bit 16 = LAN A, bit 17 = LAN B, set both to
> + *				duplicate to both LANs.

[Severity: Low]
[Finding: main-2-7]
[Sources: opus-5]
This header still serves the EMAC and switch firmware, but the bit table
now only describes the LRE interpretation.  The Block_length description
for bits 8..12 and the Port description for bits 16..17, including
"Port = 2 goes over MAC table look-up", are gone, bits 9, 11 and 12 are
left undocumented, and PRUETH_BD_PORT_MASK/SHIFT are removed.

icssm_prueth_tx_enqueue() only programs the LAN bits inside
if (prueth_is_lre(prueth)), so could the comment note that the RED_INFO,
HostRecv and LAN meanings apply to the LRE firmware only, and keep the
EMAC/switch meanings alongside them?

Thanks for looking at these.
-- 
This is an AI-generated review.