On Fri, Mar 05, 2021 at 12:09:37PM +0100, Arnaud POULIQUEN wrote: > > > 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. I don't think it is a rule but in the past few years the trend has been to use a capital letter. I was convinced the documentation had a capital letter but you have proven that it doesn't so you can ignore this part if you wish. > > [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. Yes, it would make this patchset quite simple. > > > > >> > >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx> > >> --- > >> 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 > >>
