Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Alvin Šipraga]

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?