Message-ID: <2e4255a3-08ef-44f9-a57f-6a681171c8e6@foss.st.com>
Date: Mon, 19 Feb 2024 11:29:45 +0100
From: Christophe ROULLIER <christophe.roullier@...s.st.com>
To: Florian Fainelli <f.fainelli@...il.com>, Jakub Kicinski <kuba@...nel.org>,
        <kim.tatt.chuah@...el.com>,
        "Ong, Boon Leong" <boon.leong.ong@...el.com>,
        <vee.khee.wong@...el.com>
CC: <davem@...emloft.net>, Alexandre Torgue <alexandre.torgue@...s.st.com>,
        <netdev@...r.kernel.org>
Subject: Re: VLAN "issue" on STMMAC


On 2/16/24 18:23, Florian Fainelli wrote:
> Adding original authors,
Good, you are right
>
> On 2/16/24 06:35, Christophe ROULLIER wrote:
>> Hello,
>>
>> I've a question concerning following commit:
>>
>> ed64639bc1e0899d89120b82af52e74fcbeebf6a :
>>
>> net: stmmac: Add support for VLAN Rx filtering
>>
>> Add support for VLAN ID-based filtering by the MAC controller for MAC
>>
>> drivers that support it. Only the 12-bit VID field is used.
>>
>> Signed-off-by: Chuah Kim Tatt kim.tatt.chuah@...el.com
>> Signed-off-by: Ong Boon Leong boon.leong.ong@...el.com
>> Signed-off-by: Wong Vee Khee vee.khee.wong@...el.com
>> Signed-off-by: David S. Miller davem@...emloft.net
>>
>> So now with this commit is no more possible to create some VLAN than 
>> previously (it depends of number of HW Tx queue) (one VLAN max)
>>
>> root@...32mp1:~# ip link add link end0 name end0.200 type vlan id 200
>> [   61.207767] 8021q: 802.1Q VLAN Support v1.8
>> [   61.210629] 8021q: adding VLAN 0 to HW filter on device end0
>> [   61.230515] stm32-dwmac 5800a000.ethernet end0: Adding VLAN ID 0 
>> is not supported
>
> OK, so here the VLAN module was not yet loaded, so as part of the 
> operation, we get the module, load it, which triggers the 802.1q 
> driver to install a VLAN ID filter for VLAN ID #0, that fails because 
> there is a single VLAN supported, and this apparently means VLAN 
> promiscuous... not sure why that is an error, if this means accepting 
> all VLANs, then this means we need to filter in software, so it really 
> should not be an error IMHO.

Module VLAN (Config VLAN_8021Q) is well loaded

root@...32mp1:~# lsmod
Module                  Size  Used by
8021q                  28672  0
garp                   16384  1 8021q
mrp                    20480  1 8021q
...

>
>> root@...32mp1:~# ip link add link end0 name end0.300 type vlan id 300
>> [   71.403195] stm32-dwmac 5800a000.ethernet end0: Only single VLAN 
>> ID supported
>> RTNETLINK answers: Operation not permitted
>> root@...32mp1:~#
>>
>> I've tried to deactivate VLAN filtering with ethtool, but not 
>> possible ("fixed" value)
>>
>> root@...32mp1:~# ethtool -k end0 | grep -i vlan
>> rx-vlan-offload: on [fixed]
>> tx-vlan-offload: on [fixed]
>> rx-vlan-filter: on [fixed]
>> vlan-challenged: off [fixed]
>> tx-vlan-stag-hw-insert: on [fixed]
>> rx-vlan-stag-hw-parse: on [fixed]
>> rx-vlan-stag-filter: on [fixed]
>> root@...32mp1:~#
>> root@...32mp1:~# ethtool -K end0 rxvlan off
>> Actual changes:
>> rx-vlan-hw-parse: on [requested off]
>> Could not change any device features
>>
>> Do you know if there are possibility to force creation of VLAN ID 
>> (may be in full SW ?) and keep the rest of Ethernet Frame processing 
>> to GMAC HW.
>
> It is not clear to me how -EPERM ended up being chosen as a return 
> code here rather than -EOPNOTSUPP which would be allow for the upper 
> layers to decide how to play through, not that it would matter much here,
> because we sort of expect the operation not to fail.
>
> Can you confirm that dwmac4_get_num_vlan() does return 1 single VLAN 
> filter? Also, given the comment about single VLAN and VID #0 meaning 
> VLAN promiscuous does this work:
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c 
> b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> index 6b6d0de09619..e2134a5e6d7e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> @@ -492,10 +492,8 @@ static int dwmac4_add_hw_vlan_rx_fltr(struct 
> net_device *dev,
>         /* Single Rx VLAN Filter */
>         if (hw->num_vlan == 1) {
>                 /* For single VLAN filter, VID 0 means VLAN 
> promiscuous */
> -               if (vid == 0) {
> -                       netdev_warn(dev, "Adding VLAN ID 0 is not 
> supported\n");
> -                       return -EPERM;
> -               }
> +               if (vid == 0)
> +                       return 0;
>
>                 if (hw->vlan_filter[0] & GMAC_VLAN_TAG_VID) {
>                         netdev_err(dev, "Only single VLAN ID 
> supported\n");
>
I confirm that dwmac4_get_num_vlan() return 1.

I've tested your patch in dwmac4_add_hw_vlan_rx_fltr, for sure no more 
warning but same issue :-(

root@...32mp1:~# ip link add link end0 name end0.200 type vlan id 200
[   73.536876] 8021q: 802.1Q VLAN Support v1.8
[   73.539853] 8021q: adding VLAN 0 to HW filter on device end0

root@...32mp1:~# ip link add link end0 name end0.300 type vlan id 300
[  121.560317] stm32-dwmac 5800a000.ethernet end0: Only single VLAN ID 
supported
RTNETLINK answers: Operation not permitted

root@...32mp1:~# ethtool -k end0 | grep -i vlan
rx-vlan-offload: on [fixed]
tx-vlan-offload: on [fixed]
rx-vlan-filter: on [fixed]
vlan-challenged: off [fixed]
tx-vlan-stag-hw-insert: on [fixed]
rx-vlan-stag-hw-parse: on [fixed]
rx-vlan-stag-filter: on [fixed]

I've also try to remove second "return -EPERM;"

+               if (vid == 0)
+                       return 0;

          if (hw->vlan_filter[0] & GMAC_VLAN_TAG_VID) {
              netdev_err(dev, "Only single VLAN ID supported\n");
-            return -EPERM;
+//            return -EPERM;
          }

And now it ok to create some VLANs, but it is clearly not clean.

Is it possible/consistent to have a DT property to bypass HW VLAN 
capabilies (hw->num_vlan and hw->vlan_filter ..) and manage VLAN in 
software ?

Thanks for your feedback

Powered by blists - more mailing lists