Re: [PATCH net-next 2/5] net: rpmsg-eth: Add basic rpmsg skeleton

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

On 28/07/25 6:10 pm, Krzysztof Kozlowski wrote:
> On 28/07/2025 10:10, MD Danish Anwar wrote:
>> Hi Krzysztof,
>>
>> On 25/07/25 12:48 am, Krzysztof Kozlowski wrote:
>>> On 23/07/2025 10:03, MD Danish Anwar wrote:
>>>> This patch introduces a basic RPMSG Ethernet driver skeleton. It adds
>>>
>>> Please do not use "This commit/patch/change", but imperative mood. See
>>> longer explanation here:
>>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>>
>>
>> Sure. I will fix this in v2.
>>
>>>> support for creating virtual Ethernet devices over RPMSG channels,
>>>> allowing user-space programs to send and receive messages using a
>>>> standard Ethernet protocol. The driver includes message handling,
>>>> probe, and remove functions, along with necessary data structures.
>>>>
>>>
>>>
>>> ...
>>>
>>>> +
>>>> +/**
>>>> + * rpmsg_eth_get_shm_info - Get shared memory info from device tree
>>>> + * @common: Pointer to rpmsg_eth_common structure
>>>> + *
>>>> + * Return: 0 on success, negative error code on failure
>>>> + */
>>>> +static int rpmsg_eth_get_shm_info(struct rpmsg_eth_common *common)
>>>> +{
>>>> +	struct device_node *peer;
>>>> +	const __be32 *reg;
>>>> +	u64 start_address;
>>>> +	int prop_size;
>>>> +	int reg_len;
>>>> +	u64 size;
>>>> +
>>>> +	peer = of_find_node_by_name(NULL, "virtual-eth-shm");
>>>
>>>
>>> This is new ABI and I do not see earlier patch documenting it.
>>>
>>> You cannot add undocumented ABI... but even if you documented it, I am
>>> sorry, but I am pretty sure it is wrong. Why are you choosing random
>>> nodes just because their name by pure coincidence is "virtual-eth-shm"?
>>> I cannot name my ethernet like that?
>>>
>>
>> This series adds a new virtual ethernet driver. The tx / rx happens in a
>> shared memory block. I need to have a way for the driver to know what is
>> the address / size of this block. This driver can be used by any
>> vendors. The vendors can create a new node in their dt and specify the
>> base address / size of the shared memory block.
>>
>> I wanted to keep the name of the node constant so that the driver can
>> just look for this name and then grab the address and size.
> 
> You should not.
> 
>>
>> I can create a new binding file for this but I didn't create thinking
>> it's a virtual device not a physical and I wasn't sure if bindings can
>> be created for virtual devices.
> 
> So you added undocumented ABI intentionally, sorry, that's a no go.
> 
>>
>> In my use case, I am reserving this shared memory and during reserving I
>> named the node "virtual-eth-shm". The memory is reserved by the
>> ti_k3_r5_remoteproc.c driver. The DT change is not part of this series
>> but can be found
>> https://gist.github.com/danish-ti/cdd10525ad834fdb20871ab411ff94fb
>>
>> The idea is any vendor who want to use this driver, should name their dt
>> node as "virtual-eth-shm" (if they also need to reserve the memory) so
>> that the driver can take the address from DT and use it for tx / rx.
>>
>> If this is not the correct way, can you please let me know of some other
>> way to handle this.
>>
>> One idea I had was to create a new binding for this node, and use
>> compatible string to access the node in driver. But the device is
>> virtual and not physical so I thought that might not be the way to go so
>> I went with the current approach.
> 
> virtual devices do not go to DTS anyway. How do you imagine this works?
> You add it to DTS but you do not add bindings and you expect checks to
> succeed?
> 
> Provide details how you checked your DTS compliance.
> 
> 

This is my device tree patch [1]. I ran these two commands before and
after applying the patch and checked the diff.

	make dt_binding_check
	make dtbs_check

I didn't see any new error / warning getting introduced due to the patch

After applying the patch I also ran,

	make CHECK_DTBS=y ti/k3-am642-evm.dtb

I still don't see any warnings / error.


If you look at the DT patch, you'll see I am adding a new node in the
`reserved-memory`. I am not creating a completely new undocumented node.
Instead I am creating a new node under reserved-memory as the shared
memory used by rpmsg-eth driver needs to be reserved first. This memory
is reserved by the ti_k3_r5_remoteproc driver by k3_reserved_mem_init().

It's just that I am naming this node as "virtual-eth-shm@a0400000" and
then using the same name in driver to get the base_address and size
mentioned in this node.


> 
> Best regards,
> Krzysztof


[1]
https://gist.githubusercontent.com/danish-ti/fd3e630227ae5b165e12eabd91b0dc9d/raw/67d7c15cd1c47a29c0cfd3674d7cd6233ef1bea5/0001-arch-arm64-dts-k3-am64-Add-shared-memory-node.patch

-- 
Thanks and Regards,
Danish