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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.



Best regards,
Krzysztof