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

Andrew Lunn
2022-08-29T12:53:51Z

> > 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