Message-ID: <20210823001953.rsss4fvnvkcqtebj@skbuf>
Date:   Mon, 23 Aug 2021 03:19:53 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
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

On Sun, Aug 22, 2021 at 11:56:04PM +0000, Alvin Šipraga wrote:
> > 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?

You don't _have_ to use the rtl8366 ops for VLAN, especially if they
don't make sense, do you?

> > 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

What are priv0 and wan0? Are they local interfaces of your board, put in
loopback with switch ports? Are they external devices?

What does DHCP mean? Is there a server there, or does it mean that the
wan0 interface gets IP over DHCP? Where is the DHCP server? Why is "DHCP"
relevant?

>
> 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.

swp2 egresses packets only in VLAN 1. In your example, how would any
packet become tagged in VLAN 1? VLAN 1 is a pvid on all ports which are
members of it.

> 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);

>
> 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.


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).

Powered by blists - more mailing lists