On 3/4/21 7:40 PM, Mathieu Poirier wrote:
> There has to be a capital letter at the start of the title:
>
> rpmsg: char: No dynamic endpoint management for the default one
>
> Please fix for all the patches.
Ok, I will update the subjects with capital letter in my next revision.
Just for my information, is it a new rule? kernel documentation [1] gives a
canonical subject and an example without capital letter.
[1]
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#the-canonical-patch-format
>
> On Fri, Feb 19, 2021 at 12:15:00PM +0100, Arnaud Pouliquen wrote:
>> Do not dynamically manage the default endpoint. The ept address must
>> not change.
>> This update is needed to manage the RPMSG_CREATE_DEV_IOCTL. In this
>> case a default endpoint is used and it's address must not change or
>> been reused by another service.
>
> The above is very difficult to understand. I am not sure about introducing
> RPMSG_CREATE_DEV_IOCTL in this patchset. More on that in an upcoming comment.
The purpose of this revision was mainly to provide a view of what we could do to
provide a more generic control interface.
To simplify the review I can remove the RPMSG_CREATE_DEV_IOCTL management and
send it as a next step, in a separate patchset.
>
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> drivers/rpmsg/rpmsg_char.c | 28 +++++++++++++++++++++-------
>> 1 file changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index c98b0e69679b..8d3f9d6c20ad 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -114,14 +114,23 @@ static int rpmsg_eptdev_open(struct inode *inode,
>> struct file *filp)
>> struct rpmsg_endpoint *ept;
>> struct rpmsg_device *rpdev = eptdev->rpdev;
>> struct device *dev = &eptdev->dev;
>> + u32 addr = eptdev->chinfo.src;
>>
>> get_device(dev);
>>
>> - ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
>> - if (!ept) {
>> - dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
>> - put_device(dev);
>> - return -EINVAL;
>> + /*
>> + * The RPMsg device can has been created by a ns announcement. In this
>> + * case a default endpoint has been created. Reuse it to avoid to manage
>> + * a new address on each open close.
>> + */
>
> Here too it is very difficult to understand because the comment
> doesn't not describe what the code does. The code creates an enpoint if it
> has not been created, which means /dev/rpmsgX was created from the ioctl.
Right, not enough explicit
Thanks,
Arnaud
>
>> + ept = rpdev->ept;
>> + if (!ept || addr != ept->addr) {
>> + ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev,
>> eptdev->chinfo);
>> + if (!ept) {
>> + dev_err(dev, "failed to open %s\n",
>> eptdev->chinfo.name);
>> + put_device(dev);
>> + return -EINVAL;
>> + }
>> }
>>
>> eptdev->ept = ept;
>> @@ -133,12 +142,17 @@ static int rpmsg_eptdev_open(struct inode *inode,
>> struct file *filp)
>> static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
>> {
>> struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
>> + struct rpmsg_device *rpdev = eptdev->rpdev;
>> struct device *dev = &eptdev->dev;
>>
>> - /* Close the endpoint, if it's not already destroyed by the parent */
>> + /*
>> + * Close the endpoint, if it's not already destroyed by the parent and
>> it is not the
>> + * default one.
>> + */
>> mutex_lock(&eptdev->ept_lock);
>> if (eptdev->ept) {
>> - rpmsg_destroy_ept(eptdev->ept);
>> + if (eptdev->ept != rpdev->ept)
>> + rpmsg_destroy_ept(eptdev->ept);
>> eptdev->ept = NULL;
>> }
>> mutex_unlock(&eptdev->ept_lock);
>> --
>> 2.17.1
>>
