linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net v3 0/3] net: stmmac: Fixes for stmmac Tx VLAN insert and EST
@ 2025-10-17  6:11 Rohan G Thomas via B4 Relay
  2025-10-17  6:11 ` [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload Rohan G Thomas via B4 Relay
                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Rohan G Thomas via B4 Relay @ 2025-10-17  6:11 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng
  Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel,
	Rohan G Thomas, Matthew Gerlach

This patchset includes following fixes for stmmac Tx VLAN insert and
EST implementations:
   1. Disable STAG insertion offloading, as DWMAC IPs doesn't support
      offload of STAG for double VLAN packets and CTAG for single VLAN
      packets when using the same register configuration. The current
      configuration in the driver is undocumented and is adding an
      additional 802.1Q tag with VLAN ID 0 for double VLAN packets.
   2. Consider Tx VLAN offload tag length for maxSDU estimation.
   3. Fix GCL bounds check

Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
---
Changes in v3:
- Commit for disabling 802.1AD tag insertion offload is introduced in
  to this patchset
- Add just VLAN_HLEN to sdu_len when 802.1Q tag offload is requested
- Link to v2: https://lore.kernel.org/r/20250915-qbv-fixes-v2-0-ec90673bb7d4@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 (3):
      net: stmmac: vlan: Disable 802.1AD tag insertion offload
      net: stmmac: Consider Tx VLAN offload tag length for maxSDU
      net: stmmac: est: Fix GCL bounds checks

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 37 ++++++++++-------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   |  4 +--
 drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c |  2 +-
 3 files changed, 19 insertions(+), 24 deletions(-)
---
base-commit: cb85ca4c0a349e246cd35161088aa3689ae5c580
change-id: 20250911-qbv-fixes-4ae64de86ee7

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




^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload
  2025-10-17  6:11 [PATCH net v3 0/3] net: stmmac: Fixes for stmmac Tx VLAN insert and EST Rohan G Thomas via B4 Relay
@ 2025-10-17  6:11 ` Rohan G Thomas via B4 Relay
  2025-10-17 12:42   ` Russell King (Oracle)
  2025-10-17  6:11 ` [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU Rohan G Thomas via B4 Relay
  2025-10-17  6:11 ` [PATCH net v3 3/3] net: stmmac: est: Fix GCL bounds checks Rohan G Thomas via B4 Relay
  2 siblings, 1 reply; 19+ messages in thread
From: Rohan G Thomas via B4 Relay @ 2025-10-17  6:11 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng
  Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel,
	Rohan G Thomas

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

The DWMAC IP's VLAN tag insertion offload does not support inserting
STAG (802.1AD) and CTAG (802.1Q) types in bytes 13 and 14 using the
same MAC_VLAN_Incl and MAC_VLAN_Inner_Incl register configurations.

Currently, MAC_VLAN_Incl is configured to offload only STAG type
insertion. However, the DWMAC IP inserts a CTAG type when the inner
VLAN ID field of the descriptor is not configured, and a STAG type
when it is configured. This behavior is not documented and leads to
inconsistent double VLAN tagging.

Additionally, an unexpected CTAG with VLAN ID 0 is inserted, resulting
in frames like:

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

To avoid this undocumented and incorrect behavior, disable 802.1AD tag
insertion offload. Also, don't set CSVL bit. As per the data book,
when this bit is set, S-VLAN type (0x88A8) is inserted in the 13th and
14th bytes of transmitted packets and when this bit is reset, C-VLAN
type (0x8100) is inserted in the 13th and 14th bytes of transmitted
packets.

Fixes: 30d932279dc2 ("net: stmmac: Add support for VLAN Insertion Offload")
Fixes: e94e3f3b51ce ("net: stmmac: Add support for VLAN Insertion Offload in GMAC4+")
Fixes: 1d2c7a5fee31 ("net: stmmac: Refactor VLAN implementation")
Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
Reviewed-by: Boon Khai Ng <boon.khai.ng@altera.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 19 +++++--------------
 drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c |  2 +-
 2 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 650d75b73e0b0ecd02d35dd5d6a8742d45188c47..dedaaef3208bfadc105961029f79d0d26c3289d8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4089,18 +4089,11 @@ static int stmmac_release(struct net_device *dev)
 static bool stmmac_vlan_insert(struct stmmac_priv *priv, struct sk_buff *skb,
 			       struct stmmac_tx_queue *tx_q)
 {
-	u16 tag = 0x0, inner_tag = 0x0;
-	u32 inner_type = 0x0;
+	u16 tag = 0x0;
 	struct dma_desc *p;
 
-	if (!priv->dma_cap.vlins)
+	if (!priv->dma_cap.vlins || !skb_vlan_tag_present(skb))
 		return false;
-	if (!skb_vlan_tag_present(skb))
-		return false;
-	if (skb->vlan_proto == htons(ETH_P_8021AD)) {
-		inner_tag = skb_vlan_tag_get(skb);
-		inner_type = STMMAC_VLAN_INSERT;
-	}
 
 	tag = skb_vlan_tag_get(skb);
 
@@ -4109,7 +4102,7 @@ static bool stmmac_vlan_insert(struct stmmac_priv *priv, struct sk_buff *skb,
 	else
 		p = &tx_q->dma_tx[tx_q->cur_tx];
 
-	if (stmmac_set_desc_vlan_tag(priv, p, tag, inner_tag, inner_type))
+	if (stmmac_set_desc_vlan_tag(priv, p, tag, 0x0, 0x0))
 		return false;
 
 	stmmac_set_tx_owner(priv, p);
@@ -7573,11 +7566,9 @@ int stmmac_dvr_probe(struct device *device,
 		ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
 		ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER;
 	}
-	if (priv->dma_cap.vlins) {
+	if (priv->dma_cap.vlins)
 		ndev->features |= NETIF_F_HW_VLAN_CTAG_TX;
-		if (priv->dma_cap.dvlan)
-			ndev->features |= NETIF_F_HW_VLAN_STAG_TX;
-	}
+
 #endif
 	priv->msg_enable = netif_msg_init(debug, default_msg_level);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c
index 0b6f6228ae35db3d855d8d386c3806a007a9d176..ff02a79c00d4f52a458edde1bcc08a0895b2c1c1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c
@@ -212,7 +212,7 @@ static void vlan_enable(struct mac_device_info *hw, u32 type)
 
 	value = readl(ioaddr + VLAN_INCL);
 	value |= VLAN_VLTI;
-	value |= VLAN_CSVL; /* Only use SVLAN */
+	value &= ~VLAN_CSVL; /* Only use CVLAN */
 	value &= ~VLAN_VLC;
 	value |= (type << VLAN_VLC_SHIFT) & VLAN_VLC;
 	writel(value, ioaddr + VLAN_INCL);

-- 
2.43.7




^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-17  6:11 [PATCH net v3 0/3] net: stmmac: Fixes for stmmac Tx VLAN insert and EST Rohan G Thomas via B4 Relay
  2025-10-17  6:11 ` [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload Rohan G Thomas via B4 Relay
@ 2025-10-17  6:11 ` Rohan G Thomas via B4 Relay
  2025-10-17  7:36   ` G Thomas, Rohan
                     ` (3 more replies)
  2025-10-17  6:11 ` [PATCH net v3 3/3] net: stmmac: est: Fix GCL bounds checks Rohan G Thomas via B4 Relay
  2 siblings, 4 replies; 19+ messages in thread
From: Rohan G Thomas via B4 Relay @ 2025-10-17  6:11 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng
  Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel,
	Rohan G Thomas, Matthew Gerlach

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 if Tx VLAN offload is
requested for the packet. Add 4 bytes for 802.1Q tag.

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 | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index dedaaef3208bfadc105961029f79d0d26c3289d8..23bf4a3d324b7f8e8c3067ed4d47b436a89c97d3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4500,6 +4500,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];
@@ -4516,13 +4517,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,
@@ -4535,8 +4529,18 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_BUSY;
 	}
 
