When non-bridged, non-vlan'ed mv88e6xxx port is moving down, error
message is logged:
failed to kill vid 0081/0 for device eth_cu_1000_4
This is caused by call from __vlan_vid_del() with vin set to zero, over
call chain this results into _mv88e6xxx_port_vlan_del() called with
vid=0, and mv88e6xxx_vtu_get() called from there returns -EINVAL.
On symmetric path moving port up, call goes through
mv88e6xxx_port_vlan_prepare() that calls mv88e6xxx_port_check_hw_vlan()
that returns -EOPNOTSUPP for zero vid.
This patch changes mv88e6xxx_vtu_get() to also return -EOPNOTSUPP for
zero vid, then this error code is explicitly cleared in
dsa_slave_vlan_rx_kill_vid() and error message is no longer logged.
Signed-off-by: Nikita Yushchenko <[email protected]>
---
drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 28414db979b0..6b77fde5f0e4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1392,7 +1392,7 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
int err;
if (!vid)
- return -EINVAL;
+ return -EOPNOTSUPP;
entry->vid = vid - 1;
entry->valid = false;
--
2.11.0
Hi Nikita,
On Fri, 31 May 2019 10:35:14 +0300, Nikita Yushchenko <[email protected]> wrote:
> When non-bridged, non-vlan'ed mv88e6xxx port is moving down, error
> message is logged:
>
> failed to kill vid 0081/0 for device eth_cu_1000_4
>
> This is caused by call from __vlan_vid_del() with vin set to zero, over
> call chain this results into _mv88e6xxx_port_vlan_del() called with
> vid=0, and mv88e6xxx_vtu_get() called from there returns -EINVAL.
>
> On symmetric path moving port up, call goes through
> mv88e6xxx_port_vlan_prepare() that calls mv88e6xxx_port_check_hw_vlan()
> that returns -EOPNOTSUPP for zero vid.
>
> This patch changes mv88e6xxx_vtu_get() to also return -EOPNOTSUPP for
> zero vid, then this error code is explicitly cleared in
> dsa_slave_vlan_rx_kill_vid() and error message is no longer logged.
>
> Signed-off-by: Nikita Yushchenko <[email protected]>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 28414db979b0..6b77fde5f0e4 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1392,7 +1392,7 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
> int err;
>
> if (!vid)
> - return -EINVAL;
> + return -EOPNOTSUPP;
>
> entry->vid = vid - 1;
> entry->valid = false;
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.
*/
Thanks,
Vivien
> 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.
> */
Hi Vivien
If we put this in rx_kill_vid, we should probably have something
similar in rx_add_vid, just in case the kernel does start using VID 0.
Andrew
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.
>> */
>
> Hi Vivien
>
> If we put this in rx_kill_vid, we should probably have something
> similar in rx_add_vid, just in case the kernel does start using VID 0.
Kernel currently does, but it is caught in
mv88e6xxx_port_check_hw_vlan() and returns -ENOTSUPP from there.
Hi Nikita,
On Fri, 31 May 2019 17:46:29 +0300, Nikita Yushchenko <[email protected]> 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.
Thanks,
Vivien
>> 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.
I'm ok with either solution.
On 5/31/19 7:37 AM, 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.
>> */
>
> Hi Vivien
>
> If we put this in rx_kill_vid, we should probably have something
> similar in rx_add_vid, just in case the kernel does start using VID 0.
We use the prepare/commit model in the rx_add_vid() path so we deal with
-EOPNOTSUPP, that was caught fairly early on by Heiner when I added
programming of VLAN filtering through rx_{add,kill}_vid.
Nikita's patcha s it stands is correct and would make both
mv88e6xxx_port_check_hw_vlan() and mv88e6xxx_vtu_get() consistent.
I will respond to other comments.
--
Florian
On 5/31/19 8:00 AM, Vivien Didelot wrote:
> Hi Nikita,
>
> On Fri, 31 May 2019 17:46:29 +0300, Nikita Yushchenko <[email protected]> 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
On 5/31/19 7:31 AM, Vivien Didelot wrote:
> Hi Nikita,
>
> On Fri, 31 May 2019 10:35:14 +0300, Nikita Yushchenko <[email protected]> wrote:
>> When non-bridged, non-vlan'ed mv88e6xxx port is moving down, error
>> message is logged:
>>
>> failed to kill vid 0081/0 for device eth_cu_1000_4
>>
>> This is caused by call from __vlan_vid_del() with vin set to zero, over
>> call chain this results into _mv88e6xxx_port_vlan_del() called with
>> vid=0, and mv88e6xxx_vtu_get() called from there returns -EINVAL.
>>
>> On symmetric path moving port up, call goes through
>> mv88e6xxx_port_vlan_prepare() that calls mv88e6xxx_port_check_hw_vlan()
>> that returns -EOPNOTSUPP for zero vid.
>>
>> This patch changes mv88e6xxx_vtu_get() to also return -EOPNOTSUPP for
>> zero vid, then this error code is explicitly cleared in
>> dsa_slave_vlan_rx_kill_vid() and error message is no longer logged.
>>
>> Signed-off-by: Nikita Yushchenko <[email protected]>
>> ---
>> drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 28414db979b0..6b77fde5f0e4 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -1392,7 +1392,7 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
>> int err;
>>
>> if (!vid)
>> - return -EINVAL;
>> + return -EOPNOTSUPP;
>>
>> entry->vid = vid - 1;
>> entry->valid = false;
>
> 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?
See my comment about the usage of VLAN ID == 0 with non mv88e6xxx
switches this would break VLAN filtering/isolation for non bridged port.
>
> 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.
> */
>
>
> Thanks,
> Vivien
>
--
Florian
Hi Florian,
On Fri, 31 May 2019 09:36:13 -0700, Florian Fainelli <[email protected]> wrote:
> > 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.
Are you saying that switches without per-port VLAN filtering controls
will program VID 0, and thus put all non bridged ports into the same VLAN,
allowing them to talk to each other?
Thanks,
Vivien
On 5/31/19 11:19 AM, Vivien Didelot wrote:
> Hi Florian,
>
> On Fri, 31 May 2019 09:36:13 -0700, Florian Fainelli <[email protected]> wrote:
>>> 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.
>
> Are you saying that switches without per-port VLAN filtering controls
> will program VID 0, and thus put all non bridged ports into the same VLAN,
> allowing them to talk to each other?
Because VLAN filtering is global to the switch, non-bridged ports must
have a default VLAN programmed, otherwise any untagged frame would
result in a VID volation. That default VLAN (0 for non-bridged) cannot
be the same as the bridge's default_pvid (typically 1) otherwise other
things like multicast would break (it gets checked differently than UC
traffic).
There is an additional bitmask that controls whether ports can talk to
each other (at least with B53 switches).
--
Florian
Hi Florian,
On Fri, 31 May 2019 09:34:03 -0700, Florian Fainelli <[email protected]> wrote:
> On 5/31/19 7:37 AM, 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.
> >> */
> >
> > Hi Vivien
> >
> > If we put this in rx_kill_vid, we should probably have something
> > similar in rx_add_vid, just in case the kernel does start using VID 0.
>
> We use the prepare/commit model in the rx_add_vid() path so we deal with
> -EOPNOTSUPP, that was caught fairly early on by Heiner when I added
> programming of VLAN filtering through rx_{add,kill}_vid.
>
> Nikita's patcha s it stands is correct and would make both
> mv88e6xxx_port_check_hw_vlan() and mv88e6xxx_vtu_get() consistent.
OK, I'll double check if I can simplify the management of VID 0 in mv88e6xxx to
match what other switches do. In the meantime, Nikita's approach is consistent.
Thank you,
Vivien
On Fri, 31 May 2019 10:35:14 +0300, Nikita Yushchenko <[email protected]> wrote:
> When non-bridged, non-vlan'ed mv88e6xxx port is moving down, error
> message is logged:
>
> failed to kill vid 0081/0 for device eth_cu_1000_4
>
> This is caused by call from __vlan_vid_del() with vin set to zero, over
> call chain this results into _mv88e6xxx_port_vlan_del() called with
> vid=0, and mv88e6xxx_vtu_get() called from there returns -EINVAL.
>
> On symmetric path moving port up, call goes through
> mv88e6xxx_port_vlan_prepare() that calls mv88e6xxx_port_check_hw_vlan()
> that returns -EOPNOTSUPP for zero vid.
>
> This patch changes mv88e6xxx_vtu_get() to also return -EOPNOTSUPP for
> zero vid, then this error code is explicitly cleared in
> dsa_slave_vlan_rx_kill_vid() and error message is no longer logged.
>
> Signed-off-by: Nikita Yushchenko <[email protected]>
Reviewed-by: Vivien Didelot <[email protected]>
From: Nikita Yushchenko <[email protected]>
Date: Fri, 31 May 2019 10:35:14 +0300
> When non-bridged, non-vlan'ed mv88e6xxx port is moving down, error
> message is logged:
>
> failed to kill vid 0081/0 for device eth_cu_1000_4
>
> This is caused by call from __vlan_vid_del() with vin set to zero, over
> call chain this results into _mv88e6xxx_port_vlan_del() called with
> vid=0, and mv88e6xxx_vtu_get() called from there returns -EINVAL.
>
> On symmetric path moving port up, call goes through
> mv88e6xxx_port_vlan_prepare() that calls mv88e6xxx_port_check_hw_vlan()
> that returns -EOPNOTSUPP for zero vid.
>
> This patch changes mv88e6xxx_vtu_get() to also return -EOPNOTSUPP for
> zero vid, then this error code is explicitly cleared in
> dsa_slave_vlan_rx_kill_vid() and error message is no longer logged.
>
> Signed-off-by: Nikita Yushchenko <[email protected]>
Applied.