Re: [PATCH net-next 0/9] net: dsa: define port types

Re: [PATCH net-next 0/9] net: dsa: define port types

[Egil Hjelmeland]

 > The DSA code currently has 3 bitmaps in the dsa_switch structure:
 > cpu_port_mask, dsa_port_mask and enabled_port_mask.


Hi Vivien

First I must apologize to everybody for not replying in-thread. Problem
is that I was not subscribed to netdev. But now I am, so I promise it
will not happen again :-)

So to the point. I think DSA need to keep track of multicast
memberships. As it is now, dsa_switch_mdb_add() include
the CPU/DSA port(s) in the multicast. But  multicast is never
removed from the CPU/DSA port(s).

One option could be that DSA remember mulitcast memberships as bitmaps
of ports. Then it could be handy to have the CPU and DSA ports as
bitmaps too.

It might not fall out that way in the end. But anyway, my suggestion is:
Do patch 1 - 6, which isolates the drivers from the internal structure
of dsa. But hold on to 7 - 9 until there is a plan for better multicast
handling in DSA.

BR
Egil

Re: [PATCH net-next 0/9] net: dsa: define port types

[Vivien Didelot]

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

>  > The DSA code currently has 3 bitmaps in the dsa_switch structure:
>  > cpu_port_mask, dsa_port_mask and enabled_port_mask.
>
>
> First I must apologize to everybody for not replying in-thread. Problem
> is that I was not subscribed to netdev. But now I am, so I promise it
> will not happen again :-)
>
> So to the point. I think DSA need to keep track of multicast
> memberships. As it is now, dsa_switch_mdb_add() include
> the CPU/DSA port(s) in the multicast. But  multicast is never
> removed from the CPU/DSA port(s).

We don't have support for this yet, but we do need to be able to remove
multicast from CPU port, if your linux processing is not interested
about a given slave multicast traffic.

> One option could be that DSA remember mulitcast memberships as bitmaps
> of ports. Then it could be handy to have the CPU and DSA ports as
> bitmaps too.
>
> It might not fall out that way in the end. But anyway, my suggestion is:
> Do patch 1 - 6, which isolates the drivers from the internal structure
> of dsa. But hold on to 7 - 9 until there is a plan for better multicast
> handling in DSA.

No. There is no need to have such bitmaps in dsa_switch. The port type
is a static information which belongs in dsa_port as an enum. For
multicast we'll see what is more handy, but caching can make things more
complex. Until then if we do need bitmaps, we'll provide helpers such as
dsa_user_ports. In a next iteration we'll provide better helpers taking
an unsigned long * as parameter so that we use the bitmap API and
finally get rid of the fixed length data type.


Thanks,

        Vivien

Re: [PATCH net-next 0/9] net: dsa: define port types

[Andrew Lunn]

On Fri, Oct 27, 2017 at 02:56:51PM +0200, Egil Hjelmeland wrote:
> > The DSA code currently has 3 bitmaps in the dsa_switch structure:
> > cpu_port_mask, dsa_port_mask and enabled_port_mask.
> 
> 
> Hi Vivien
> 
> First I must apologize to everybody for not replying in-thread. Problem
> is that I was not subscribed to netdev. But now I am, so I promise it
> will not happen again :-)
> 
> So to the point. I think DSA need to keep track of multicast
> memberships. As it is now, dsa_switch_mdb_add() include
> the CPU/DSA port(s) in the multicast. But  multicast is never
> removed from the CPU/DSA port(s).

Hi Egil

You should take a look at my patches from a few weeks back. I hope to
make another version next week. These deal with multicast on the CPU
port, or better said, the host wanting to receive a multicast group.

      Andrew

Re: [PATCH net-next 0/9] net: dsa: define port types

[Egil Hjelmeland]

Den 27. okt. 2017 21:37, skrev Andrew Lunn:
> On Fri, Oct 27, 2017 at 02:56:51PM +0200, Egil Hjelmeland wrote:
>>> The DSA code currently has 3 bitmaps in the dsa_switch structure:
>>> cpu_port_mask, dsa_port_mask and enabled_port_mask.
>>
>>
>> Hi Vivien
>>
>> First I must apologize to everybody for not replying in-thread. Problem
>> is that I was not subscribed to netdev. But now I am, so I promise it
>> will not happen again :-)
>>
>> So to the point. I think DSA need to keep track of multicast
>> memberships. As it is now, dsa_switch_mdb_add() include
>> the CPU/DSA port(s) in the multicast. But  multicast is never
>> removed from the CPU/DSA port(s).
> 
> Hi Egil
> 
> You should take a look at my patches from a few weeks back. I hope to
> make another version next week. These deal with multicast on the CPU
> port, or better said, the host wanting to receive a multicast group.
> 
>        Andrew
> 

