[PATCH net v2 0/2] net: stmmac: Minor fixes for stmmac EST implementation

[PATCH net v2 0/2] net: stmmac: Minor fixes for stmmac EST implementation

[Rohan G Thomas via B4 Relay]

This patchset includes following minor fixes for stmmac EST
implementation:
   1. Fix GCL bounds checks
   2. Consider Tx VLAN offload tag length for maxSDU

Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
---
Changes in v2:
- Use GENMASK instead of BIT for clarity and consistency
- Link to v1: https://lore.kernel.org/r/20250911-qbv-fixes-v1-0-e81e9597cf1f@altera.com

---
Rohan G Thomas (2):
      net: stmmac: est: Fix GCL bounds checks
      net: stmmac: Consider Tx VLAN offload tag length for maxSDU

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++++++++++++++-------
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   |  4 ++--
 2 files changed, 20 insertions(+), 9 deletions(-)
---
base-commit: 5b5ba63a54cc7cb050fa734dbf495ffd63f9cbf7
change-id: 20250911-qbv-fixes-4ae64de86ee7

Best regards,
-- 
Rohan G Thomas <rohan.g.thomas@altera.com>

[PATCH net v2 1/2] net: stmmac: est: Fix GCL bounds checks

[Rohan G Thomas via B4 Relay]

From: Rohan G Thomas <rohan.g.thomas@altera.com>

Fix the bounds checks for the hw supported maximum GCL entry
count and gate interval time.

Fixes: b60189e0392f ("net: stmmac: Integrate EST with TAPRIO scheduler API")
Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 694d6ee1438197bd4434af6e9b78f022e94ff98f..9c84bde848263976cf08f286751ee09e2dbac09b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -981,7 +981,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	if (qopt->cmd == TAPRIO_CMD_DESTROY)
 		goto disable;
 
-	if (qopt->num_entries >= dep)
+	if (qopt->num_entries > dep)
 		return -EINVAL;
 	if (!qopt->cycle_time)
 		return -ERANGE;
@@ -1012,7 +1012,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 		s64 delta_ns = qopt->entries[i].interval;
 		u32 gates = qopt->entries[i].gate_mask;
 
-		if (delta_ns > GENMASK(wid, 0))
+		if (delta_ns > GENMASK(wid - 1, 0))
 			return -ERANGE;
 		if (gates > GENMASK(31 - wid, 0))
 			return -ERANGE;

-- 
2.26.2

[PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[Rohan G Thomas via B4 Relay]

From: Rohan G Thomas <rohan.g.thomas@altera.com>

On hardware with Tx VLAN offload enabled, add the VLAN tag
length to the skb length before checking the Qbv maxSDU.
Add 4 bytes for 802.1Q an add 8 bytes for 802.1AD tagging.

Fixes: c5c3e1bfc9e0 ("net: stmmac: Offload queueMaxSDU from tc-taprio")
Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8c8ca5999bd8ad369eafa0cd8448a15da55be86b..c06c947ef7764bf40291a556984651f4edd7cb74 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4537,6 +4537,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	bool has_vlan, set_ic;
 	int entry, first_tx;
 	dma_addr_t des;
+	u32 sdu_len;
 
 	tx_q = &priv->dma_conf.tx_queue[queue];
 	txq_stats = &priv->xstats.txq_stats[queue];
@@ -4553,13 +4554,6 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 			return stmmac_tso_xmit(skb, dev);
 	}
 
