netdev - Re: [PATCH rfc v2 08/10] net: dsa: qca8k: Pass error code from reply decoder to requester

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Yy8h/+Fp0u5HQyzV@lunn.ch>
Date:   Sat, 24 Sep 2022 17:27:59 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev <netdev@...r.kernel.org>,
        "mattias.forsblad@...il.com" <mattias.forsblad@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Christian Marangi <ansuelsmth@...il.com>
Subject: Re: [PATCH rfc v2 08/10] net: dsa: qca8k: Pass error code from reply
 decoder to requester

On Sat, Sep 24, 2022 at 02:42:50PM +0000, Vladimir Oltean wrote:
> On Sat, Sep 24, 2022 at 04:32:15PM +0200, Andrew Lunn wrote:
> > > My understanding of the autocast function (I could be wrong) is that
> > > it's essentially one request with 10 (or how many ports there are)
> > > responses. At least this is what the code appears to handle.
> > 
> > The autocast packet handling does not fit the model. I already
> > excluded it from parts of the patchset. I might need to exclude it
> > from more. It is something i need to understand more. I did find a
> > leaked data sheet for the qca8337, but i've not had time to read it
> > yet.
> > 
> > Either the model needs to change a bit, or we don't convert this part
> > of the code to use shared functions, or maybe we can do a different
> > implementation in the driver for statistics access.
> 
> I was thinking, as a complement to your series, maybe we could make the
> response processing also generic (so instead of the tagger calling a
> driver-provided handler, let it call a dsa_inband_response() instead).

I agree i've mostly looked at the dsa_inband_request side. Not having
any hardware means i've been trying to keep the patches simple,
obviously, and bug free. It seems i've failed at this last part.

I hope we can iterate the dsa_inband_response() afterwards. My
patchset is a good size on its own, so maybe we should get that merged
first, before starting on dsa_inband_response(). I would also like to
get some basic mv88e6xxx code merged, so it gives me a test system.

> This would look through the list of queued ds->requests, and have a
> (*match_cb)() which returns an action, be it "packet doesn't match this
> request", "packet matches, please remove request from list", or "packet
> matches, but still keep request in list". In addition, the queued
> request will also have a (*cb)(), which is the action to execute on
> match from a response. The idea is that if we bother to provide a
> generic implementation within DSA, at least we could try to make its
> core async, and just offer sychronous wrappers if this is what drivers
> wish to use (like a generic cb() which calls complete()).

The autocast is just different. What i don't know is, does any other
switch have anything similar? I guess every switch with inband is
going to have Request/reply, so i think we should concentrate on that.
Another thing we need to decide is, do we want to allow multiple in
flight request/reply. If we say No, all need is to be able to
determine is if a reply is late reply we should discard, or the reply
to the current request. That is much simpler than a generic match the
reply to a list of requests. For the moment, i would prefer KISS, lets
get something working for two devices. We can make it more complex
later.

    Andrew