Date:   Mon, 23 Aug 2021 10:06:39 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Vladimir Oltean <olteanv@...il.com>
CC:     Alvin Šipraga <alvin@...s.dk>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Michael Rasmussen <MIR@...g-olufsen.dk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb
 subdriver for RTL8365MB-VC

Hi Vladimir,

On 8/23/21 4:12 AM, Vladimir Oltean wrote:
> Your hardware switch does not know about the existence of brwan0. It
> just sees what packets the tagger is sending to it. And in your example,
> no one will be sending packets to the switch that the switch must untag.
> Packets in VLAN 1 are sent as untagged by the bridge, as a tcpdump on
> swp2 will show.

Understood.

> 
>>>> 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?
>>>
>>> With the exception of ports with the TX_FWD_OFFLOAD feature where the
>>> VLAN is always left in the packet, the bridge will pop the VLAN ID on
>>> transmission if that VLAN is configured as egress-untagged in the
>>> software VLAN database corresponding to the destination bridge port.
>>> See br_handle_vlan:
>>>
>>> 	/* If the skb will be sent using forwarding offload, the assumption is
>>> 	 * that the switchdev will inject the packet into hardware together
>>> 	 * with the bridge VLAN, so that it can be forwarded according to that
>>> 	 * VLAN. The switchdev should deal with popping the VLAN header in
>>> 	 * hardware on each egress port as appropriate. So only strip the VLAN
>>> 	 * header if forwarding offload is not being used.
>>> 	 */
>>> 	if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED &&
>>> 	    !br_switchdev_frame_uses_tx_fwd_offload(skb))
>>> 		__vlan_hwaccel_clear_tag(skb);
>>
>> Right, so that answers my question immediately above: of course it's not
>> the case - the bridge will pop the tag before sending it to swp2.
>>
>>>
>>>>
>>>> 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?
>>>
>>> I don't think it's a matter of whether _I_ think that they are
>>> unnecessary. Are they necessary? Are these code paths really exercised?
>>> What happens if you delete them? These are unanswered questions.
>>
>> The code paths are exercised, insofar as they are called when I
>> configure my bridge.
> 
> See? That's exactly where the problem is: "they are called". Let me
> explain why they shouldn't.
> 
> When a port joins a bridge, dsa_slave_changeupper() will call
> dsa_port_bridge_join(). This will dive a bit into DSA internals but will
> finally return -EOPNOTSUPP because ds->ops->port_bridge_join is NULL.
> This triggers the error path of dsa_broadcast(DSA_NOTIFIER_BRIDGE_JOIN)
> which sets back dp->bridge_dev to NULL. The port should behave as
> standalone.
> 
> Now I just went through the whole code path and this does not happen for
> plain bridging: in lack of a ds->ops->port_bridge_join method, DSA is
> still happy to return zero, although I don't understand why - I recall
> writing a patch specifically for that. Anyway. I just rewrote it and
> posted it to the list.

