On 8/23/21 12:48 AM, Vladimir Oltean wrote:
> On Sun, Aug 22, 2021 at 09:31:42PM +0200, Alvin Šipraga wrote:
>> +static bool rtl8365mb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
>
> Maybe it would be more efficient to make smi->ops->is_vlan_valid optional?
That would work. I'll make a note to do it for v2.
>
>> +{
>> + if (vlan > RTL8365MB_VIDMAX)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> +static int rtl8365mb_enable_vlan(struct realtek_smi *smi, bool enable)
>> +{
>> + dev_dbg(smi->dev, "%s VLAN\n", enable ? "enable" : "disable");
>> + return regmap_update_bits(
>> + smi->map, RTL8365MB_VLAN_CTRL_REG, RTL8365MB_VLAN_CTRL_EN_MASK,
>> + FIELD_PREP(RTL8365MB_VLAN_CTRL_EN_MASK, enable ? 1 : 0));
>> +}
>> +
>> +static int rtl8365mb_enable_vlan4k(struct realtek_smi *smi, bool enable)
>> +{
>> + return rtl8365mb_enable_vlan(smi, enable);
>> +}
>
> I'm not going to lie, the realtek_smi_ops VLAN methods seem highly
> cryptic to me. Why do you do the same thing from .enable_vlan4k as from
> .enable_vlan? What are these supposed to do in the first place?
> Or to quote from rtl8366_vlan_add: "what's with this 4k business?"
I think realtek-smi was written with rtl8366rb.c in mind, which appears
to have different control registers for VLAN and VLAN4k modes, whatever
that's supposed to mean. Since the RTL8365MB doesn't distinguish between
the two, I just route one to the other. The approach is one of caution,
since I don't want to break the other driver (I don't have hardware to
test for regressions). Maybe Linus can chime in?
>
> Also, stupid question: what do you need the VLAN ops for if you haven't
> implemented .port_bridge_join and .port_bridge_leave? How have you
> tested them?
I have to admit that I am also in some doubt about that. To illustrate,
this is a typical configuration I have been testing:
br0
+
|
+----------+-----+-----+----------+
| | | |
(DHCP) + + + + (static IP)
wan0 brwan0 swp2 swp3 brpriv0 priv0
| + 1 P u + 1 P u + 1 P u + +
| | | | 2 | 2 P u |
| | | | | |
+-----------+ + + +-----------+
LAN PRIV
n P u
^ ^ ^
| | |
| | `--- Egress Untagged
| `----- Port VLAN ID (PVID)
`------- VLAN ID n
In this configuration, priv0 is used to communicate directly with the
PRIV device over VLAN2. PRIV can also access the wider LAN by
transmitting untagged frames. My understanding was that the VLAN
configuration is necessary for e.g. packets to be untagged properly on
swp2 egress. But are you suggesting that this is being done in software
already? I.e. we are sending untagged frames from CPU->switch without
any VLAN tag?
In case you think the VLAN ops are unnecessary given that
.port_bridge_{join,leave} aren't implemented, do you think they should
be removed in their entirety from the current patch?
