diff mbox series

[11/14] net: ethernet: mtk_eth_soc: fix VLAN rx hardware acceleration

Message ID 20221107185452.90711-11-nbd@nbd.name (mailing list archive)
State New, archived
Headers show
Series [01/14] net: ethernet: mtk_eth_soc: account for vlan in rx header length | expand

Commit Message

Felix Fietkau Nov. 7, 2022, 6:54 p.m. UTC 
- enable VLAN untagging for PDMA rx
- make it possible to disable the feature via ethtool
- pass VLAN tag to the DSA driver
- untag special tag on PDMA only if no non-DSA devices are in use
- disable special tag untagging on 7986 for now, since it's not working yet

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 36 +++++++++++++--------
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  3 ++
 2 files changed, 25 insertions(+), 14 deletions(-)

Comments

Vladimir Oltean Nov. 7, 2022, 11:32 p.m. UTC | #1 
On Mon, Nov 07, 2022 at 07:54:49PM +0100, Felix Fietkau wrote:
> - enable VLAN untagging for PDMA rx
> - make it possible to disable the feature via ethtool
> - pass VLAN tag to the DSA driver
> - untag special tag on PDMA only if no non-DSA devices are in use
> - disable special tag untagging on 7986 for now, since it's not working yet

What is the downside of not enabling VLAN RX offloading, is it a
performance or a functional need to have it?

> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 36 +++++++++++++--------
>  drivers/net/ethernet/mediatek/mtk_eth_soc.h |  3 ++
>  2 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index ab31dda2cd66..3b8995a483aa 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -2015,16 +2015,9 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
>  						htons(RX_DMA_VPID(trxd.rxd4)),
>  						RX_DMA_VID(trxd.rxd4));
>  			} else if (trxd.rxd2 & RX_DMA_VTAG) {
> -				__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
> +				__vlan_hwaccel_put_tag(skb, htons(RX_DMA_VPID(trxd.rxd3)),

Why make this change? The network stack doesn't like it when you feed it
non-standard VLAN protocols, as you've noticed.

>  						       RX_DMA_VID(trxd.rxd3));
>  			}
> -
> -			/* If the device is attached to a dsa switch, the special
> -			 * tag inserted in VLAN field by hw switch can * be offloaded
> -			 * by RX HW VLAN offload. Clear vlan info.

What is the format of this special tag, what does it contain? The same
thing as what mtk_tag_rcv() parses?

> -			 */
> -			if (netdev_uses_dsa(netdev))
> -				__vlan_hwaccel_clear_tag(skb);

If the DSA switch information is present in the VLAN hwaccel, and the
VLAN hwaccel is cleared, and that up until this change,
NETIF_F_HW_VLAN_CTAG_RX was not configurable, it means that every
mtk_soc_data with MTK_HW_FEATURES would be broken as a DSA master?

