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

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <d856807f-dead-4e93-bef6-0d25744cc041@ti.com>
Date: Fri, 25 Jul 2025 12:34:53 +0530
From: "Anwar, Md Danish" <a0501179@...com>
To: Andrew Lunn <andrew@...n.ch>, 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

Hi Andrew,

On 7/24/2025 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 not check the base_addr and trust the info we get from
device tree. Just check the offsets we are getting from firmware is
within the shared memory block or not (using base_addr + size)

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

Sure I will try to do the same.

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

Sure I will try to move everything to offsets and not use pointers.

> 	Andrew

-- 
Thanks and Regards,
Md Danish Anwar