linux-kernel - 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: <322d8745-7eae-4a68-4606-d9fdb19b4662@linux.intel.com>
Date: Thu, 30 May 2024 11:25:16 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: "Ng, Boon Khai" <boon.khai.ng@...el.com>
cc: Andrew Lunn <andrew@...n.ch>, 
    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>
Subject: RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net:
 stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping

On Thu, 30 May 2024, Ng, Boon Khai wrote:

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

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

You should generalize the existing functions into some other file within 
stmmac/ folder and call those functions from both dwmac4_core and dwxgmac2_core.
Do the rework of existing function & callers first and add the new bits 
in another patch in the patch series.

Unfortunately, it's hard to catch copy-paste like this from other files 
when not very familiar with the driver.


-- 
 i.