Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0

From: Florian Fainelli <f.fainelli@gmail.com>
To: Vivien Didelot <vivien.didelot@gmail.com>,
	Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Cc: "Andrew Lunn" <andrew@lunn.ch>,
	"David S. Miller" <davem@davemloft.net>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"Marek Behún" <marek.behun@nic.cz>,
	"Russell King" <rmk+kernel@armlinux.org.uk>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Chris Healy" <cphealy@gmail.com>
Subject: Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
Date: Fri, 31 May 2019 09:36:13 -0700	[thread overview]
Message-ID: <38a5dbac-a8e7-325a-225f-b97774f7bb81@gmail.com> (raw)
In-Reply-To: <20190531110017.GB2075@t480s.localdomain>

On 5/31/19 8:00 AM, Vivien Didelot wrote:
> Hi Nikita,
> 
> On Fri, 31 May 2019 17:46:29 +0300, Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:
>>
>>
>> 31.05.2019 17:37, Andrew Lunn wrote:
>>>> I'm not sure that I like the semantic of it, because the driver can actually
>>>> support VID 0 per-se, only the kernel does not use VLAN 0. Thus I would avoid
>>>> calling the port_vlan_del() ops for VID 0, directly into the upper DSA layer.
>>>>
>>>> Florian, Andrew, wouldn't the following patch be more adequate?
>>>>
>>>>     diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>>>>     index 1e2ae9d59b88..80f228258a92 100644
>>>>     --- a/net/dsa/slave.c
>>>>     +++ b/net/dsa/slave.c
>>>>     @@ -1063,6 +1063,10 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
>>>>             struct bridge_vlan_info info;
>>>>             int ret;
>>>>      
>>>>     +       /* VID 0 has a special meaning and is never programmed in hardware */
>>>>     +       if (!vid)
>>>>     +               return 0;
>>>>     +
>>>>             /* Check for a possible bridge VLAN entry now since there is no
>>>>              * need to emulate the switchdev prepare + commit phase.
>>>>              */
>>>  
>> Kernel currently does, but it is caught in
>> mv88e6xxx_port_check_hw_vlan() and returns -ENOTSUPP from there.
> 
> But VID 0 has a special meaning for the kernel, it means the port's private
> database (when it is isolated, non-bridged), it is not meant to be programmed
> in the switch. That's why I would've put that knowledge into the DSA layer,
> which job is to translate the kernel operations to the (dumb) DSA drivers.
> 
> I hope I'm seeing things correctly here.

Your first part about the fact that it's the port private database is
true, the fact that it is not programmed into the HW actually depends on
what the switch is capable of doing. With mv88e6xxx you have per-port
VLAN filtering controls, but other switches that do not have that
capability need to program VID == 0 into the HW to continue maintaining
VLAN filtering on a non bridged port while a bridge has enslaved other
ports of the switch.

AFAICT, mv88e6xx is the only driver that attempts to catch vid == 0 and
return something to the upper layers about it.
-- 
Florian