[PATCH] rpmsg: char: return an error if device already open

[PATCH] rpmsg: char: return an error if device already open

[Arnaud Pouliquen]

The rpmsg_create_ept function is invoked when the device is opened.
As only one endpoint must be created per device. It is not
possible to open the same device twice.
The fix consists in returning -EBUSY when device is already
opened.

Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 4bbbacdbf3bb..360a1ab0a9c4 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -127,6 +127,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 	struct rpmsg_device *rpdev = eptdev->rpdev;
 	struct device *dev = &eptdev->dev;
 
+	if (eptdev->ept)
+		return -EBUSY;
+
 	get_device(dev);
 
 	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
-- 
2.17.1

Re: [PATCH] rpmsg: char: return an error if device already open

[Mathieu Poirier]

On Wed, Jan 06, 2021 at 02:37:14PM +0100, Arnaud Pouliquen wrote:
> The rpmsg_create_ept function is invoked when the device is opened.
> As only one endpoint must be created per device. It is not
> possible to open the same device twice.
> The fix consists in returning -EBUSY when device is already
> opened.
> 
> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 4bbbacdbf3bb..360a1ab0a9c4 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -127,6 +127,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  	struct rpmsg_device *rpdev = eptdev->rpdev;
>  	struct device *dev = &eptdev->dev;
>  
> +	if (eptdev->ept)
> +		return -EBUSY;
> +

I rarely had to work so hard to review a 2 line patch...

As far as I can tell the actual code is doing the right thing.  If user space is
trying to open the same eptdev more than once function rpmsg_create_ept() should
complain and the operation denied, wich is what the current code is doing.  

There is currently two customers for this API - SMD and GLINK.  The SMD code is
quite clear that if the channel is already open, the operation will be
denied [1].  The GLINK code isn't as clear but the fact that it returns NULL on
error conditions [2] is a good indication that things are working the same way.

What kind of use case are you looking to address?  Is there any way you can use
rpdev->ops->create_ept() as it is currently done?

Thanks,
Mathieu

[1]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/rpmsg/qcom_smd.c#L920
[2]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/rpmsg/qcom_glink_native.c#L1149

>  	get_device(dev);
>  
>  	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
> -- 
> 2.17.1
> 

Re: [Linux-stm32] [PATCH] rpmsg: char: return an error if device already open

[Arnaud POULIQUEN]

Hi Mathieu,


On 1/14/21 8:05 PM, Mathieu Poirier wrote:
> On Wed, Jan 06, 2021 at 02:37:14PM +0100, Arnaud Pouliquen wrote:
>> The rpmsg_create_ept function is invoked when the device is opened.
>> As only one endpoint must be created per device. It is not
>> possible to open the same device twice.
>> The fix consists in returning -EBUSY when device is already
>> opened.
>>
>> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 4bbbacdbf3bb..360a1ab0a9c4 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -127,6 +127,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>>  	struct rpmsg_device *rpdev = eptdev->rpdev;
>>  	struct device *dev = &eptdev->dev;
>>  
>> +	if (eptdev->ept)
>> +		return -EBUSY;
>> +
> 
> I rarely had to work so hard to review a 2 line patch...

That means that my commit description was not enough explicit...

> 
> As far as I can tell the actual code is doing the right thing.  If user space is
> trying to open the same eptdev more than once function rpmsg_create_ept() should
> complain and the operation denied, wich is what the current code is doing.  
> 
> There is currently two customers for this API - SMD and GLINK.  The SMD code is
> quite clear that if the channel is already open, the operation will be
> denied [1].  The GLINK code isn't as clear but the fact that it returns NULL on
> error conditions [2] is a good indication that things are working the same way.
> 
> What kind of use case are you looking to address?  Is there any way you can use
> rpdev->ops->create_ept() as it is currently done?

This patch was part of the IOCTL rpmsg series. I sent it separately at Bjorn's
request [1].

I detect the issue using the RPMSG_ADDR_ANY for the source address when tested
it with the rpmsf_virtio bus. In this case at each sys open of the device, a new
endpoint is created because a new source address is allocated.

