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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.