Re: [PATCH v8 11/13] rpmsg: char: Introduce the "rpmsg-raw" channel

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

On 1/18/22 3:36 PM, Bjorn Andersson wrote:
> On Tue 18 Jan 05:04 CST 2022, Arnaud POULIQUEN wrote:
> 
>>
>>
>> On 1/18/22 12:03 AM, Bjorn Andersson wrote:
>>> On Tue 07 Dec 02:08 CST 2021, Arnaud Pouliquen wrote:
>>>
>>>> Allows to probe the endpoint device on a remote name service announcement,
>>>> by registering a rpmsg_driverfor the "rpmsg-raw" channel.
> 
> Thought of this after replying yesterday, I really would like this to be
> updated to include an explanation of _why_ this is a good thing. What is
> the use case etc.

Not sure to understand your point here. Do you mean that "Allows to probe the
endpoint device on a remote name service announcement" is not a sufficient
reason?
Would the following explanation address your concern?

"
For the rpmsg virtio backend, the current implementation of the rpmsg char
only allows to instantiate static(i.e. prefixed source and destination addresses)
end points, and only on the Linux user space initiative.

This patch defines the the "rpmsg-raw" channel and register it to the rpmsg bus.
This registration allows:
- To create the channel at the initiative of the remote proc initiative relying 
on the name service announcement mechanism.
- To use the channel object instead of the endpoint, thus preventing the user 
space from having the knowledge of the remote processor's endpoint addresses.
- To rely on udev to be inform when a /dev/rpmsgX is created on remote processor 
request, indicating that the remote processor is ready to communicate.
"

> 
>>>>
>>>> With this patch the /dev/rpmsgX interface can be instantiated by the remote
>>>> firmware.
> 
> It would be nice if this mentioned why you can rely on udev events and
> rpmsgexport.
> 

By rpmsgexport you mean the tool available on your github to create a endpoint?
Could you details what you have in mind? I don't see a relationship with
this patch.

