Re: [net-next PATCH 06/10] net: dsa: realtek: rtl8365mb: add VLAN support

[Luiz Angelo Daros de Luca]

Hi Linus,

Thanks for the review, I took some time to evaluate the possibility of
reusing rtl8366-core.

> This patch ignores the existing abstractions for generic RTL8366 VLAN
> handling in rtl8366-core.c and realtek.h, which is actually compiled
> and bundled into the same object with the RTL8365MB driver.

While rtl8366-core is currently only used by RTL8366, I understand the
goal of sharing VLAN handling across these drivers. However, RTL8365MB
differs from RTL8366RB in how VLANs and PVIDs are implemented.

1) In RTL8366, VLAN4K, VLANMC and PVID are programmed together, and
the helpers in rtl8366-core assume this 1:1 relationship. In
RTL8365MB, the VLANMC is exclusively used as an indirect way to set
the port VID used in the PVID (port pvid points to vlan MC table index
and vlan MC entry has the VID number). In this patch, the VLANMC is
conditionally allocated only when a port uses it as PVID and
deallocated when no ports use it. VLAN MC membership is not checked by
the HW but I still keep it in sync when it is allocated.

As VLANMC, at least in RTL8365MB, is effectively only used for PVID,
I'll put all vlan MC methods behind public PVID methods in the
following v2. There is no need to expose them for now. For the next
RTL8367D family, VLANMC is gone and there are dedicated registers to
set the PVID VID number for each port.

2) In RTL8365MB, vlan is always enabled, controlling filtering by port
(.port_vlan_filtering), while in RTL8366, it is enabled by vlan_add.

3) The abstraction doesn't set the framefilter in RTL8366 nor does it
clear PVID status. I'm using framefilter as a way to disable PVID,
dropping untagged frames, together with setting PVID VLAN MC index to
0 and keeping an empty (vid=0) vlan config at that index.

Without clearing the PVID state in RTL8366, removing a PVID VLAN
member and re-adding it as a non-PVID member will leave it configured
as PVID.

4) I don't know how IVL would be handled in RTL8366... but that's a
topic for another patch.

> I understand that it is always easier to develop and test something
> for "just this one chip", but I predictably have to be the one to ask
> if this can be done in the shared code.

I agree there is room for sharing logic in the future, both for newer
areas (table access, VLAN, L2) and existing ones (phy_read/write), but
this likely requires rethinking the abstraction and harmonizing the
assumptions on each driver first. It is possible that the hardware
behavior between RTL8366 and RTL8365MB is more similar than what the
current drivers suggest. However, the existing rtl8366-core
abstraction does not model this in a way that can be directly reused
here. Aligning both drivers would likely require reworking that
abstraction rather than adapting the RTL8365MB implementation to fit
it.

This could be explored in the future as a separate effort as it
changes how RTL8366RB driver behave. For now, I would prefer to keep
this implementation local to RTL8365MB and then follow up with a
separate effort to reconcile both drivers and extract shared logic.
Some of that work will likely become clearer while adding RTL8367D
family support.

Regards,

Luiz