Message-ID: <81273487-a450-4b28-abcc-c97273ca7b32@lunn.ch>
Date: Wed, 23 Jul 2025 18:24:20 +0200
From: Andrew Lunn <andrew@...n.ch>
To: MD Danish Anwar <danishanwar@...com>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, Jonathan Corbet <corbet@....net>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Mengyuan Lou <mengyuanlou@...-swift.com>,
	Michael Ellerman <mpe@...erman.id.au>,
	Madhavan Srinivasan <maddy@...ux.ibm.com>,
	Fan Gong <gongfan1@...wei.com>, Lee Trager <lee@...ger.us>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Lukas Bulwahn <lukas.bulwahn@...hat.com>,
	Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>,
	netdev@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 1/5] net: rpmsg-eth: Add Documentation for
 RPMSG-ETH Driver

> --- a/Documentation/networking/device_drivers/ethernet/index.rst
> +++ b/Documentation/networking/device_drivers/ethernet/index.rst
> @@ -61,6 +61,7 @@ Contents:
>     wangxun/txgbevf
>     wangxun/ngbe
>     wangxun/ngbevf
> +   rpmsg_eth

This list is sorted. Please insert at the right location. I made the
same comment to somebody else this week as well....

> +This driver is generic and can be used by any vendor. Vendors can develop their
> +own firmware for the remote processor to make it compatible with this driver.
> +The firmware must adhere to the shared memory layout, RPMSG communication
> +protocol, and data exchange requirements described in this documentation.

Could you add a link to TIs firmware? It would be a good reference
implementation. But i guess that needs to wait until the driver is
merged and the ABI is stable.

