All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
@ 2019-05-31  7:35 Nikita Yushchenko
  2019-05-31 14:31 ` Vivien Didelot
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Nikita Yushchenko @ 2019-05-31  7:35 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli
  Cc: David S. Miller, Heiner Kallweit, Marek Behún, Russell King,
	netdev, linux-kernel, Chris Healy, Nikita Yushchenko

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 <nikita.yoush@cogentembedded.com>
---
 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31  7:35 [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0 Nikita Yushchenko
@ 2019-05-31 14:31 ` Vivien Didelot
  2019-05-31 14:37   ` Andrew Lunn
  2019-05-31 16:44   ` Florian Fainelli
  2019-05-31 20:15 ` Vivien Didelot
  2019-06-02 20:54 ` David Miller
  2 siblings, 2 replies; 15+ messages in thread
From: Vivien Didelot @ 2019-05-31 14:31 UTC (permalink / raw)
  To: Nikita Yushchenko
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Heiner Kallweit,
	Marek Behún, Russell King, netdev, linux-kernel,
	Chris Healy, Nikita Yushchenko

Hi Nikita,

On Fri, 31 May 2019 10:35:14 +0300, Nikita Yushchenko <nikita.yoush@cogentembedded.com> 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 <nikita.yoush@cogentembedded.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31 14:31 ` Vivien Didelot
@ 2019-05-31 14:37   ` Andrew Lunn
  2019-05-31 14:46     ` Nikita Yushchenko
  2019-05-31 16:34     ` Florian Fainelli
  2019-05-31 16:44   ` Florian Fainelli
  1 sibling, 2 replies; 15+ messages in thread
From: Andrew Lunn @ 2019-05-31 14:37 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: Nikita Yushchenko, Florian Fainelli, David S. Miller,
	Heiner Kallweit, Marek Behún, Russell King, netdev,
	linux-kernel, Chris Healy

> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31 14:37   ` Andrew Lunn
@ 2019-05-31 14:46     ` Nikita Yushchenko
  2019-05-31 15:00       ` Vivien Didelot
  2019-05-31 16:34     ` Florian Fainelli
  1 sibling, 1 reply; 15+ messages in thread
From: Nikita Yushchenko @ 2019-05-31 14:46 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot
  Cc: Florian Fainelli, David S. Miller, Heiner Kallweit,
	Marek Behún, Russell King, netdev, linux-kernel,
	Chris Healy



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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31 14:46     ` Nikita Yushchenko
@ 2019-05-31 15:00       ` Vivien Didelot
  2019-05-31 15:02         ` Nikita Yushchenko
  2019-05-31 16:36         ` Florian Fainelli
  0 siblings, 2 replies; 15+ messages in thread
From: Vivien Didelot @ 2019-05-31 15:00 UTC (permalink / raw)
  To: Nikita Yushchenko
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Heiner Kallweit,
	Marek Behún, Russell King, netdev, linux-kernel,
	Chris Healy

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.


Thanks,
Vivien

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31 15:00       ` Vivien Didelot
@ 2019-05-31 15:02         ` Nikita Yushchenko
  2019-05-31 16:36         ` Florian Fainelli
  1 sibling, 0 replies; 15+ messages in thread
From: Nikita Yushchenko @ 2019-05-31 15:02 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Heiner Kallweit,
	Marek Behún, Russell King, netdev, linux-kernel,
	Chris Healy

>> 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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31 14:37   ` Andrew Lunn
  2019-05-31 14:46     ` Nikita Yushchenko
@ 2019-05-31 16:34     ` Florian Fainelli
  2019-05-31 20:14       ` Vivien Didelot
  1 sibling, 1 reply; 15+ messages in thread
From: Florian Fainelli @ 2019-05-31 16:34 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot
  Cc: Nikita Yushchenko, David S. Miller, Heiner Kallweit,
	Marek Behún, Russell King, netdev, linux-kernel,
	Chris Healy

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31 15:00       ` Vivien Didelot
  2019-05-31 15:02         ` Nikita Yushchenko
@ 2019-05-31 16:36         ` Florian Fainelli
  2019-05-31 18:19           ` Vivien Didelot
  1 sibling, 1 reply; 15+ messages in thread
From: Florian Fainelli @ 2019-05-31 16:36 UTC (permalink / raw)
  To: Vivien Didelot, Nikita Yushchenko
  Cc: Andrew Lunn, David S. Miller, Heiner Kallweit, Marek Behún,
	Russell King, netdev, linux-kernel, Chris Healy

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31 14:31 ` Vivien Didelot
  2019-05-31 14:37   ` Andrew Lunn
