[RFC PATCH v2 5/8] net: sparx5: add switching, vlan and mactable support

[Steen Hegelund]

Hi Andrew,

On 21.12.2020 01:25, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
>> +
>> +static inline int sparx5_mact_get_status(struct sparx5 *sparx5)
>> +{
>> +     return spx5_rd(sparx5, LRN_COMMON_ACCESS_CTRL);
>> +}
>> +
>> +static inline int sparx5_mact_wait_for_completion(struct sparx5 *sparx5)
>> +{
>> +     u32 val;
>> +
>> +     return readx_poll_timeout(sparx5_mact_get_status,
>> +             sparx5, val,
>> +             LRN_COMMON_ACCESS_CTRL_MAC_TABLE_ACCESS_SHOT_GET(val) == 0,
>> +             TABLE_UPDATE_SLEEP_US, TABLE_UPDATE_TIMEOUT_US);
>> +}
>
>No inline functions in C files please.

OK.

>
>> +void sparx5_mact_init(struct sparx5 *sparx5)
>> +{
>> +     mutex_init(&sparx5->lock);
>> +
>> +     mutex_lock(&sparx5->lock);
>> +
>> +     /*  Flush MAC table */
>> +     spx5_wr(LRN_COMMON_ACCESS_CTRL_CPU_ACCESS_CMD_SET(MAC_CMD_CLEAR_ALL) |
>> +             LRN_COMMON_ACCESS_CTRL_MAC_TABLE_ACCESS_SHOT_SET(1),
>> +             sparx5, LRN_COMMON_ACCESS_CTRL);
>> +
>> +     if (sparx5_mact_wait_for_completion(sparx5) != 0)
>> +             dev_warn(sparx5->dev, "MAC flush error\n");
>> +
>> +     mutex_unlock(&sparx5->lock);
>
>It always seems odd to me, when you initialise a mutex, and then
>immediately take it. Who are you locking against? I'm not saying it is
>wrong though, especially if you have code in spx5_wr() and spx5_rd()
>which check the lock is actually taken. I've found a number of locking
>bugs in mv88e6xxx by having such checks.

The driver has a workqueue and a notifier callback that may want to
access the table, and will have to wait in line to be served, but since
they have not yet been activated at this point, you are probably correct
in saying that locking is not needed at init time.
I will investigate this..

>
>> +
>> +     sparx5_set_ageing(sparx5, 10 * MSEC_PER_SEC); /* 10 sec */
>
>BR_DEFAULT_AGEING_TIME is 300 seconds. Is this the same thing?

Same thing but different value.  I think this should change to same
default, but I will test it out.
>
>> +static int sparx5_port_bridge_join(struct sparx5_port *port,
>> +                                struct net_device *bridge)
>> +{
>> +     struct sparx5 *sparx5 = port->sparx5;
>> +
>> +     if (bitmap_empty(sparx5->bridge_mask, SPX5_PORTS))
>> +             /* First bridged port */
>> +             sparx5->hw_bridge_dev = bridge;
>> +     else
>> +             if (sparx5->hw_bridge_dev != bridge)
>> +                     /* This is adding the port to a second bridge, this is
>> +                      * unsupported
>> +                      */
>> +                     return -ENODEV;
>
>Just checking my understanding. You have a 64 port switch, which only
>supports a single bridge?

Yes, at least for the initial version. The switch has some
virtualization features, but were not using that just yet.

>
>-EOPNOTSUPP seems like a better return code.
>
>> +
>> +     set_bit(port->portno, sparx5->bridge_mask);
>> +
>> +     /* Port enters in bridge mode therefor don't need to copy to CPU
>> +      * frames for multicast in case the bridge is not requesting them
>> +      */
>> +     __dev_mc_unsync(port->ndev, sparx5_mc_unsync);
>
>Did you copy that from the mellanox driver? I think in DSA we take the
>opposite approach. Multicast/broadcast goes to the CPU until the CPU
>says it does not want it.
>
>
It is "inspired" by the Ocelot driver. MC is explicitly opted in.

>> +static void sparx5_port_bridge_leave(struct sparx5_port *port,
>> +                                  struct net_device *bridge)
>> +{
>> +     struct sparx5 *sparx5 = port->sparx5;
>> +
>> +     clear_bit(port->portno, sparx5->bridge_mask);
>> +     if (bitmap_empty(sparx5->bridge_mask, SPX5_PORTS))
>> +             sparx5->hw_bridge_dev = NULL;
>> +
>> +     /* Clear bridge vlan settings before updating the port settings */
>> +     port->vlan_aware = 0;
>> +     port->pvid = NULL_VID;
>> +     port->vid = NULL_VID;
>> +
>> +     /* Port enters in host more therefore restore mc list */
>
>s/more/mode

OK.

>
>> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_vlan.c
>> @@ -0,0 +1,223 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/* Microchip Sparx5 Switch driver
>> + *
>> + * Copyright (c) 2020 Microchip Technology Inc. and its subsidiaries.
>> + */
>> +
>> +#include "sparx5_main.h"
>> +
>> +static int sparx5_vlant_set_mask(struct sparx5 *sparx5, u16 vid)
>
>Is the t in vlant typ0?

No, it is probably short for "vlan table". Again the inspiration comes
from the Ocelot implementation.

>
>> +int sparx5_vlan_vid_add(struct sparx5_port *port, u16 vid, bool pvid,
>> +                     bool untagged)
>> +{
>> +     struct sparx5 *sparx5 = port->sparx5;
>> +     int ret;
>> +
>> +     /* Make the port a member of the VLAN */
>> +     set_bit(port->portno, sparx5->vlan_mask[vid]);
>> +     ret = sparx5_vlant_set_mask(sparx5, vid);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* Default ingress vlan classification */
>> +     if (pvid)
>> +             port->pvid = vid;
>> +
>> +     /* Untagged egress vlan clasification */
>
>classification

OK.

>
>> +     if (untagged && port->vid != vid) {
>> +             if (port->vid) {
>> +                     netdev_err(port->ndev,
>> +                                "Port already has a native VLAN: %d\n",
>> +                                port->vid);
>> +                     return -EBUSY;
>> +             }
>> +             port->vid = vid;
>> +     }
>> +
>> +     sparx5_vlan_port_apply(sparx5, port);
>> +
>> +     return 0;
>> +}
>
>
>> +void sparx5_update_fwd(struct sparx5 *sparx5)
>> +{
>> +     u32 mask[3];
>> +     DECLARE_BITMAP(workmask, SPX5_PORTS);
>> +     int port;
>> +
>> +     /* Divide up fwd mask in 32 bit words */
>> +     bitmap_to_arr32(mask, sparx5->bridge_fwd_mask, SPX5_PORTS);
>> +
>> +     /* Update flood masks */
>> +     for (port = PGID_UC_FLOOD; port <= PGID_BCAST; port++) {
>> +             spx5_wr(mask[0], sparx5, ANA_AC_PGID_CFG(port));
>> +             spx5_wr(mask[1], sparx5, ANA_AC_PGID_CFG1(port));
>> +             spx5_wr(mask[2], sparx5, ANA_AC_PGID_CFG2(port));
>> +     }
>> +
>> +     /* Update SRC masks */
>> +     for (port = 0; port < SPX5_PORTS; port++) {
>> +             if (test_bit(port, sparx5->bridge_fwd_mask)) {
>> +                     /* Allow to send to all bridged but self */
>> +                     bitmap_copy(workmask, sparx5->bridge_fwd_mask, SPX5_PORTS);
>> +                     clear_bit(port, workmask);
>> +                     bitmap_to_arr32(mask, workmask, SPX5_PORTS);
>> +                     spx5_wr(mask[0], sparx5, ANA_AC_SRC_CFG(port));
>> +                     spx5_wr(mask[1], sparx5, ANA_AC_SRC_CFG1(port));
>> +                     spx5_wr(mask[2], sparx5, ANA_AC_SRC_CFG2(port));
>> +             } else {
>> +                     spx5_wr(0, sparx5, ANA_AC_SRC_CFG(port));
>> +                     spx5_wr(0, sparx5, ANA_AC_SRC_CFG1(port));
>> +                     spx5_wr(0, sparx5, ANA_AC_SRC_CFG2(port));
>> +             }
>
>Humm, interesting. This seems to control what other ports a port can
>send to. That is one of the basic features you need for supporting
>multiple bridges. So i assume your problems is you cannot partition
>the MAC table?
>
No, the MAC table is VLAN aware.

>
>    Andrew

BR
Steen

---------------------------------------
Steen Hegelund
steen.hegelund at microchip.com