From: "Alvin Šipraga" <ALSI@bang-olufsen.dk>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: "Alvin Šipraga" <alvin@pqrs.dk>,
"Linus Walleij" <linus.walleij@linaro.org>,
"Andrew Lunn" <andrew@lunn.ch>,
"Vivien Didelot" <vivien.didelot@gmail.com>,
"Florian Fainelli" <f.fainelli@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
"Jakub Kicinski" <kuba@kernel.org>,
"Rob Herring" <robh+dt@kernel.org>,
"Heiner Kallweit" <hkallweit1@gmail.com>,
"Russell King" <linux@armlinux.org.uk>,
"Michael Rasmussen" <MIR@bang-olufsen.dk>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC
Date: Mon, 23 Aug 2021 01:22:53 +0000 [thread overview]
Message-ID: <75d2820b-9429-5145-c02d-9c5ce8ceb78f@bang-olufsen.dk> (raw)
In-Reply-To: <20210823001953.rsss4fvnvkcqtebj@skbuf>
On 8/23/21 2:19 AM, Vladimir Oltean wrote:
> 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?
I was mainly referring to the enablement of VLAN versus VLAN4k when I
said it didn't make sense. The add/del ops seemed to fit well though, so
I sought to reuse code where I could. But they make use of
.enable_{vlan,vlan4k}, hence the weird stub you see.
>
>>> 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?
Sorry, I should have clarified. They are veth interface pairs, with one
end added to the bridge, and the other end used as a standalone "VLAN
unaware" interface to perform specific functions. wan0 is used as a
typical Ethernet interface for contacting the Internet and other devices
on the LAN. priv0 is an interface used to communicate with a private
device PRIV over VLAN2.
>
> 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?
It was meant to indicate that it is getting an IP address over DHCP over
the LAN.
>
>>
>> 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.
I thought that frames forwarded by the bridge from brwan0 to swp2 would
be tagged VLAN 1, and that the switch port should untag it. Is that not
the case?
>
>> 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. 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.
>
>
> 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?
next prev parent reply other threads:[~2021-08-23 1:23 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-22 19:31 [RFC PATCH net-next 0/5] net: dsa: add support for RTL8365MB-VC Alvin Šipraga
2021-08-22 19:31 ` [RFC PATCH net-next 1/5] net: dsa: realtek-smi: fix mdio_free bug on module unload Alvin Šipraga
2021-08-22 21:40 ` Andrew Lunn
2021-08-22 22:33 ` Alvin Šipraga
2021-08-22 23:16 ` Andrew Lunn
2021-08-27 22:06 ` Linus Walleij
2021-08-28 10:50 ` Alvin Šipraga
2021-08-22 21:54 ` Vladimir Oltean
2021-08-22 22:42 ` Alvin Šipraga
2021-08-22 23:10 ` Vladimir Oltean
2021-08-22 19:31 ` [RFC PATCH net-next 2/5] dt-bindings: net: dsa: realtek-smi: document new compatible rtl8365mb Alvin Šipraga
2021-08-22 21:44 ` Andrew Lunn
2021-08-23 10:15 ` Florian Fainelli
2021-08-24 16:51 ` Rob Herring
2021-08-27 22:08 ` Linus Walleij
2021-08-22 19:31 ` [RFC PATCH net-next 3/5] net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag Alvin Šipraga
2021-08-22 22:02 ` Andrew Lunn
2021-08-22 22:50 ` Alvin Šipraga
2021-08-22 23:14 ` Andrew Lunn
2021-08-22 23:27 ` Alvin Šipraga
2021-08-22 22:13 ` Vladimir Oltean
2021-08-22 23:11 ` Alvin Šipraga
2021-08-22 23:25 ` Vladimir Oltean
2021-08-22 23:37 ` Alvin Šipraga
2021-08-22 23:45 ` Vladimir Oltean
2021-08-23 0:28 ` Alvin Šipraga
2021-08-23 0:31 ` Vladimir Oltean
2021-08-22 19:31 ` [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC Alvin Šipraga
2021-08-22 22:48 ` Vladimir Oltean
2021-08-22 23:56 ` Alvin Šipraga
2021-08-23 0:19 ` Vladimir Oltean
2021-08-23 1:22 ` Alvin Šipraga [this message]
2021-08-23 2:12 ` Vladimir Oltean
2021-08-23 10:06 ` Alvin Šipraga
2021-08-23 10:31 ` Vladimir Oltean
2021-08-23 10:54 ` Alvin Šipraga
2021-08-23 13:13 ` Andrew Lunn
2021-08-23 13:20 ` Alvin Šipraga
2021-08-27 22:24 ` Linus Walleij
2021-08-22 23:04 ` Andrew Lunn
2021-08-22 23:25 ` Alvin Šipraga
2021-08-23 1:14 ` Andrew Lunn
2021-08-23 10:08 ` Alvin Šipraga
2021-08-23 4:37 ` DENG Qingfang
2021-08-23 10:11 ` Alvin Šipraga
2021-08-22 19:31 ` [RFC PATCH net-next 5/5] net: phy: realtek: add support for RTL8365MB-VC internal PHYs Alvin Šipraga
2021-08-23 10:13 ` Florian Fainelli
2021-08-27 22:27 ` Linus Walleij
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=75d2820b-9429-5145-c02d-9c5ce8ceb78f@bang-olufsen.dk \
--to=alsi@bang-olufsen.dk \
--cc=MIR@bang-olufsen.dk \
--cc=alvin@pqrs.dk \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=f.fainelli@gmail.com \
--cc=hkallweit1@gmail.com \
--cc=kuba@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=robh+dt@kernel.org \
--cc=vivien.didelot@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).