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@xxxxxxxxxxx>
>>>> ---
>>>>  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@xxxxxxxxxxx/
>>
>> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxx
>>> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
>>>