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

[MD Danish Anwar <danishanwar@ti.com>]

On 24/07/25 10:07 pm, Andrew Lunn wrote:
>> 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.
> 

Sure. I will make this change in v2.

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

I will drop all these pointers and use a offset based approach in v2.

Thanks for the feedback.

-- 
Thanks and Regards,
Danish