>  		}
>  
>  		skb_record_rx_queue(skb, 0);
> @@ -2847,15 +2840,17 @@ static netdev_features_t mtk_fix_features(struct net_device *dev,
>  
>  static int mtk_set_features(struct net_device *dev, netdev_features_t features)
>  {
> -	int err = 0;
> -
> -	if (!((dev->features ^ features) & NETIF_F_LRO))
> -		return 0;
> +	struct mtk_mac *mac = netdev_priv(dev);
> +	struct mtk_eth *eth = mac->hw;
> +	netdev_features_t diff = dev->features ^ features;
>  
> -	if (!(features & NETIF_F_LRO))
> +	if ((diff & NETIF_F_LRO) && !(features & NETIF_F_LRO))
>  		mtk_hwlro_netdev_disable(dev);
>  
> -	return err;
> +	/* Set RX VLAN offloading */
> +	mtk_w32(eth, !!(features & NETIF_F_HW_VLAN_CTAG_RX), MTK_CDMP_EG_CTRL);

Nit: do this only if (diff & NETIF_F_HW_VLAN_CTAG_RX).

> +
> +	return 0;
>  }
>  
>  /* wait for DMA to finish whatever it is doing before we start using it again */
> @@ -3176,6 +3171,15 @@ static int mtk_open(struct net_device *dev)
>  	else
>  		refcount_inc(&eth->dma_refcnt);
>  
> +	/* Hardware special tag parsing needs to be disabled if at least
> +	 * one MAC does not use DSA.
> +	 */

Don't understand why, sorry.

> +	if (!netdev_uses_dsa(dev)) {
> +		u32 val = mtk_r32(eth, MTK_CDMP_IG_CTRL);
> +		val &= ~MTK_CDMP_STAG_EN;
> +		mtk_w32(eth, val, MTK_CDMP_IG_CTRL);
> +	}
> +
>  	phylink_start(mac->phylink);
>  	netif_tx_start_all_queues(dev);
>  
> @@ -3469,6 +3473,10 @@ static int mtk_hw_init(struct mtk_eth *eth)
>  	 */
>  	val = mtk_r32(eth, MTK_CDMQ_IG_CTRL);
>  	mtk_w32(eth, val | MTK_CDMQ_STAG_EN, MTK_CDMQ_IG_CTRL);
> +	if (MTK_HAS_CAPS(eth->soc->caps, MTK_NETSYS_V2)) {
> +		val = mtk_r32(eth, MTK_CDMP_IG_CTRL);
> +		mtk_w32(eth, val | MTK_CDMP_STAG_EN, MTK_CDMP_IG_CTRL);
> +	}
>  
>  	/* Enable RX VLan Offloading */
>  	mtk_w32(eth, 1, MTK_CDMP_EG_CTRL);
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index e09b2483c70c..26b2323185ef 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -93,6 +93,9 @@
>  #define MTK_CDMQ_IG_CTRL	0x1400
>  #define MTK_CDMQ_STAG_EN	BIT(0)
>  
> +/* CDMQ Exgress Control Register */
> +#define MTK_CDMQ_EG_CTRL	0x1404
> +
>  /* CDMP Ingress Control Register */
>  #define MTK_CDMP_IG_CTRL	0x400
>  #define MTK_CDMP_STAG_EN	BIT(0)
> -- 
> 2.38.1
>
Felix Fietkau Nov. 8, 2022, 6:17 a.m. UTC | #2 
On 08.11.22 00:32, Vladimir Oltean wrote:
> On Mon, Nov 07, 2022 at 07:54:49PM +0100, Felix Fietkau wrote:
>> - enable VLAN untagging for PDMA rx
>> - make it possible to disable the feature via ethtool
>> - pass VLAN tag to the DSA driver
>> - untag special tag on PDMA only if no non-DSA devices are in use
>> - disable special tag untagging on 7986 for now, since it's not working yet
> 
> What is the downside of not enabling VLAN RX offloading, is it a
> performance or a functional need to have it?
> 
>> 
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 36 +++++++++++++--------
>>  drivers/net/ethernet/mediatek/mtk_eth_soc.h |  3 ++
>>  2 files changed, 25 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
>> index ab31dda2cd66..3b8995a483aa 100644
>> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
>> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
>> @@ -2015,16 +2015,9 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
>>  						htons(RX_DMA_VPID(trxd.rxd4)),
>>  						RX_DMA_VID(trxd.rxd4));
>>  			} else if (trxd.rxd2 & RX_DMA_VTAG) {
>> -				__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
>> +				__vlan_hwaccel_put_tag(skb, htons(RX_DMA_VPID(trxd.rxd3)),
> 
> Why make this change? The network stack doesn't like it when you feed it
> non-standard VLAN protocols, as you've noticed.
To make the hardware untag the DSA special tag, which is faster than 
doing it in the DSA tag driver.

>>  						       RX_DMA_VID(trxd.rxd3));
>>  			}
>> -
>> -			/* If the device is attached to a dsa switch, the special
>> -			 * tag inserted in VLAN field by hw switch can * be offloaded
>> -			 * by RX HW VLAN offload. Clear vlan info.
> 
> What is the format of this special tag, what does it contain? The same
> thing as what mtk_tag_rcv() parses?
Yes

>> -			 */
>> -			if (netdev_uses_dsa(netdev))
>> -				__vlan_hwaccel_clear_tag(skb);
> 
> If the DSA switch information is present in the VLAN hwaccel, and the
> VLAN hwaccel is cleared, and that up until this change,
> NETIF_F_HW_VLAN_CTAG_RX was not configurable, it means that every
> mtk_soc_data with MTK_HW_FEATURES would be broken as a DSA master?Before my change, the hardware wasn't actually untagging packets with 
the DSA special tag. Because of that, the code that I'm removing here 
was never used.