[1]https://patchwork.kernel.org/project/linux-remoteproc/patch/20201222105726.16906-11-arnaud.pouliquen@foss.st.com/

Thanks,
Arnaud

> 
> Thanks,
> Mathieu
> 
> [1]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/rpmsg/qcom_smd.c#L920
> [2]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/rpmsg/qcom_glink_native.c#L1149
> 
>>  	get_device(dev);
>>  
>>  	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
>> -- 
>> 2.17.1
>>
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
> 

Re: [Linux-stm32] [PATCH] rpmsg: char: return an error if device already open

[Mathieu Poirier]

On Fri, Jan 15, 2021 at 10:13:35AM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> 
> On 1/14/21 8:05 PM, Mathieu Poirier wrote:
> > On Wed, Jan 06, 2021 at 02:37:14PM +0100, Arnaud Pouliquen wrote:
> >> The rpmsg_create_ept function is invoked when the device is opened.
> >> As only one endpoint must be created per device. It is not
> >> possible to open the same device twice.
> >> The fix consists in returning -EBUSY when device is already
> >> opened.
> >>
> >> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_char.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >> index 4bbbacdbf3bb..360a1ab0a9c4 100644
> >> --- a/drivers/rpmsg/rpmsg_char.c
> >> +++ b/drivers/rpmsg/rpmsg_char.c
> >> @@ -127,6 +127,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
> >>  	struct rpmsg_device *rpdev = eptdev->rpdev;
> >>  	struct device *dev = &eptdev->dev;
> >>  
> >> +	if (eptdev->ept)
> >> +		return -EBUSY;
> >> +
> > 
> > I rarely had to work so hard to review a 2 line patch...
> 
> That means that my commit description was not enough explicit...
> 
> > 
> > As far as I can tell the actual code is doing the right thing.  If user space is
> > trying to open the same eptdev more than once function rpmsg_create_ept() should
> > complain and the operation denied, wich is what the current code is doing.  
> > 
> > There is currently two customers for this API - SMD and GLINK.  The SMD code is
> > quite clear that if the channel is already open, the operation will be
> > denied [1].  The GLINK code isn't as clear but the fact that it returns NULL on
> > error conditions [2] is a good indication that things are working the same way.
> > 
> > What kind of use case are you looking to address?  Is there any way you can use
> > rpdev->ops->create_ept() as it is currently done?
> 
> This patch was part of the IOCTL rpmsg series. I sent it separately at Bjorn's
> request [1].
>

I am looking at [1] later today - I will get back to this patch when I have more
context.
 
> I detect the issue using the RPMSG_ADDR_ANY for the source address when tested
> it with the rpmsf_virtio bus. In this case at each sys open of the device, a new
> endpoint is created because a new source address is allocated.
> 
> [1]https://patchwork.kernel.org/project/linux-remoteproc/patch/20201222105726.16906-11-arnaud.pouliquen@foss.st.com/
> 
> Thanks,
> Arnaud
> 
> > 
> > Thanks,
> > Mathieu
> > 
> > [1]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/rpmsg/qcom_smd.c#L920
> > [2]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/rpmsg/qcom_glink_native.c#L1149
> > 
> >>  	get_device(dev);
> >>  
> >>  	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
> >> -- 
> >> 2.17.1
> >>
> > _______________________________________________
> > Linux-stm32 mailing list
> > Linux-stm32@st-md-mailman.stormreply.com
> > https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
> > 

Re: [Linux-stm32] [PATCH] rpmsg: char: return an error if device already open

[Bjorn Andersson]

On Fri 15 Jan 03:13 CST 2021, Arnaud POULIQUEN wrote:

