| Message ID | 20210219111501.14261-16-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | introduce a generic IOCTL interface for RPMsg channels management | expand |
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. 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. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > --- > 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. > + 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 >
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 <arnaud.pouliquen@foss.st.com> >> --- >> 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 >>
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@foss.st.com> > >> --- > >> 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 > >>
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. + */ + 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);
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. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> --- drivers/rpmsg/rpmsg_char.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)