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

From: Ilpo Järvinen
Date: Thu May 30 2024 - 04:25:50 EST


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.