> Hi Mathieu,
> 
> 
> On 1/14/21 8:05 PM, Mathieu Poirier wrote:
> > On Wed, Jan 06, 2021 at 02:37:14PM +0100, Arnaud Pouliquen wrote:
> >> The rpmsg_create_ept function is invoked when the device is opened.
> >> As only one endpoint must be created per device. It is not
> >> possible to open the same device twice.
> >> The fix consists in returning -EBUSY when device is already
> >> opened.
> >>
> >> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_char.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >> index 4bbbacdbf3bb..360a1ab0a9c4 100644
> >> --- a/drivers/rpmsg/rpmsg_char.c
> >> +++ b/drivers/rpmsg/rpmsg_char.c
> >> @@ -127,6 +127,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
> >>  	struct rpmsg_device *rpdev = eptdev->rpdev;
> >>  	struct device *dev = &eptdev->dev;
> >>  
> >> +	if (eptdev->ept)
> >> +		return -EBUSY;
> >> +
> > 
> > I rarely had to work so hard to review a 2 line patch...
> 
> That means that my commit description was not enough explicit...
> 
> > 
> > As far as I can tell the actual code is doing the right thing.  If user space is
> > trying to open the same eptdev more than once function rpmsg_create_ept() should
> > complain and the operation denied, wich is what the current code is doing.  
> > 
> > There is currently two customers for this API - SMD and GLINK.  The SMD code is
> > quite clear that if the channel is already open, the operation will be
> > denied [1].  The GLINK code isn't as clear but the fact that it returns NULL on
> > error conditions [2] is a good indication that things are working the same way.
> > 
> > What kind of use case are you looking to address?  Is there any way you can use
> > rpdev->ops->create_ept() as it is currently done?
> 
> This patch was part of the IOCTL rpmsg series. I sent it separately at Bjorn's
> request [1].
> 

I apparently didn't spend as much effort as Mathieu thinking about the
details. I do believe that he's right, at least both GLINK and SMD
_should_ return -EBUSY if we try to open an already open channel -
either because the kernel has bound a driver to the channel or because
rpmsg_char already has it opened.

> I detect the issue using the RPMSG_ADDR_ANY for the source address when tested
> it with the rpmsf_virtio bus. In this case at each sys open of the device, a new
> endpoint is created because a new source address is allocated.
> 

In SMD and GLINK channels are identified solely by their name and hence
it's not possible to have duplicates. As this isn't the case for virtio
I didn't have any objections to it and that's why I asked you to resend
it separately.

But in line with GLINK/SMD, what would the expected behavior be if I
with the virtio backend open a rpmsg_char which is already bound to a
kernel driver?

Do you think we should get another "channel" or do you think the virtio
driver should detect this and return -EBUSY? (I.e. render this patch
unnecessary)

Regards,
Bjorn

> [1]https://patchwork.kernel.org/project/linux-remoteproc/patch/20201222105726.16906-11-arnaud.pouliquen@foss.st.com/
> 
> Thanks,
> Arnaud
> 
> > 
> > Thanks,
> > Mathieu
> > 
> > [1]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/rpmsg/qcom_smd.c#L920
> > [2]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/rpmsg/qcom_glink_native.c#L1149
> > 
> >>  	get_device(dev);
> >>  
> >>  	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
> >> -- 
> >> 2.17.1
> >>
> > _______________________________________________
> > Linux-stm32 mailing list
> > Linux-stm32@st-md-mailman.stormreply.com
> > https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
> > 

Re: [Linux-stm32] [PATCH] rpmsg: char: return an error if device already open

[Arnaud POULIQUEN]

Hi Bjorn,

