Received-SPF: pass (google.com: domain of linux-kernel+bounces-195058-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Thu, 30 May 2024 11:25:16 +0300 (EEST)
To: "Ng, Boon Khai" <boon.khai.ng@intel.com>
cc: Andrew Lunn <andrew@lunn.ch>, 
    Alexandre Torgue <alexandre.torgue@foss.st.com>, 
    Jose Abreu <joabreu@synopsys.com>, 
    "David S . Miller" <davem@davemloft.net>, 
    Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, 
    Paolo Abeni <pabeni@redhat.com>, 
    Maxime Coquelin <mcoquelin.stm32@gmail.com>, 
    "netdev@vger.kernel.org" <netdev@vger.kernel.org>, 
    "linux-stm32@st-md-mailman.stormreply.com" <linux-stm32@st-md-mailman.stormreply.com>, 
    "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, 
    "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
    "Ang, Tien Sung" <tien.sung.ang@intel.com>, 
    "G Thomas, Rohan" <rohan.g.thomas@intel.com>, 
    "Looi, Hong Aun" <hong.aun.looi@intel.com>, 
    Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: RE: [Enable Designware XGMAC VLAN Stripping Feature v2 1/1] net:
 stmmac: dwxgmac2: Add support for HW-accelerated VLAN Stripping
In-Reply-To: <DM8PR11MB5751194374C75EC5D5889D6AC1F32@DM8PR11MB5751.namprd11.prod.outlook.com>
Message-ID: <322d8745-7eae-4a68-4606-d9fdb19b4662@linux.intel.com>
References: <20240527093339.30883-1-boon.khai.ng@intel.com> <20240527093339.30883-2-boon.khai.ng@intel.com> <48176576-e1d2-4c45-967a-91cabb982a21@lunn.ch> <DM8PR11MB5751469FAA2B01EB6CEB7B50C1F12@DM8PR11MB5751.namprd11.prod.outlook.com>
 <48673551-cada-4194-865f-bc04c1e19c29@lunn.ch> <DM8PR11MB5751194374C75EC5D5889D6AC1F32@DM8PR11MB5751.namprd11.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

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.