From: Andrew Lunn <andrew@lunn.ch> To: Steen Hegelund <steen.hegelund@microchip.com> Cc: "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Russell King <linux@armlinux.org.uk>, Lars Povlsen <lars.povlsen@microchip.com>, Bjarni Jonasson <bjarni.jonasson@microchip.com>, Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Madalin Bucur <madalin.bucur@oss.nxp.com>, Nicolas Ferre <nicolas.ferre@microchip.com>, Mark Einon <mark.einon@gmail.com>, Masahiro Yamada <masahiroy@kernel.org>, Arnd Bergmann <arnd@arndb.de>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH v2 5/8] net: sparx5: add switching, vlan and mactable support Date: Mon, 21 Dec 2020 01:25:56 +0100 [thread overview] Message-ID: <20201221002556.GC3107610@lunn.ch> (raw) In-Reply-To: <20201217075134.919699-6-steen.hegelund@microchip.com> > +++ 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. > +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. > + > + sparx5_set_ageing(sparx5, 10 * MSEC_PER_SEC); /* 10 sec */ BR_DEFAULT_AGEING_TIME is 300 seconds. Is this the same thing? > +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? -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. > +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 > +++ 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? > +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 > + 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? Andrew
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Lunn <andrew@lunn.ch> To: Steen Hegelund <steen.hegelund@microchip.com> Cc: Bjarni Jonasson <bjarni.jonasson@microchip.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, linux-kernel@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Madalin Bucur <madalin.bucur@oss.nxp.com>, netdev@vger.kernel.org, Masahiro Yamada <masahiroy@kernel.org>, Russell King <linux@armlinux.org.uk>, Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>, linux-arm-kernel@lists.infradead.org, Mark Einon <mark.einon@gmail.com>, Jakub Kicinski <kuba@kernel.org>, "David S. Miller" <davem@davemloft.net>, Lars Povlsen <lars.povlsen@microchip.com> Subject: Re: [RFC PATCH v2 5/8] net: sparx5: add switching, vlan and mactable support Date: Mon, 21 Dec 2020 01:25:56 +0100 [thread overview] Message-ID: <20201221002556.GC3107610@lunn.ch> (raw) In-Reply-To: <20201217075134.919699-6-steen.hegelund@microchip.com> > +++ 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. > +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. > + > + sparx5_set_ageing(sparx5, 10 * MSEC_PER_SEC); /* 10 sec */ BR_DEFAULT_AGEING_TIME is 300 seconds. Is this the same thing? > +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? -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. > +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 > +++ 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? > +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 > + 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? Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-12-21 0:27 UTC|newest] Thread overview: 75+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-17 7:51 [RFC PATCH v2 0/8] Adding the Sparx5 Switch Driver Steen Hegelund 2020-12-17 7:51 ` Steen Hegelund 2020-12-17 7:51 ` [RFC PATCH v2 1/8] dt-bindings: net: sparx5: Add sparx5-switch bindings Steen Hegelund 2020-12-17 7:51 ` Steen Hegelund 2020-12-19 17:54 ` Andrew Lunn 2020-12-19 17:54 ` Andrew Lunn 2020-12-21 0:55 ` Florian Fainelli 2020-12-21 0:55 ` Florian Fainelli 2020-12-21 10:00 ` Steen Hegelund 2020-12-21 10:00 ` Steen Hegelund 2020-12-21 21:40 ` Rob Herring 2020-12-21 21:40 ` Rob Herring 2020-12-22 7:30 ` Steen Hegelund 2020-12-22 7:30 ` Steen Hegelund 2020-12-17 7:51 ` [RFC PATCH v2 2/8] net: sparx5: add the basic sparx5 driver Steen Hegelund 2020-12-19 19:11 ` Andrew Lunn 2020-12-19 19:11 ` Andrew Lunn 2020-12-22 13:50 ` Steen Hegelund 2020-12-22 13:50 ` Steen Hegelund 2020-12-22 15:01 ` Andrew Lunn 2020-12-22 15:01 ` Andrew Lunn 2020-12-22 16:56 ` Alexandre Belloni 2020-12-22 16:56 ` Alexandre Belloni 2020-12-23 9:03 ` Lars Povlsen 2020-12-23 9:03 ` Lars Povlsen 2020-12-23 8:52 ` Steen Hegelund 2020-12-23 8:52 ` Steen Hegelund 2020-12-17 7:51 ` [RFC PATCH v2 3/8] net: sparx5: add hostmode with phylink support Steen Hegelund 2020-12-17 7:51 ` Steen Hegelund 2020-12-19 19:51 ` Andrew Lunn 2020-12-19 19:51 ` Andrew Lunn 2020-12-22 9:46 ` Steen Hegelund 2020-12-22 9:46 ` Steen Hegelund 2020-12-22 14:41 ` Andrew Lunn 2020-12-22 14:41 ` Andrew Lunn 2020-12-23 13:29 ` Steen Hegelund 2020-12-23 13:29 ` Steen Hegelund 2020-12-23 20:58 ` Alexandre Belloni 2020-12-23 20:58 ` Alexandre Belloni 2020-12-23 21:05 ` Andrew Lunn 2020-12-23 21:05 ` Andrew Lunn 2020-12-17 7:51 ` [RFC PATCH v2 4/8] net: sparx5: add port module support Steen Hegelund 2020-12-17 7:51 ` Steen Hegelund 2020-12-20 23:35 ` Andrew Lunn 2020-12-20 23:35 ` Andrew Lunn 2020-12-22 14:55 ` Bjarni Jonasson 2020-12-22 14:55 ` Bjarni Jonasson 2020-12-22 15:08 ` Andrew Lunn 2020-12-22 15:08 ` Andrew Lunn 2020-12-17 7:51 ` [RFC PATCH v2 5/8] net: sparx5: add switching, vlan and mactable support Steen Hegelund 2020-12-17 7:51 ` Steen Hegelund 2020-12-21 0:25 ` Andrew Lunn [this message] 2020-12-21 0:25 ` Andrew Lunn 2020-12-23 13:54 ` Steen Hegelund 2020-12-23 13:54 ` Steen Hegelund 2020-12-17 7:51 ` [RFC PATCH v2 6/8] net: sparx5: add calendar bandwidth allocation support Steen Hegelund 2020-12-17 7:51 ` Steen Hegelund 2020-12-17 7:51 ` [RFC PATCH v2 7/8] net: sparx5: add ethtool configuration and statistics support Steen Hegelund 2020-12-17 7:51 ` Steen Hegelund 2020-12-19 23:31 ` Andrew Lunn 2020-12-19 23:31 ` Andrew Lunn 2020-12-17 7:51 ` [RFC PATCH v2 8/8] arm64: dts: sparx5: Add the Sparx5 switch node Steen Hegelund 2020-12-17 7:51 ` Steen Hegelund 2020-12-19 20:24 ` Andrew Lunn 2020-12-19 20:24 ` Andrew Lunn 2020-12-23 14:31 ` Steen Hegelund 2020-12-23 14:31 ` Steen Hegelund 2020-12-23 15:49 ` Andrew Lunn 2020-12-23 15:49 ` Andrew Lunn 2020-12-21 0:58 ` [RFC PATCH v2 0/8] Adding the Sparx5 Switch Driver Florian Fainelli 2020-12-21 0:58 ` Florian Fainelli 2020-12-21 14:31 ` Steen Hegelund 2020-12-21 14:31 ` Steen Hegelund 2020-12-22 11:29 ` Lars Povlsen 2020-12-22 11:29 ` Lars Povlsen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201221002556.GC3107610@lunn.ch \ --to=andrew@lunn.ch \ --cc=UNGLinuxDriver@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=arnd@arndb.de \ --cc=bjarni.jonasson@microchip.com \ --cc=davem@davemloft.net \ --cc=kuba@kernel.org \ --cc=lars.povlsen@microchip.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=madalin.bucur@oss.nxp.com \ --cc=mark.einon@gmail.com \ --cc=masahiroy@kernel.org \ --cc=netdev@vger.kernel.org \ --cc=nicolas.ferre@microchip.com \ --cc=steen.hegelund@microchip.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.