Re: [PATCH net-next 1/5] net: rpmsg-eth: Add Documentation for RPMSG-ETH Driver

[Andrew Lunn <andrew@lunn.ch>]

> Linux first send a rpmsg request with msg type = RPMSG_ETH_REQ_SHM_INFO
> i.e. requesting for the shared memory info.
> 
> Once firmware recieves this request it sends response with below fields,
> 
> 	num_pkt_bufs, buff_slot_size, base_addr, tx_offset, rx_offset
> 
> In the device tree, while reserving the shared memory for rpmsg_eth
> driver, the base address and the size of the shared memory block is
> mentioned. I have mentioned that in the documentation as well

If it is in device tree, why should Linux ask for the base address and
length? That just seems like a source of errors, and added complexity.

In general, we just trust DT. It is a source of truth. So i would
delete all this backwards and forwards and just use the values from
DT. Just check the magic numbers are in place.

> The same `base_addr` is used by firmware for the shared memory. During
> the rpmsg callback, firmware shares this `base_addr` and during
> rpmsg_eth_validate_handshake() driver checks if the base_addr shared by
> firmware is same as the one described in DT or not. Driver only proceeds
> if it's same.

So there is a big assumption here. That both are sharing the same MMU,
or maybe IOMMU. Or both CPUs have configured their MMU/IOMMU so that
the pages appear at the same physical address. I think this is a
problem, and the design should avoid anything which makes this
assumptions. The data structures within the share memory should only
refer to offsets from the base of the shared memory, not absolute
values. Or an index into the table of buffers, 0..N.

> >> +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.
> > 
> 
> Below are the structure definitions.
> 
> struct rpmsg_eth_shared_mem {
> 	struct rpmsg_eth_shm_index *head;
> 	struct rpmsg_eth_shm_buf *buf;
> 	struct rpmsg_eth_shm_index *tail;
> } __packed;
> 
> struct rpmsg_eth_shm_index {
> 	u32 magic_num;
> 	u32 index;
> }  __packed;

So index is the index into the array of fixed size buffers. That is
fine, it is not a pointer, so you don't need to worry about address
spaces. However, head and tail are pointers, so for those you do need
to worry about address spaces. But why do you even need them? Just put
the indexes directly into rpmsg_eth_shared_mem. The four index values
can be in the first few words of the shared memory, fixed offset from
the beginning, KISS.

	Andrew