| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <48673551-cada-4194-865f-bc04c1e19c29@lunn.ch>
Date: Tue, 28 May 2024 15:02:17 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "Ng, Boon Khai" <boon.khai.ng@...el.com>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>,
Jose Abreu <joabreu@...opsys.com>,
"David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-stm32@...md-mailman.stormreply.com" <linux-stm32@...md-mailman.stormreply.com>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Ang, Tien Sung" <tien.sung.ang@...el.com>,
"G Thomas, Rohan" <rohan.g.thomas@...el.com>,
"Looi, Hong Aun" <hong.aun.looi@...el.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Ilpo Jarvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net:
stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
> So, for this XGMAC VLAN patch, the idea of getting the VLAN id from the descriptor is the same, but
> The register bit filed of getting the VLAN packet VALID is different. Thus, it need to be implemented separately.
Please wrap your emails to around 78 characters.
It is well know this driver is a mess. I just wanted to check you are
not adding to be mess by simply cut/pasting rather than refactoring
code.
Lets look at the code. From your patch:
+static void dwxgmac2_rx_hw_vlan(struct mac_device_info *hw,
+ struct dma_desc *rx_desc, struct sk_buff *skb)
+{
+ if (hw->desc->get_rx_vlan_valid(rx_desc)) {
+ u16 vid = hw->desc->get_rx_vlan_tci(rx_desc);
+
+ __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
+ }
+}
+
and
static void dwmac4_rx_hw_vlan(struct mac_device_info *hw,
struct dma_desc *rx_desc, struct sk_buff *skb)
{
if (hw->desc->get_rx_vlan_valid(rx_desc)) {
u16 vid = hw->desc->get_rx_vlan_tci(rx_desc);
__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
}
}
Looks identical to me.
>From your patch:
static void dwxgmac2_set_hw_vlan_mode(struct mac_device_info *hw)
+{
+ void __iomem *ioaddr = hw->pcsr;
+ u32 val = readl(ioaddr + XGMAC_VLAN_TAG);
+
+ val &= ~XGMAC_VLAN_TAG_CTRL_EVLS_MASK;
+
+ if (hw->hw_vlan_en)
+ /* Always strip VLAN on Receive */
+ val |= XGMAC_VLAN_TAG_STRIP_ALL;
+ else
+ /* Do not strip VLAN on Receive */
+ val |= XGMAC_VLAN_TAG_STRIP_NONE;
+
+ /* Enable outer VLAN Tag in Rx DMA descriptro */
+ val |= XGMAC_VLAN_TAG_CTRL_EVLRXS;
+ writel(val, ioaddr + XGMAC_VLAN_TAG);
+}
static void dwmac4_set_hw_vlan_mode(struct mac_device_info *hw)
{
void __iomem *ioaddr = hw->pcsr;
u32 value = readl(ioaddr + GMAC_VLAN_TAG);
value &= ~GMAC_VLAN_TAG_CTRL_EVLS_MASK;
if (hw->hw_vlan_en)
/* Always strip VLAN on Receive */
value |= GMAC_VLAN_TAG_STRIP_ALL;
else
/* Do not strip VLAN on Receive */
value |= GMAC_VLAN_TAG_STRIP_NONE;
/* Enable outer VLAN Tag in Rx DMA descriptor */
value |= GMAC_VLAN_TAG_CTRL_EVLRXS;
writel(value, ioaddr + GMAC_VLAN_TAG);
}
The basic flow is the same. Lets look at the #defines:
#define XGMAC_VLAN_TAG 0x00000050
#define GMAC_VLAN_TAG 0x00000050
#define GMAC_VLAN_TAG_CTRL_EVLS_MASK GENMASK(22, 21)
#define GMAC_VLAN_TAG_CTRL_EVLS_SHIFT 21
+#define XGMAC_VLAN_TAG_CTRL_EVLS_MASK GENMASK(22, 21)
+#define XGMAC_VLAN_TAG_CTRL_EVLS_SHIFT 21
+#define XGMAC_VLAN_TAG_STRIP_NONE FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x0)
+#define XGMAC_VLAN_TAG_STRIP_PASS FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x1)
+#define XGMAC_VLAN_TAG_STRIP_FAIL FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x2)
+#define XGMAC_VLAN_TAG_STRIP_ALL FIELD_PREP(XGMAC_VLAN_TAG_CTRL_EVLS_MASK, 0x3)
#define GMAC_VLAN_TAG_STRIP_NONE (0x0 << GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
#define GMAC_VLAN_TAG_STRIP_PASS (0x1 << GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
#define GMAC_VLAN_TAG_STRIP_FAIL (0x2 << GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
#define GMAC_VLAN_TAG_STRIP_ALL (0x3 << GMAC_VLAN_TAG_CTRL_EVLS_SHIFT)
This is less obvious a straight cut/paste, but they are in fact
identical.
#define GMAC_VLAN_TAG_CTRL_EVLRXS BIT(24)
#define XGMAC_VLAN_TAG_CTRL_EVLRXS BIT(24)
So this also looks identical to me, but maybe i'm missing something
subtle.
+static inline u16 dwxgmac2_wrback_get_rx_vlan_tci(struct dma_desc *p)
+{
+ return le32_to_cpu(p->des0) & XGMAC_RDES0_VLAN_TAG_MASK;
+}
+
static u16 dwmac4_wrback_get_rx_vlan_tci(struct dma_desc *p)
{
return (le32_to_cpu(p->des0) & RDES0_VLAN_TAG_MASK);
}
#define RDES0_VLAN_TAG_MASK GENMASK(15, 0)
#define XGMAC_RDES0_VLAN_TAG_MASK GENMASK(15, 0)
More identical code.
+static inline bool dwxgmac2_wrback_get_rx_vlan_valid(struct dma_desc *p)
+{
+ u32 et_lt;
+
+ et_lt = FIELD_GET(XGMAC_RDES3_ET_LT, le32_to_cpu(p->des3));
+
+ return et_lt >= XGMAC_ET_LT_VLAN_STAG &&
+ et_lt <= XGMAC_ET_LT_DVLAN_STAG_CTAG;
+}
static bool dwmac4_wrback_get_rx_vlan_valid(struct dma_desc *p)
{
return ((le32_to_cpu(p->des3) & RDES3_LAST_DESCRIPTOR) &&
(le32_to_cpu(p->des3) & RDES3_RDES0_VALID));
}
#define RDES3_RDES0_VALID BIT(25)
#define RDES3_LAST_DESCRIPTOR BIT(28)
#define XGMAC_RDES3_ET_LT GENMASK(19, 16)
+#define XGMAC_ET_LT_VLAN_STAG 8
+#define XGMAC_ET_LT_VLAN_CTAG 9
+#define XGMAC_ET_LT_DVLAN_CTAG_CTAG 10
This does actually look different.
Please take a step back and see if you can help clean up some of the
mess in this driver by refactoring bits of identical code, rather than
copy/pasting it.
Andrew
Powered by blists - more mailing lists