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 10:54:16 +0000 [thread overview]
Message-ID: <482f70ca-21b5-ee20-88e6-7f7d75fae898@bang-olufsen.dk> (raw)
In-Reply-To: <20210823103138.iwbtrksra2f6vl4d@skbuf>
On 8/23/21 12:31 PM, Vladimir Oltean wrote:
> On Mon, Aug 23, 2021 at 10:06:39AM +0000, Alvin Šipraga wrote:
>> 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-rt6 #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
>
> This is an unintended consequence for sure. The bridge is persistent and
> finds a leak in our defense, see __vlan_vid_add:
>
> /* Try switchdev op first. In case it is not supported, fallback to
> * 8021q add.
> */
> err = br_switchdev_port_vlan_add(dev, v->vid, flags, extack);
> if (err == -EOPNOTSUPP)
> return vlan_vid_add(dev, br->vlan_proto, v->vid);
>
> We return -EOPNOTSUPP to br_switchdev_port_vlan_add, then the bridge
> tries with vlan_vid_add, which makes us think it's an 8021q upper, and
> we say "oh, yes, but sure!"
>
> Btw, the fact that DSA thinks it's an 8021q upper is also the reason why
> your VLAN gets added with different flags, see dsa_slave_vlan_rx_add_vid:
>
> /* This API only allows programming tagged, non-PVID VIDs */
>
> There is a larger problem at hand, which is that the logic behind
> dsa_slave_vlan_rx_add_vid currently adds VLANs to hardware even for many
> switch drivers that don't need that. It does not even give the switch
> driver the opportunity to distinguish between a bridge VLAN and a VLAN
> coming from a VLAN upper interface. I need to think about that too.
>
> This should work if you replace all:
>
> case SWITCHDEV_OBJ_ID_PORT_VLAN:
> if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
> return -EOPNOTSUPP;
>
> with:
>
> case SWITCHDEV_OBJ_ID_PORT_VLAN:
> if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
> return 0;
>
> but I need a bit more time to think of any drawbacks of doing that.
OK, I expected something like that. Removing the VLAN ops should allow
me to ignore this particular edge case, so I will not dwell on this. But
thanks for the explanation.
>
>> [ 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?
>
> See above for the 2 corner cases that exist. The only reason why
> dsa_slave_vlan_rx_add_vid() exists is to work around some hardware
> quirks where some switches cannot put their standalone ports in
> VLAN-unaware mode. So to accept VLAN-tagged packets, DSA needs to trap
> the vlan_vid_add() calls to perform VLAN RX filtering on these
> standalone ports. You do not need this functionality at all, but we do
> not distinguish between switches that need it and switches that don't,
> hence the issues.
Understood.
>
>>> 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.
>
> I fail to see any reason why any external factors would modify the state
> of a standalone switch port.
You have explained the requirements of a standalone switch port in clear
terms, so I can see now why my belief was misguided.
>
>>>> 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.
>
> See above.
>
>>>>> 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.
>
> Even if you find the time to work on bridge offloading, standalone ports
> should still behave like that: no learning, VLAN-unaware, no ingress
> filtering, forward only to the CPU, flood all packets. You may find that
> the switchover from one state to the other is a bit tricky, but it needs
> to be consistent.
Understood.
You've given me a lot to think about now, and I think I have got the
answers I need to follow up on the feedback. Thanks for taking the time
to explain all of this - it was incredibly helpful.
Please expect some delay before I come back with a v2 series, as I have
other priorities nagging me at work. But I will definitely follow up in
due course.
Kind regards,
Alvin
next prev parent reply other threads:[~2021-08-23 10:54 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
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 [this message]
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=482f70ca-21b5-ee20-88e6-7f7d75fae898@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).