| Message ID | 20210623150504.14450-4-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rpmsg: char: introduce the rpmsg-raw channel | expand |
On Wed, Jun 23, 2021 at 05:05:03PM +0200, Arnaud Pouliquen wrote: > Current implementation create/destroy a new endpoint on each > rpmsg_eptdev_open/rpmsg_eptdev_release calls. > > For a rpmsg device created by the NS announcement mechanism we need to > use a unique static endpoint that is the default rpmsg device endpoint > associated to the channel. > > This patch prepares the introduction of a rpmsg channel device for the > char device. The rpmsg channel device will require a default endpoint to > communicate to the remote processor. > > Add the static_ept field in rpmsg_eptdev structure. This boolean > determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call. > > - If static_ept == false: > Use the legacy behavior by creating a new endpoint each time > rpmsg_eptdev_open is called and release it when rpmsg_eptdev_release > is called on /dev/rpmsgX device open/close. > > - If static_ept == true: > use the rpmsg device default endpoint for the communication. > - Address the update of _rpmsg_chrdev_eptdev_create in e separate patch for readability. > > Add protection in rpmsg_eptdev_ioctl to prevent to destroy a default endpoint. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > --- > update vs V1: > - remove the management of the default endpoint creation from rpmsg_eptdev_open. > > --- > drivers/rpmsg/rpmsg_char.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > index 50b7d4b00175..a75dce1e29d8 100644 > --- a/drivers/rpmsg/rpmsg_char.c > +++ b/drivers/rpmsg/rpmsg_char.c > @@ -45,6 +45,8 @@ static DEFINE_IDA(rpmsg_minor_ida); > * @queue_lock: synchronization of @queue operations > * @queue: incoming message queue > * @readq: wait object for incoming queue > + * @static_ept: specify if the endpoint has to be created at each device opening or > + * if the default endpoint should be used. > */ > struct rpmsg_eptdev { > struct device dev; > @@ -59,6 +61,8 @@ struct rpmsg_eptdev { > spinlock_t queue_lock; > struct sk_buff_head queue; > wait_queue_head_t readq; > + > + bool static_ept; > }; > > int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) > @@ -116,7 +120,15 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp) > > get_device(dev); > > - ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo); > + /* > + * If the static_ept is set to true, the rpmsg device default endpoint is used. > + * Else a new endpoint is created on open that will be destroyed on release. > + */ > + if (eptdev->static_ept) > + ept = rpdev->ept; > + else > + 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); > @@ -137,7 +149,8 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp) > /* Close the endpoint, if it's not already destroyed by the parent */ > mutex_lock(&eptdev->ept_lock); > if (eptdev->ept) { > - rpmsg_destroy_ept(eptdev->ept); > + if (!eptdev->static_ept) > + rpmsg_destroy_ept(eptdev->ept); > eptdev->ept = NULL; > } > mutex_unlock(&eptdev->ept_lock); > @@ -264,6 +277,10 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd, > if (cmd != RPMSG_DESTROY_EPT_IOCTL) > return -EINVAL; > > + /* Don't allow to destroy a default endpoint. */ > + if (!eptdev->rpdev || eptdev->ept == eptdev->rpdev->ept) Did you find a scenario where eptdev->rpdev would not be valid when this is called? To me if this code is called __rpmsg_chrdev_eptdev_create() has setup the rpdev pointer and therefore it will be valid. If there is a scenario where it is possible that eptdev->rpdev is invalid then please add a comment with the details. Otherwise simply remove the first part of the condition. Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > + return -EPERM; > + > return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL); > } > > -- > 2.17.1 >
Hello Mathieu, On 6/30/21 10:48 PM, Mathieu Poirier wrote: > On Wed, Jun 23, 2021 at 05:05:03PM +0200, Arnaud Pouliquen wrote: >> Current implementation create/destroy a new endpoint on each >> rpmsg_eptdev_open/rpmsg_eptdev_release calls. >> >> For a rpmsg device created by the NS announcement mechanism we need to >> use a unique static endpoint that is the default rpmsg device endpoint >> associated to the channel. >> >> This patch prepares the introduction of a rpmsg channel device for the >> char device. The rpmsg channel device will require a default endpoint to >> communicate to the remote processor. >> >> Add the static_ept field in rpmsg_eptdev structure. This boolean >> determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call. >> >> - If static_ept == false: >> Use the legacy behavior by creating a new endpoint each time >> rpmsg_eptdev_open is called and release it when rpmsg_eptdev_release >> is called on /dev/rpmsgX device open/close. >> >> - If static_ept == true: >> use the rpmsg device default endpoint for the communication. >> - Address the update of _rpmsg_chrdev_eptdev_create in e separate patch for readability. >> >> Add protection in rpmsg_eptdev_ioctl to prevent to destroy a default endpoint. >> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >> --- >> update vs V1: >> - remove the management of the default endpoint creation from rpmsg_eptdev_open. >> >> --- >> drivers/rpmsg/rpmsg_char.c | 21 +++++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >> index 50b7d4b00175..a75dce1e29d8 100644 >> --- a/drivers/rpmsg/rpmsg_char.c >> +++ b/drivers/rpmsg/rpmsg_char.c >> @@ -45,6 +45,8 @@ static DEFINE_IDA(rpmsg_minor_ida); >> * @queue_lock: synchronization of @queue operations >> * @queue: incoming message queue >> * @readq: wait object for incoming queue >> + * @static_ept: specify if the endpoint has to be created at each device opening or >> + * if the default endpoint should be used. >> */ >> struct rpmsg_eptdev { >> struct device dev; >> @@ -59,6 +61,8 @@ struct rpmsg_eptdev { >> spinlock_t queue_lock; >> struct sk_buff_head queue; >> wait_queue_head_t readq; >> + >> + bool static_ept; >> }; >> >> int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) >> @@ -116,7 +120,15 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp) >> >> get_device(dev); >> >> - ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo); >> + /* >> + * If the static_ept is set to true, the rpmsg device default endpoint is used. >> + * Else a new endpoint is created on open that will be destroyed on release. >> + */ >> + if (eptdev->static_ept) >> + ept = rpdev->ept; >> + else >> + 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); >> @@ -137,7 +149,8 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp) >> /* Close the endpoint, if it's not already destroyed by the parent */ >> mutex_lock(&eptdev->ept_lock); >> if (eptdev->ept) { >> - rpmsg_destroy_ept(eptdev->ept); >> + if (!eptdev->static_ept) >> + rpmsg_destroy_ept(eptdev->ept); >> eptdev->ept = NULL; >> } >> mutex_unlock(&eptdev->ept_lock); >> @@ -264,6 +277,10 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd, >> if (cmd != RPMSG_DESTROY_EPT_IOCTL) >> return -EINVAL; >> >> + /* Don't allow to destroy a default endpoint. */ >> + if (!eptdev->rpdev || eptdev->ept == eptdev->rpdev->ept) > > Did you find a scenario where eptdev->rpdev would not be valid when this is > called? To me if this code is called __rpmsg_chrdev_eptdev_create() has setup > the rpdev pointer and therefore it will be valid. > > If there is a scenario where it is possible that eptdev->rpdev is invalid then > please add a comment with the details. Otherwise simply remove the first part > of the condition. > > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> Good catch! you are right, it seems an over protection, i will verify this making a new revision that will take into account your other remarks. Thanks for all your advices and patchset reviews related to rpmsg_char restructuring. Arnaud > >> + return -EPERM; >> + >> return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL); >> } >> >> -- >> 2.17.1 >>
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index 50b7d4b00175..a75dce1e29d8 100644 --- a/drivers/rpmsg/rpmsg_char.c +++ b/drivers/rpmsg/rpmsg_char.c @@ -45,6 +45,8 @@ static DEFINE_IDA(rpmsg_minor_ida); * @queue_lock: synchronization of @queue operations * @queue: incoming message queue * @readq: wait object for incoming queue + * @static_ept: specify if the endpoint has to be created at each device opening or + * if the default endpoint should be used. */ struct rpmsg_eptdev { struct device dev; @@ -59,6 +61,8 @@ struct rpmsg_eptdev { spinlock_t queue_lock; struct sk_buff_head queue; wait_queue_head_t readq; + + bool static_ept; }; int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) @@ -116,7 +120,15 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp) get_device(dev); - ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo); + /* + * If the static_ept is set to true, the rpmsg device default endpoint is used. + * Else a new endpoint is created on open that will be destroyed on release. + */ + if (eptdev->static_ept) + ept = rpdev->ept; + else + 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); @@ -137,7 +149,8 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp) /* Close the endpoint, if it's not already destroyed by the parent */ mutex_lock(&eptdev->ept_lock); if (eptdev->ept) { - rpmsg_destroy_ept(eptdev->ept); + if (!eptdev->static_ept) + rpmsg_destroy_ept(eptdev->ept); eptdev->ept = NULL; } mutex_unlock(&eptdev->ept_lock); @@ -264,6 +277,10 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd, if (cmd != RPMSG_DESTROY_EPT_IOCTL) return -EINVAL; + /* Don't allow to destroy a default endpoint. */ + if (!eptdev->rpdev || eptdev->ept == eptdev->rpdev->ept) + return -EPERM; + return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL); }
Current implementation create/destroy a new endpoint on each rpmsg_eptdev_open/rpmsg_eptdev_release calls. For a rpmsg device created by the NS announcement mechanism we need to use a unique static endpoint that is the default rpmsg device endpoint associated to the channel. This patch prepares the introduction of a rpmsg channel device for the char device. The rpmsg channel device will require a default endpoint to communicate to the remote processor. Add the static_ept field in rpmsg_eptdev structure. This boolean determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call. - If static_ept == false: Use the legacy behavior by creating a new endpoint each time rpmsg_eptdev_open is called and release it when rpmsg_eptdev_release is called on /dev/rpmsgX device open/close. - If static_ept == true: use the rpmsg device default endpoint for the communication. - Address the update of _rpmsg_chrdev_eptdev_create in e separate patch for readability. Add protection in rpmsg_eptdev_ioctl to prevent to destroy a default endpoint. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> --- update vs V1: - remove the management of the default endpoint creation from rpmsg_eptdev_open. --- drivers/rpmsg/rpmsg_char.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)