>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> ---
>>>>    drivers/rpmsg/rpmsg_char.c | 64 ++++++++++++++++++++++++++++++++++++++
>>>>    drivers/rpmsg/rpmsg_ctrl.c |  7 +++--
>>>>    2 files changed, 69 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>>> index cf97839f5833..92b44630e03a 100644
>>>> --- a/drivers/rpmsg/rpmsg_char.c
>>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>>> @@ -435,6 +435,58 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
>>>>    }
>>>>    EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>>>> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>>> +{
>>>> +	struct rpmsg_channel_info chinfo;
>>>> +	struct rpmsg_eptdev *eptdev;
>>>> +	struct device *dev = &rpdev->dev;
>>>> +
>>>> +	memcpy(chinfo.name, rpdev->id.name, RPMSG_NAME_SIZE);
>>>> +	chinfo.src = rpdev->src;
>>>> +	chinfo.dst = rpdev->dst;
>>>> +
>>>> +	eptdev = rpmsg_chrdev_eptdev_alloc(rpdev, dev);
>>>> +	if (IS_ERR(eptdev))
>>>> +		return PTR_ERR(eptdev);
>>>> +
>>>> +	/*
>>>> +	 * Create the default endpoint associated to the rpmsg device and provide rpmsg_eptdev
>>>> +	 * structure as callback private data.
>>>
>>> If the only this the probe function does is to create a new endpoint
>>> with the same properties as the rpdev, why can't you just specify a
>>> callback on the rpmsg_chrdev_driver?
>>>
>>> As this isn't the typical way you create a default endpoint I think the
>>> reasoning behind this warrants a proper explanation in the commit
>>> message.
>>>
>>
>> As mentioned in [PATCH v8 09/13] rpmsg: Introduce rpmsg_create_default_ept function
>> "This helper function allows rpmsg drivers to create a default endpoint
>> on runtime with an associated private context."
>>
>> Here the private context is the eptdev structure. I need to create the
>> structure first, before
>> the endpoint.
>> I will add more details in the commit message as you suggest.
> 
> Okay, I think the important part to document is _why_ this has to happen
> in this order - in particular since I suspect that this reason might not
> be unique to this driver.

Right.
In the rpmsg-client-sample implementation [1], the endpoint callback uses the 
dev_get_drvdata to retrieve the device context.
If the dev->driver_data is used for some other purpose (this is the case for the 
rpmsg char) you need to use the "*priv" parameter.

The rpmsg-core seems expecting implementation based dev->driver_data use, when 
use rpmsg bus probing mechanism.

[1] 
https://elixir.bootlin.com/linux/latest/source/samples/rpmsg/rpmsg_client_sample.c#L29

> 
>>
>> An alternative could be to directly set default_ept->priv but as the
>> rpmsg_create_default_ept
>> "priv" parameter is forwarded to the rpmsg backend (using create_ept ops).
>> This could introduces unexpected side effect.
>>
> 
> I'm not sure I understand the unexpected side effect of reassigning priv
> here.

I have no concrete use case in mind just that the priv parameter is provided to
the rpmsg backend. My assumption is that, calling rpmsg_create_ept function, there
is no protection from the use of the priv parameter in backend for some other 
purpose.
As the rpmsg_dev_probe calls rpmsg_create_ept with priv = NULL, This parameter will
not be sent by the backend.
But perhaps this is some over protection?
Please just tell me if you prefer that I suppress rpmsg_create_default_ept API and
set directly the default_ept->priv field.

Regards,
Arnaud

> 
> Regards,
> Bjorn
> 
>>>> +	 * Do not allow the creation and release of an endpoint on /dev/rpmsgX open and close,
>>>> +	 * reuse the default endpoint instead
>>>
>>> This sentence doesn't tell me anything about this code snippet and
>>> doesn't indicate that it relates to the snippet added elsewhere in this
>>> file by the previous patch.
>>>
>>>> +	 */
>>>> +	eptdev->default_ept = rpmsg_create_default_ept(rpdev, rpmsg_ept_cb, eptdev, chinfo);
>>>> +	if (!eptdev->default_ept) {
>>>> +		dev_err(&rpdev->dev, "failed to create %s\n", chinfo.name);
>>>> +		put_device(dev);
>>>> +		kfree(eptdev);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return rpmsg_chrdev_eptdev_add(eptdev, chinfo);
>>>> +}
>>>> +
>>>> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
>>>> +	if (ret)
>>>> +		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
>>>> +}
>>>> +
>>>> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
>>>> +	{ .name	= "rpmsg-raw" },
>>>> +	{ },
>>>> +};
>>>> +
>>>> +static struct rpmsg_driver rpmsg_chrdev_driver = {
>>>> +	.probe = rpmsg_chrdev_probe,
>>>> +	.remove = rpmsg_chrdev_remove,
>>>> +	.id_table = rpmsg_chrdev_id_table,
>>>> +	.drv.name = "rpmsg_chrdev",
>>>> +};
>>>> +
>>>>    static int rpmsg_chrdev_init(void)
>>>>    {
>>>>    	int ret;
>>>> @@ -445,12 +497,24 @@ static int rpmsg_chrdev_init(void)
>>>>    		return ret;
>>>>    	}
>>>> +	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
>>>> +	if (ret < 0) {
>>>> +		pr_err("rpmsg: failed to register rpmsg raw driver\n");
>>>> +		goto free_region;
>>>> +	}
>>>> +
>>>>    	return 0;
>>>> +
>>>> +free_region:
>>>> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>>>> +
>>>> +	return ret;
>>>>    }
>>>>    postcore_initcall(rpmsg_chrdev_init);
>>>>    static void rpmsg_chrdev_exit(void)
>>>>    {
>>>> +	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
>>>>    	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>>>>    }
>>>>    module_exit(rpmsg_chrdev_exit);
>>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>>>> index 59d2bd264fdb..298e75dc7774 100644
>>>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>>>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>>>> @@ -10,6 +10,9 @@
>>>>     * Based on rpmsg performance statistics driver by Michal Simek, which in turn
>>>>     * was based on TI & Google OMX rpmsg driver.
>>>>     */
>>>> +
>>>> +#define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
>>>
>>> These changes seems unrelated to above.
>>
>> I apparently broke something in my patchset here during a rebase. The previous
>> irrelevant comment you pointed out to me is also a consequence. My apologies
>> for this dirty patch...
>>
>> Thanks,
>> Arnaud
>>
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> +
>>>>    #include <linux/cdev.h>
>>>>    #include <linux/device.h>
>>>>    #include <linux/fs.h>
>>>> @@ -193,13 +196,13 @@ static int rpmsg_ctrldev_init(void)
>>>>    	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_ctrl");
>>>>    	if (ret < 0) {
>>>> -		pr_err("rpmsg: failed to allocate char dev region\n");
>>>> +		pr_err("failed to allocate char dev region\n");
>>>>    		return ret;
>>>>    	}
>>>>    	ret = register_rpmsg_driver(&rpmsg_ctrldev_driver);
>>>>    	if (ret < 0) {
>>>> -		pr_err("rpmsg ctrl: failed to register rpmsg driver\n");
>>>> +		pr_err("failed to register rpmsg driver\n");
>>>>    		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>>>>    	}
>>>> -- 
>>>> 2.17.1
>>>>