Message-ID: <61b38e7f.1c69fb81.96d1c.7933@mx.google.com>
Date:   Fri, 10 Dec 2021 18:29:32 +0100
From:   Ansuel Smith <ansuelsmth@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [RFC PATCH v2 net-next 0/4] DSA master state tracking

On Fri, Dec 10, 2021 at 05:15:30PM +0000, Vladimir Oltean wrote:
> On Fri, Dec 10, 2021 at 06:10:45PM +0100, Ansuel Smith wrote:
> > On Fri, Dec 10, 2021 at 05:02:42PM +0000, Vladimir Oltean wrote:
> > > On Fri, Dec 10, 2021 at 04:37:52AM +0100, Ansuel Smith wrote:
> > > > On Thu, Dec 09, 2021 at 07:39:23PM +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/
> > > > > 
> > > > > 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.

> 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

Powered by blists - more mailing lists