| Message ID | 20201218223852.2717102-4-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Reduce coupling between DSA and Broadcom SYSTEMPORT driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 139 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 12/18/2020 2:38 PM, Vladimir Oltean wrote: > The SYSTEMPORT driver maps each port of the embedded Broadcom DSA switch > port to a certain queue of the master Ethernet controller. For that it > currently uses a dedicated notifier infrastructure which was added in > commit 60724d4bae14 ("net: dsa: Add support for DSA specific notifiers"). > > However, since commit 2f1e8ea726e9 ("net: dsa: link interfaces with the > DSA master to get rid of lockdep warnings"), DSA is actually an upper of > the Broadcom SYSTEMPORT as far as the netdevice adjacency lists are > concerned. So naturally, the plain NETDEV_CHANGEUPPER net device notifiers > are emitted. It looks like there is enough API exposed by DSA to the > outside world already to make the call_dsa_notifiers API redundant. So > let's convert its only user to plain netdev notifiers. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/ethernet/broadcom/bcmsysport.c | 76 +++++++++------------- > drivers/net/ethernet/broadcom/bcmsysport.h | 2 +- > 2 files changed, 32 insertions(+), 46 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c > index 82541352b1eb..c5df235975e7 100644 > --- a/drivers/net/ethernet/broadcom/bcmsysport.c > +++ b/drivers/net/ethernet/broadcom/bcmsysport.c > @@ -2311,33 +2311,22 @@ static const struct net_device_ops bcm_sysport_netdev_ops = { > .ndo_select_queue = bcm_sysport_select_queue, > }; > > -static int bcm_sysport_map_queues(struct notifier_block *nb, > - struct dsa_notifier_register_info *info) > +static int bcm_sysport_map_queues(struct net_device *dev, > + struct net_device *slave_dev) > { > + struct dsa_port *dp = dsa_port_from_netdev(slave_dev); > + struct bcm_sysport_priv *priv = netdev_priv(dev); > struct bcm_sysport_tx_ring *ring; > - struct bcm_sysport_priv *priv; > - struct net_device *slave_dev; > unsigned int num_tx_queues; > unsigned int q, qp, port; > - struct net_device *dev; > - > - priv = container_of(nb, struct bcm_sysport_priv, dsa_notifier); > - if (priv->netdev != info->master) > - return 0; There are systems with two SYSTEMPORT network devices registered and therfore this check was intended to avoid programmig the incorrect network device upon notification, however now that we have proper upper/lower linking, and given the decisions made by bcm_sf2.c, only one out of the two SYSTEMPORT network devices will act as a DSA master, therefore this should no longer be necessary. I would like to test this before giving this patch a Acked-by or Teteed-by tag, net-next is still closed and this should only take a few hours if there not any non-maskable real life interrupts showing up.
On 12/18/2020 2:38 PM, Vladimir Oltean wrote: > The SYSTEMPORT driver maps each port of the embedded Broadcom DSA switch > port to a certain queue of the master Ethernet controller. For that it > currently uses a dedicated notifier infrastructure which was added in > commit 60724d4bae14 ("net: dsa: Add support for DSA specific notifiers"). > > However, since commit 2f1e8ea726e9 ("net: dsa: link interfaces with the > DSA master to get rid of lockdep warnings"), DSA is actually an upper of > the Broadcom SYSTEMPORT as far as the netdevice adjacency lists are > concerned. So naturally, the plain NETDEV_CHANGEUPPER net device notifiers > are emitted. It looks like there is enough API exposed by DSA to the > outside world already to make the call_dsa_notifiers API redundant. So > let's convert its only user to plain netdev notifiers. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> The CHANGEUPPER has a slightly different semantic than the current DSA notifier, and so events that would look like this during bcm_sysport_init_tx_ring() (good): [ 6.781064] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0 [ 6.789214] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0 [ 6.797337] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0 [ 6.805464] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0 [ 6.813583] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1 [ 6.821701] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1 [ 6.829819] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1 [ 6.837944] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1 [ 6.846063] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=2 [ 6.854183] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=2 [ 6.862303] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=2 [ 6.870425] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=2 [ 6.878544] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=5 [ 6.886663] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=5 [ 6.894783] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=5 [ 6.902906] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=5 now we are getting (bad): [ 6.678157] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0 [ 6.686302] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0 [ 6.694434] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0 [ 6.702554] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0 [ 6.710679] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0 [ 6.718797] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0 [ 6.726914] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0 [ 6.735033] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0 [ 6.743156] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1 [ 6.751275] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1 [ 6.759395] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1 [ 6.767514] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1 [ 6.775636] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1 [ 6.783754] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1 [ 6.791874] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1 [ 6.799992] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1 Looking further in bcm_sysport_map_queues() we are getting the following: 6.223042] brcm-systemport 9300000.ethernet eth0: mapping q=0, p=0 [ 6.229369] brcm-systemport 9300000.ethernet eth0: mapping q=1, p=0 [ 6.235659] brcm-systemport 9300000.ethernet eth0: mapping q=2, p=0 [ 6.241945] brcm-systemport 9300000.ethernet eth0: mapping q=3, p=0 [ 6.248232] brcm-systemport 9300000.ethernet eth0: mapping q=4, p=0 [ 6.254519] brcm-systemport 9300000.ethernet eth0: mapping q=5, p=0 [ 6.260805] brcm-systemport 9300000.ethernet eth0: mapping q=6, p=0 [ 6.267092] brcm-systemport 9300000.ethernet eth0: mapping q=7, p=0 which means that the call to netif_set_real_num_tx_queues() that is executed for the SYSTEMPORT Lite is not taking effect because it is after the register_netdevice(). Insead of using a CHANGEUPPER notifier, we can use a REGISTER notifier event and doing that works just fine with the same semantics as the DSA notifier being removed. This incremental patch on top of your patch works for me (tm): https://github.com/ffainelli/linux/commit/f5095ab5c1f31db133d62273928b224674626b75 Thanks!
On Fri, Dec 18, 2020 at 08:08:56PM -0800, Florian Fainelli wrote: > On 12/18/2020 2:38 PM, Vladimir Oltean wrote: > > The SYSTEMPORT driver maps each port of the embedded Broadcom DSA switch > > port to a certain queue of the master Ethernet controller. For that it > > currently uses a dedicated notifier infrastructure which was added in > > commit 60724d4bae14 ("net: dsa: Add support for DSA specific notifiers"). > > > > However, since commit 2f1e8ea726e9 ("net: dsa: link interfaces with the > > DSA master to get rid of lockdep warnings"), DSA is actually an upper of > > the Broadcom SYSTEMPORT as far as the netdevice adjacency lists are > > concerned. So naturally, the plain NETDEV_CHANGEUPPER net device notifiers > > are emitted. It looks like there is enough API exposed by DSA to the > > outside world already to make the call_dsa_notifiers API redundant. So > > let's convert its only user to plain netdev notifiers. > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > The CHANGEUPPER has a slightly different semantic than the current DSA > notifier, and so events that would look like this during > bcm_sysport_init_tx_ring() (good): > > [ 6.781064] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0 > [ 6.789214] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0 > [ 6.797337] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0 > [ 6.805464] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0 > [ 6.813583] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1 > [ 6.821701] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1 > [ 6.829819] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1 > [ 6.837944] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1 > [ 6.846063] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=2 > [ 6.854183] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=2 > [ 6.862303] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=2 > [ 6.870425] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=2 > [ 6.878544] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=5 > [ 6.886663] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=5 > [ 6.894783] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=5 > [ 6.902906] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=5 > > now we are getting (bad): > > [ 6.678157] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0 > [ 6.686302] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0 > [ 6.694434] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0 > [ 6.702554] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0 > [ 6.710679] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0 > [ 6.718797] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0 > [ 6.726914] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0 > [ 6.735033] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0 > [ 6.743156] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1 > [ 6.751275] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1 > [ 6.759395] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1 > [ 6.767514] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1 > [ 6.775636] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1 > [ 6.783754] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1 > [ 6.791874] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1 > [ 6.799992] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1 > > Looking further in bcm_sysport_map_queues() we are getting the following: > > 6.223042] brcm-systemport 9300000.ethernet eth0: mapping q=0, p=0 > [ 6.229369] brcm-systemport 9300000.ethernet eth0: mapping q=1, p=0 > [ 6.235659] brcm-systemport 9300000.ethernet eth0: mapping q=2, p=0 > [ 6.241945] brcm-systemport 9300000.ethernet eth0: mapping q=3, p=0 > [ 6.248232] brcm-systemport 9300000.ethernet eth0: mapping q=4, p=0 > [ 6.254519] brcm-systemport 9300000.ethernet eth0: mapping q=5, p=0 > [ 6.260805] brcm-systemport 9300000.ethernet eth0: mapping q=6, p=0 > [ 6.267092] brcm-systemport 9300000.ethernet eth0: mapping q=7, p=0 > > which means that the call to netif_set_real_num_tx_queues() that is > executed for the SYSTEMPORT Lite is not taking effect because it is > after the register_netdevice(). Insead of using a CHANGEUPPER notifier, > we can use a REGISTER notifier event and doing that works just fine with > the same semantics as the DSA notifier being removed. This incremental > patch on top of your patch works for me (tm): > > https://github.com/ffainelli/linux/commit/f5095ab5c1f31db133d62273928b224674626b75 This is odd, the netif_set_real_num_tx_queues() call should not fail or be ignored even if the interface was registered. I had tested this already on my enetc + felix combo on LS1028A. static int enetc_dsa_join(struct net_device *dev, struct net_device *slave_dev) { int err; netdev_err(slave_dev, "Hello!\n"); err = netif_set_real_num_tx_queues(slave_dev, slave_dev->num_tx_queues / 2); if (err) return err; netdev_err(slave_dev, "New number of real TX queues: %d\n", slave_dev->real_num_tx_queues); return 0; } prints: [ 7.002328] mscc_felix 0000:00:00.5 swp0 (uninitialized): PHY [0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.021190] mscc_felix 0000:00:00.5 swp0: Hello! [ 7.028657] mscc_felix 0000:00:00.5 swp0: New number of real TX queues: 4 [ 7.035589] mscc_felix 0000:00:00.5 swp0: Hello! [ 7.040380] mscc_felix 0000:00:00.5 swp0: New number of real TX queues: 4 [ 7.290236] mscc_felix 0000:00:00.5 swp1 (uninitialized): PHY [0000:00:00.3:11] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.314383] mscc_felix 0000:00:00.5 swp1: Hello! [ 7.321292] mscc_felix 0000:00:00.5 swp1: New number of real TX queues: 4 [ 7.328223] mscc_felix 0000:00:00.5 swp1: Hello! [ 7.332967] mscc_felix 0000:00:00.5 swp1: New number of real TX queues: 4 [ 7.574254] mscc_felix 0000:00:00.5 swp2 (uninitialized): PHY [0000:00:00.3:12] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.598431] mscc_felix 0000:00:00.5 swp2: Hello! [ 7.605215] mscc_felix 0000:00:00.5 swp2: New number of real TX queues: 4 [ 7.612145] mscc_felix 0000:00:00.5 swp2: Hello! [ 7.616889] mscc_felix 0000:00:00.5 swp2: New number of real TX queues: 4 [ 7.858868] mscc_felix 0000:00:00.5 swp3 (uninitialized): PHY [0000:00:00.3:13] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) [ 7.884240] mscc_felix 0000:00:00.5 swp3: Hello! [ 7.891086] mscc_felix 0000:00:00.5 swp3: New number of real TX queues: 4 [ 7.898018] mscc_felix 0000:00:00.5 swp3: Hello! [ 7.902763] mscc_felix 0000:00:00.5 swp3: New number of real TX queues: 4 (I am not sure why the notifier is called twice though) You are saying that here: num_tx_queues = slave_dev->real_num_tx_queues; num_tx_queues remains assigned to 8? Does this mean that netif_set_real_num_tx_queues has returned an error code? Can you check why?
On 12/19/2020 4:12 AM, Vladimir Oltean wrote: > On Fri, Dec 18, 2020 at 08:08:56PM -0800, Florian Fainelli wrote: >> On 12/18/2020 2:38 PM, Vladimir Oltean wrote: >>> The SYSTEMPORT driver maps each port of the embedded Broadcom DSA switch >>> port to a certain queue of the master Ethernet controller. For that it >>> currently uses a dedicated notifier infrastructure which was added in >>> commit 60724d4bae14 ("net: dsa: Add support for DSA specific notifiers"). >>> >>> However, since commit 2f1e8ea726e9 ("net: dsa: link interfaces with the >>> DSA master to get rid of lockdep warnings"), DSA is actually an upper of >>> the Broadcom SYSTEMPORT as far as the netdevice adjacency lists are >>> concerned. So naturally, the plain NETDEV_CHANGEUPPER net device notifiers >>> are emitted. It looks like there is enough API exposed by DSA to the >>> outside world already to make the call_dsa_notifiers API redundant. So >>> let's convert its only user to plain netdev notifiers. >>> >>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> >> >> The CHANGEUPPER has a slightly different semantic than the current DSA >> notifier, and so events that would look like this during >> bcm_sysport_init_tx_ring() (good): >> >> [ 6.781064] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0 >> [ 6.789214] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0 >> [ 6.797337] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0 >> [ 6.805464] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0 >> [ 6.813583] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1 >> [ 6.821701] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1 >> [ 6.829819] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1 >> [ 6.837944] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1 >> [ 6.846063] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=2 >> [ 6.854183] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=2 >> [ 6.862303] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=2 >> [ 6.870425] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=2 >> [ 6.878544] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=5 >> [ 6.886663] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=5 >> [ 6.894783] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=5 >> [ 6.902906] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=5 >> >> now we are getting (bad): >> >> [ 6.678157] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0 >> [ 6.686302] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0 >> [ 6.694434] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0 >> [ 6.702554] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0 >> [ 6.710679] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=0 >> [ 6.718797] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=0 >> [ 6.726914] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=0 >> [ 6.735033] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=0 >> [ 6.743156] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1 >> [ 6.751275] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1 >> [ 6.759395] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1 >> [ 6.767514] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1 >> [ 6.775636] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=0,port=1 >> [ 6.783754] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=1,port=1 >> [ 6.791874] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=2,port=1 >> [ 6.799992] brcm-systemport 9300000.ethernet eth0: TDMA cfg, size=256, switch q=3,port=1 >> >> Looking further in bcm_sysport_map_queues() we are getting the following: >> >> 6.223042] brcm-systemport 9300000.ethernet eth0: mapping q=0, p=0 >> [ 6.229369] brcm-systemport 9300000.ethernet eth0: mapping q=1, p=0 >> [ 6.235659] brcm-systemport 9300000.ethernet eth0: mapping q=2, p=0 >> [ 6.241945] brcm-systemport 9300000.ethernet eth0: mapping q=3, p=0 >> [ 6.248232] brcm-systemport 9300000.ethernet eth0: mapping q=4, p=0 >> [ 6.254519] brcm-systemport 9300000.ethernet eth0: mapping q=5, p=0 >> [ 6.260805] brcm-systemport 9300000.ethernet eth0: mapping q=6, p=0 >> [ 6.267092] brcm-systemport 9300000.ethernet eth0: mapping q=7, p=0 >> >> which means that the call to netif_set_real_num_tx_queues() that is >> executed for the SYSTEMPORT Lite is not taking effect because it is >> after the register_netdevice(). Insead of using a CHANGEUPPER notifier, >> we can use a REGISTER notifier event and doing that works just fine with >> the same semantics as the DSA notifier being removed. This incremental >> patch on top of your patch works for me (tm): >> >> https://github.com/ffainelli/linux/commit/f5095ab5c1f31db133d62273928b224674626b75 > > This is odd, the netif_set_real_num_tx_queues() call should not fail or > be ignored even if the interface was registered. I had tested this already > on my enetc + felix combo on LS1028A. Yes that part is fine, see below. > > static int enetc_dsa_join(struct net_device *dev, > struct net_device *slave_dev) > { > int err; > > netdev_err(slave_dev, "Hello!\n"); > > err = netif_set_real_num_tx_queues(slave_dev, > slave_dev->num_tx_queues / 2); > if (err) > return err; > > netdev_err(slave_dev, "New number of real TX queues: %d\n", > slave_dev->real_num_tx_queues); > > return 0; > } > > prints: > > [ 7.002328] mscc_felix 0000:00:00.5 swp0 (uninitialized): PHY [0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) > [ 7.021190] mscc_felix 0000:00:00.5 swp0: Hello! > [ 7.028657] mscc_felix 0000:00:00.5 swp0: New number of real TX queues: 4 > [ 7.035589] mscc_felix 0000:00:00.5 swp0: Hello! > [ 7.040380] mscc_felix 0000:00:00.5 swp0: New number of real TX queues: 4 > [ 7.290236] mscc_felix 0000:00:00.5 swp1 (uninitialized): PHY [0000:00:00.3:11] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) > [ 7.314383] mscc_felix 0000:00:00.5 swp1: Hello! > [ 7.321292] mscc_felix 0000:00:00.5 swp1: New number of real TX queues: 4 > [ 7.328223] mscc_felix 0000:00:00.5 swp1: Hello! > [ 7.332967] mscc_felix 0000:00:00.5 swp1: New number of real TX queues: 4 > [ 7.574254] mscc_felix 0000:00:00.5 swp2 (uninitialized): PHY [0000:00:00.3:12] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) > [ 7.598431] mscc_felix 0000:00:00.5 swp2: Hello! > [ 7.605215] mscc_felix 0000:00:00.5 swp2: New number of real TX queues: 4 > [ 7.612145] mscc_felix 0000:00:00.5 swp2: Hello! > [ 7.616889] mscc_felix 0000:00:00.5 swp2: New number of real TX queues: 4 > [ 7.858868] mscc_felix 0000:00:00.5 swp3 (uninitialized): PHY [0000:00:00.3:13] driver [Microsemi GE VSC8514 SyncE] (irq=POLL) > [ 7.884240] mscc_felix 0000:00:00.5 swp3: Hello! > [ 7.891086] mscc_felix 0000:00:00.5 swp3: New number of real TX queues: 4 > [ 7.898018] mscc_felix 0000:00:00.5 swp3: Hello! > [ 7.902763] mscc_felix 0000:00:00.5 swp3: New number of real TX queues: 4 > > (I am not sure why the notifier is called twice though) This is the actual issue that messes up the queue assignment for bcmsysport because we assign queue/switch port pairs and don't really expect to be re-doing that once ring->inspect is set to true. > > You are saying that here: > > num_tx_queues = slave_dev->real_num_tx_queues; > > num_tx_queues remains assigned to 8? Does this mean that netif_set_real_num_tx_queues > has returned an error code? Can you check why? > The call to netif_set_real_num_tx_queues() succeeds and slave_dev->real_num_tx_queues is changed to 4 accordingly. The loop that assigns the internal queue mapping (priv->ring_map) is correctly limited to 4, however we get two calls per switch port instead of one. I did not have much time to debug why we get called twice but I will be looking into this tomorrow.
On 12/20/2020 8:53 PM, Florian Fainelli wrote: > > The call to netif_set_real_num_tx_queues() succeeds and > slave_dev->real_num_tx_queues is changed to 4 accordingly. The loop that > assigns the internal queue mapping (priv->ring_map) is correctly limited > to 4, however we get two calls per switch port instead of one. I did not > have much time to debug why we get called twice but I will be looking > into this tomorrow. There was not any bug other than there are two instances of a SYSTEMPORT device in my system and they both receive the same notification. So we do need to qualify which of the notifier block matches the device of interest, because if we do extract the private structure from the device being notified, it is always going to match. Incremental fixup here: https://github.com/ffainelli/linux/commit/0eea16e706a73c56a36d701df483ff73211aae7f and you can add Tested-by: Florian Fainelli <f.fainelli@gmail.com> when you resubmit. Thanks, this is a really nice cleanup.
On Mon, Dec 21, 2020 at 02:33:16PM -0800, Florian Fainelli wrote: > On 12/20/2020 8:53 PM, Florian Fainelli wrote: > > The call to netif_set_real_num_tx_queues() succeeds and > > slave_dev->real_num_tx_queues is changed to 4 accordingly. The loop that > > assigns the internal queue mapping (priv->ring_map) is correctly limited > > to 4, however we get two calls per switch port instead of one. I did not > > have much time to debug why we get called twice but I will be looking > > into this tomorrow. > > There was not any bug other than there are two instances of a SYSTEMPORT > device in my system and they both receive the same notification. > > So we do need to qualify which of the notifier block matches the device > of interest, because if we do extract the private structure from the > device being notified, it is always going to match. > > Incremental fixup here: > > https://github.com/ffainelli/linux/commit/0eea16e706a73c56a36d701df483ff73211aae7f ...duh. And when you come to think that I had deleted that code in my patch, not understanding what it's for... Coincidentally this is also the reason why I got the prints twice. Sorry :( > > and you can add Tested-by: Florian Fainelli <f.fainelli@gmail.com> when > you resubmit. > > Thanks, this is a really nice cleanup. Thanks. Do you think we need some getters for dp->index and dp->ds->index, to preserve some sort of data structure encapsulation from the outside world (although it's not as if the members of struct dsa_switch and struct dsa_port still couldn't be accessed directly)? But then, there's the other aspect. We would have some shiny accessors for DSA properties, but we're resetting the net_device's number of TX queues. So much for data encapsulation.
On 12/21/2020 3:06 PM, Vladimir Oltean wrote: > On Mon, Dec 21, 2020 at 02:33:16PM -0800, Florian Fainelli wrote: >> On 12/20/2020 8:53 PM, Florian Fainelli wrote: >>> The call to netif_set_real_num_tx_queues() succeeds and >>> slave_dev->real_num_tx_queues is changed to 4 accordingly. The loop that >>> assigns the internal queue mapping (priv->ring_map) is correctly limited >>> to 4, however we get two calls per switch port instead of one. I did not >>> have much time to debug why we get called twice but I will be looking >>> into this tomorrow. >> >> There was not any bug other than there are two instances of a SYSTEMPORT >> device in my system and they both receive the same notification. >> >> So we do need to qualify which of the notifier block matches the device >> of interest, because if we do extract the private structure from the >> device being notified, it is always going to match. >> >> Incremental fixup here: >> >> https://github.com/ffainelli/linux/commit/0eea16e706a73c56a36d701df483ff73211aae7f > > ...duh. > And when you come to think that I had deleted that code in my patch, not > understanding what it's for... Coincidentally this is also the reason > why I got the prints twice. Sorry :( No worries, I had it "automatically" in my experiment with the REGISTER/UNREGISTER and it only clicked this morning this was the key thing here. > >> >> and you can add Tested-by: Florian Fainelli <f.fainelli@gmail.com> when >> you resubmit. >> >> Thanks, this is a really nice cleanup. > > Thanks. > > Do you think we need some getters for dp->index and dp->ds->index, to preserve > some sort of data structure encapsulation from the outside world (although it's > not as if the members of struct dsa_switch and struct dsa_port still couldn't > be accessed directly)? > > But then, there's the other aspect. We would have some shiny accessors for DSA > properties, but we're resetting the net_device's number of TX queues. > So much for data encapsulation. If we move the dsa_port structure definition to be more private, and say within dsa_priv.h, we will have to create quite some bit of churn within the DSA driver to make them use getters and setters. Russell did a nice job with the encapsulation with phylink and that would really be a good model to follow, however this was a clean slate. It seems to me for now that this is not worth the trouble. Despite accessing the TX queues directly, the original DSA notifier was trying to provide all the necessary data to the recipient of the notification without having to know too much about what a DSA device is but the amount of code eliminated is of superior value IMHO.
On Mon, Dec 21, 2020 at 03:17:02PM -0800, Florian Fainelli wrote: > > Do you think we need some getters for dp->index and dp->ds->index, to preserve > > some sort of data structure encapsulation from the outside world (although it's > > not as if the members of struct dsa_switch and struct dsa_port still couldn't > > be accessed directly)? > > > > But then, there's the other aspect. We would have some shiny accessors for DSA > > properties, but we're resetting the net_device's number of TX queues. > > So much for data encapsulation. > > If we move the dsa_port structure definition to be more private, and say > within dsa_priv.h, we will have to create quite some bit of churn within > the DSA driver to make them use getters and setters. Russell did a nice > job with the encapsulation with phylink and that would really be a good > model to follow, however this was a clean slate. It seems to me for now > that this is not worth the trouble. We could make include/net/dsa.h a semi-private ("friend") header that the DSA drivers could keep including, but the non-DSA world wouldn't. Then we could create a new one in its place, with just the stuff that the outside world needs: netdev_uses_dsa, etc. > Despite accessing the TX queues directly, the original DSA notifier was > trying to provide all the necessary data to the recipient of the > notification without having to know too much about what a DSA device is I know. Personally, accessing dp->cpu_dp->master directly is where I was going to draw the line and say that it's better to just keep the code as is. What motivated me to make the change in the first place was the realization that we now have the linkage visible from the outside world. > but the amount of code eliminated is of superior value IMHO. It's still a compromise, really. The atomic DSA notifier was ok, but if we could do without it, why not...
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index 82541352b1eb..c5df235975e7 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -2311,33 +2311,22 @@ static const struct net_device_ops bcm_sysport_netdev_ops = { .ndo_select_queue = bcm_sysport_select_queue, }; -static int bcm_sysport_map_queues(struct notifier_block *nb, - struct dsa_notifier_register_info *info) +static int bcm_sysport_map_queues(struct net_device *dev, + struct net_device *slave_dev) { + struct dsa_port *dp = dsa_port_from_netdev(slave_dev); + struct bcm_sysport_priv *priv = netdev_priv(dev); struct bcm_sysport_tx_ring *ring; - struct bcm_sysport_priv *priv; - struct net_device *slave_dev; unsigned int num_tx_queues; unsigned int q, qp, port; - struct net_device *dev; - - priv = container_of(nb, struct bcm_sysport_priv, dsa_notifier); - if (priv->netdev != info->master) - return 0; - - dev = info->master; /* We can't be setting up queue inspection for non directly attached * switches */ - if (info->switch_number) + if (dp->ds->index) return 0; - if (dev->netdev_ops != &bcm_sysport_netdev_ops) - return 0; - - port = info->port_number; - slave_dev = info->info.dev; + port = dp->index; /* On SYSTEMPORT Lite we have twice as less queues, so we cannot do a * 1:1 mapping, we can only do a 2:1 mapping. By reducing the number of @@ -2377,27 +2366,16 @@ static int bcm_sysport_map_queues(struct notifier_block *nb, return 0; } -static int bcm_sysport_unmap_queues(struct notifier_block *nb, - struct dsa_notifier_register_info *info) +static int bcm_sysport_unmap_queues(struct net_device *dev, + struct net_device *slave_dev) { + struct dsa_port *dp = dsa_port_from_netdev(slave_dev); + struct bcm_sysport_priv *priv = netdev_priv(dev); struct bcm_sysport_tx_ring *ring; - struct bcm_sysport_priv *priv; - struct net_device *slave_dev; unsigned int num_tx_queues; - struct net_device *dev; unsigned int q, qp, port; - priv = container_of(nb, struct bcm_sysport_priv, dsa_notifier); - if (priv->netdev != info->master) - return 0; - - dev = info->master; - - if (dev->netdev_ops != &bcm_sysport_netdev_ops) - return 0; - - port = info->port_number; - slave_dev = info->info.dev; + port = dp->index; num_tx_queues = slave_dev->real_num_tx_queues; @@ -2418,17 +2396,25 @@ static int bcm_sysport_unmap_queues(struct notifier_block *nb, return 0; } -static int bcm_sysport_dsa_notifier(struct notifier_block *nb, - unsigned long event, void *ptr) +static int bcm_sysport_netdevice_event(struct notifier_block *nb, + unsigned long event, void *ptr) { - int ret = NOTIFY_DONE; + struct net_device *dev = netdev_notifier_info_to_dev(ptr); + struct netdev_notifier_changeupper_info *info = ptr; + int ret = 0; switch (event) { - case DSA_PORT_REGISTER: - ret = bcm_sysport_map_queues(nb, ptr); - break; - case DSA_PORT_UNREGISTER: - ret = bcm_sysport_unmap_queues(nb, ptr); + case NETDEV_CHANGEUPPER: + if (dev->netdev_ops != &bcm_sysport_netdev_ops) + return NOTIFY_DONE; + + if (!dsa_slave_dev_check(info->upper_dev)) + return NOTIFY_DONE; + + if (info->linking) + ret = bcm_sysport_map_queues(dev, info->upper_dev); + else + ret = bcm_sysport_unmap_queues(dev, info->upper_dev); break; } @@ -2600,9 +2586,9 @@ static int bcm_sysport_probe(struct platform_device *pdev) priv->rx_max_coalesced_frames = 1; u64_stats_init(&priv->syncp); - priv->dsa_notifier.notifier_call = bcm_sysport_dsa_notifier; + priv->netdev_notifier.notifier_call = bcm_sysport_netdevice_event; - ret = register_dsa_notifier(&priv->dsa_notifier); + ret = register_netdevice_notifier(&priv->netdev_notifier); if (ret) { dev_err(&pdev->dev, "failed to register DSA notifier\n"); goto err_deregister_fixed_link; @@ -2629,7 +2615,7 @@ static int bcm_sysport_probe(struct platform_device *pdev) return 0; err_deregister_notifier: - unregister_dsa_notifier(&priv->dsa_notifier); + unregister_netdevice_notifier(&priv->netdev_notifier); err_deregister_fixed_link: if (of_phy_is_fixed_link(dn)) of_phy_deregister_fixed_link(dn); @@ -2647,7 +2633,7 @@ static int bcm_sysport_remove(struct platform_device *pdev) /* Not much to do, ndo_close has been called * and we use managed allocations */ - unregister_dsa_notifier(&priv->dsa_notifier); + unregister_netdevice_notifier(&priv->netdev_notifier); unregister_netdev(dev); if (of_phy_is_fixed_link(dn)) of_phy_deregister_fixed_link(dn); diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h index 3a5cb6f128f5..fefd3ccf0379 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.h +++ b/drivers/net/ethernet/broadcom/bcmsysport.h @@ -787,7 +787,7 @@ struct bcm_sysport_priv { struct u64_stats_sync syncp; /* map information between switch port queues and local queues */ - struct notifier_block dsa_notifier; + struct notifier_block netdev_notifier; unsigned int per_port_num_tx_queues; struct bcm_sysport_tx_ring *ring_map[DSA_MAX_PORTS * 8];
The SYSTEMPORT driver maps each port of the embedded Broadcom DSA switch port to a certain queue of the master Ethernet controller. For that it currently uses a dedicated notifier infrastructure which was added in commit 60724d4bae14 ("net: dsa: Add support for DSA specific notifiers"). However, since commit 2f1e8ea726e9 ("net: dsa: link interfaces with the DSA master to get rid of lockdep warnings"), DSA is actually an upper of the Broadcom SYSTEMPORT as far as the netdevice adjacency lists are concerned. So naturally, the plain NETDEV_CHANGEUPPER net device notifiers are emitted. It looks like there is enough API exposed by DSA to the outside world already to make the call_dsa_notifiers API redundant. So let's convert its only user to plain netdev notifiers. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/ethernet/broadcom/bcmsysport.c | 76 +++++++++------------- drivers/net/ethernet/broadcom/bcmsysport.h | 2 +- 2 files changed, 32 insertions(+), 46 deletions(-)