netdev - Re: [PATCH net-next v2 1/3] dsa: Implement RMU layer in DSA

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Ywy0RrY0Ih3lBBxx@lunn.ch>
Date:   Mon, 29 Aug 2022 14:42:46 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Mattias Forsblad <mattias.forsblad@...il.com>
Cc:     netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v2 1/3] dsa: Implement RMU layer in DSA

> > Hi Mattias
> > 
> > Vladimer pointed you towards the qca driver, in a comment for your
> > RFC. qca already has support for switch commands via Ethernet frames.
> > 
> > The point he was trying to make is that you should look at that
> > code. The concept of executing a command via an Ethernet frame, and
> > expecting a reply via an Ethernet frame is generic. The format of
> > those frames is specific to the switch. We want the generic parts to
> > look the same for all switches. If possible, we want to implement it
> > once in the dsa core, so all switch drivers share it. Less code,
> > better tested code, less bugs, easier maintenance.
> > 
> > Take a look at qca_tagger_data. Please use the same mechanism with
> 
> This I can do which makes sense.
> 
> > mv88e6xxx. But also look deeper. What else can be shared? You need a
> 
> I can also make a generic dsa_eth_tx_timeout routine which
> handles the sending and receiving of cmd frames.
> 
> > buffer to put the request in, you need to send it, you need to wait
> 
> The skb is the buffer and it's up to the driver to decode it properly?

Yes, the tagger layer passes the skb, which contains the complete
Ethernet frame, plus additional meta data. But you need to watch out
for the life cycle of that skb:

        /* Ethernet mgmt read/write packet */
        if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK) {
                if (likely(tagger_data->rw_reg_ack_handler))
                        tagger_data->rw_reg_ack_handler(ds, skb);
                return NULL;
        }

returning NULL means the DSA core will free the skb when the call to
qca_tag_rcv() returns. So either you need to take a copy of the data,
clone the skb, or change this code somehow. See what makes most sense
for generic code.


> I've looked into the qca driver and that uses a small static buffer
> for replies, no buffer lifetime cycle.
> 
> > for the reply, you need to pass the results to the driver, you need to
> > free that buffer afterwards. That should all be common. Look at these
> > parts in the qca driver. Can you make them generic, move them into the
> > DSA core? Are there other parts which could be shared?
> 
> I cannot change the qca code as I have no way of verifying that the
> resulting code works.

You can change it. You can at least compile test your change. The QCA
developers are also quite active, and so will probably help out
testing whatever you change. And ideally, you want lots of simple,
obviously correct changes, which i can review and say look correct.

Changing code which you cannot test is very normal in Linux, do your
best, and ask for testing.

      Andrew