>>  		}
>>  
>>  		skb_record_rx_queue(skb, 0);
>> @@ -2847,15 +2840,17 @@ static netdev_features_t mtk_fix_features(struct net_device *dev,
>>  
>>  static int mtk_set_features(struct net_device *dev, netdev_features_t features)
>>  {
>> -	int err = 0;
>> -
>> -	if (!((dev->features ^ features) & NETIF_F_LRO))
>> -		return 0;
>> +	struct mtk_mac *mac = netdev_priv(dev);
>> +	struct mtk_eth *eth = mac->hw;
>> +	netdev_features_t diff = dev->features ^ features;
>>  
>> -	if (!(features & NETIF_F_LRO))
>> +	if ((diff & NETIF_F_LRO) && !(features & NETIF_F_LRO))
>>  		mtk_hwlro_netdev_disable(dev);
>>  
>> -	return err;
>> +	/* Set RX VLAN offloading */
>> +	mtk_w32(eth, !!(features & NETIF_F_HW_VLAN_CTAG_RX), MTK_CDMP_EG_CTRL);
> 
> Nit: do this only if (diff & NETIF_F_HW_VLAN_CTAG_RX).
Will fix that in v2.
> 
>> +
>> +	return 0;
>>  }
>>  
>>  /* wait for DMA to finish whatever it is doing before we start using it again */
>> @@ -3176,6 +3171,15 @@ static int mtk_open(struct net_device *dev)
>>  	else
>>  		refcount_inc(&eth->dma_refcnt);
>>  
>> +	/* Hardware special tag parsing needs to be disabled if at least
>> +	 * one MAC does not use DSA.
>> +	 */
> 
> Don't understand why, sorry.
The hardware has multiple ethernet MACs connected to the same DMA ring. 
The parsing flag can't be turned off per MAC and it makes the hardware 
assume that a DSA special tag is present.

>> +	if (!netdev_uses_dsa(dev)) {
>> +		u32 val = mtk_r32(eth, MTK_CDMP_IG_CTRL);
>> +		val &= ~MTK_CDMP_STAG_EN;
>> +		mtk_w32(eth, val, MTK_CDMP_IG_CTRL);
>> +	}
>> +
>>  	phylink_start(mac->phylink);
>>  	netif_tx_start_all_queues(dev);
>>  

- Felix
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index ab31dda2cd66..3b8995a483aa 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -2015,16 +2015,9 @@  static int mtk_poll_rx(struct napi_struct *napi, int budget,
 						htons(RX_DMA_VPID(trxd.rxd4)),
 						RX_DMA_VID(trxd.rxd4));
 			} else if (trxd.rxd2 & RX_DMA_VTAG) {
-				__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
+				__vlan_hwaccel_put_tag(skb, htons(RX_DMA_VPID(trxd.rxd3)),
 						       RX_DMA_VID(trxd.rxd3));
 			}
-
-			/* If the device is attached to a dsa switch, the special
-			 * tag inserted in VLAN field by hw switch can * be offloaded
-			 * by RX HW VLAN offload. Clear vlan info.
-			 */
-			if (netdev_uses_dsa(netdev))
-				__vlan_hwaccel_clear_tag(skb);
 		}
 
 		skb_record_rx_queue(skb, 0);
@@ -2847,15 +2840,17 @@  static netdev_features_t mtk_fix_features(struct net_device *dev,
 
 static int mtk_set_features(struct net_device *dev, netdev_features_t features)
 {
-	int err = 0;
-
-	if (!((dev->features ^ features) & NETIF_F_LRO))
-		return 0;
+	struct mtk_mac *mac = netdev_priv(dev);
+	struct mtk_eth *eth = mac->hw;
+	netdev_features_t diff = dev->features ^ features;
 
-	if (!(features & NETIF_F_LRO))
+	if ((diff & NETIF_F_LRO) && !(features & NETIF_F_LRO))
 		mtk_hwlro_netdev_disable(dev);
 
-	return err;
+	/* Set RX VLAN offloading */
+	mtk_w32(eth, !!(features & NETIF_F_HW_VLAN_CTAG_RX), MTK_CDMP_EG_CTRL);
+
+	return 0;
 }
 
 /* wait for DMA to finish whatever it is doing before we start using it again */
@@ -3176,6 +3171,15 @@  static int mtk_open(struct net_device *dev)
 	else
 		refcount_inc(&eth->dma_refcnt);
 
+	/* Hardware special tag parsing needs to be disabled if at least
+	 * one MAC does not use DSA.
+	 */
+	if (!netdev_uses_dsa(dev)) {
+		u32 val = mtk_r32(eth, MTK_CDMP_IG_CTRL);
+		val &= ~MTK_CDMP_STAG_EN;
+		mtk_w32(eth, val, MTK_CDMP_IG_CTRL);
+	}
+
 	phylink_start(mac->phylink);
 	netif_tx_start_all_queues(dev);
 
@@ -3469,6 +3473,10 @@  static int mtk_hw_init(struct mtk_eth *eth)
 	 */
 	val = mtk_r32(eth, MTK_CDMQ_IG_CTRL);
 	mtk_w32(eth, val | MTK_CDMQ_STAG_EN, MTK_CDMQ_IG_CTRL);
+	if (MTK_HAS_CAPS(eth->soc->caps, MTK_NETSYS_V2)) {
+		val = mtk_r32(eth, MTK_CDMP_IG_CTRL);
+		mtk_w32(eth, val | MTK_CDMP_STAG_EN, MTK_CDMP_IG_CTRL);
+	}
 
 	/* Enable RX VLan Offloading */
 	mtk_w32(eth, 1, MTK_CDMP_EG_CTRL);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index e09b2483c70c..26b2323185ef 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -93,6 +93,9 @@ 
 #define MTK_CDMQ_IG_CTRL	0x1400
 #define MTK_CDMQ_STAG_EN	BIT(0)
 
+/* CDMQ Exgress Control Register */
+#define MTK_CDMQ_EG_CTRL	0x1404
+
 /* CDMP Ingress Control Register */
 #define MTK_CDMP_IG_CTRL	0x400
 #define MTK_CDMP_STAG_EN	BIT(0)