Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: use the hw tx queues

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.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://sashiko.dev/
---
    net: dsa: mv88e6xxx: use the hw tx queues

This patch adds support for transmitting packets to specific hardware queues
on Marvell 88E6xxx switches. It exposes the hardware transmit queues to the
networking stack and modifies the DSA tagger to embed the queue mapping into
the priority field of the DSA tag.

[ ... ]

> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index 2a2c4fb61a65c..96c8fbedbd3b4 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -179,6 +179,7 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  			dsa_header[2] &= ~0x10;
>  		}
>  	} else {
> +		u16 queue = skb_get_queue_mapping(skb) & 0x7;
>  		u16 vid;
>  
>  		vid = br_dev ? MV88E6XXX_VID_BRIDGED : MV88E6XXX_VID_STANDALONE;
> @@ -191,7 +192,7 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  
>  		dsa_header[0] = (cmd << 6) | tag_dev;
>  		dsa_header[1] = tag_port << 3;
> -		dsa_header[2] = vid >> 8;
> +		dsa_header[2] = (queue << 5) | vid >> 8;

[Severity: High]
Does this code map the Linux transmit queue index directly to the 802.1p
Priority (PRI) field, potentially breaking hardware queue utilization?

The PRI field in the Marvell DSA tag corresponds to the 802.1p Priority
Code Point (PCP), rather than a direct hardware queue index. By default,
Marvell switches use a Priority-to-Queue table to map the 8 possible PCP
values (0-7) to physical egress queues. 

For a 4-queue switch, PCP 0-1 map to Q0, 2-3 to Q1, 4-5 to Q2, and 6-7 to Q3.
Since Linux creates 4 TX queues for such a device, skb_get_queue_mapping(skb)
will only return values from 0 to 3. Placing 0-3 directly into the PRI field
means the switch will see PCPs 0-3 and map them strictly to physical Q0 and
Q1. Hardware queues 2 and 3 would become completely unreachable for untagged
traffic.

Should the tagger instead derive the PRI field from skb->priority (capped
to 0-7), as the priority needs to represent the PCP rather than the
compacted Linux queue index?

>  		dsa_header[3] = vid & 0xff;
>  	}
-- 
This is an AI-generated review.