Date:   Thu, 22 Sep 2022 20:09:06 +0200
From:   Christian Marangi <ansuelsmth@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev <netdev@...r.kernel.org>, mattias.forsblad@...il.com,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH rfc v2 00/10] DSA: Move parts of inband signalling into
 the DSA

On Thu, Sep 22, 2022 at 07:58:11PM +0200, Andrew Lunn wrote:
> This is an RFC patchset.
> 
> Mattias Forsblad proposal for adding some core helpers to DSA for
> inband signalling is going in a good direction, but there are a couple
> of things which i think can be better. This patchset offs an
> alternative to
> 
> patch 2/7: net: dsa: Add convenience functions for frame handling
> 
> and
> 
> patch 7/7 net: dsa: qca8k: Use new convenience functions
> 
> This patchset takes the abstraction further, putting more into the
> core. It also makes the qca8k fully use the abstraction unlike 7/7.
> 
> The end result has a slightly different structure, in that there is a
> struct dsa_inband of which qca8k has two instances of this. Doing this
> avoids the custom completion code. If qca8k can have multiple parallel
> request/replies in flight, it seems likely other devices can as well,
> so this should be part of the abstraction.
> 
> Since i don't have the qck8 hardware, i hope that lots of small
> patches make the review work easier, and finding the introduced bugs
> is quicker.
> 
> The MIB handling of the qc8k is somewhat odd. It would be nice to work
> on that further and try to make it better fit the model used
> here. That work can be done later, and probably is more invasive than
> the step by step approach taken here.
> 
> Another aim has been to make it easy to merge Mattias mv88e6xxx
> patches with this patchset. The basic API is the same, so i think it
> should be possible.
> 
> These are compile tested only....
> 
> This version addresses all the comments on the previous version except
> for:
> 
> Making the inband data structure short lived, allocated per request.
> This needs some more though.

Considering a low power system this can be problematic. Tagging already
cause perf regression, allocating the struct on top of the skb
allocation can cause even more perf regression if inband is also used
for port state polling.

> 
> Siliently truncating the reply when it is bigger than the response
> buffer.

This can be problematic and also silent truncation is asking for bugs
IMHO. These kind of special packet handling are well defined so anything
that diverge from the implementation is probably a corrupted request.

> 
> Adding documentation to dsa.rst.
> 
> Also, it is not known if the crash reported in the last patch is fixed
> or not.

Will test this and refer back to the replated patch if the crash is
still there.

> 
> This code can also be found in
> 
> https://github.com/lunn/linux v6.0-rc4-net-next-inband
> 
> Andrew Lunn (10):
>   net: dsa: qca8k: Fix inconsistent use of jiffies vs milliseconds
>   net: dsa: qca8k: Move completion into DSA core
>   net: dsa: qca8K: Move queuing for request frame into the core
>   net: dsa: qca8k: dsa_inband_request: More normal return values
>   net: dsa: qca8k: Drop replies with wrong sequence numbers
>   net: dsa: qca8k: Move request sequence number handling into core
>   net: dsa: qca8k: Refactor sequence number mismatch to use error code
>   net: dsa: qca8k: Pass error code from reply decoder to requester
>   net: dsa: qca8k: Pass response buffer via dsa_rmu_request
>   net: dsa: qca8: Move inband mutex into DSA core
> 
>  drivers/net/dsa/qca/qca8k-8xxx.c | 237 ++++++++-----------------------
>  drivers/net/dsa/qca/qca8k.h      |   8 +-
>  include/net/dsa.h                |  33 +++++
>  net/dsa/dsa.c                    |  89 ++++++++++++
>  4 files changed, 183 insertions(+), 184 deletions(-)
> 
> -- 
> 2.37.2
> 

-- 
	Ansuel

Powered by blists - more mailing lists