+	sdu_len = skb->len;
 	/* Check if VLAN can be inserted by HW */
 	has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
+	if (has_vlan)
+		sdu_len += VLAN_HLEN;
+
+	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;
+	}
 
 	entry = tx_q->cur_tx;
 	first_entry = entry;

-- 
2.43.7




^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH net v3 3/3] net: stmmac: est: Fix GCL bounds checks
  2025-10-17  6:11 [PATCH net v3 0/3] net: stmmac: Fixes for stmmac Tx VLAN insert and EST Rohan G Thomas via B4 Relay
  2025-10-17  6:11 ` [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload Rohan G Thomas via B4 Relay
  2025-10-17  6:11 ` [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU Rohan G Thomas via B4 Relay
@ 2025-10-17  6:11 ` Rohan G Thomas via B4 Relay
  2 siblings, 0 replies; 19+ messages in thread
From: Rohan G Thomas via B4 Relay @ 2025-10-17  6:11 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng
  Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel,
	Rohan G Thomas, Matthew Gerlach

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 97e89a604abd7a01bb8e904c38f10716e0a911c1..3b4d4696afe96afe58e0c936429f51c22ae145be 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.43.7




^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-17  6:11 ` [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU Rohan G Thomas via B4 Relay
@ 2025-10-17  7:36   ` G Thomas, Rohan
  2025-10-17 12:21     ` Vadim Fedorenko
  2025-10-17 12:44   ` Russell King (Oracle)
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: G Thomas, Rohan @ 2025-10-17  7:36 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng
  Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel,
	Matthew Gerlach

Hi All,

On 10/17/2025 11:41 AM, Rohan G Thomas via B4 Relay wrote:
>   
> +	sdu_len = skb->len;
>   	/* Check if VLAN can be inserted by HW */
>   	has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
> +	if (has_vlan)
> +		sdu_len += VLAN_HLEN;
> +
> +	if (priv->est && priv->est->enable &&
> +	    priv->est->max_sdu[queue] &&
> +	    skb->len > priv->est->max_sdu[queue]){

I just noticed an issue with the reworked fix after sending the patch.
The condition should be: sdu_len > priv->est->max_sdu[queue]

I’ll send a corrected version, and will wait for any additional comments
in the meantime.

> +		priv->xstats.max_sdu_txq_drop[queue]++;
> +		goto max_sdu_err;
> +	}
>   
>   	entry = tx_q->cur_tx;
>   	first_entry = entry;
> 

Best Regards,
Rohan



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-17  7:36   ` G Thomas, Rohan
@ 2025-10-17 12:21     ` Vadim Fedorenko
  2025-10-18  1:50       ` G Thomas, Rohan
  0 siblings, 1 reply; 19+ messages in thread
From: Vadim Fedorenko @ 2025-10-17 12:21 UTC (permalink / raw)
  To: G Thomas, Rohan, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
	Jose Abreu, Rohan G Thomas, Boon Khai Ng
  Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel,
	Matthew Gerlach

On 17/10/2025 08:36, G Thomas, Rohan wrote:
> Hi All,
> 
> On 10/17/2025 11:41 AM, Rohan G Thomas via B4 Relay wrote:
>> +    sdu_len = skb->len;
>>       /* Check if VLAN can be inserted by HW */
>>       has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
>> +    if (has_vlan)
>> +        sdu_len += VLAN_HLEN;
>> +
>> +    if (priv->est && priv->est->enable &&
>> +        priv->est->max_sdu[queue] &&
>> +        skb->len > priv->est->max_sdu[queue]){
> 
> I just noticed an issue with the reworked fix after sending the patch.
> The condition should be: sdu_len > priv->est->max_sdu[queue]
> 
> I’ll send a corrected version, and will wait for any additional comments
> in the meantime.

Well, even though it's a copy of original code, it would be good to
improve some formatting and add a space at the end of if statement to
make it look like 'if () {'>
>> +        priv->xstats.max_sdu_txq_drop[queue]++;
>> +        goto max_sdu_err;
>> +    }
>>       entry = tx_q->cur_tx;
>>       first_entry = entry;
>>
> 
> Best Regards,
> Rohan
> 



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload
  2025-10-17  6:11 ` [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload Rohan G Thomas via B4 Relay
@ 2025-10-17 12:42   ` Russell King (Oracle)
  2025-10-18  1:56     ` G Thomas, Rohan
  0 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2025-10-17 12:42 UTC (permalink / raw)
  To: rohan.g.thomas
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel

On Fri, Oct 17, 2025 at 02:11:19PM +0800, Rohan G Thomas via B4 Relay wrote:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 650d75b73e0b0ecd02d35dd5d6a8742d45188c47..dedaaef3208bfadc105961029f79d0d26c3289d8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4089,18 +4089,11 @@ static int stmmac_release(struct net_device *dev)
>  static bool stmmac_vlan_insert(struct stmmac_priv *priv, struct sk_buff *skb,
>  			       struct stmmac_tx_queue *tx_q)
>  {
> -	u16 tag = 0x0, inner_tag = 0x0;
> -	u32 inner_type = 0x0;
> +	u16 tag = 0x0;
>  	struct dma_desc *p;

#include <stdnetdevcodeformat.h> - Please maintain reverse christmas-
tree order.

I haven't yet referred to the databook, so there may be more comments
coming next week.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-17  6:11 ` [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU Rohan G Thomas via B4 Relay
  2025-10-17  7:36   ` G Thomas, Rohan
@ 2025-10-17 12:44   ` Russell King (Oracle)
  2025-10-18  2:06     ` G Thomas, Rohan
  2025-10-17 17:16   ` kernel test robot
  2025-10-27  9:03   ` G Thomas, Rohan
  3 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2025-10-17 12:44 UTC (permalink / raw)
  To: rohan.g.thomas
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel, Matthew Gerlach

On Fri, Oct 17, 2025 at 02:11:20PM +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 if Tx VLAN offload is
> requested for the packet. Add 4 bytes for 802.1Q tag.

This needs to say _why_. Please describe the problem that the current
code suffers from. (e.g. the packet becomes too long for the queue to
handle, which causes it to be dropped - which is my guess.)

We shouldn't be guessing the reasons behind changes.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-17  6:11 ` [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU Rohan G Thomas via B4 Relay
  2025-10-17  7:36   ` G Thomas, Rohan
  2025-10-17 12:44   ` Russell King (Oracle)
@ 2025-10-17 17:16   ` kernel test robot
  2025-10-27  9:03   ` G Thomas, Rohan
  3 siblings, 0 replies; 19+ messages in thread
From: kernel test robot @ 2025-10-17 17:16 UTC (permalink / raw)
  To: Rohan G Thomas via B4 Relay, Andrew Lunn, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin,
	Alexandre Torgue, Jose Abreu, Rohan G Thomas, Boon Khai Ng
  Cc: llvm, oe-kbuild-all, netdev, linux-stm32, linux-arm-kernel,
	linux-kernel, Matthew Gerlach

Hi Rohan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on cb85ca4c0a349e246cd35161088aa3689ae5c580]

url:    https://github.com/intel-lab-lkp/linux/commits/Rohan-G-Thomas-via-B4-Relay/net-stmmac-vlan-Disable-802-1AD-tag-insertion-offload/20251017-144104
base:   cb85ca4c0a349e246cd35161088aa3689ae5c580
patch link:    https://lore.kernel.org/r/20251017-qbv-fixes-v3-2-d3a42e32646a%40altera.com
patch subject: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20251018/202510180039.zqD7oO26-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251018/202510180039.zqD7oO26-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510180039.zqD7oO26-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:4503:6: warning: variable 'sdu_len' set but not used [-Wunused-but-set-variable]
    4503 |         u32 sdu_len;
         |             ^
   1 warning generated.


vim +/sdu_len +4503 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

  4478	
  4479	/**
  4480	 *  stmmac_xmit - Tx entry point of the driver
  4481	 *  @skb : the socket buffer
  4482	 *  @dev : device pointer
  4483	 *  Description : this is the tx entry point of the driver.
  4484	 *  It programs the chain or the ring and supports oversized frames
  4485	 *  and SG feature.
  4486	 */
  4487	static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
  4488	{
  4489		unsigned int first_entry, tx_packets, enh_desc;
  4490		struct stmmac_priv *priv = netdev_priv(dev);
  4491		unsigned int nopaged_len = skb_headlen(skb);
  4492		int i, csum_insertion = 0, is_jumbo = 0;
  4493		u32 queue = skb_get_queue_mapping(skb);
  4494		int nfrags = skb_shinfo(skb)->nr_frags;
  4495		int gso = skb_shinfo(skb)->gso_type;
  4496		struct stmmac_txq_stats *txq_stats;
  4497		struct dma_edesc *tbs_desc = NULL;
  4498		struct dma_desc *desc, *first;
  4499		struct stmmac_tx_queue *tx_q;
  4500		bool has_vlan, set_ic;
  4501		int entry, first_tx;
  4502		dma_addr_t des;
> 4503		u32 sdu_len;
  4504	
  4505		tx_q = &priv->dma_conf.tx_queue[queue];
  4506		txq_stats = &priv->xstats.txq_stats[queue];
  4507		first_tx = tx_q->cur_tx;
  4508	
  4509		if (priv->tx_path_in_lpi_mode && priv->eee_sw_timer_en)
  4510			stmmac_stop_sw_lpi(priv);
  4511	
  4512		/* Manage oversized TCP frames for GMAC4 device */
  4513		if (skb_is_gso(skb) && priv->tso) {
  4514			if (gso & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))
  4515				return stmmac_tso_xmit(skb, dev);
  4516			if (priv->plat->has_gmac4 && (gso & SKB_GSO_UDP_L4))
  4517				return stmmac_tso_xmit(skb, dev);
  4518		}
  4519	
  4520		if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {
  4521			if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
  4522				netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
  4523									queue));
  4524				/* This is a hard error, log it. */
  4525				netdev_err(priv->dev,
  4526					   "%s: Tx Ring full when queue awake\n",
  4527					   __func__);
  4528			}
  4529			return NETDEV_TX_BUSY;
  4530		}
  4531	
  4532		sdu_len = skb->len;
  4533		/* Check if VLAN can be inserted by HW */
  4534		has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
  4535		if (has_vlan)
  4536			sdu_len += VLAN_HLEN;
  4537	
  4538		if (priv->est && priv->est->enable &&
  4539		    priv->est->max_sdu[queue] &&
  4540		    skb->len > priv->est->max_sdu[queue]){
  4541			priv->xstats.max_sdu_txq_drop[queue]++;
  4542			goto max_sdu_err;
  4543		}
  4544	
  4545		entry = tx_q->cur_tx;
  4546		first_entry = entry;
  4547		WARN_ON(tx_q->tx_skbuff[first_entry]);
  4548	
  4549		csum_insertion = (skb->ip_summed == CHECKSUM_PARTIAL);
  4550		/* DWMAC IPs can be synthesized to support tx coe only for a few tx
  4551		 * queues. In that case, checksum offloading for those queues that don't
  4552		 * support tx coe needs to fallback to software checksum calculation.
  4553		 *
  4554		 * Packets that won't trigger the COE e.g. most DSA-tagged packets will
  4555		 * also have to be checksummed in software.
  4556		 */
  4557		if (csum_insertion &&
  4558		    (priv->plat->tx_queues_cfg[queue].coe_unsupported ||
  4559		     !stmmac_has_ip_ethertype(skb))) {
  4560			if (unlikely(skb_checksum_help(skb)))
  4561				goto dma_map_err;
  4562			csum_insertion = !csum_insertion;
  4563		}
  4564	
  4565		if (likely(priv->extend_desc))
  4566			desc = (struct dma_desc *)(tx_q->dma_etx + entry);
  4567		else if (tx_q->tbs & STMMAC_TBS_AVAIL)
  4568			desc = &tx_q->dma_entx[entry].basic;
  4569		else
  4570			desc = tx_q->dma_tx + entry;
  4571	
  4572		first = desc;
  4573	
  4574		if (has_vlan)
  4575			stmmac_set_desc_vlan(priv, first, STMMAC_VLAN_INSERT);
  4576	
  4577		enh_desc = priv->plat->enh_desc;
  4578		/* To program the descriptors according to the size of the frame */
  4579		if (enh_desc)
  4580			is_jumbo = stmmac_is_jumbo_frm(priv, skb->len, enh_desc);
  4581	
  4582		if (unlikely(is_jumbo)) {
  4583			entry = stmmac_jumbo_frm(priv, tx_q, skb, csum_insertion);
  4584			if (unlikely(entry < 0) && (entry != -EINVAL))
  4585				goto dma_map_err;
  4586		}
  4587	
  4588		for (i = 0; i < nfrags; i++) {
  4589			const skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
  4590			int len = skb_frag_size(frag);
  4591			bool last_segment = (i == (nfrags - 1));
  4592	
  4593			entry = STMMAC_GET_ENTRY(entry, priv->dma_conf.dma_tx_size);
  4594			WARN_ON(tx_q->tx_skbuff[entry]);
  4595	
  4596			if (likely(priv->extend_desc))
  4597				desc = (struct dma_desc *)(tx_q->dma_etx + entry);
  4598			else if (tx_q->tbs & STMMAC_TBS_AVAIL)
  4599				desc = &tx_q->dma_entx[entry].basic;
  4600			else
  4601				desc = tx_q->dma_tx + entry;
  4602	
  4603			des = skb_frag_dma_map(priv->device, frag, 0, len,
  4604					       DMA_TO_DEVICE);
  4605			if (dma_mapping_error(priv->device, des))
  4606				goto dma_map_err; /* should reuse desc w/o issues */
  4607	
  4608			tx_q->tx_skbuff_dma[entry].buf = des;
  4609	
  4610			stmmac_set_desc_addr(priv, desc, des);
  4611	
  4612			tx_q->tx_skbuff_dma[entry].map_as_page = true;
  4613			tx_q->tx_skbuff_dma[entry].len = len;
  4614			tx_q->tx_skbuff_dma[entry].last_segment = last_segment;
  4615			tx_q->tx_skbuff_dma[entry].buf_type = STMMAC_TXBUF_T_SKB;
  4616	
  4617			/* Prepare the descriptor and set the own bit too */
  4618			stmmac_prepare_tx_desc(priv, desc, 0, len, csum_insertion,
  4619					priv->mode, 1, last_segment, skb->len);
  4620		}
  4621	
  4622		/* Only the last descriptor gets to point to the skb. */
  4623		tx_q->tx_skbuff[entry] = skb;
  4624		tx_q->tx_skbuff_dma[entry].buf_type = STMMAC_TXBUF_T_SKB;
  4625	
  4626		/* According to the coalesce parameter the IC bit for the latest
  4627		 * segment is reset and the timer re-started to clean the tx status.
  4628		 * This approach takes care about the fragments: desc is the first
  4629		 * element in case of no SG.
  4630		 */
  4631		tx_packets = (entry + 1) - first_tx;
  4632		tx_q->tx_count_frames += tx_packets;
  4633	
  4634		if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && priv->hwts_tx_en)
  4635			set_ic = true;
  4636		else if (!priv->tx_coal_frames[queue])
  4637			set_ic = false;
  4638		else if (tx_packets > priv->tx_coal_frames[queue])
  4639			set_ic = true;
  4640		else if ((tx_q->tx_count_frames %
  4641			  priv->tx_coal_frames[queue]) < tx_packets)
  4642			set_ic = true;
  4643		else
  4644			set_ic = false;
  4645	
  4646		if (set_ic) {
  4647			if (likely(priv->extend_desc))
  4648				desc = &tx_q->dma_etx[entry].basic;
  4649			else if (tx_q->tbs & STMMAC_TBS_AVAIL)
  4650				desc = &tx_q->dma_entx[entry].basic;
  4651			else
  4652				desc = &tx_q->dma_tx[entry];
  4653	
  4654			tx_q->tx_count_frames = 0;
  4655			stmmac_set_tx_ic(priv, desc);
  4656		}
  4657	
  4658		/* We've used all descriptors we need for this skb, however,
  4659		 * advance cur_tx so that it references a fresh descriptor.
  4660		 * ndo_start_xmit will fill this descriptor the next time it's
  4661		 * called and stmmac_tx_clean may clean up to this descriptor.
  4662		 */
  4663		entry = STMMAC_GET_ENTRY(entry, priv->dma_conf.dma_tx_size);
  4664		tx_q->cur_tx = entry;
  4665	
  4666		if (netif_msg_pktdata(priv)) {
  4667			netdev_dbg(priv->dev,
  4668				   "%s: curr=%d dirty=%d f=%d, e=%d, first=%p, nfrags=%d",
  4669				   __func__, tx_q->cur_tx, tx_q->dirty_tx, first_entry,
  4670				   entry, first, nfrags);
  4671	
  4672			netdev_dbg(priv->dev, ">>> frame to be transmitted: ");
  4673			print_pkt(skb->data, skb->len);
  4674		}
  4675	
  4676		if (unlikely(stmmac_tx_avail(priv, queue) <= (MAX_SKB_FRAGS + 1))) {
  4677			netif_dbg(priv, hw, priv->dev, "%s: stop transmitted packets\n",
  4678				  __func__);
  4679			netif_tx_stop_queue(netdev_get_tx_queue(priv->dev, queue));
  4680		}
  4681	
  4682		u64_stats_update_begin(&txq_stats->q_syncp);
  4683		u64_stats_add(&txq_stats->q.tx_bytes, skb->len);
  4684		if (set_ic)
  4685			u64_stats_inc(&txq_stats->q.tx_set_ic_bit);
  4686		u64_stats_update_end(&txq_stats->q_syncp);
  4687	
  4688		if (priv->sarc_type)
  4689			stmmac_set_desc_sarc(priv, first, priv->sarc_type);
  4690	
  4691		/* Ready to fill the first descriptor and set the OWN bit w/o any
  4692		 * problems because all the descriptors are actually ready to be
  4693		 * passed to the DMA engine.
  4694		 */
  4695		if (likely(!is_jumbo)) {
  4696			bool last_segment = (nfrags == 0);
  4697	
  4698			des = dma_map_single(priv->device, skb->data,
  4699					     nopaged_len, DMA_TO_DEVICE);
  4700			if (dma_mapping_error(priv->device, des))
  4701				goto dma_map_err;
  4702	
  4703			tx_q->tx_skbuff_dma[first_entry].buf = des;
  4704			tx_q->tx_skbuff_dma[first_entry].buf_type = STMMAC_TXBUF_T_SKB;
  4705			tx_q->tx_skbuff_dma[first_entry].map_as_page = false;
  4706	
  4707			stmmac_set_desc_addr(priv, first, des);
  4708	
  4709			tx_q->tx_skbuff_dma[first_entry].len = nopaged_len;
  4710			tx_q->tx_skbuff_dma[first_entry].last_segment = last_segment;
  4711	
  4712			if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
  4713				     priv->hwts_tx_en)) {
  4714				/* declare that device is doing timestamping */
  4715				skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
  4716				stmmac_enable_tx_timestamp(priv, first);
  4717			}
  4718	
  4719			/* Prepare the first descriptor setting the OWN bit too */
  4720			stmmac_prepare_tx_desc(priv, first, 1, nopaged_len,
  4721					csum_insertion, priv->mode, 0, last_segment,
  4722					skb->len);
  4723		}
  4724	
  4725		if (tx_q->tbs & STMMAC_TBS_EN) {
  4726			struct timespec64 ts = ns_to_timespec64(skb->tstamp);
  4727	
  4728			tbs_desc = &tx_q->dma_entx[first_entry];
  4729			stmmac_set_desc_tbs(priv, tbs_desc, ts.tv_sec, ts.tv_nsec);
  4730		}
  4731	
  4732		stmmac_set_tx_owner(priv, first);
  4733	
  4734		netdev_tx_sent_queue(netdev_get_tx_queue(dev, queue), skb->len);
  4735	
  4736		stmmac_enable_dma_transmission(priv, priv->ioaddr, queue);
  4737		skb_tx_timestamp(skb);
  4738		stmmac_flush_tx_descriptors(priv, queue);
  4739		stmmac_tx_timer_arm(priv, queue);
  4740	
  4741		return NETDEV_TX_OK;
  4742	
  4743	dma_map_err:
  4744		netdev_err(priv->dev, "Tx DMA map failed\n");
  4745	max_sdu_err:
  4746		dev_kfree_skb(skb);
  4747		priv->xstats.tx_dropped++;
  4748		return NETDEV_TX_OK;
  4749	}
  4750	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-17 12:21     ` Vadim Fedorenko
@ 2025-10-18  1:50       ` G Thomas, Rohan
  2025-10-23 10:59         ` Russell King (Oracle)
  0 siblings, 1 reply; 19+ messages in thread
From: G Thomas, Rohan @ 2025-10-18  1:50 UTC (permalink / raw)
  To: Vadim Fedorenko, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
	Jose Abreu, Rohan G Thomas, Boon Khai Ng
  Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel,
	Matthew Gerlach

Hi Vadim,

On 10/17/2025 5:51 PM, Vadim Fedorenko wrote:
> On 17/10/2025 08:36, G Thomas, Rohan wrote:
>> Hi All,
>>
>> On 10/17/2025 11:41 AM, Rohan G Thomas via B4 Relay wrote:
>>> +    sdu_len = skb->len;
>>>       /* Check if VLAN can be inserted by HW */
>>>       has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
>>> +    if (has_vlan)
>>> +        sdu_len += VLAN_HLEN;
>>> +
>>> +    if (priv->est && priv->est->enable &&
>>> +        priv->est->max_sdu[queue] &&
>>> +        skb->len > priv->est->max_sdu[queue]){
>>
>> I just noticed an issue with the reworked fix after sending the patch.
>> The condition should be: sdu_len > priv->est->max_sdu[queue]
>>
>> I’ll send a corrected version, and will wait for any additional comments
>> in the meantime.
> 
> Well, even though it's a copy of original code, it would be good to
> improve some formatting and add a space at the end of if statement to
> make it look like 'if () {'>

Thanks for pointing this out. I'll fix the formatting in the next version.

>>> +        priv->xstats.max_sdu_txq_drop[queue]++;
>>> +        goto max_sdu_err;
>>> +    }
>>>       entry = tx_q->cur_tx;
>>>       first_entry = entry;
>>>
>>
>> Best Regards,
>> Rohan
>>
> 

Best Regards,
Rohan



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload
  2025-10-17 12:42   ` Russell King (Oracle)
