diff mbox series

[v5,15/16] rpmsg: char: no dynamic endpoint management for the default one

Message ID 20210219111501.14261-16-arnaud.pouliquen@foss.st.com (mailing list archive)
State Not Applicable, archived
Headers show
Series introduce a generic IOCTL interface for RPMsg channels management | expand

Commit Message

Arnaud Pouliquen Feb. 19, 2021, 11:15 a.m. UTC
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(-)

Comments

Mathieu Poirier March 4, 2021, 6:40 p.m. UTC | #1
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
>
Arnaud Pouliquen March 5, 2021, 11:09 a.m. UTC | #2
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
>>
Mathieu Poirier March 5, 2021, 5:39 p.m. UTC | #3
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 mbox series

Patch

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);