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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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/[email protected]/
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
> [email protected]
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
>
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 <[email protected]>
> >> ---
> >> 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/[email protected]/
>
> 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
> > [email protected]
> > https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
> >
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 <[email protected]>
> >> ---
> >> 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/[email protected]/
>
> 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
> > [email protected]
> > https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
> >