From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-08 22:32:49
This patch set is provided solely for review purposes (therefore not to
be applied anywhere) and for Ansuel to test whether they resolve the
slowdown reported here:
https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
It does conflict with net-next due to other patches that are in my tree,
and which were also posted here and would need to be picked ("Rework DSA
bridge TX forwarding offload API"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
Additionally, for Ansuel's work there is also a logical dependency with
this series ("Replace DSA dp->priv with tagger-owned storage"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
To get both dependency series, the following commands should be sufficient:
git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
where "git b4" is an alias in ~/.gitconfig:
[b4]
midmask = https://lore.kernel.org/r/%s
[alias]
b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
The patches posted here are mainly to offer a consistent
"master_up"/"master_going_down" chain of events to switches, without
duplicates, and always starting with "master_up" and ending with
"master_going_down". This way, drivers should know when they can perform
Ethernet-based register access.
Vladimir Oltean (7):
net: dsa: only bring down user ports assigned to a given DSA master
net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
function
net: dsa: use dsa_tree_for_each_user_port in
dsa_tree_master_going_down()
net: dsa: provide switch operations for tracking the master state
net: dsa: stop updating master MTU from master.c
net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
net: dsa: replay master state events in
dsa_tree_{setup,teardown}_master
include/net/dsa.h | 8 +++++++
net/dsa/dsa2.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--
net/dsa/dsa_priv.h | 11 ++++++++++
net/dsa/master.c | 29 +++-----------------------
net/dsa/slave.c | 32 +++++++++++++++-------------
net/dsa/switch.c | 29 ++++++++++++++++++++++++++
6 files changed, 118 insertions(+), 43 deletions(-)
--
2.25.1
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-08 22:32:50
This is an adaptation of commit c0a8a9c27493 ("net: dsa: automatically
bring user ports down when master goes down") for multiple DSA masters.
When a DSA master goes down, only the user ports under its control
should go down too, the others can still send/receive traffic.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 3 +++
1 file changed, 3 insertions(+)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-08 22:32:51
For symmetry with a yet-to-be-added function named dsa_tree_master_up,
move the logic that handles a NETDEV_GOING_DOWN netdev notifier on a DSA
master into a dedicated function.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/dsa2.c | 19 +++++++++++++++++++
net/dsa/dsa_priv.h | 2 ++
net/dsa/slave.c | 25 ++++++-------------------
3 files changed, 27 insertions(+), 19 deletions(-)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-08 22:32:52
Consume 3 lines of code by using the dedicated iterator over the user
ports of a DSA switch tree.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/dsa2.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-08 22:32:55
Certain drivers may need to send management traffic to the switch for
things like register access, FDB dump, etc, to accelerate what their
slow bus (SPI, I2C, MDIO) can already do.
Ethernet is faster (especially in bulk transactions) but is also more
unreliable, since the user may decide to bring the DSA master down (or
not bring it up), therefore severing the link between the host and the
attached switch.
Drivers needing Ethernet-based register access already should have
fallback logic to the slow bus if the Ethernet method fails, but that
fallback may be based on a timeout, and the I/O to the switch may slow
down to a halt if the master is down, because every Ethernet packet will
have to time out. The driver also doesn't have the option to turn off
Ethernet-based I/O momentarily, because it wouldn't know when to turn it
back on.
Which is where this change comes in. By tracking NETDEV_UP and
NETDEV_GOING_DOWN events on the DSA master, we should know when this
interface becomes available for traffic. Provide this information to
switches so they can use it as they wish.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/net/dsa.h | 8 ++++++++
net/dsa/dsa2.c | 14 ++++++++++++++
net/dsa/dsa_priv.h | 9 +++++++++
net/dsa/slave.c | 12 ++++++++++++
net/dsa/switch.c | 29 +++++++++++++++++++++++++++++
5 files changed, 72 insertions(+)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-08 22:33:00
The dev_set_mtu() call from dsa_master_setup() has been effectively
superseded by the dsa_slave_change_mtu(slave_dev, ETH_DATA_LEN) that is
done from dsa_slave_create() for each user port. This function also
updates the master MTU according to the largest user port MTU from the
tree. Therefore, updating the master MTU through a separate code path
isn't needed.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/master.c | 25 +------------------------
1 file changed, 1 insertion(+), 24 deletions(-)
@@ -330,28 +330,13 @@ static const struct attribute_group dsa_group = {.attrs=dsa_slave_attrs,};-staticvoiddsa_master_reset_mtu(structnet_device*dev)-{-interr;--rtnl_lock();-err=dev_set_mtu(dev,ETH_DATA_LEN);-if(err)-netdev_dbg(dev,-"Unable to reset MTU to exclude DSA overheads\n");-rtnl_unlock();-}-staticstructlock_class_keydsa_master_addr_list_lock_key;intdsa_master_setup(structnet_device*dev,structdsa_port*cpu_dp){-conststructdsa_device_ops*tag_ops=cpu_dp->tag_ops;structdsa_switch*ds=cpu_dp->ds;structdevice_link*consumer_link;-intmtu,ret;--mtu=ETH_DATA_LEN+dsa_tag_protocol_overhead(tag_ops);+intret;/* The DSA master must use SET_NETDEV_DEV for this to work. */consumer_link=device_link_add(ds->dev,dev->dev.parent,
@@ -361,13 +346,6 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)"Failed to create a device link to DSA switch %s\n",dev_name(ds->dev));-rtnl_lock();-ret=dev_set_mtu(dev,mtu);-rtnl_unlock();-if(ret)-netdev_warn(dev,"error %d setting MTU to %d to include DSA overhead\n",-ret,mtu);-/* If we use a tagging format that doesn't have an ethertype*field,makesurethatallpacketsfromthispointonget*senttothetagformat'sreceivefunction.
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-08 22:33:01
DSA needs to simulate master tracking events when a binding is first
with a DSA master established and torn down, in order to give drivers
the simplifying guarantee that ->master_up and ->master_going_down calls
are made in exactly this order. To avoid races, we need to block the
reception of NETDEV_UP/NETDEV_GOING_DOWN events in the netdev notifier
chain while we are changing the master's dev->dsa_ptr (this changes what
netdev_uses_dsa(dev) reports).
The dsa_master_setup() and dsa_master_teardown() functions optionally
require the rtnl_mutex to be held, if the tagger needs the master to be
promiscuous, these functions call dev_set_promiscuity(). Move the
rtnl_lock() from that function and make it top-level.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/dsa2.c | 8 ++++++++
net/dsa/master.c | 4 ++--
2 files changed, 10 insertions(+), 2 deletions(-)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-08 22:33:02
In order for switch driver to be able to make simple and reliable use of
the master tracking operations, they must also be notified of the
initial state of the DSA master, not just of the changes. This is
because they might enable certain features only during the time when
they know that the DSA master is up and running.
Therefore, this change explicitly checks the state of the DSA master
under the same rtnl_mutex as we were holding during the
dsa_master_setup() and dsa_master_teardown() call. The idea being that
if the DSA master became operational in between the moment in which it
became a DSA master (dsa_master_setup set dev->dsa_ptr) and the moment
when we checked for master->flags & IFF_UP, there is a chance that we
would emit a ->master_up() event twice. We need to avoid that by
serializing the concurrent netdevice event with us. If the netdevice
event started before, we force it to finish before we begin, because we
take rtnl_lock before making netdev_uses_dsa() return true. So we also
handle that early event and do nothing on it. Similarly, if the
dev_open() attempt is concurrent with us, it will attempt to take the
rtnl_mutex, but we're holding it. We'll see that the master flag IFF_UP
isn't set, then when we release the rtnl_mutex we'll process the
NETDEV_UP notifier.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/dsa2.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-12-09 03:06:05
On Thu, Dec 09, 2021 at 12:32:23AM +0200, Vladimir Oltean wrote:
This patch set is provided solely for review purposes (therefore not to
be applied anywhere) and for Ansuel to test whether they resolve the
slowdown reported here:
https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
It does conflict with net-next due to other patches that are in my tree,
and which were also posted here and would need to be picked ("Rework DSA
bridge TX forwarding offload API"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
Additionally, for Ansuel's work there is also a logical dependency with
this series ("Replace DSA dp->priv with tagger-owned storage"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
To get both dependency series, the following commands should be sufficient:
git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
where "git b4" is an alias in ~/.gitconfig:
[b4]
midmask = https://lore.kernel.org/r/%s
[alias]
b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
The patches posted here are mainly to offer a consistent
"master_up"/"master_going_down" chain of events to switches, without
duplicates, and always starting with "master_up" and ending with
"master_going_down". This way, drivers should know when they can perform
Ethernet-based register access.
Vladimir Oltean (7):
net: dsa: only bring down user ports assigned to a given DSA master
net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
function
net: dsa: use dsa_tree_for_each_user_port in
dsa_tree_master_going_down()
net: dsa: provide switch operations for tracking the master state
net: dsa: stop updating master MTU from master.c
net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
net: dsa: replay master state events in
dsa_tree_{setup,teardown}_master
include/net/dsa.h | 8 +++++++
net/dsa/dsa2.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--
net/dsa/dsa_priv.h | 11 ++++++++++
net/dsa/master.c | 29 +++-----------------------
net/dsa/slave.c | 32 +++++++++++++++-------------
net/dsa/switch.c | 29 ++++++++++++++++++++++++++
6 files changed, 118 insertions(+), 43 deletions(-)
--
2.25.1
I applied this patch and it does work correctly. Sadly the problem is
not solved and still the packet are not tracked correctly. What I notice
is that everything starts to work as soon as the master is set to
promiiscuous mode. Wonder if we should track that event instead of
simple up?
Here is a bootlog [0]. I added some log when the function timeouts and when
master up is actually called.
Current implementation for this is just a bool that is set to true on
master up and false on master going down. (final version should use
locking to check if an Ethernet transation is in progress)
[0] https://pastebin.com/7w2kgG7a
--
Ansuel
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-09 14:28:34
On Thu, Dec 09, 2021 at 04:05:59AM +0100, Ansuel Smith wrote:
On Thu, Dec 09, 2021 at 12:32:23AM +0200, Vladimir Oltean wrote:
quoted
This patch set is provided solely for review purposes (therefore not to
be applied anywhere) and for Ansuel to test whether they resolve the
slowdown reported here:
https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
It does conflict with net-next due to other patches that are in my tree,
and which were also posted here and would need to be picked ("Rework DSA
bridge TX forwarding offload API"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
Additionally, for Ansuel's work there is also a logical dependency with
this series ("Replace DSA dp->priv with tagger-owned storage"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
To get both dependency series, the following commands should be sufficient:
git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
where "git b4" is an alias in ~/.gitconfig:
[b4]
midmask = https://lore.kernel.org/r/%25s
[alias]
b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
The patches posted here are mainly to offer a consistent
"master_up"/"master_going_down" chain of events to switches, without
duplicates, and always starting with "master_up" and ending with
"master_going_down". This way, drivers should know when they can perform
Ethernet-based register access.
Vladimir Oltean (7):
net: dsa: only bring down user ports assigned to a given DSA master
net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
function
net: dsa: use dsa_tree_for_each_user_port in
dsa_tree_master_going_down()
net: dsa: provide switch operations for tracking the master state
net: dsa: stop updating master MTU from master.c
net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
net: dsa: replay master state events in
dsa_tree_{setup,teardown}_master
include/net/dsa.h | 8 +++++++
net/dsa/dsa2.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--
net/dsa/dsa_priv.h | 11 ++++++++++
net/dsa/master.c | 29 +++-----------------------
net/dsa/slave.c | 32 +++++++++++++++-------------
net/dsa/switch.c | 29 ++++++++++++++++++++++++++
6 files changed, 118 insertions(+), 43 deletions(-)
--
2.25.1
I applied this patch and it does work correctly. Sadly the problem is
not solved and still the packet are not tracked correctly. What I notice
is that everything starts to work as soon as the master is set to
promiiscuous mode. Wonder if we should track that event instead of
simple up?
Here is a bootlog [0]. I added some log when the function timeouts and when
master up is actually called.
Current implementation for this is just a bool that is set to true on
master up and false on master going down. (final version should use
locking to check if an Ethernet transation is in progress)
[0] https://pastebin.com/7w2kgG7a
This is strange. What MAC DA do the ack packets have? Could you give us
a pcap with the request and reply packets (not necessarily now)?
Can you try to set ".promisc_on_master = true" in qca_netdev_ops?
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-12-09 14:45:27
On Thu, Dec 09, 2021 at 02:28:30PM +0000, Vladimir Oltean wrote:
On Thu, Dec 09, 2021 at 04:05:59AM +0100, Ansuel Smith wrote:
quoted
On Thu, Dec 09, 2021 at 12:32:23AM +0200, Vladimir Oltean wrote:
quoted
This patch set is provided solely for review purposes (therefore not to
be applied anywhere) and for Ansuel to test whether they resolve the
slowdown reported here:
https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
It does conflict with net-next due to other patches that are in my tree,
and which were also posted here and would need to be picked ("Rework DSA
bridge TX forwarding offload API"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
Additionally, for Ansuel's work there is also a logical dependency with
this series ("Replace DSA dp->priv with tagger-owned storage"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
To get both dependency series, the following commands should be sufficient:
git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
where "git b4" is an alias in ~/.gitconfig:
[b4]
midmask = https://lore.kernel.org/r/%25s
[alias]
b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
The patches posted here are mainly to offer a consistent
"master_up"/"master_going_down" chain of events to switches, without
duplicates, and always starting with "master_up" and ending with
"master_going_down". This way, drivers should know when they can perform
Ethernet-based register access.
Vladimir Oltean (7):
net: dsa: only bring down user ports assigned to a given DSA master
net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
function
net: dsa: use dsa_tree_for_each_user_port in
dsa_tree_master_going_down()
net: dsa: provide switch operations for tracking the master state
net: dsa: stop updating master MTU from master.c
net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
net: dsa: replay master state events in
dsa_tree_{setup,teardown}_master
include/net/dsa.h | 8 +++++++
net/dsa/dsa2.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--
net/dsa/dsa_priv.h | 11 ++++++++++
net/dsa/master.c | 29 +++-----------------------
net/dsa/slave.c | 32 +++++++++++++++-------------
net/dsa/switch.c | 29 ++++++++++++++++++++++++++
6 files changed, 118 insertions(+), 43 deletions(-)
--
2.25.1
I applied this patch and it does work correctly. Sadly the problem is
not solved and still the packet are not tracked correctly. What I notice
is that everything starts to work as soon as the master is set to
promiiscuous mode. Wonder if we should track that event instead of
simple up?
Here is a bootlog [0]. I added some log when the function timeouts and when
master up is actually called.
Current implementation for this is just a bool that is set to true on
master up and false on master going down. (final version should use
locking to check if an Ethernet transation is in progress)
[0] https://pastebin.com/7w2kgG7a
This is strange. What MAC DA do the ack packets have? Could you give us
a pcap with the request and reply packets (not necessarily now)?
If you want I can give you a pcap from a router bootup to the setup with
no ethernet cable attached. I notice the switch sends some packet at the
bootup for some reason but they are not Ethernet mdio packet or other
type. It seems they are not even tagged (doesn't have qca tag) as the
header mode is disabled by default)
Let me know if you need just a pcap for the Ethernet mdio transaction or
from a bootup. I assume it would be better from a bootup? (they are not
tons of packet and the mdio Ethernet ones are easy to notice.)
Can you try to set ".promisc_on_master = true" in qca_netdev_ops?
I already tried and here [0] is a log. I notice with promisc_on_master
the "eth0 entered promiscuous mode" is missing. Is that correct?
Unless I was tired and misread the code, the info should be printed
anyway. Also looking at the comments for promisc_on_master I don't think
that should be applied to this tagger.
[0] https://pastebin.com/MN2ttVpr
--
Ansuel
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-09 17:33:30
On Thu, Dec 09, 2021 at 03:44:14PM +0100, Ansuel Smith wrote:
On Thu, Dec 09, 2021 at 02:28:30PM +0000, Vladimir Oltean wrote:
quoted
On Thu, Dec 09, 2021 at 04:05:59AM +0100, Ansuel Smith wrote:
quoted
On Thu, Dec 09, 2021 at 12:32:23AM +0200, Vladimir Oltean wrote:
quoted
This patch set is provided solely for review purposes (therefore not to
be applied anywhere) and for Ansuel to test whether they resolve the
slowdown reported here:
https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
It does conflict with net-next due to other patches that are in my tree,
and which were also posted here and would need to be picked ("Rework DSA
bridge TX forwarding offload API"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
Additionally, for Ansuel's work there is also a logical dependency with
this series ("Replace DSA dp->priv with tagger-owned storage"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
To get both dependency series, the following commands should be sufficient:
git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
where "git b4" is an alias in ~/.gitconfig:
[b4]
midmask = https://lore.kernel.org/r/%25s
[alias]
b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
The patches posted here are mainly to offer a consistent
"master_up"/"master_going_down" chain of events to switches, without
duplicates, and always starting with "master_up" and ending with
"master_going_down". This way, drivers should know when they can perform
Ethernet-based register access.
Vladimir Oltean (7):
net: dsa: only bring down user ports assigned to a given DSA master
net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
function
net: dsa: use dsa_tree_for_each_user_port in
dsa_tree_master_going_down()
net: dsa: provide switch operations for tracking the master state
net: dsa: stop updating master MTU from master.c
net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
net: dsa: replay master state events in
dsa_tree_{setup,teardown}_master
include/net/dsa.h | 8 +++++++
net/dsa/dsa2.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--
net/dsa/dsa_priv.h | 11 ++++++++++
net/dsa/master.c | 29 +++-----------------------
net/dsa/slave.c | 32 +++++++++++++++-------------
net/dsa/switch.c | 29 ++++++++++++++++++++++++++
6 files changed, 118 insertions(+), 43 deletions(-)
--
2.25.1
I applied this patch and it does work correctly. Sadly the problem is
not solved and still the packet are not tracked correctly. What I notice
is that everything starts to work as soon as the master is set to
promiiscuous mode. Wonder if we should track that event instead of
simple up?
Here is a bootlog [0]. I added some log when the function timeouts and when
master up is actually called.
Current implementation for this is just a bool that is set to true on
master up and false on master going down. (final version should use
locking to check if an Ethernet transation is in progress)
[0] https://pastebin.com/7w2kgG7a
This is strange. What MAC DA do the ack packets have? Could you give us
a pcap with the request and reply packets (not necessarily now)?
If you want I can give you a pcap from a router bootup to the setup with
no ethernet cable attached. I notice the switch sends some packet at the
bootup for some reason but they are not Ethernet mdio packet or other
type. It seems they are not even tagged (doesn't have qca tag) as the
header mode is disabled by default)
Let me know if you need just a pcap for the Ethernet mdio transaction or
from a bootup. I assume it would be better from a bootup? (they are not
tons of packet and the mdio Ethernet ones are easy to notice.)
Anything that contains some request and response packets should do, as
long as they're relatively easy to spot. But as stated, this can wait
for a while, I don't think that promiscuity is the issue, after your
second reply.
quoted
Can you try to set ".promisc_on_master = true" in qca_netdev_ops?
I already tried and here [0] is a log. I notice with promisc_on_master
the "eth0 entered promiscuous mode" is missing. Is that correct?
Unless I was tired and misread the code, the info should be printed
anyway. Also looking at the comments for promisc_on_master I don't think
that should be applied to this tagger.
[0] https://pastebin.com/MN2ttVpr
It isn't missing, it's right there on line 11.
I think the problem is that we also need to track the operstate of the
master (netif_oper_up via NETDEV_CHANGE) before declaring it as good to go.
You can see that this is exactly the line after which the timeouts disappear:
[ 7.146901] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
I didn't really want to go there, because now I'm not sure how to
synthesize the information for the switch drivers to consume it.
Anyway I've prepared a v2 patchset and I'll send it out very soon.
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-12-09 17:47:46
On Thu, Dec 09, 2021 at 05:33:17PM +0000, Vladimir Oltean wrote:
On Thu, Dec 09, 2021 at 03:44:14PM +0100, Ansuel Smith wrote:
quoted
On Thu, Dec 09, 2021 at 02:28:30PM +0000, Vladimir Oltean wrote:
quoted
On Thu, Dec 09, 2021 at 04:05:59AM +0100, Ansuel Smith wrote:
quoted
On Thu, Dec 09, 2021 at 12:32:23AM +0200, Vladimir Oltean wrote:
quoted
This patch set is provided solely for review purposes (therefore not to
be applied anywhere) and for Ansuel to test whether they resolve the
slowdown reported here:
https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
It does conflict with net-next due to other patches that are in my tree,
and which were also posted here and would need to be picked ("Rework DSA
bridge TX forwarding offload API"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
Additionally, for Ansuel's work there is also a logical dependency with
this series ("Replace DSA dp->priv with tagger-owned storage"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
To get both dependency series, the following commands should be sufficient:
git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
where "git b4" is an alias in ~/.gitconfig:
[b4]
midmask = https://lore.kernel.org/r/%25s
[alias]
b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
The patches posted here are mainly to offer a consistent
"master_up"/"master_going_down" chain of events to switches, without
duplicates, and always starting with "master_up" and ending with
"master_going_down". This way, drivers should know when they can perform
Ethernet-based register access.
Vladimir Oltean (7):
net: dsa: only bring down user ports assigned to a given DSA master
net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
function
net: dsa: use dsa_tree_for_each_user_port in
dsa_tree_master_going_down()
net: dsa: provide switch operations for tracking the master state
net: dsa: stop updating master MTU from master.c
net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
net: dsa: replay master state events in
dsa_tree_{setup,teardown}_master
include/net/dsa.h | 8 +++++++
net/dsa/dsa2.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--
net/dsa/dsa_priv.h | 11 ++++++++++
net/dsa/master.c | 29 +++-----------------------
net/dsa/slave.c | 32 +++++++++++++++-------------
net/dsa/switch.c | 29 ++++++++++++++++++++++++++
6 files changed, 118 insertions(+), 43 deletions(-)
--
2.25.1
I applied this patch and it does work correctly. Sadly the problem is
not solved and still the packet are not tracked correctly. What I notice
is that everything starts to work as soon as the master is set to
promiiscuous mode. Wonder if we should track that event instead of
simple up?
Here is a bootlog [0]. I added some log when the function timeouts and when
master up is actually called.
Current implementation for this is just a bool that is set to true on
master up and false on master going down. (final version should use
locking to check if an Ethernet transation is in progress)
[0] https://pastebin.com/7w2kgG7a
This is strange. What MAC DA do the ack packets have? Could you give us
a pcap with the request and reply packets (not necessarily now)?
If you want I can give you a pcap from a router bootup to the setup with
no ethernet cable attached. I notice the switch sends some packet at the
bootup for some reason but they are not Ethernet mdio packet or other
type. It seems they are not even tagged (doesn't have qca tag) as the
header mode is disabled by default)
Let me know if you need just a pcap for the Ethernet mdio transaction or
from a bootup. I assume it would be better from a bootup? (they are not
tons of packet and the mdio Ethernet ones are easy to notice.)
Anything that contains some request and response packets should do, as
long as they're relatively easy to spot. But as stated, this can wait
for a while, I don't think that promiscuity is the issue, after your
second reply.
Ok will send a pcap. Any preferred way to send it?
quoted
quoted
Can you try to set ".promisc_on_master = true" in qca_netdev_ops?
I already tried and here [0] is a log. I notice with promisc_on_master
the "eth0 entered promiscuous mode" is missing. Is that correct?
Unless I was tired and misread the code, the info should be printed
anyway. Also looking at the comments for promisc_on_master I don't think
that should be applied to this tagger.
[0] https://pastebin.com/MN2ttVpr
It isn't missing, it's right there on line 11.
Oww didn't notice that!
I think the problem is that we also need to track the operstate of the
master (netif_oper_up via NETDEV_CHANGE) before declaring it as good to go.
You can see that this is exactly the line after which the timeouts disappear:
[ 7.146901] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
I didn't really want to go there, because now I'm not sure how to
synthesize the information for the switch drivers to consume it.
Anyway I've prepared a v2 patchset and I'll send it out very soon.
Wonder if we should leave the driver decide when it's ready by parsing
the different state? (And change
the up ops to something like a generic change?)
--
Ansuel
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-12-09 17:56:22
On Thu, Dec 09, 2021 at 06:44:38PM +0100, Ansuel Smith wrote:
quoted
I think the problem is that we also need to track the operstate of the
master (netif_oper_up via NETDEV_CHANGE) before declaring it as good to go.
You can see that this is exactly the line after which the timeouts disappear:
[ 7.146901] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
I didn't really want to go there, because now I'm not sure how to
synthesize the information for the switch drivers to consume it.
Anyway I've prepared a v2 patchset and I'll send it out very soon.
Wonder if we should leave the driver decide when it's ready by parsing
the different state? (And change
the up ops to something like a generic change?)
There isn't just one state to track, which is precisely the problem that
I had to deal with for v2. The master is operational during the time
frame between NETDEV_UP and NETDEV_GOING_DOWN, intersected with the
interval during which netif_oper_up(master) is true. So in the simple
state propagation approach, DSA would need to provide at least two ops
to switches, one for admin state and the other for oper state. And the
switch driver would need to AND the two and keep state by itself.
Letting the driver make the decision would have been acceptable to me if
we could have 3 ops and a common implementation, something like this:
static void qca8k_master_state_change(struct dsa_switch *ds,
const struct dsa_master *master)
{
bool operational = (master->flags & IFF_UP) && netif_oper_up(master);
}
.master_admin_state_change = qca8k_master_state_change,
.master_oper_state_change = qca8k_master_state_change,
but the problem is that during NETDEV_GOING_DOWN, master->flags & IFF_UP
is still true, so this wouldn't work. And replacing the NETDEV_GOING_DOWN
notifier with the NETDEV_DOWN one would solve that problem, but it would
no longer guarantee that the switch can disable this feature without
timeouts before the master is down - because now it _is_ down.
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-12-09 18:16:58
On Thu, Dec 09, 2021 at 05:56:17PM +0000, Vladimir Oltean wrote:
On Thu, Dec 09, 2021 at 06:44:38PM +0100, Ansuel Smith wrote:
quoted
quoted
I think the problem is that we also need to track the operstate of the
master (netif_oper_up via NETDEV_CHANGE) before declaring it as good to go.
You can see that this is exactly the line after which the timeouts disappear:
[ 7.146901] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
I didn't really want to go there, because now I'm not sure how to
synthesize the information for the switch drivers to consume it.
Anyway I've prepared a v2 patchset and I'll send it out very soon.
Wonder if we should leave the driver decide when it's ready by parsing
the different state? (And change
the up ops to something like a generic change?)
There isn't just one state to track, which is precisely the problem that
I had to deal with for v2. The master is operational during the time
frame between NETDEV_UP and NETDEV_GOING_DOWN, intersected with the
interval during which netif_oper_up(master) is true. So in the simple
state propagation approach, DSA would need to provide at least two ops
to switches, one for admin state and the other for oper state. And the
switch driver would need to AND the two and keep state by itself.
Letting the driver make the decision would have been acceptable to me if
we could have 3 ops and a common implementation, something like this:
static void qca8k_master_state_change(struct dsa_switch *ds,
const struct dsa_master *master)
{
bool operational = (master->flags & IFF_UP) && netif_oper_up(master);
}
.master_admin_state_change = qca8k_master_state_change,
.master_oper_state_change = qca8k_master_state_change,
but the problem is that during NETDEV_GOING_DOWN, master->flags & IFF_UP
is still true, so this wouldn't work. And replacing the NETDEV_GOING_DOWN
notifier with the NETDEV_DOWN one would solve that problem, but it would
no longer guarantee that the switch can disable this feature without
timeouts before the master is down - because now it _is_ down.
Ok will have to test v2 and check if this is also fixed.
--
Ansuel
From: Ansuel Smith <ansuelsmth@gmail.com> Date: 2021-12-09 18:34:51
On Thu, Dec 09, 2021 at 05:57:20PM +0000, Vladimir Oltean wrote:
On Thu, Dec 09, 2021 at 06:44:38PM +0100, Ansuel Smith wrote:
quoted
Ok will send a pcap. Any preferred way to send it?
Email attachment should be fine.
This is a pcap done on the cpu port eth0.
All the packet with lenght 60 are the Ethernet mdio packet.
The capture is done with no cable connected so don't know why there are
some broadcast for ipv6.
Special care about the fact that the qca tag is always present in the
EtherType.
Mdio request qca tag 8181
mdio ack qca tag b887
(I should really investigate the extra packet... In theory they are not
sent (and i remember checking this) by the tagger as they have bit 5:4
set to 0x2... From Documentation these bit are reserved and in our code
we always set them to zero. The switch by itself sends broadcast tagged
packet and respond itself?)
I attached the pcap.
--
Ansuel