On 1/25/21 4:31 PM, Bjorn Andersson wrote:
> On Fri 15 Jan 03:13 CST 2021, Arnaud POULIQUEN wrote:
> 
>> Hi Mathieu,
>>
>>
>> On 1/14/21 8:05 PM, Mathieu Poirier wrote:
>>> On Wed, Jan 06, 2021 at 02:37:14PM +0100, Arnaud Pouliquen wrote:
>>>> The rpmsg_create_ept function is invoked when the device is opened.
>>>> As only one endpoint must be created per device. It is not
>>>> possible to open the same device twice.
>>>> The fix consists in returning -EBUSY when device is already
>>>> opened.
>>>>
>>>> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> ---
>>>>  drivers/rpmsg/rpmsg_char.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>>> index 4bbbacdbf3bb..360a1ab0a9c4 100644
>>>> --- a/drivers/rpmsg/rpmsg_char.c
>>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>>> @@ -127,6 +127,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>>>>  	struct rpmsg_device *rpdev = eptdev->rpdev;
>>>>  	struct device *dev = &eptdev->dev;
>>>>  
>>>> +	if (eptdev->ept)
>>>> +		return -EBUSY;
>>>> +
>>>
>>> I rarely had to work so hard to review a 2 line patch...
>>
>> That means that my commit description was not enough explicit...
>>
>>>
>>> As far as I can tell the actual code is doing the right thing.  If user space is
>>> trying to open the same eptdev more than once function rpmsg_create_ept() should
>>> complain and the operation denied, wich is what the current code is doing.  
>>>
>>> There is currently two customers for this API - SMD and GLINK.  The SMD code is
>>> quite clear that if the channel is already open, the operation will be
>>> denied [1].  The GLINK code isn't as clear but the fact that it returns NULL on
>>> error conditions [2] is a good indication that things are working the same way.
>>>
>>> What kind of use case are you looking to address?  Is there any way you can use
>>> rpdev->ops->create_ept() as it is currently done?
>>
>> This patch was part of the IOCTL rpmsg series. I sent it separately at Bjorn's
>> request [1].
>>
> 
> I apparently didn't spend as much effort as Mathieu thinking about the
> details. I do believe that he's right, at least both GLINK and SMD
> _should_ return -EBUSY if we try to open an already open channel -
> either because the kernel has bound a driver to the channel or because
> rpmsg_char already has it opened.
> 
>> I detect the issue using the RPMSG_ADDR_ANY for the source address when tested
>> it with the rpmsf_virtio bus. In this case at each sys open of the device, a new
>> endpoint is created because a new source address is allocated.
>>
> 
> In SMD and GLINK channels are identified solely by their name and hence
> it's not possible to have duplicates. As this isn't the case for virtio
> I didn't have any objections to it and that's why I asked you to resend
> it separately.
> 
> But in line with GLINK/SMD, what would the expected behavior be if I
> with the virtio backend open a rpmsg_char which is already bound to a
> kernel driver?

I guess that user applications should expect same behavior, regardless of the
backend. So this patch would ensure the check at RPMsg service level.

> 
> Do you think we should get another "channel" or do you think the virtio
> driver should detect this and return -EBUSY? (I.e. render this patch
> unnecessary)

Regarding virtio implementation we could create a new channel only if either the
channel name, either the local address or the remote address is different.
So a channel would be created only if one address is set to RPMSG_ADDR_ANY.
But could be a nightmare to handle it, in case of multi open/close.

In my opinion the channel should be created only on /dev/rpmsgX creation.

That said, if this patch generates too much discussion. we can also leave it on
hold until figure out how to adapt the RPMsg char for the virtio backend.

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>> [1]https://patchwork.kernel.org/project/linux-remoteproc/patch/20201222105726.16906-11-arnaud.pouliquen@foss.st.com/
>>
>> Thanks,
>> Arnaud
>>
>>>
>>> Thanks,
>>> Mathieu
>>>
>>> [1]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/rpmsg/qcom_smd.c#L920
>>> [2]. https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/rpmsg/qcom_glink_native.c#L1149
>>>
>>>>  	get_device(dev);
>>>>  
>>>>  	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
>>>> -- 
>>>> 2.17.1
>>>>
>>> _______________________________________________
>>> Linux-stm32 mailing list
>>> Linux-stm32@st-md-mailman.stormreply.com
>>> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
>>>

Re: [PATCH] rpmsg: char: return an error if device already open

[patchwork-bot+linux-remoteproc]

Hello:

This patch was applied to andersson/remoteproc.git (refs/heads/for-next):

On Wed, 6 Jan 2021 14:37:14 +0100 you wrote:
> The rpmsg_create_ept function is invoked when the device is opened.
> As only one endpoint must be created per device. It is not
> possible to open the same device twice.
> The fix consists in returning -EBUSY when device is already
> opened.
> 
> Fixes: c0cdc19f84a4 ("rpmsg: Driver for user space endpoint interface")
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> 
> [...]

Here is the summary with links:
  - rpmsg: char: return an error if device already open
    https://git.kernel.org/andersson/remoteproc/c/964e8bedd5a1

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html