[v2] net: stmmac: Minor fixes for stmmac EST implementation

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

Posted by Rohan G Thomas via B4 Relay 2 weeks, 3 days ago

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

Posted by Jakub Kicinski 2 weeks ago

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

Posted by Jakub Kicinski 2 weeks ago

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

Posted by G Thomas, Rohan 1 week, 1 day ago

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

Posted by G Thomas, Rohan 2 weeks ago

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