Date:   Fri, 10 Dec 2021 20:10:21 +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 07:04:48PM +0100, Ansuel Smith wrote:
> 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:
> > > 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
> 
> 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

Sorry for the triple message spam... I have a solution. It seems packet
are processed as soon as dev_activate is called (so a qdisk is assigned)
By adding another bool like master_oper_ready and

void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
                                      struct net_device *master,
                                      bool up);

static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
                                        struct net_device *master)
{
       struct dsa_notifier_master_state_info info;
       struct dsa_port *cpu_dp = master->dsa_ptr;

       info.master = master;
       info.operational = cpu_dp->master_admin_up && cpu_dp->master_oper_up && cpu_dp->master_oper_ready;

       dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
}

void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
                                      struct net_device *master,
                                      bool up)
{
       struct dsa_port *cpu_dp = master->dsa_ptr;
       bool notify = false;

       if ((cpu_dp->master_oper_ready && cpu_dp->master_oper_ready) !=
           (cpu_dp->master_oper_ready && up))
               notify = true;

       cpu_dp->master_oper_ready = up;

       if (notify)
               dsa_tree_master_state_change(dst, master);
}

In slave.c at the NETDEV_CHANGE event the additional
dsa_tree_master_oper_state_ready(dst, dev, dev_ingress_queue(dev));
we have no timeout function. I just tested this and it works right away.

Think we need this additional check to make sure the tagger can finally
accept packet from the switch.

With this added I think this is ready.

-- 
	Ansuel

Powered by blists - more mailing lists