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

Vladimir Oltean
2022-11-07T23:32:59Z

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
>