I tested your patch with some small modifications to make it apply (I'm 
running 5.14-rc5 right now and it's not so trivial to bump right now - 
let me know if you think it's important).

However I still observe the VLAN ops of my driver getting called (now 
with "tagged, no PVID", which is not what I thought was intended - 
previously it was "untagged, PVID"):

[   45.727777] realtek-smi ethernet-switch swp2: configuring for phy/ 
link mode
[   45.730173] realtek-smi ethernet-switch: add VLAN 1 on port 2, 
tagged, no PVID
[   45.733457] CPU: 1 PID: 595 Comm: systemd-network Tainted: G 
   O      5.14.0-rc5-20210811-1-r
t6 #1
[   45.733477] Hardware name: B&O (DT)
[   45.733481] Call trace:
[   45.733482]  dump_backtrace+0x0/0x1f8
[   45.733500]  show_stack+0x1c/0x28
[   45.733508]  dump_stack_lvl+0x64/0x7c
[   45.733516]  dump_stack+0x14/0x2c
[   45.733524]  rtl8365mb_set_vlan_4k+0x3c/0xa6c [realtek_smi]
[   45.733547]  rtl8366_set_vlan+0xb8/0x1f8 [realtek_smi]
[   45.733564]  rtl8366_vlan_add+0x174/0x228 [realtek_smi]
[   45.733582]  dsa_switch_event+0x2c4/0xde8
[   45.733591]  notifier_call_chain+0x80/0xd8
[   45.733598]  raw_notifier_call_chain+0x1c/0x28
[   45.733603]  dsa_tree_notify+0x18/0x38
[   45.733612]  dsa_port_vlan_add+0x54/0x78
[   45.733620]  dsa_slave_vlan_rx_add_vid+0x80/0x130
[   45.733627]  vlan_add_rx_filter_info+0x5c/0x80
[   45.733636]  vlan_vid_add+0xec/0x1c8
[   45.733643]  __vlan_add+0x748/0x8c8
[   45.733650]  nbp_vlan_add+0xf4/0x170
[   45.733656]  br_vlan_info.isra.0+0x6c/0x120
[   45.733662]  br_process_vlan_info+0x244/0x368
[   45.733669]  br_afspec+0x170/0x190
[   45.733674]  br_setlink+0x174/0x218
[   45.733679]  rtnl_bridge_setlink+0xbc/0x258
[   45.733688]  rtnetlink_rcv_msg+0x11c/0x338
...

I hope it's clear that even with software bridging, I still want to use 
VLAN to achieve the network topology I described in one of my previous 
replies. I think we are in agreement now that this should be handled 
entirely in software, with the switch being completely VLAN-unaware and 
not touching the VLAN tags. To that end I think I will strip all the 
VLAN ops from the v2 series to make this unambiguous. But regardless of 
that, shouldn't your patch ensure that no VLAN operations are offloaded 
to the switch hardware if .port_bridge_{join,leave} are not implemented?

> 
> I can understand why a lot of things didn't make sense for you. I thought
> we were on the same page about what is happening, but we weren't.

Yeah, the fact that my VLAN ops were still getting called led me to 
believe that there was still utility in keeping them there. I was not 
aware of the details of the implementation, but your explanation is 
making things a lot clearer to me. I hope you can answer the above 
question which I think will clear up any other misunderstandings I might 
have here.

> 
>> Perhaps I could rephrase my question as follows: If
>> the switch driver behaves properly (i.e. does not strip or tag frames)
>> despite the switch being VLAN-aware, is it a problem?
>>
>> (We can of course argue whether the switch is behaving correctly with my
>> driver, but the question assumes that it is.)
>>
>> The VLAN code will be of use when implementing bridge offload, so I'm
>> seeking some advice from you with regards to the process. I can remove
>> all the VLAN stuff and resubmit the driver such that the switch behaves
>> in a completely VLAN-unaware fashion, but that will require some
>> backtracking and the work will have to be done again if any offloading
>> is to be implemented. So if we can agree that it doesn't cause any harm,
>> I would think that it's OK to keep it in.
> 
> With DSA now doing the right thing with the patch I just sent, I hope it is
> now clearer why having VLAN ops does not make sense if you don't offload
> the bridge. They were not supposed to be called.

Per the above, your explanation makes sense, except that my VLAN ops are 
still getting called. If I can understand why that's (not) supposed to 
happen, I think we'll be on the same page.

> 
>>> My best guess is: you have a problem with transmitting VLAN-tagged
>>> packets on a port, even if that port doesn't offload the bridge
>>> forwarding process. You keep transmitting the packet to the switch as
>>> VLAN-tagged and the switch keeps stripping the tag. You need the VLAN
>>> ops to configure the VLAN 2 as egress-tagged on the port, so the switch
>>> will leave it alone.
>>> It all has to do with the KEEP bit from the xmit DSA header. The switch
>>> has VLAN ingress filtering disabled but is not VLAN-unaware. A standalone
>>> port (one which does not offload a Linux bridge) is expected to be
>>> completely VLAN-unaware and not inject or strip any VLAN header from any
>>> packet, at least not in any user-visible manner. It should behave just
>>> like any other network interface. Packet in, packet out, and the skb
>>> that the network stack sees, after stripping the DSA tag, should look
>>> like the packet that was on the wire (and similarly in the reverse direction).
>>>
>>
>> I am actually enabling VLAN ingress filtering. And I don't have a
>> problem transmitting VLAN 2-tagged packets on swp3 in my example.
>> Whether or not the driver is following the best practices - I'm not
>> sure. Following on from above: is the best practice to make the switch
>> completely VLAN-unaware if I am submitting a driver which does not
>> support any bridge offloading?
> 
> VLAN unaware, no ingress filtering, no address learning, all ports
> forward to the CPU port and only to the CPU port.

Got it. I'll make sure this is the case in v2 unless I find the time to 
work on the offloading functionality in the interim. Thanks again.

Powered by blists - more mailing lists