@ 2019-05-31 16:44   ` Florian Fainelli
  1 sibling, 0 replies; 15+ messages in thread
From: Florian Fainelli @ 2019-05-31 16:44 UTC (permalink / raw)
  To: Vivien Didelot, Nikita Yushchenko
  Cc: Andrew Lunn, David S. Miller, Heiner Kallweit, Marek Behún,
	Russell King, netdev, linux-kernel, Chris Healy

On 5/31/19 7:31 AM, Vivien Didelot wrote:
> Hi Nikita,
> 
> On Fri, 31 May 2019 10:35:14 +0300, Nikita Yushchenko <nikita.yoush@cogentembedded.com> 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 <nikita.yoush@cogentembedded.com>
>> ---
>>  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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31 16:36         ` Florian Fainelli
@ 2019-05-31 18:19           ` Vivien Didelot
  2019-05-31 19:33             ` Florian Fainelli
  0 siblings, 1 reply; 15+ messages in thread
From: Vivien Didelot @ 2019-05-31 18:19 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Nikita Yushchenko, Andrew Lunn, David S. Miller, Heiner Kallweit,
	Marek Behún, Russell King, netdev, linux-kernel,
	Chris Healy

Hi Florian,

On Fri, 31 May 2019 09:36:13 -0700, Florian Fainelli <f.fainelli@gmail.com> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31 18:19           ` Vivien Didelot
@ 2019-05-31 19:33             ` Florian Fainelli
  0 siblings, 0 replies; 15+ messages in thread
From: Florian Fainelli @ 2019-05-31 19:33 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: Nikita Yushchenko, Andrew Lunn, David S. Miller, Heiner Kallweit,
	Marek Behún, Russell King, netdev, linux-kernel,
	Chris Healy

On 5/31/19 11:19 AM, Vivien Didelot wrote:
> Hi Florian,
> 
> On Fri, 31 May 2019 09:36:13 -0700, Florian Fainelli <f.fainelli@gmail.com> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31 16:34     ` Florian Fainelli
@ 2019-05-31 20:14       ` Vivien Didelot
  0 siblings, 0 replies; 15+ messages in thread
From: Vivien Didelot @ 2019-05-31 20:14 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Andrew Lunn, Nikita Yushchenko, David S. Miller, Heiner Kallweit,
	Marek Behún, Russell King, netdev, linux-kernel,
	Chris Healy

Hi Florian,

On Fri, 31 May 2019 09:34:03 -0700, Florian Fainelli <f.fainelli@gmail.com> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31  7:35 [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0 Nikita Yushchenko
  2019-05-31 14:31 ` Vivien Didelot
@ 2019-05-31 20:15 ` Vivien Didelot
  2019-06-02 20:54 ` David Miller
  2 siblings, 0 replies; 15+ messages in thread
From: Vivien Didelot @ 2019-05-31 20:15 UTC (permalink / raw)
  To: Nikita Yushchenko
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, Heiner Kallweit,
	Marek Behún, Russell King, netdev, linux-kernel,
	Chris Healy, Nikita Yushchenko

On Fri, 31 May 2019 10:35:14 +0300, Nikita Yushchenko <nikita.yoush@cogentembedded.com> 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 <nikita.yoush@cogentembedded.com>

Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
  2019-05-31  7:35 [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0 Nikita Yushchenko
  2019-05-31 14:31 ` Vivien Didelot
  2019-05-31 20:15 ` Vivien Didelot
@ 2019-06-02 20:54 ` David Miller
  2 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2019-06-02 20:54 UTC (permalink / raw)
  To: nikita.yoush
  Cc: andrew, vivien.didelot, f.fainelli, hkallweit1, marek.behun,
	rmk+kernel, netdev, linux-kernel, cphealy

From: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
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 <nikita.yoush@cogentembedded.com>

Applied.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
@ 2019-05-31  7:27 Nikita Yushchenko
  0 siblings, 0 replies; 15+ messages in thread
From: Nikita Yushchenko @ 2019-05-31  7:27 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli
  Cc: David S. Miller, Heiner Kallweit, Marek Behún, Russell King,
	netdev, linux-kernel, Chris Healy, Nikita Yushchenko

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.
---
 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2019-06-02 20:54 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-31  7:35 [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0 Nikita Yushchenko
2019-05-31 14:31 ` Vivien Didelot
2019-05-31 14:37   ` Andrew Lunn
2019-05-31 14:46     ` Nikita Yushchenko
2019-05-31 15:00       ` Vivien Didelot
2019-05-31 15:02         ` Nikita Yushchenko
2019-05-31 16:36         ` Florian Fainelli
2019-05-31 18:19           ` Vivien Didelot
2019-05-31 19:33             ` Florian Fainelli
2019-05-31 16:34     ` Florian Fainelli
2019-05-31 20:14       ` Vivien Didelot
2019-05-31 16:44   ` Florian Fainelli
2019-05-31 20:15 ` Vivien Didelot
2019-06-02 20:54 ` David Miller
  -- strict thread matches above, loose matches on Subject: below --
2019-05-31  7:27 Nikita Yushchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.