@ 2025-10-18  1:56     ` G Thomas, Rohan
  2025-10-23  3:31       ` G Thomas, Rohan
  0 siblings, 1 reply; 19+ messages in thread
From: G Thomas, Rohan @ 2025-10-18  1:56 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel

Hi Russell,

On 10/17/2025 6:12 PM, Russell King (Oracle) wrote:
> On Fri, Oct 17, 2025 at 02:11:19PM +0800, Rohan G Thomas via B4 Relay wrote:
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 650d75b73e0b0ecd02d35dd5d6a8742d45188c47..dedaaef3208bfadc105961029f79d0d26c3289d8 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -4089,18 +4089,11 @@ static int stmmac_release(struct net_device *dev)
>>   static bool stmmac_vlan_insert(struct stmmac_priv *priv, struct sk_buff *skb,
>>   			       struct stmmac_tx_queue *tx_q)
>>   {
>> -	u16 tag = 0x0, inner_tag = 0x0;
>> -	u32 inner_type = 0x0;
>> +	u16 tag = 0x0;
>>   	struct dma_desc *p;
> 
> #include <stdnetdevcodeformat.h> - Please maintain reverse christmas-
> tree order.

Thanks for pointing this out. I'll fix the declaration order in the next 
revision.

> 
> I haven't yet referred to the databook, so there may be more comments
> coming next week.
> 

Sure! Will wait for your feedback before sending the next revision.

Best Regards,
Rohan


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-17 12:44   ` Russell King (Oracle)
@ 2025-10-18  2:06     ` G Thomas, Rohan
  2025-10-23 10:58       ` Russell King (Oracle)
  0 siblings, 1 reply; 19+ messages in thread
From: G Thomas, Rohan @ 2025-10-18  2:06 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel, Matthew Gerlach

Hi Russell,

Thanks, I'll update the commit message.

On 10/17/2025 6:14 PM, Russell King (Oracle) wrote:
> On Fri, Oct 17, 2025 at 02:11:20PM +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 if Tx VLAN offload is
>> requested for the packet. Add 4 bytes for 802.1Q tag.
> 
> This needs to say _why_. Please describe the problem that the current
> code suffers from. (e.g. the packet becomes too long for the queue to
> handle, which causes it to be dropped - which is my guess.)
> 
> We shouldn't be guessing the reasons behind changes.
> 

Queue maxSDU requirement of 802.1 Qbv standard requires mac to drop
packets that exceeds maxSDU length and maxSDU doesn't include preamble,
destination and source address, or FCS but includes ethernet type and 
VLAN header.

On hardware with Tx VLAN offload enabled, VLAN header length is not
included in the skb->len, when Tx VLAN offload is requested. This leads
to incorrect length checks and allows transmission of oversized packets.
Add the VLAN_HLEN to the skb->len before checking the Qbv maxSDU if Tx
VLAN offload is requested for the packet.

This patch ensures that the VLAN header length (`VLAN_HLEN`) is
accounted for in the SDU length check when VLAN offload is requested.

Best Regards,
Rohan


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload
  2025-10-18  1:56     ` G Thomas, Rohan
@ 2025-10-23  3:31       ` G Thomas, Rohan
  2025-10-23 10:54         ` Russell King (Oracle)
  0 siblings, 1 reply; 19+ messages in thread
From: G Thomas, Rohan @ 2025-10-23  3:31 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Ng, Boon Khai, netdev@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org

Hi Russell,

On 10/18/2025 7:26 AM, G Thomas, Rohan wrote:
> Hi Russell,
> 
> On 10/17/2025 6:12 PM, Russell King (Oracle) wrote:
>> On Fri, Oct 17, 2025 at 02:11:19PM +0800, Rohan G Thomas via B4 Relay wrote:
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> index 650d75b73e0b0ecd02d35dd5d6a8742d45188c47..dedaaef3208bfadc105961029f79d0d26c3289d8 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> @@ -4089,18 +4089,11 @@ static int stmmac_release(struct net_device *dev)
>>>    static bool stmmac_vlan_insert(struct stmmac_priv *priv, struct sk_buff *skb,
>>>    			       struct stmmac_tx_queue *tx_q)
>>>    {
>>> -	u16 tag = 0x0, inner_tag = 0x0;
>>> -	u32 inner_type = 0x0;
>>> +	u16 tag = 0x0;
>>>    	struct dma_desc *p;
>>
>> #include <stdnetdevcodeformat.h> - Please maintain reverse christmas-
>> tree order.
> 
> Thanks for pointing this out. I'll fix the declaration order in the next
> revision.
> 
>>
>> I haven't yet referred to the databook, so there may be more comments
>> coming next week.
>>
> 
> Sure! Will wait for your feedback before sending the next revision.

Just checking in — have you had a chance to review the patch further? Or
would it be okay for me to go ahead and send the next revision for
review?

> 
> Best Regards,
> Rohan

Best Regards,
Rohan


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload
  2025-10-23  3:31       ` G Thomas, Rohan
@ 2025-10-23 10:54         ` Russell King (Oracle)
  2025-10-24  3:03           ` G Thomas, Rohan
  0 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2025-10-23 10:54 UTC (permalink / raw)
  To: G Thomas, Rohan
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Ng, Boon Khai, netdev@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org

On Thu, Oct 23, 2025 at 09:01:20AM +0530, G Thomas, Rohan wrote:
> Hi Russell,
> 
> On 10/18/2025 7:26 AM, G Thomas, Rohan wrote:
> > Hi Russell,
> > 
> > On 10/17/2025 6:12 PM, Russell King (Oracle) wrote:
> > > On Fri, Oct 17, 2025 at 02:11:19PM +0800, Rohan G Thomas via B4 Relay wrote:
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > index 650d75b73e0b0ecd02d35dd5d6a8742d45188c47..dedaaef3208bfadc105961029f79d0d26c3289d8 100644
> > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > @@ -4089,18 +4089,11 @@ static int stmmac_release(struct net_device *dev)
> > > >    static bool stmmac_vlan_insert(struct stmmac_priv *priv, struct sk_buff *skb,
> > > >    			       struct stmmac_tx_queue *tx_q)
> > > >    {
> > > > -	u16 tag = 0x0, inner_tag = 0x0;
> > > > -	u32 inner_type = 0x0;
> > > > +	u16 tag = 0x0;
> > > >    	struct dma_desc *p;
> > > 
> > > #include <stdnetdevcodeformat.h> - Please maintain reverse christmas-
> > > tree order.
> > 
> > Thanks for pointing this out. I'll fix the declaration order in the next
> > revision.
> > 
> > > 
> > > I haven't yet referred to the databook, so there may be more comments
> > > coming next week.
> > > 
> > 
> > Sure! Will wait for your feedback before sending the next revision.
> 
> Just checking in — have you had a chance to review the patch further? Or
> would it be okay for me to go ahead and send the next revision for
> review?

I've checked my version of the databook, and the core version that has
VLINS/DVLAN and my databook doesn't cover this. So I'm afraid I can't
review further.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-18  2:06     ` G Thomas, Rohan
@ 2025-10-23 10:58       ` Russell King (Oracle)
  0 siblings, 0 replies; 19+ messages in thread
From: Russell King (Oracle) @ 2025-10-23 10:58 UTC (permalink / raw)
  To: G Thomas, Rohan
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel, Matthew Gerlach

On Sat, Oct 18, 2025 at 07:36:26AM +0530, G Thomas, Rohan wrote:
> Hi Russell,
> 
> Thanks, I'll update the commit message.
> 
> On 10/17/2025 6:14 PM, Russell King (Oracle) wrote:
> > On Fri, Oct 17, 2025 at 02:11:20PM +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 if Tx VLAN offload is
> > > requested for the packet. Add 4 bytes for 802.1Q tag.
> > 
> > This needs to say _why_. Please describe the problem that the current
> > code suffers from. (e.g. the packet becomes too long for the queue to
> > handle, which causes it to be dropped - which is my guess.)
> > 
> > We shouldn't be guessing the reasons behind changes.
> > 
> 
> Queue maxSDU requirement of 802.1 Qbv standard requires mac to drop
> packets that exceeds maxSDU length and maxSDU doesn't include preamble,
> destination and source address, or FCS but includes ethernet type and VLAN
> header.
> 
> On hardware with Tx VLAN offload enabled, VLAN header length is not
> included in the skb->len, when Tx VLAN offload is requested. This leads
> to incorrect length checks and allows transmission of oversized packets.
> Add the VLAN_HLEN to the skb->len before checking the Qbv maxSDU if Tx
> VLAN offload is requested for the packet.
> 
> This patch ensures that the VLAN header length (`VLAN_HLEN`) is
> accounted for in the SDU length check when VLAN offload is requested.

Please put that in the commit message, thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-18  1:50       ` G Thomas, Rohan
@ 2025-10-23 10:59         ` Russell King (Oracle)
  2025-10-23 16:03           ` G Thomas, Rohan
  0 siblings, 1 reply; 19+ messages in thread
From: Russell King (Oracle) @ 2025-10-23 10:59 UTC (permalink / raw)
  To: G Thomas, Rohan
  Cc: Vadim Fedorenko, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
	Jose Abreu, Rohan G Thomas, Boon Khai Ng, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel, Matthew Gerlach

On Sat, Oct 18, 2025 at 07:20:03AM +0530, G Thomas, Rohan wrote:
> Hi Vadim,
> 
> On 10/17/2025 5:51 PM, Vadim Fedorenko wrote:
> > On 17/10/2025 08:36, G Thomas, Rohan wrote:
> > > Hi All,
> > > 
> > > On 10/17/2025 11:41 AM, Rohan G Thomas via B4 Relay wrote:
> > > > +    sdu_len = skb->len;
> > > >       /* Check if VLAN can be inserted by HW */
> > > >       has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
> > > > +    if (has_vlan)
> > > > +        sdu_len += VLAN_HLEN;
> > > > +
> > > > +    if (priv->est && priv->est->enable &&
> > > > +        priv->est->max_sdu[queue] &&
> > > > +        skb->len > priv->est->max_sdu[queue]){
> > > 
> > > I just noticed an issue with the reworked fix after sending the patch.
> > > The condition should be: sdu_len > priv->est->max_sdu[queue]
> > > 
> > > I’ll send a corrected version, and will wait for any additional comments
> > > in the meantime.
> > 
> > Well, even though it's a copy of original code, it would be good to
> > improve some formatting and add a space at the end of if statement to
> > make it look like 'if () {'>
> 
> Thanks for pointing this out. I'll fix the formatting in the next version.

I suggest:

First patch - fix formatting.
Second patch - move the code.

We have a general rule that when code is moved, it should be moved with
no changes - otherwise it makes review much harder.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-23 10:59         ` Russell King (Oracle)
@ 2025-10-23 16:03           ` G Thomas, Rohan
  0 siblings, 0 replies; 19+ messages in thread
From: G Thomas, Rohan @ 2025-10-23 16:03 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Vadim Fedorenko, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
	Jose Abreu, Rohan G Thomas, Boon Khai Ng, netdev, linux-stm32,
	linux-arm-kernel, linux-kernel, Matthew Gerlach

Hi Russell,

On 10/23/2025 4:29 PM, Russell King (Oracle) wrote:
> On Sat, Oct 18, 2025 at 07:20:03AM +0530, G Thomas, Rohan wrote:
>> Hi Vadim,
>>
>> On 10/17/2025 5:51 PM, Vadim Fedorenko wrote:
>>> On 17/10/2025 08:36, G Thomas, Rohan wrote:
>>>> Hi All,
>>>>
>>>> On 10/17/2025 11:41 AM, Rohan G Thomas via B4 Relay wrote:
>>>>> +    sdu_len = skb->len;
>>>>>        /* Check if VLAN can be inserted by HW */
>>>>>        has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
>>>>> +    if (has_vlan)
>>>>> +        sdu_len += VLAN_HLEN;
>>>>> +
>>>>> +    if (priv->est && priv->est->enable &&
>>>>> +        priv->est->max_sdu[queue] &&
>>>>> +        skb->len > priv->est->max_sdu[queue]){
>>>>
>>>> I just noticed an issue with the reworked fix after sending the patch.
>>>> The condition should be: sdu_len > priv->est->max_sdu[queue]
>>>>
>>>> I’ll send a corrected version, and will wait for any additional comments
>>>> in the meantime.
>>>
>>> Well, even though it's a copy of original code, it would be good to
>>> improve some formatting and add a space at the end of if statement to
>>> make it look like 'if () {'>
>>
>> Thanks for pointing this out. I'll fix the formatting in the next version.
> 
> I suggest:
> 
> First patch - fix formatting.
> Second patch - move the code.
> 
> We have a general rule that when code is moved, it should be moved with
> no changes - otherwise it makes review much harder.
> 

Thanks for the suggestion. Will do it in separate patches.

> Thanks.
> 

Best Regards,
Rohan



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload
  2025-10-23 10:54         ` Russell King (Oracle)
@ 2025-10-24  3:03           ` G Thomas, Rohan
  0 siblings, 0 replies; 19+ messages in thread
From: G Thomas, Rohan @ 2025-10-24  3:03 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Ng, Boon Khai, netdev@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org

Hi Russell,

On 10/23/2025 4:24 PM, Russell King (Oracle) wrote:
> On Thu, Oct 23, 2025 at 09:01:20AM +0530, G Thomas, Rohan wrote:
>> Hi Russell,
>>
>> On 10/18/2025 7:26 AM, G Thomas, Rohan wrote:
>>> Hi Russell,
>>>
>>> On 10/17/2025 6:12 PM, Russell King (Oracle) wrote:
>>>> On Fri, Oct 17, 2025 at 02:11:19PM +0800, Rohan G Thomas via B4 Relay wrote:
>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>>> index 650d75b73e0b0ecd02d35dd5d6a8742d45188c47..dedaaef3208bfadc105961029f79d0d26c3289d8 100644
>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>>> @@ -4089,18 +4089,11 @@ static int stmmac_release(struct net_device *dev)
>>>>>     static bool stmmac_vlan_insert(struct stmmac_priv *priv, struct sk_buff *skb,
>>>>>     			       struct stmmac_tx_queue *tx_q)
>>>>>     {
>>>>> -	u16 tag = 0x0, inner_tag = 0x0;
>>>>> -	u32 inner_type = 0x0;
>>>>> +	u16 tag = 0x0;
>>>>>     	struct dma_desc *p;
>>>>
>>>> #include <stdnetdevcodeformat.h> - Please maintain reverse christmas-
>>>> tree order.
>>>
>>> Thanks for pointing this out. I'll fix the declaration order in the next
>>> revision.
>>>
>>>>
>>>> I haven't yet referred to the databook, so there may be more comments
>>>> coming next week.
>>>>
>>>
>>> Sure! Will wait for your feedback before sending the next revision.
>>
>> Just checking in — have you had a chance to review the patch further? Or
>> would it be okay for me to go ahead and send the next revision for
>> review?
> 
> I've checked my version of the databook, and the core version that has
> VLINS/DVLAN and my databook doesn't cover this. So I'm afraid I can't
> review further.
> 

Thanks for checking. Understood.

Following public document appears to include the DWMAC QoS IP databook.
Section 44 of the following user manual might be a useful reference.
https://www.infineon.com/row/public/documents/10/44/infineon-aurix-tc3xx-part2-usermanual-en.pdf

Best Regards,
Rohan


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU
  2025-10-17  6:11 ` [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU Rohan G Thomas via B4 Relay
                     ` (2 preceding siblings ...)
  2025-10-17 17:16   ` kernel test robot
@ 2025-10-27  9:03   ` G Thomas, Rohan
  3 siblings, 0 replies; 19+ messages in thread
From: G Thomas, Rohan @ 2025-10-27  9:03 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Maxime Coquelin, Alexandre Torgue, Jose Abreu,
	Rohan G Thomas, Boon Khai Ng
  Cc: netdev, linux-stm32, linux-arm-kernel, linux-kernel,
	Matthew Gerlach

Hi All,

I've noticed one more issue with this commit. Need to drop the packet
before inserting the context descriptor with VLAN. So I think I have to
keep the max_sdu check in the original place itself, but add VLAN length 
if priv->dma_cap.vlins && skb_vlan_tag_present(skb) are true. Will 
change it accordingly in the next version.

Apologies for the oversight in the initial patch.

On 10/17/2025 11:41 AM, 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 if Tx VLAN offload is
> requested for the packet. Add 4 bytes for 802.1Q tag.
> 
> 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 | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index dedaaef3208bfadc105961029f79d0d26c3289d8..23bf4a3d324b7f8e8c3067ed4d47b436a89c97d3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4500,6 +4500,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];
> @@ -4516,13 +4517,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,
> @@ -4535,8 +4529,18 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>   		return NETDEV_TX_BUSY;
>   	}
>   
> +	sdu_len = skb->len;
>   	/* Check if VLAN can be inserted by HW */
>   	has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
> +	if (has_vlan)
> +		sdu_len += VLAN_HLEN;
> +
> +	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;
> +	}
>   
>   	entry = tx_q->cur_tx;
>   	first_entry = entry;
> 

Best Regards,
Rohan


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2025-10-27  9:04 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-17  6:11 [PATCH net v3 0/3] net: stmmac: Fixes for stmmac Tx VLAN insert and EST Rohan G Thomas via B4 Relay
2025-10-17  6:11 ` [PATCH net v3 1/3] net: stmmac: vlan: Disable 802.1AD tag insertion offload Rohan G Thomas via B4 Relay
2025-10-17 12:42   ` Russell King (Oracle)
2025-10-18  1:56     ` G Thomas, Rohan
2025-10-23  3:31       ` G Thomas, Rohan
2025-10-23 10:54         ` Russell King (Oracle)
2025-10-24  3:03           ` G Thomas, Rohan
2025-10-17  6:11 ` [PATCH net v3 2/3] net: stmmac: Consider Tx VLAN offload tag length for maxSDU Rohan G Thomas via B4 Relay
2025-10-17  7:36   ` G Thomas, Rohan
2025-10-17 12:21     ` Vadim Fedorenko
2025-10-18  1:50       ` G Thomas, Rohan
2025-10-23 10:59         ` Russell King (Oracle)
2025-10-23 16:03           ` G Thomas, Rohan
2025-10-17 12:44   ` Russell King (Oracle)
2025-10-18  2:06     ` G Thomas, Rohan
2025-10-23 10:58       ` Russell King (Oracle)
2025-10-17 17:16   ` kernel test robot
2025-10-27  9:03   ` G Thomas, Rohan
2025-10-17  6:11 ` [PATCH net v3 3/3] net: stmmac: est: Fix GCL bounds checks Rohan G Thomas via B4 Relay

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).