Hi Andrew

I suppose you refer to https://www.spinics.net/lists/netdev/msg453848.html.
That will be great to have for my employer.

Regarding the offload_fwd_mark discussion: I did some experiments
yesterday afternoon on the lan9303 driver.

- Driver add the STP address to the ALR(fdb) pointing to CPU port
- In tag_lan9303.c rcv() set offload_fwd_mark if destination is
   not STP address.

In addition; I found out that my fresh lan9303_xmit_use_arl() is wrong:
Multicast and broadcasts must not be sent via the ALR.

  - use ALR on transmit if bridged and destination is unicast.

Then it looks as if the system is doing the right thing, both for
incomming and outgoing packets. Including STP BPDUs both when
STP is enabled and not.

I will have to test more though when back at office next week.

Egil

[PATCH net-next 0/9] net: dsa: define port types

[Vivien Didelot]

The DSA code currently has 3 bitmaps in the dsa_switch structure:
cpu_port_mask, dsa_port_mask and enabled_port_mask.

They are used to store the type of each switch port. This dates back
from when DSA didn't have a dsa_port structure to hold port-specific
data.

The dsa_switch structure is mainly used to communicate with DSA drivers
and must not contain such static data parsed from DTS or pdata, which
belongs the DSA core structures, such as dsa_switch_tree and dsa_port.

Also the enabled_port_mask is misleading, often misinterpreted as the
complement of disabled ports (thus including DSA and CPU ports), while
in fact it only masks the user ports.

A port can be of 3 types when it is not unused: "cpu" (interfacing with
a master device), "dsa" (interconnecting with another "dsa" port from
another switch chip), or "user" (user-facing port.)

This patchset first fixes the usage of DSA port type helpers, then
defines the DSA_PORT_TYPE_UNUSED, DSA_PORT_TYPE_CPU, DSA_PORT_TYPE_DSA,
and DSA_PORT_TYPE_USER port types, and finally removes the misleading
port bitmaps.

Vivien Didelot (9):
  net: dsa: add dsa_is_unused_port helper
  net: dsa: mv88e6xxx: skip unused ports
  net: dsa: fix dsa_is_normal_port helper
  net: dsa: rename dsa_is_normal_port helper
  net: dsa: use dsa_is_user_port everywhere
  net: dsa: introduce dsa_user_ports helper
  net: dsa: define port types
  net: dsa: use new port type in helpers
  net: dsa: remove port masks

 drivers/net/dsa/b53/b53_common.c |  2 +-
 drivers/net/dsa/bcm_sf2.c        |  9 ++++-----
 drivers/net/dsa/bcm_sf2_cfp.c    |  2 +-
 drivers/net/dsa/mt7530.c         |  6 +++---
 drivers/net/dsa/mv88e6060.c      |  5 ++---
 drivers/net/dsa/mv88e6xxx/chip.c |  5 ++++-
 drivers/net/dsa/qca8k.c          |  7 +++----
 include/net/dsa.h                | 39 ++++++++++++++++++++++++++++++---------
 net/dsa/dsa.c                    |  2 +-
 net/dsa/dsa2.c                   | 16 ++++------------
 net/dsa/legacy.c                 | 15 +++++++++------
 11 files changed, 62 insertions(+), 46 deletions(-)

-- 
2.14.3

Re: [PATCH net-next 0/9] net: dsa: define port types

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Thu, 26 Oct 2017 11:22:50 -0400

> The DSA code currently has 3 bitmaps in the dsa_switch structure:
> cpu_port_mask, dsa_port_mask and enabled_port_mask.
> 
> They are used to store the type of each switch port. This dates back
> from when DSA didn't have a dsa_port structure to hold port-specific
> data.
> 
> The dsa_switch structure is mainly used to communicate with DSA drivers
> and must not contain such static data parsed from DTS or pdata, which
> belongs the DSA core structures, such as dsa_switch_tree and dsa_port.
> 
> Also the enabled_port_mask is misleading, often misinterpreted as the
> complement of disabled ports (thus including DSA and CPU ports), while
> in fact it only masks the user ports.
> 
> A port can be of 3 types when it is not unused: "cpu" (interfacing with
> a master device), "dsa" (interconnecting with another "dsa" port from
> another switch chip), or "user" (user-facing port.)
> 
> This patchset first fixes the usage of DSA port type helpers, then
> defines the DSA_PORT_TYPE_UNUSED, DSA_PORT_TYPE_CPU, DSA_PORT_TYPE_DSA,
> and DSA_PORT_TYPE_USER port types, and finally removes the misleading
> port bitmaps.

Series applied, thanks Vivien.