> +Implementation Details
> +----------------------
> +
> +- The magic number is defined as a macro in the driver source (e.g.,
> +  ``#define RPMSG_ETH_SHM_MAGIC_NUM 0xABCDABCD``).
> +- The firmware must write this value to the ``magic_num`` field of the head and
> +  tail structures in the shared memory region.
> +- During the handshake, the Linux driver reads these fields and compares them to
> +  the expected value. If any mismatch is detected, the driver will log an error
> +  and refuse to proceed.

So the firmware always takes the role of "primary" and Linux is
"secondary"? With the current implementation, you cannot have Linux on
both ends?

I don't see this as a problem, but maybe it is worth stating as a
current limitation.

> +Shared Memory Layout
> +====================
> +
> +The RPMSG Based Virtual Ethernet Driver uses a shared memory region to exchange
> +data between the host and the remote processor. The shared memory is divided
> +into transmit and receive regions, each with its own `head` and `tail` pointers
> +to track the buffer state.
> +
> +Shared Memory Parameters
> +------------------------
> +
> +The following parameters are exchanged between the host and the firmware to
> +configure the shared memory layout:

So the host tells the firmware this? Maybe this is explained later,
but is the flow something like:

Linux makes an RPC call to the firmware with the parameters you list
below. Upon receiving that RPC, the firmware puts the magic numbers in
place. It then ACKs the RPC call? Linux then checks the magic numbers?

> +1. **num_pkt_bufs**:
> +
> +   - The total number of packet buffers available in the shared memory.
> +   - This determines the maximum number of packets that can be stored in the
> +     shared memory at any given time.
> +
> +2. **buff_slot_size**:
> +
> +   - The size of each buffer slot in the shared memory.
> +   - This includes space for the packet length, metadata, and the actual packet
> +     data.
> +
> +3. **base_addr**:
> +
> +   - The base address of the shared memory region.
> +   - This is the starting point for accessing the shared memory.

So this is the base address in the Linux address space? How should the
firmware convert this into a base address in its address space?

> +4. **tx_offset**:
> +
> +   - The offset from the `base_addr` where the transmit buffers begin.
> +   - This is used by the host to write packets for transmission.
> +
> +5. **rx_offset**:
> +
> +   - The offset from the `base_addr` where the receive buffers begin.
> +   - This is used by the host to read packets received from the remote
> +     processor.

Maybe change 'host' to 'Linux'? Or some other name, 'primary' and
'secondary'. The naming should be consistent throughout the
documentation and driver.

Part of the issue here is that you pass this information from Linux to
the firmware. When the firmware receives it, it has the complete
opposite meaning. It uses "tx_offset" to receive packets, and
"rx_offset" to send packets. This can quickly get confusing. If you
used names like "linux_tx_offset", the added context with avoid
confusion.

> +Shared Memory Structure
> +-----------------------
> +
> +The shared memory layout is as follows:
> +
> +.. code-block:: text
> +
> +      Shared Memory Layout:
> +      ---------------------------
> +      |        MAGIC_NUM        |   rpmsg_eth_shm_head
> +      |          HEAD           |
> +      ---------------------------
> +      |        MAGIC_NUM        |
> +      |        PKT_1_LEN        |
> +      |          PKT_1          |
> +      ---------------------------
> +      |           ...           |
> +      ---------------------------
> +      |        MAGIC_NUM        |
> +      |          TAIL           |   rpmsg_eth_shm_tail
> +      ---------------------------
> +
> +1. **MAGIC_NUM**:
> +
> +   - A unique identifier used to validate the shared memory region.
> +   - Ensures that the memory region is correctly initialized and accessible.
> +
> +2. **HEAD Pointer**:
> +
> +   - Tracks the start of the buffer for packet transmission or reception.
> +   - Updated by the producer (host or remote processor) after writing a packet.

Is this a pointer, or an offset from the base address? Pointers get
messy when you have multiple address spaces involved. An offset is
simpler to work with. Given that the buffers are fixed size, it could
even be an index.

> +Information Exchanged Between RPMSG Channels
> +--------------------------------------------
> +
> +1. **Requests from Host to Remote Processor**:

Another place where consistent naming would be good. Here it is the
remote processor, not firmware used earlier.

> +
> +   - `RPMSG_ETH_REQ_SHM_INFO`: Request shared memory information, such as
> +     ``num_pkt_bufs``, ``buff_slot_size``, ``base_addr``, ``tx_offset``, and
> +     ``rx_offset``.

Is this requested, or telling? I suppose the text above uses "between"
which is ambiguous.

> +3. **Notifications from Remote Processor to Host**:
> +
> +   - `RPMSG_ETH_NOTIFY_PORT_UP`: Notify that the Ethernet port is up and ready
> +     for communication.
> +   - `RPMSG_ETH_NOTIFY_PORT_DOWN`: Notify that the Ethernet port is down.
> +   - `RPMSG_ETH_NOTIFY_PORT_READY`: Notify that the Ethernet port is ready for
> +     configuration.

That needs more explanation. Why would it not be ready? 

> +   - `RPMSG_ETH_NOTIFY_REMOTE_READY`: Notify that the remote processor is ready
> +     for communication.

How does this differ from PORT_READY?

> +How-To Guide for Vendors
> +========================
> +
> +This section provides a guide for vendors to develop firmware for the remote
> +processor that is compatible with the RPMSG Based Virtual Ethernet Driver.
> +
> +1. **Implement Shared Memory Layout**:
> +
> +   - Allocate a shared memory region for packet transmission and reception.
> +   - Initialize the `MAGIC_NUM`, `num_pkt_bufs`, `buff_slot_size`, `base_addr`,
> +     `tx_offset`, and `rx_offset`.
> +
> +2. **Magic Number Requirements**
> +
> +   - The firmware must write a unique magic number (for example, ``0xABCDABCD``)

Why "for example"? Do you have a use case where some other value
should be used? Or can we just make this magic value part of the
specification?

> +- The driver assumes a specific shared memory layout and may not work with other
> +  configurations.
> +- Multicast address filtering is limited to the capabilities of the underlying
> +  RPMSG framework.

I don't think there is anything special here. The network stack always
does perfect address filtering. The driver can help out, by also doing
perfect address filtering, or imperfect address filtering, and letting
more through than actually wanted. Or it can go into promiscuous mode.

> +- The driver currently supports only one transmit and one receive queue.
> +
> +References
> +==========
> +
> +- RPMSG Framework Documentation: https://www.kernel.org/doc/html/latest/rpmsg.html

This results in 404 Not Found.

     Andrew

Powered by blists - more mailing lists