netdev - RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net: stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM8PR11MB5751194374C75EC5D5889D6AC1F32@DM8PR11MB5751.namprd11.prod.outlook.com>
Date: Thu, 30 May 2024 05:48:30 +0000
From: "Ng, Boon Khai" <boon.khai.ng@...el.com>
To: Andrew Lunn <andrew@...n.ch>
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

> 
> 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.
>

Okay sure Andrew, will take note on this.

 
> 
> 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.

Yes, some of the functions are identical, the driver has been divided 
into dwmac4_core.c and dwxgmac2_core.c initially, so to enable the
rx_hw_vlan, it is porting from dwmac4_core to dwxgmac2_core.
  
> The basic flow is the same. Lets look at the #defines:
> 
Right, the basic flow is direct copy and paste, and only the function
name is updated from dwmac4 to dwxgmac2.

> +#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.
> 

This was Ilpo suggestion to use Field prep and Field get instead.

> #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.
> 

For VLAN register mapping, they don't have much different between
The dwmac4 and dwxgmac2, but the descriptor of getting the
VLAN id and VLAN packet is valid is a little bit different.

> +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.
> 

Yes, this is the part in the descriptor where dwxgmac2 get the vlan  Valid. 
it is described in Designware cores xgmac 10G Ethernet MAC version 3.10a
page 1573.

> 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.
> 

Appreciate if you could help to point out which part that I have to cleanup?
Do you mean I should combine the identical part between dwmac4_core.c
and dwxgmac2_core.c? or I should update the part that looks different and
make them the same?

Regards,
Boon Khai