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

[Luiz Angelo Daros de Luca]

> Even if we end up splitting out VLAN, I think my comments on the other
> things that can be shared are still valid as well.

Agree. It is generally more effective to refactor and share code once
the target models are well-understood, rather than aiming at a moving
target.

> > 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.
>
> To give the whole picture, RTL8365MB wasn't even in the picture when
> this abstraction was created.

The logic is indeed similar in several places, but the overhead of
creating abstractions or indirect calls for relatively simple
operations might outweigh the benefits of reuse at this stage.

> The ASICs the driver was supposed to handle (and which were/is used
> by OpenWrt) were:
> - RTL8366RB
> - RTL8366S
> - RTL8367
>
> The abstraction was part of the outoftree "swconfig" driver for a long
> time before being migrated to DSA.
>
> I haven't yet wrapped my head around if the RTL8367 (no extra letters)
> is more RTL8366RB-ish or more RTL8365MB-ish... take a look at the
> old code if you can figure it out from register maps etc:
> https://github.com/openwrt/openwrt/tree/main/target/linux/generic/files/drivers/net/phy
> (maybe Gabor knows, put him on cc)

I have experimented with the RTL8367R (found in the TP-Link
TL-WR2543ND), which appears to belong to the RTL8367B family. From
what I’ve observed, it behaves more like the RTL8365MB. However, my
visibility is limited to a small subset of registers used in U-Boot. I
haven't been able to confirm if the switch supports a CPU tag or how
to enable it, as the registers used for the RTL8365MB did not produce
the same results.

> OK seems we should split up the VLAN handling if there is nothing
> to share in the abstraction here. We don't do abstraction for abstractions
> sake.
>
> I'd say make a patch moving all functions from realtek_ops
> that can't be reused into the rtl8366rb driver and make them static.

Merging rtl8366-core.c into rtl8366rb.c would likely simplify the
implementation significantly and remove unnecessary indirection.

> But probably keep the shared core for other stuff. Though I guess it
> might be empty then until we add more stuff :/

Not empty as we already have some functions from rtl83xx.c :-)

> > 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.
>
> The reason I push back a bit is that any kind of things that are
> "fix later" tend to never actually happen in my experience.
> Either we add the abstraction now or never.

I understand the concern regarding "technical debt." However, forcing
code sharing now carries the risk of regressions on legacy hardware
that is difficult to test. Keeping the implementations distinct for
now ensures stability for these older devices while we refine the
RTL8365MB support.

> For VLAN I'm pretty much convinced that there is not much to
> share. For port separation/isolation and possibly FDB I'm not as
> convinced.

It remains difficult to determine the best path for sharing port
isolation or FDB logic without a clearer picture of the future
rtl8366rb FDB code. Specifically, differences in IVL logic might make
shared abstractions more complex than they initially appear.

> Yours,
> Linus Walleij

Regards,
Luiz