Re: [RFC PATCH v2 net-next 0/4] DSA master state tracking

On Fri, Dec 10, 2021 at 06:29:32PM +0100, Ansuel Smith wrote:

On Fri, Dec 10, 2021 at 05:15:30PM +0000, Vladimir Oltean wrote:

quoted

On Fri, Dec 10, 2021 at 06:10:45PM +0100, Ansuel Smith wrote:

quoted

On Fri, Dec 10, 2021 at 05:02:42PM +0000, Vladimir Oltean wrote:

quoted

On Fri, Dec 10, 2021 at 04:37:52AM +0100, Ansuel Smith wrote:

quoted

On Thu, Dec 09, 2021 at 07:39:23PM +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/

The patches posted here are mainly to offer a consistent
"master_state_change" chain of events to switches, without duplicates,
and always starting with operational=true and ending with
operational=false. This way, drivers should know when they can perform
Ethernet-based register access, and need not care about more than that.

Changes in v2:
- dropped some useless patches
- also check master operstate.

Vladimir Oltean (4):
  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  | 11 +++++++
 net/dsa/dsa2.c     | 80 +++++++++++++++++++++++++++++++++++++++++++---
 net/dsa/dsa_priv.h | 13 ++++++++
 net/dsa/master.c   | 29 ++---------------
 net/dsa/slave.c    | 27 ++++++++++++++++
 net/dsa/switch.c   | 15 +++++++++
 6 files changed, 145 insertions(+), 30 deletions(-)

-- 
2.25.1

Hi, I tested this v2 and I still have 2 ethernet mdio failing on init.
I don't think we have other way to track this. Am I wrong?

All works correctly with this and promisc_on_master.
If you have other test, feel free to send me other stuff to test.

(I'm starting to think the fail is caused by some delay that the switch
require to actually start accepting packet or from the reinit? But I'm
not sure... don't know if you notice something from the pcap)

I've opened the pcap just now. The Ethernet MDIO packets are
non-standard. When the DSA master receives them, it expects the first 6
octets to be the MAC DA, because that's the format of an Ethernet frame.
But the packets have this other format, according to your own writing:

/* Specific define for in-band MDIO read/write with Ethernet packet */
#define QCA_HDR_MDIO_SEQ_LEN           4 /* 4 byte for the seq */
#define QCA_HDR_MDIO_COMMAND_LEN       4 /* 4 byte for the command */
#define QCA_HDR_MDIO_DATA1_LEN         4 /* First 4 byte for the mdio data */
#define QCA_HDR_MDIO_HEADER_LEN        (QCA_HDR_MDIO_SEQ_LEN + \
                                       QCA_HDR_MDIO_COMMAND_LEN + \
                                       QCA_HDR_MDIO_DATA1_LEN)

#define QCA_HDR_MDIO_DATA2_LEN         12 /* Other 12 byte for the mdio data */
#define QCA_HDR_MDIO_PADDING_LEN       34 /* Padding to reach the min Ethernet packet */

The first 6 octets change like crazy in your pcap. Definitely can't add
that to the RX filter of the DSA master.

So yes, promisc_on_master is precisely what you need, it exists for
situations like this.

Considering this, I guess it works?

Yes it works! We can totally accept 2 mdio timeout out of a good way to
track the master port. It's probably related to other stuff like switch
delay or other.

Wonder the next step is wait for this to be accepted and then I can
propose a v3 of my patch? Or net-next is closed now and I should just
send v3 RFC saying it does depend on this?

Wait a minute, I don't think I understood your previous reply.
With promisc_on_master, is there or is there not any timeout?

With promisc_on_master I have only 2 timeout.

quoted

My understanding was this: DSA tells you when the master is up and
operational. That information is correct, except it isn't sufficient and
you don't see the replies back. Later during boot, you have some init
scripts triggered by user space that create a bridge interface and put
the switch ports under the bridge. The bridge puts the switch interfaces
in promiscuous mode, because that's what bridges do. Then DSA propagates
the promiscuous mode from the switch ports to the DSA master, and once
the master is promiscuous, the Ethernet MDIO starts working too.
Now, with promisc_on_master set, the DSA master is already promiscuous
by the time DSA tells you that it's up and running. Hence your message
that "All works correctly with this and promisc_on_master."
What did I misunderstand?

You got all correct. But still I have these 2 timeout at the very start.
Let me give you another pastebin to make this more clear. [0]
Transaction done is when the Ethernet packet is received and processed.
I added some pr with the events received by switch.c

I should check if the tagger receive some packet before the
"function timeout". 
What I mean with "acceptable state" is that aside from the 2
timeout everything else works correctly withtout any slowdown in the
init process.

[0] https://pastebin.com/VfGB5hAQ

-- 
	Ansuel

Ok I added more tracing and packet are received to the tagger right
after the log from ipv6 "link becomes ready". That log just check if the
interface is up and if it does have a valid sched.
I notice after link becomes ready we have a CHANGE event for eth0. That
should be the correct way to understand when the cpu port is actually
usable.
(just to make it clear before the link becomes ready no packet is
received to the tagger and the completion timeouts)

-- 
	Ansuel