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:

quoted

quoted

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