Date:   Thu, 22 Sep 2022 14:45:34 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Mattias Forsblad <mattias.forsblad@...il.com>,
        netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux@...linux.org.uk,
        ansuelsmth@...il.com
Subject: Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add
 functionality to get RMON

On Thu, Sep 22, 2022 at 02:48:20PM +0300, Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 11:04:07PM +0200, Andrew Lunn wrote:
> > On Tue, Sep 20, 2022 at 04:10:53PM +0300, Vladimir Oltean wrote:
> > > On Tue, Sep 20, 2022 at 02:26:22PM +0200, Mattias Forsblad wrote:
> > > > This whole shebang was a suggestion from Andrew. I had a solution with
> > > > mv88e6xxx_rmu_available in mv88e6xxx_get_ethtool_stats which he wasn't fond of.
> > > > The mv88e6xxx_bus_ops is declared const and how am I to change the get_rmon
> > > > member? I'm not really sure on how to solve this in a better way?
> > > > Suggestions any? Maybe I've misunderstood his suggestion.
> > > 
> > > Can you point me to the beginning of that exact suggestion? I've removed
> > > everything older than v10 from my inbox, since the flow of patches was
> > > preventing me from seeing other emails.
> > 
> > What i want to do is avoid code like:
> > 
> >      if (have_rmu())
> >      	foo()
> >      else
> > 	bar()
> > 
> > There is nothing common in the MDIO MIB code and the RMU MIB code,
> > just the table of statistics. When we get to dumping the ATU, i also
> > expect there will be little in common between the MDIO and the RMU
> > functions.
> 
> Sorry, I don't understand what it is about "if (have_rmu()) foo() else bar()"
> that you don't like. Isn't the indirection via bus_ops the exact same
> thing, but expressed as an indirect function call rather than an if/else?

There was code like this deep inside the MIB code, which just looked
ugly. That is now gone.

> > Doing MIB via RMU is a big gain, but i would also like normal register
> > read and write to go via RMU, probably with some level of
> > combining. Multiple writes can be combined into one RMU operation
> > ending with a read. That should give us an mv88e6xxx_bus_ops which
> > does RMU, and we can swap the bootstrap MDIO bus_ops for the RMU
> > bus_ops.
> 
> At what level would the combining be done? I think the mv88e6xxx doesn't
> really make use of bulk operations, C45 MDIO reads with post-increment,
> that sort of thing. I could be wrong. And at some higher level, the
> register read/write code should not diverge (too much), even if the
> operation may be done over Ethernet or MDIO. So we need to find places
> which actually make useful sense of bulk reads.

I was thinking within mv88e6xxx_read() and mv88e6xxx_write(). Keep a
buffer for building requests. Each write call appends the write to the
buffer and returns 0. A read call gets appended to the buffer and then
executes the RMU. We probably also need to wrap the reg mutex, so that
when it is released, any buffered writes get executed. If the RMU
fails, we have all the information needed to do the same via MDIO.

What i was not aware of is that some registers are not supposed to be
accessed over RMU. I suppose we can make a list of them, and if there
is a read/write to such a register, execute the RMU and then perform
an MDIO operation for the restricted register.

> But then, Mattias' code structure becomes inadequate. Currently we
> serialize mv88e6xxx_master_state_change() with respect to bus accesses
> via mv88e6xxx_reg_lock(). But if we permit RMU to run in parallel with
> MDIO, we need a rwlock, such that multiple 'readers' of the conceptual
> have_rmu() function can run in parallel with each other, and just
> serialize with the RMU state changes (the 'writers').

I don't think we can allow RMU to run in parallel to MDIO. The reg
lock will probably prevent that anyway.

> 
> > I am assuming here that RMU is reliable. The QCA8K driver currently
> > falls back to MDIO if its inband function is attempted but fails.  I
> > want to stress this part, lots of data packets and see if the RMU
> > frames get dropped, or delayed too much causing failures.
> 
> I don't think you even have to stress it too much. Nothing prevents the
> user from putting a policer on the DSA master which will randomly drop
> responses. Or a shaper that will delay requests beyond the timeout.

That would be a self inflicted problem. But you are correct, we need
to fall back to MDIO.

This is one area we can experiment with. Maybe we can retry the
operation via RMU a few times? Two retries for MIBs is still going to
be a lot faster, if successful, compared to all the MDIO transactions
for all the statistics. We can also add some fall back tracking
logic. If RMU has failed for N times in a row, stop using it for 60
seconds, etc. That might be something we can put into the DSA core,
since it seems like a generic problem.

There is a lot of experimentation needed here, and it could be we need
to throw it all away and try again a few times until we have explored
the problem sufficiently to get it right...

    Andrew

Powered by blists - more mailing lists