-	if (priv->est && priv->est->enable &&
-	    priv->est->max_sdu[queue] &&
-	    skb->len > priv->est->max_sdu[queue]){
-		priv->xstats.max_sdu_txq_drop[queue]++;
-		goto max_sdu_err;
-	}
-
 	if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {
 		if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
 			netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
@@ -4575,6 +4569,23 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Check if VLAN can be inserted by HW */
 	has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
 
+	sdu_len = skb->len;
+	if (has_vlan) {
+		/* Add VLAN tag length to sdu length in case of txvlan offload */
+		if (priv->dev->features & NETIF_F_HW_VLAN_CTAG_TX)
+			sdu_len += VLAN_HLEN;
+		if (skb->vlan_proto == htons(ETH_P_8021AD) &&
+		    priv->dev->features & NETIF_F_HW_VLAN_STAG_TX)
+			sdu_len += VLAN_HLEN;
+	}
+
+	if (priv->est && priv->est->enable &&
+	    priv->est->max_sdu[queue] &&
+	    sdu_len > priv->est->max_sdu[queue]) {
+		priv->xstats.max_sdu_txq_drop[queue]++;
+		goto max_sdu_err;
+	}
+
 	entry = tx_q->cur_tx;
 	first_entry = entry;
 	WARN_ON(tx_q->tx_skbuff[first_entry]);

-- 
2.26.2

Re: [PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[Jakub Kicinski]

On Mon, 15 Sep 2025 16:17:19 +0800 Rohan G Thomas via B4 Relay wrote:
> From: Rohan G Thomas <rohan.g.thomas@altera.com>
> 
> On hardware with Tx VLAN offload enabled, add the VLAN tag
> length to the skb length before checking the Qbv maxSDU.
> Add 4 bytes for 802.1Q an add 8 bytes for 802.1AD tagging.
> 
> Fixes: c5c3e1bfc9e0 ("net: stmmac: Offload queueMaxSDU from tc-taprio")
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8c8ca5999bd8ad369eafa0cd8448a15da55be86b..c06c947ef7764bf40291a556984651f4edd7cb74 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4537,6 +4537,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	bool has_vlan, set_ic;
>  	int entry, first_tx;
>  	dma_addr_t des;
> +	u32 sdu_len;
>  
>  	tx_q = &priv->dma_conf.tx_queue[queue];
>  	txq_stats = &priv->xstats.txq_stats[queue];
> @@ -4553,13 +4554,6 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  			return stmmac_tso_xmit(skb, dev);
>  	}
>  
> -	if (priv->est && priv->est->enable &&
> -	    priv->est->max_sdu[queue] &&
> -	    skb->len > priv->est->max_sdu[queue]){
> -		priv->xstats.max_sdu_txq_drop[queue]++;
> -		goto max_sdu_err;
> -	}
> -
>  	if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {
>  		if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
>  			netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
> @@ -4575,6 +4569,23 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	/* Check if VLAN can be inserted by HW */
>  	has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
>  
> +	sdu_len = skb->len;
> +	if (has_vlan) {
> +		/* Add VLAN tag length to sdu length in case of txvlan offload */
> +		if (priv->dev->features & NETIF_F_HW_VLAN_CTAG_TX)
> +			sdu_len += VLAN_HLEN;
> +		if (skb->vlan_proto == htons(ETH_P_8021AD) &&
> +		    priv->dev->features & NETIF_F_HW_VLAN_STAG_TX)
> +			sdu_len += VLAN_HLEN;

Is the device adding the same VLAN tag twice if the proto is 8021AD?
It looks like it from the code, but how every strange..

In any case, it doesn't look like the driver is doing anything with 
the NETIF_F_HW_VLAN_* flags right? stmmac_vlan_insert() works purely
off of vlan proto. So I think we should do the same thing here?

> +	}
> +
> +	if (priv->est && priv->est->enable &&
> +	    priv->est->max_sdu[queue] &&
> +	    sdu_len > priv->est->max_sdu[queue]) {
> +		priv->xstats.max_sdu_txq_drop[queue]++;
> +		goto max_sdu_err;
> +	}
> +
>  	entry = tx_q->cur_tx;
>  	first_entry = entry;
>  	WARN_ON(tx_q->tx_skbuff[first_entry]);
> 

Re: [PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[Jakub Kicinski]

On Wed, 17 Sep 2025 15:49:20 -0700 Jakub Kicinski wrote:
> On Mon, 15 Sep 2025 16:17:19 +0800 Rohan G Thomas via B4 Relay wrote:
> > From: Rohan G Thomas <rohan.g.thomas@altera.com>
> > 
> > On hardware with Tx VLAN offload enabled, add the VLAN tag
> > length to the skb length before checking the Qbv maxSDU.
> > Add 4 bytes for 802.1Q an add 8 bytes for 802.1AD tagging.
> > 
> > Fixes: c5c3e1bfc9e0 ("net: stmmac: Offload queueMaxSDU from tc-taprio")
> > Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
> > Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++++++++++++++-------
> >  1 file changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 8c8ca5999bd8ad369eafa0cd8448a15da55be86b..c06c947ef7764bf40291a556984651f4edd7cb74 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -4537,6 +4537,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
> >  	bool has_vlan, set_ic;
> >  	int entry, first_tx;
> >  	dma_addr_t des;
> > +	u32 sdu_len;
> >  
> >  	tx_q = &priv->dma_conf.tx_queue[queue];
> >  	txq_stats = &priv->xstats.txq_stats[queue];
> > @@ -4553,13 +4554,6 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
> >  			return stmmac_tso_xmit(skb, dev);
> >  	}
> >  
> > -	if (priv->est && priv->est->enable &&
> > -	    priv->est->max_sdu[queue] &&
> > -	    skb->len > priv->est->max_sdu[queue]){
> > -		priv->xstats.max_sdu_txq_drop[queue]++;
> > -		goto max_sdu_err;
> > -	}
> > -
> >  	if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {
> >  		if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
> >  			netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
> > @@ -4575,6 +4569,23 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
> >  	/* Check if VLAN can be inserted by HW */
> >  	has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
> >  
> > +	sdu_len = skb->len;
> > +	if (has_vlan) {
> > +		/* Add VLAN tag length to sdu length in case of txvlan offload */
> > +		if (priv->dev->features & NETIF_F_HW_VLAN_CTAG_TX)
> > +			sdu_len += VLAN_HLEN;
> > +		if (skb->vlan_proto == htons(ETH_P_8021AD) &&
> > +		    priv->dev->features & NETIF_F_HW_VLAN_STAG_TX)
> > +			sdu_len += VLAN_HLEN;  
> 
> Is the device adding the same VLAN tag twice if the proto is 8021AD?
> It looks like it from the code, but how every strange..
> 
> In any case, it doesn't look like the driver is doing anything with 
> the NETIF_F_HW_VLAN_* flags right? stmmac_vlan_insert() works purely
> off of vlan proto. So I think we should do the same thing here?

I suppose the double tagging depends on the exact SKU but first check
looks unnecessary. Maybe stmmac_vlan_insert() should return the number
of vlans it decided to insert?

> > +	}
> > +
> > +	if (priv->est && priv->est->enable &&
> > +	    priv->est->max_sdu[queue] &&
> > +	    sdu_len > priv->est->max_sdu[queue]) {
> > +		priv->xstats.max_sdu_txq_drop[queue]++;
> > +		goto max_sdu_err;
> > +	}
> > +
> >  	entry = tx_q->cur_tx;
> >  	first_entry = entry;
> >  	WARN_ON(tx_q->tx_skbuff[first_entry]);
> >   
> 

Re: [PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[G Thomas, Rohan]

Hi Jakub,

Thanks for reviewing the patch.

On 9/18/2025 4:24 AM, Jakub Kicinski wrote:
> On Wed, 17 Sep 2025 15:49:20 -0700 Jakub Kicinski wrote:
>> On Mon, 15 Sep 2025 16:17:19 +0800 Rohan G Thomas via B4 Relay wrote:
>>> From: Rohan G Thomas <rohan.g.thomas@altera.com>
>>>
>>> On hardware with Tx VLAN offload enabled, add the VLAN tag
>>> length to the skb length before checking the Qbv maxSDU.
>>> Add 4 bytes for 802.1Q an add 8 bytes for 802.1AD tagging.
>>>
>>> Fixes: c5c3e1bfc9e0 ("net: stmmac: Offload queueMaxSDU from tc-taprio")
>>> Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
>>> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
>>> ---
>>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++++++++++++++-------
>>>   1 file changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> index 8c8ca5999bd8ad369eafa0cd8448a15da55be86b..c06c947ef7764bf40291a556984651f4edd7cb74 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> @@ -4537,6 +4537,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>>   	bool has_vlan, set_ic;
>>>   	int entry, first_tx;
>>>   	dma_addr_t des;
>>> +	u32 sdu_len;
>>>   
>>>   	tx_q = &priv->dma_conf.tx_queue[queue];
>>>   	txq_stats = &priv->xstats.txq_stats[queue];
>>> @@ -4553,13 +4554,6 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>>   			return stmmac_tso_xmit(skb, dev);
>>>   	}
>>>   
>>> -	if (priv->est && priv->est->enable &&
>>> -	    priv->est->max_sdu[queue] &&
>>> -	    skb->len > priv->est->max_sdu[queue]){
>>> -		priv->xstats.max_sdu_txq_drop[queue]++;
>>> -		goto max_sdu_err;
>>> -	}
>>> -
>>>   	if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {
>>>   		if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
>>>   			netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
>>> @@ -4575,6 +4569,23 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>>   	/* Check if VLAN can be inserted by HW */
>>>   	has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
>>>   
>>> +	sdu_len = skb->len;
>>> +	if (has_vlan) {
>>> +		/* Add VLAN tag length to sdu length in case of txvlan offload */
>>> +		if (priv->dev->features & NETIF_F_HW_VLAN_CTAG_TX)
>>> +			sdu_len += VLAN_HLEN;
>>> +		if (skb->vlan_proto == htons(ETH_P_8021AD) &&
>>> +		    priv->dev->features & NETIF_F_HW_VLAN_STAG_TX)
>>> +			sdu_len += VLAN_HLEN;
>>
>> Is the device adding the same VLAN tag twice if the proto is 8021AD?
>> It looks like it from the code, but how every strange..
>>
>> In any case, it doesn't look like the driver is doing anything with
>> the NETIF_F_HW_VLAN_* flags right? stmmac_vlan_insert() works purely
>> off of vlan proto. So I think we should do the same thing here?
> 
> I suppose the double tagging depends on the exact SKU but first check
> looks unnecessary. Maybe stmmac_vlan_insert() should return the number
> of vlans it decided to insert?
> 

Agreed, those checks using NETIF_F_HW_VLAN_*_TX flags are redundant, as
stmmac_vlan_insert() already returns true. As you suggested I'll update 
stmmac_vlan_insert() to return the VLAN header length it decides to 
insert, so the logic can be simplified and made more concise.

>>> +	}
>>> +
>>> +	if (priv->est && priv->est->enable &&
>>> +	    priv->est->max_sdu[queue] &&
>>> +	    sdu_len > priv->est->max_sdu[queue]) {
>>> +		priv->xstats.max_sdu_txq_drop[queue]++;
>>> +		goto max_sdu_err;
>>> +	}
>>> +
>>>   	entry = tx_q->cur_tx;
>>>   	first_entry = entry;
>>>   	WARN_ON(tx_q->tx_skbuff[first_entry]);
>>>    
>>
> 

Best Regards,
Rohan

Re: [PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[G Thomas, Rohan]

Hi Jakub,

On 9/18/2025 4:24 AM, Jakub Kicinski wrote:
>>> +	sdu_len = skb->len;
>>> +	if (has_vlan) {
>>> +		/* Add VLAN tag length to sdu length in case of txvlan offload */
>>> +		if (priv->dev->features & NETIF_F_HW_VLAN_CTAG_TX)
>>> +			sdu_len += VLAN_HLEN;
>>> +		if (skb->vlan_proto == htons(ETH_P_8021AD) &&
>>> +		    priv->dev->features & NETIF_F_HW_VLAN_STAG_TX)
>>> +			sdu_len += VLAN_HLEN;
>>
>> Is the device adding the same VLAN tag twice if the proto is 8021AD?
>> It looks like it from the code, but how every strange..
>>
>> In any case, it doesn't look like the driver is doing anything with
>> the NETIF_F_HW_VLAN_* flags right? stmmac_vlan_insert() works purely
>> off of vlan proto. So I think we should do the same thing here?
> 
> I suppose the double tagging depends on the exact SKU but first check
> looks unnecessary. Maybe stmmac_vlan_insert() should return the number
> of vlans it decided to insert?
> 

I overlooked the behavior of stmmac_vlan_insert(). It seems the hardware
supports inserting only one VLAN tag at a time, with the default setting
being SVLAN for 802.1AD and CVLAN for 802.1Q. I'll update the patch to
simply add VLAN_HLEN when stmmac_vlan_insert() returns true. Please let
me know if you have any further concerns.

Best Regards,
Rohan

Re: [PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[Jakub Kicinski]

On Wed, 24 Sep 2025 10:24:44 +0530 G Thomas, Rohan wrote:
> >> Is the device adding the same VLAN tag twice if the proto is 8021AD?
> >> It looks like it from the code, but how every strange..
> >>
> >> In any case, it doesn't look like the driver is doing anything with
> >> the NETIF_F_HW_VLAN_* flags right? stmmac_vlan_insert() works purely
> >> off of vlan proto. So I think we should do the same thing here?  
> > 
> > I suppose the double tagging depends on the exact SKU but first check
> > looks unnecessary. Maybe stmmac_vlan_insert() should return the number
> > of vlans it decided to insert?
> >   
> 
> I overlooked the behavior of stmmac_vlan_insert(). It seems the hardware
> supports inserting only one VLAN tag at a time, with the default setting
> being SVLAN for 802.1AD and CVLAN for 802.1Q. I'll update the patch to
> simply add VLAN_HLEN when stmmac_vlan_insert() returns true. Please let
> me know if you have any further concerns.

SG, no further concerns.

Please make sure to CC "Ng, Boon Khai" <boon.khai.ng@altera.com>
who wrote the VLAN support (IIRC).

RE: [PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[Ng, Boon Khai]

> Please make sure to CC "Ng, Boon Khai" <boon.khai.ng@altera.com> who wrote
> the VLAN support (IIRC).

Working closely with Rohan on this.

Re: [PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[G Thomas, Rohan]

Hi Jakub,

On 9/25/2025 4:35 AM, Jakub Kicinski wrote:
> On Wed, 24 Sep 2025 10:24:44 +0530 G Thomas, Rohan wrote:
>>>> Is the device adding the same VLAN tag twice if the proto is 8021AD?
>>>> It looks like it from the code, but how every strange..
>>>>
>>>> In any case, it doesn't look like the driver is doing anything with
>>>> the NETIF_F_HW_VLAN_* flags right? stmmac_vlan_insert() works purely
>>>> off of vlan proto. So I think we should do the same thing here?
>>>
>>> I suppose the double tagging depends on the exact SKU but first check
>>> looks unnecessary. Maybe stmmac_vlan_insert() should return the number
>>> of vlans it decided to insert?
>>>    
>>
>> I overlooked the behavior of stmmac_vlan_insert(). It seems the hardware
>> supports inserting only one VLAN tag at a time, with the default setting
>> being SVLAN for 802.1AD and CVLAN for 802.1Q. I'll update the patch to
>> simply add VLAN_HLEN when stmmac_vlan_insert() returns true. Please let
>> me know if you have any further concerns.
> 
> SG, no further concerns.
> 
> Please make sure to CC "Ng, Boon Khai" <boon.khai.ng@altera.com>
> who wrote the VLAN support (IIRC).

While testing 802.1AD with XGMAC hardware using a simple ping test, I
observed an unexpected behavior: the hardware appears to insert an
additional 802.1Q CTAG with VLAN ID 0. Despite this, the ping test
functions correctly.

Here’s a snapshot from the pcap captured at the remote end. Outer VLAN
tag used is 100 and inner VLAN tag used is 200.

Frame 1: 110 bytes on wire (880 bits), 110 bytes captured (880 bits)
Ethernet II, Src: <src> (<src>), Dst: <dst> (<dst>)
IEEE 802.1ad, ID: 100
802.1Q Virtual LAN, PRI: 0, DEI: 0, ID: 0(unexpected)
802.1Q Virtual LAN, PRI: 0, DEI: 0, ID: 200
Internet Protocol Version 4, Src: 192.168.4.10, Dst: 192.168.4.11
Internet Control Message Protocol

I’m working with Boon Khai to understand whether this behavior is
expected or SKU-specific.

Any insights will be helpful for us.

Best Regards,
Rohan

Re: [PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[Jakub Kicinski]

On Thu, 25 Sep 2025 16:33:21 +0530 G Thomas, Rohan wrote:
> While testing 802.1AD with XGMAC hardware using a simple ping test, I
> observed an unexpected behavior: the hardware appears to insert an
> additional 802.1Q CTAG with VLAN ID 0. Despite this, the ping test
> functions correctly.
> 
> Here’s a snapshot from the pcap captured at the remote end. Outer VLAN
> tag used is 100 and inner VLAN tag used is 200.
> 
> Frame 1: 110 bytes on wire (880 bits), 110 bytes captured (880 bits)
> Ethernet II, Src: <src> (<src>), Dst: <dst> (<dst>)
> IEEE 802.1ad, ID: 100
> 802.1Q Virtual LAN, PRI: 0, DEI: 0, ID: 0(unexpected)
> 802.1Q Virtual LAN, PRI: 0, DEI: 0, ID: 200
> Internet Protocol Version 4, Src: 192.168.4.10, Dst: 192.168.4.11
> Internet Control Message Protocol

And the packet arrives at the driver with only the .1Q ID 200 pushed?

Indeed, that looks like a problem with the driver+HW interaction.
IDK what the right terminology is but IIRC VLAN 0 is not a real VLAN,
just an ID reserved for frames that don't have a VLAN ID but want to
use the priority field. Which explains why it "works", receiver just
ignores that tag. But it's definitely not correct because switches
on the network will no see the real C-TAG after the S-TAG is stripped.

Re: [PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[G Thomas, Rohan]

Hi Jakub,

On 9/26/2025 7:22 AM, Jakub Kicinski wrote:
> On Thu, 25 Sep 2025 16:33:21 +0530 G Thomas, Rohan wrote:
>> While testing 802.1AD with XGMAC hardware using a simple ping test, I
>> observed an unexpected behavior: the hardware appears to insert an
>> additional 802.1Q CTAG with VLAN ID 0. Despite this, the ping test
>> functions correctly.
>>
>> Here’s a snapshot from the pcap captured at the remote end. Outer VLAN
>> tag used is 100 and inner VLAN tag used is 200.
>>
>> Frame 1: 110 bytes on wire (880 bits), 110 bytes captured (880 bits)
>> Ethernet II, Src: <src> (<src>), Dst: <dst> (<dst>)
>> IEEE 802.1ad, ID: 100
>> 802.1Q Virtual LAN, PRI: 0, DEI: 0, ID: 0(unexpected)
>> 802.1Q Virtual LAN, PRI: 0, DEI: 0, ID: 200
>> Internet Protocol Version 4, Src: 192.168.4.10, Dst: 192.168.4.11
>> Internet Control Message Protocol
> 
> And the packet arrives at the driver with only the .1Q ID 200 pushed?
>

Yes, the packet arrives the driver with only 802.1Q ID.

[  210.192912] stmmaceth 10830000.ethernet eth0: >>> frame to be 
transmitted:
[  210.192917] len = 46 byte, buf addr: 0x0000000067c78222
[  210.192923] 00000000: xx xx xx xx xx xx xx xx xx xx xx xx 81 00 00 c8
[  210.192928] 00000010: 08 06 00 01 08 00 06 04 00 02 46 9b 06 1b 5b b6
[  210.192931] 00000020: c0 a8 04 0a c8 a3 62 0e d7 04 c0 a8 04 0b
> Indeed, that looks like a problem with the driver+HW interaction.
> IDK what the right terminology is but IIRC VLAN 0 is not a real VLAN,
> just an ID reserved for frames that don't have a VLAN ID but want to
> use the priority field. Which explains why it "works", receiver just
> ignores that tag. But it's definitely not correct because switches
> on the network will no see the real C-TAG after the S-TAG is stripped.

Yes, we are trying to figure out the right configuration for the driver
so that the right tag is inserted by the driver for double and single
VLANs. Based on the register configuration options for MAC_VLAN_Incl and
MAC_Inner_VLAN_Incl registers and descriptor configuration options
available, the hardware may not support simultaneous offloading of STAG
for 802.1AD double-tagged packets and CTAG for 802.1Q single-tagged
packets. If that is the case disable STAG insertion offloading may be
the right approach.

Best Regards,
Rohan

Re: [PATCH net v2 2/2] net: stmmac: Consider Tx VLAN offload tag length for maxSDU

[G Thomas, Rohan]

Hi Jakub,

On 9/26/2025 10:17 PM, G Thomas, Rohan wrote:
> Hi Jakub,
> 
> On 9/26/2025 7:22 AM, Jakub Kicinski wrote:
>> On Thu, 25 Sep 2025 16:33:21 +0530 G Thomas, Rohan wrote:
>>> While testing 802.1AD with XGMAC hardware using a simple ping test, I
>>> observed an unexpected behavior: the hardware appears to insert an
>>> additional 802.1Q CTAG with VLAN ID 0. Despite this, the ping test
>>> functions correctly.
>>>
>>> Here’s a snapshot from the pcap captured at the remote end. Outer VLAN
>>> tag used is 100 and inner VLAN tag used is 200.
>>>
>>> Frame 1: 110 bytes on wire (880 bits), 110 bytes captured (880 bits)
>>> Ethernet II, Src: <src> (<src>), Dst: <dst> (<dst>)
>>> IEEE 802.1ad, ID: 100
>>> 802.1Q Virtual LAN, PRI: 0, DEI: 0, ID: 0(unexpected)
>>> 802.1Q Virtual LAN, PRI: 0, DEI: 0, ID: 200
>>> Internet Protocol Version 4, Src: 192.168.4.10, Dst: 192.168.4.11
>>> Internet Control Message Protocol
>>
>> And the packet arrives at the driver with only the .1Q ID 200 pushed?
>>
> 
> Yes, the packet arrives the driver with only 802.1Q ID.
> 
> [  210.192912] stmmaceth 10830000.ethernet eth0: >>> frame to be 
> transmitted:
> [  210.192917] len = 46 byte, buf addr: 0x0000000067c78222
> [  210.192923] 00000000: xx xx xx xx xx xx xx xx xx xx xx xx 81 00 00 c8
> [  210.192928] 00000010: 08 06 00 01 08 00 06 04 00 02 46 9b 06 1b 5b b6
> [  210.192931] 00000020: c0 a8 04 0a c8 a3 62 0e d7 04 c0 a8 04 0b
>> Indeed, that looks like a problem with the driver+HW interaction.
>> IDK what the right terminology is but IIRC VLAN 0 is not a real VLAN,
>> just an ID reserved for frames that don't have a VLAN ID but want to
>> use the priority field. Which explains why it "works", receiver just
>> ignores that tag. But it's definitely not correct because switches
>> on the network will no see the real C-TAG after the S-TAG is stripped.
> 
> Yes, we are trying to figure out the right configuration for the driver
> so that the right tag is inserted by the driver for double and single
> VLANs. Based on the register configuration options for MAC_VLAN_Incl and
> MAC_Inner_VLAN_Incl registers and descriptor configuration options
> available, the hardware may not support simultaneous offloading of STAG
> for 802.1AD double-tagged packets and CTAG for 802.1Q single-tagged
> packets. If that is the case disable STAG insertion offloading may be
> the right approach.
> 
> Best Regards,
> Rohan

I’ve aligned internally with Boon Khai and conducted further validation
across various configurations of the MAC_VLAN_Incl register and Tx
descriptors. Based on our analysis, it appears that the hardware appears
to not support simultaneous offloading of both S-TAG for double VLAN
(802.1AD) packets and C-TAG for single VLAN (802.1Q) packets.

As per the XGMAC databook: CSVL bit of MAC_VLAN_Incl register controls
the VLAN type of the tag inserted in 13th and 14th bytes and CSVL bit of
MAC_Inner_VLAN_Incl register controls the VLAN type of the tag inserted
in 17th and 18th bytes of the packet.

Currently driver is configured to insert only STAG for MAC_VLAN_Incl
register while the MAC_Inner_VLAN_Incl register is not configured.
However Tx descriptors are configured for both Outer and Inner tag for
802.1AD packets.

Current configuration used in the driver is ambiguous and not documented
in the databook. So we think it is more accurate to disable
NETIF_F_HW_VLAN_STAG_TX for DWMAC IPs. Please let us know if anyone has
concerns with this approach or if we might be missing some info.

Best Regards,
Rohan