diff mbox series

[v2,04/16] rpmsg: ctrl: implement the ioctl function to create device

Message ID 20201222105726.16906-5-arnaud.pouliquen@foss.st.com (mailing list archive)
State New, archived
Headers show
Series introduce generic IOCTL interface for RPMsg channels management | expand

Commit Message

Arnaud Pouliquen Dec. 22, 2020, 10:57 a.m. UTC
Implement the ioctl function that parses the list of
rpmsg drivers registered to create an associated device.
To be ISO user API, in a first step, the driver_override
is only allowed for the RPMsg raw service, supported by the
rpmsg_char driver.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

Comments

Bjorn Andersson Jan. 5, 2021, 1:33 a.m. UTC | #1
On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> Implement the ioctl function that parses the list of
> rpmsg drivers registered to create an associated device.
> To be ISO user API, in a first step, the driver_override
> is only allowed for the RPMsg raw service, supported by the
> rpmsg_char driver.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 065e2e304019..8381b5b2b794 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
> +{
> +	struct rpmsg_ctl_info *drv_info;
> +
> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
> +		if (drv_info->ctrl->service == service)
> +			return drv_info->ctrl->drv_name;
> +	}
> +
> +	return NULL;
> +}
> +
>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>  				 unsigned long arg)
>  {
>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> -
> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
> +	void __user *argp = (void __user *)arg;
> +	struct rpmsg_channel_info chinfo;
> +	struct rpmsg_endpoint_info eptinfo;
> +	struct rpmsg_device *newch;
> +
> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> +		return -EFAULT;
> +
> +	/*
> +	 * In a frst step only the rpmsg_raw service is supported.
> +	 * The override is foorced to RPMSG_RAW_SERVICE
> +	 */
> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
> +	if (!chinfo.driver_override)
> +		return -ENODEV;
> +
> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> +	chinfo.src = eptinfo.src;
> +	chinfo.dst = eptinfo.dst;
> +
> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);

Afaict this would create and announce and endpoint (or possibly find a
endpoint announced by the other side of the link).

In the case of the Qualcomm transports, and as been discussed to
introduce for virtio in the past, the channel actually have a state. So
opening/announcing it here means that we have no way to close and reopen
this channel later?


It would also mean that we announce to the firmware that there's an
application in Linux now ready to receive data on this channel - but
that won't be the case until someone actually open the created cdev (or
tty in your case) - which quite likely will result in data loss.

I think instead of piggybacking on the rpmsg_device we should just carry
these "raw exports to userspace" in some other construct - perhaps a
auxiliary_bus, or if we still only care for char and tty, not split them
up at all using the device model.

Regards,
Bjorn

> +	if (!newch) {
> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
> +		return -ENXIO;
> +	}
>  
>  	return 0;
>  };
> -- 
> 2.17.1
>
Arnaud Pouliquen Jan. 5, 2021, 6:07 p.m. UTC | #2
On 1/5/21 2:33 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:
> 
>> Implement the ioctl function that parses the list of
>> rpmsg drivers registered to create an associated device.
>> To be ISO user API, in a first step, the driver_override
>> is only allowed for the RPMsg raw service, supported by the
>> rpmsg_char driver.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>> index 065e2e304019..8381b5b2b794 100644
>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
>>  	return 0;
>>  }
>>  
>> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
>> +{
>> +	struct rpmsg_ctl_info *drv_info;
>> +
>> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
>> +		if (drv_info->ctrl->service == service)
>> +			return drv_info->ctrl->drv_name;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>>  				 unsigned long arg)
>>  {
>>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
>> -
>> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
>> +	void __user *argp = (void __user *)arg;
>> +	struct rpmsg_channel_info chinfo;
>> +	struct rpmsg_endpoint_info eptinfo;
>> +	struct rpmsg_device *newch;
>> +
>> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>> +		return -EINVAL;
>> +
>> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>> +		return -EFAULT;
>> +
>> +	/*
>> +	 * In a frst step only the rpmsg_raw service is supported.
>> +	 * The override is foorced to RPMSG_RAW_SERVICE
>> +	 */
>> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
>> +	if (!chinfo.driver_override)
>> +		return -ENODEV;
>> +
>> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>> +	chinfo.src = eptinfo.src;
>> +	chinfo.dst = eptinfo.dst;
>> +
>> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
> 
> Afaict this would create and announce and endpoint (or possibly find a
> endpoint announced by the other side of the link).

It depends on how rpdev is initialized[1].
 - For the rpmsg_char no default endpoint is created. The endpoint is created on
/dev/rpmsgX open. So the channel is created but not announced.
=> both sides have to know the the destination address for virtio implementation.

- For the rpmsg TTY the endpoint should be created by default by the RPMsg core
and associated to the rpdev. An announcement is sent to the remote side.

[1]https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_core.c#L445

> 
> In the case of the Qualcomm transports, and as been discussed to
> introduce for virtio in the past, the channel actually have a state. So
> opening/announcing it here means that we have no way to close and reopen
> this channel later?

In this first series I just focused to de-correlate the control part from the
rpmsg char. A main difference is that a channel is associated to a cdev.

But the ioctrl can be extended to close the cdev and the associated channel
(implemented in my V1).
else the rpmsg device is automatically remove by the rpmsg bus.
> 
> 
> It would also mean that we announce to the firmware that there's an
> application in Linux now ready to receive data on this channel - but
> that won't be the case until someone actually open the created cdev (or
> tty in your case) - which quite likely will result in data loss.

With the virtio implementation it is potentially already the case. When Linux
receive an NS announcement, there is no mechanism to inform the remote firmware
that Linux is ready to receive data. Some OpenAMP lib user already point out
this issue.
In glink driver seems that there is no such issue as
qcom_glink_send_open/close_req allow to provide information on endpoint state.

I would propose to address this in a next step.

> 
> I think instead of piggybacking on the rpmsg_device we should just carry
> these "raw exports to userspace" in some other construct - perhaps a
> auxiliary_bus, 

I'm not familiar with auxilary-bus but seems very similar to the rpmsg_bus...
I wonder if this could lead to code duplication in RPMsg service drivers to
support the control but also the NS announcement.

or if we still only care for char and tty, not split them
> up at all using the device model.

The initial requirement was to extend the control interface implemented in
rpmsg_char to other services before introducing new one.

So probably as a first step we have to clarify the requirements to determine the
solution to implement.

Here is my point of view on the induced requirements:
- Allow to create a service from Linux user application:
	- with a specific name
	- with or without name service announcement.
- Allow to probe the same service by receiving either a NS announcement from the
remote firmware or a Linux user application request.
- Use these services independently of the RPMsg transport implementation (e.g be
able to use RPMSg char with the RPMsg virtio bus).

This requirements explain my approach: associate a service to a RPMsg device in
order to be able to probe using the same driver either by the remote firmware NS
announcement or by a Linux user application.

Is the requirements I detailed match with what you had in mind?

Please, could you detail your views on the use of the auxilary bus in this context?

We can also think about an alternative to keep rpmsg_char unchanged for legacy
support.
 - only create a RPMsg ctrl for new RPMsg services
 - enable it for virtio_rpmsg_bus (In this case the rpmsg char cannot be probed
by remote firmware, but allows communication between fixed addresses)

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>> +	if (!newch) {
>> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
>> +		return -ENXIO;
>> +	}
>>  
>>  	return 0;
>>  };
>> -- 
>> 2.17.1
>>
Mathieu Poirier Jan. 21, 2021, 11:52 p.m. UTC | #3
On Tue, Dec 22, 2020 at 11:57:14AM +0100, Arnaud Pouliquen wrote:
> Implement the ioctl function that parses the list of
> rpmsg drivers registered to create an associated device.
> To be ISO user API, in a first step, the driver_override
> is only allowed for the RPMsg raw service, supported by the
> rpmsg_char driver.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 065e2e304019..8381b5b2b794 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
> +{
> +	struct rpmsg_ctl_info *drv_info;
> +
> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
> +		if (drv_info->ctrl->service == service)
> +			return drv_info->ctrl->drv_name;
> +	}
> +

I'm unsure about the above... To me this looks like what the .match() function
of a bus would do.  And when I read Bjorn's comment he brought up the
auxiliary_bus.  I don't know about the auxiliary_bus but it is worth looking
into.  Registering with a bus would streamline a lot of the code in this
patchset.

I'm out of time for today - I will continue tomorrow.

Thanks,
Mathieu

> +	return NULL;
> +}
> +
>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>  				 unsigned long arg)
>  {
>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> -
> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
> +	void __user *argp = (void __user *)arg;
> +	struct rpmsg_channel_info chinfo;
> +	struct rpmsg_endpoint_info eptinfo;
> +	struct rpmsg_device *newch;
> +
> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> +		return -EFAULT;
> +
> +	/*
> +	 * In a frst step only the rpmsg_raw service is supported.
> +	 * The override is foorced to RPMSG_RAW_SERVICE
> +	 */
> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
> +	if (!chinfo.driver_override)
> +		return -ENODEV;
> +
> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> +	chinfo.src = eptinfo.src;
> +	chinfo.dst = eptinfo.dst;
> +
> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
> +	if (!newch) {
> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
> +		return -ENXIO;
> +	}
>  
>  	return 0;
>  };
> -- 
> 2.17.1
>
Arnaud Pouliquen Jan. 22, 2021, 1:05 p.m. UTC | #4
Hi Mathieu,

On 1/22/21 12:52 AM, Mathieu Poirier wrote:
> On Tue, Dec 22, 2020 at 11:57:14AM +0100, Arnaud Pouliquen wrote:
>> Implement the ioctl function that parses the list of
>> rpmsg drivers registered to create an associated device.
>> To be ISO user API, in a first step, the driver_override
>> is only allowed for the RPMsg raw service, supported by the
>> rpmsg_char driver.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>> index 065e2e304019..8381b5b2b794 100644
>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
>>  	return 0;
>>  }
>>  
>> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
>> +{
>> +	struct rpmsg_ctl_info *drv_info;
>> +
>> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
>> +		if (drv_info->ctrl->service == service)
>> +			return drv_info->ctrl->drv_name;
>> +	}
>> +
> 
> I'm unsure about the above... To me this looks like what the .match() function
> of a bus would do.  And when I read Bjorn's comment he brought up the
> auxiliary_bus.  I don't know about the auxiliary_bus but it is worth looking
> into.  Registering with a bus would streamline a lot of the code in this
> patchset.

As answered Bjorn, we already have the RPMsg bus to manage the rpmsg devices.
Look like duplication from my POV, except if the IOCTL does not manage channel
but only endpoint.

In my design I considered that the rpmsg_ctrl creates a channel associated to a
rpmsg_device such as the RPMsg ns_announcement.

Based on this assumption, if we implement the auxiliary_bus (or other) for the
rpmsg_ctrl a RPMsg driver will have to manage the probe by rpmsg_bus and by the
auxillary bus. The probe from the auxiliary bus would lead to the creation of an
RPMsg device on the rpmsg_bus, so a duplication with cross dependencies and
would probably make tricky the remove part.

That said, I think the design depends on the functionality that should be
implemented in the rpmsg_ctrl. Here is an alternative approach based on the
auxiliary bus, which I'm starting to think about:

The current approach of the rpmsg_char driver is to use the IOCTRL interface to
instantiate a cdev with an endpoint (the RPMsg device is associated with the
ioctl dev). This would correspond to the use of an auxiliary bus to manage local
endpoint creations.

We could therefore consider an RPMsg name service based on an RPmsg device. This
RPMsg device would register a kind of "RPMsg service endpoint" driver on the
auxiliary rpmsg_ioctl bus.
The rpmsg_ctrl will be used to instantiate the endpoints for this RPMsg device.
on user application request the rpmsg_ctrl will call the appropriate auxiliary
device to create an endpoint.

If we consider that one objective of this series is to allow application to
initiate the communication with the remote processor, so to be able to initiate
the service (ns announcement sent to the remote processor).
This implies that:
-either the RPMsg device has been probed first by a remote ns announcement or by
a Linux kernel driver using the "driver_override", to register an auxiliary
device. In this case an endpoint will be created associated to the RPMsg service
- or create a RPMsg device on first ioctl endpoint creation request, if it does
not exist (that could trig a NS announcement to remote processor).

But I'm not sure that this approach would work with QCOM RPMsg backends...

> 
> I'm out of time for today - I will continue tomorrow.

It seems to me that the main point to step forward is to clarify the global
design and features of the rpmsg-ctrl.
Depending on the decision taken, this series could be trashed and rewritten from
a blank page...To not lost to much time on the series don't hesitate to limit
the review to the minimum.

Thanks,
Arnaud

> 
> Thanks,
> Mathieu
> 
>> +	return NULL;
>> +}
>> +
>>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>>  				 unsigned long arg)
>>  {
>>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
>> -
>> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
>> +	void __user *argp = (void __user *)arg;
>> +	struct rpmsg_channel_info chinfo;
>> +	struct rpmsg_endpoint_info eptinfo;
>> +	struct rpmsg_device *newch;
>> +
>> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>> +		return -EINVAL;
>> +
>> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>> +		return -EFAULT;
>> +
>> +	/*
>> +	 * In a frst step only the rpmsg_raw service is supported.
>> +	 * The override is foorced to RPMSG_RAW_SERVICE
>> +	 */
>> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
>> +	if (!chinfo.driver_override)
>> +		return -ENODEV;
>> +
>> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>> +	chinfo.src = eptinfo.src;
>> +	chinfo.dst = eptinfo.dst;
>> +
>> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
>> +	if (!newch) {
>> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
>> +		return -ENXIO;
>> +	}
>>  
>>  	return 0;
>>  };
>> -- 
>> 2.17.1
>>
Mathieu Poirier Jan. 22, 2021, 8:59 p.m. UTC | #5
On Fri, Jan 22, 2021 at 02:05:27PM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 1/22/21 12:52 AM, Mathieu Poirier wrote:
> > On Tue, Dec 22, 2020 at 11:57:14AM +0100, Arnaud Pouliquen wrote:
> >> Implement the ioctl function that parses the list of
> >> rpmsg drivers registered to create an associated device.
> >> To be ISO user API, in a first step, the driver_override
> >> is only allowed for the RPMsg raw service, supported by the
> >> rpmsg_char driver.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 41 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> >> index 065e2e304019..8381b5b2b794 100644
> >> --- a/drivers/rpmsg/rpmsg_ctrl.c
> >> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> >> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
> >>  	return 0;
> >>  }
> >>  
> >> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
> >> +{
> >> +	struct rpmsg_ctl_info *drv_info;
> >> +
> >> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
> >> +		if (drv_info->ctrl->service == service)
> >> +			return drv_info->ctrl->drv_name;
> >> +	}
> >> +
> > 
> > I'm unsure about the above... To me this looks like what the .match() function
> > of a bus would do.  And when I read Bjorn's comment he brought up the
> > auxiliary_bus.  I don't know about the auxiliary_bus but it is worth looking
> > into.  Registering with a bus would streamline a lot of the code in this
> > patchset.
> 
> As answered Bjorn, we already have the RPMsg bus to manage the rpmsg devices.
> Look like duplication from my POV, except if the IOCTL does not manage channel
> but only endpoint.
> 
> In my design I considered that the rpmsg_ctrl creates a channel associated to a
> rpmsg_device such as the RPMsg ns_announcement.
> 
> Based on this assumption, if we implement the auxiliary_bus (or other) for the
> rpmsg_ctrl a RPMsg driver will have to manage the probe by rpmsg_bus and by the
> auxillary bus. The probe from the auxiliary bus would lead to the creation of an
> RPMsg device on the rpmsg_bus, so a duplication with cross dependencies and
> would probably make tricky the remove part.
> 
> That said, I think the design depends on the functionality that should be
> implemented in the rpmsg_ctrl. Here is an alternative approach based on the
> auxiliary bus, which I'm starting to think about:
> 
> The current approach of the rpmsg_char driver is to use the IOCTRL interface to
> instantiate a cdev with an endpoint (the RPMsg device is associated with the
> ioctl dev). This would correspond to the use of an auxiliary bus to manage local
> endpoint creations.
> 
> We could therefore consider an RPMsg name service based on an RPmsg device. This
> RPMsg device would register a kind of "RPMsg service endpoint" driver on the
> auxiliary rpmsg_ioctl bus.
> The rpmsg_ctrl will be used to instantiate the endpoints for this RPMsg device.
> on user application request the rpmsg_ctrl will call the appropriate auxiliary
> device to create an endpoint.
> 
> If we consider that one objective of this series is to allow application to
> initiate the communication with the remote processor, so to be able to initiate
> the service (ns announcement sent to the remote processor).
> This implies that:
> -either the RPMsg device has been probed first by a remote ns announcement or by
> a Linux kernel driver using the "driver_override", to register an auxiliary
> device. In this case an endpoint will be created associated to the RPMsg service
> - or create a RPMsg device on first ioctl endpoint creation request, if it does
> not exist (that could trig a NS announcement to remote processor).
> 
> But I'm not sure that this approach would work with QCOM RPMsg backends...
>

I don't think there is a way forward with this set without a clear understanding
of the Glink and SMD drivers.  I have already spent a fair amount of time in the
Glink driver and will continue on Monday with SMD.  

> > 
> > I'm out of time for today - I will continue tomorrow.
> 
> It seems to me that the main point to step forward is to clarify the global
> design and features of the rpmsg-ctrl.
> Depending on the decision taken, this series could be trashed and rewritten from
> a blank page...To not lost to much time on the series don't hesitate to limit
> the review to the minimum.
> 

I doubt you will ever get clear guidelines on the whole solution.  I will get
back to you once I am done with the SMD driver, which should be in the
latter part of next week.

> Thanks,
> Arnaud
> 
> > 
> > Thanks,
> > Mathieu
> > 
> >> +	return NULL;
> >> +}
> >> +
> >>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
> >>  				 unsigned long arg)
> >>  {
> >>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> >> -
> >> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
> >> +	void __user *argp = (void __user *)arg;
> >> +	struct rpmsg_channel_info chinfo;
> >> +	struct rpmsg_endpoint_info eptinfo;
> >> +	struct rpmsg_device *newch;
> >> +
> >> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> >> +		return -EINVAL;
> >> +
> >> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> >> +		return -EFAULT;
> >> +
> >> +	/*
> >> +	 * In a frst step only the rpmsg_raw service is supported.
> >> +	 * The override is foorced to RPMSG_RAW_SERVICE
> >> +	 */
> >> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
> >> +	if (!chinfo.driver_override)
> >> +		return -ENODEV;
> >> +
> >> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> >> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> >> +	chinfo.src = eptinfo.src;
> >> +	chinfo.dst = eptinfo.dst;
> >> +
> >> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
> >> +	if (!newch) {
> >> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
> >> +		return -ENXIO;
> >> +	}
> >>  
> >>  	return 0;
> >>  };
> >> -- 
> >> 2.17.1
> >>
Arnaud Pouliquen Jan. 25, 2021, 10:52 a.m. UTC | #6
Hi Mathieu,

On 1/22/21 9:59 PM, Mathieu Poirier wrote:
> On Fri, Jan 22, 2021 at 02:05:27PM +0100, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>> On 1/22/21 12:52 AM, Mathieu Poirier wrote:
>>> On Tue, Dec 22, 2020 at 11:57:14AM +0100, Arnaud Pouliquen wrote:
>>>> Implement the ioctl function that parses the list of
>>>> rpmsg drivers registered to create an associated device.
>>>> To be ISO user API, in a first step, the driver_override
>>>> is only allowed for the RPMsg raw service, supported by the
>>>> rpmsg_char driver.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> ---
>>>>  drivers/rpmsg/rpmsg_ctrl.c | 43 ++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 41 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>>>> index 065e2e304019..8381b5b2b794 100644
>>>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>>>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>>>> @@ -56,12 +56,51 @@ static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static const char *rpmsg_ctrl_get_drv_name(u32 service)
>>>> +{
>>>> +	struct rpmsg_ctl_info *drv_info;
>>>> +
>>>> +	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
>>>> +		if (drv_info->ctrl->service == service)
>>>> +			return drv_info->ctrl->drv_name;
>>>> +	}
>>>> +
>>>
>>> I'm unsure about the above... To me this looks like what the .match() function
>>> of a bus would do.  And when I read Bjorn's comment he brought up the
>>> auxiliary_bus.  I don't know about the auxiliary_bus but it is worth looking
>>> into.  Registering with a bus would streamline a lot of the code in this
>>> patchset.
>>
>> As answered Bjorn, we already have the RPMsg bus to manage the rpmsg devices.
>> Look like duplication from my POV, except if the IOCTL does not manage channel
>> but only endpoint.
>>
>> In my design I considered that the rpmsg_ctrl creates a channel associated to a
>> rpmsg_device such as the RPMsg ns_announcement.
>>
>> Based on this assumption, if we implement the auxiliary_bus (or other) for the
>> rpmsg_ctrl a RPMsg driver will have to manage the probe by rpmsg_bus and by the
>> auxillary bus. The probe from the auxiliary bus would lead to the creation of an
>> RPMsg device on the rpmsg_bus, so a duplication with cross dependencies and
>> would probably make tricky the remove part.
>>
>> That said, I think the design depends on the functionality that should be
>> implemented in the rpmsg_ctrl. Here is an alternative approach based on the
>> auxiliary bus, which I'm starting to think about:
>>
>> The current approach of the rpmsg_char driver is to use the IOCTRL interface to
>> instantiate a cdev with an endpoint (the RPMsg device is associated with the
>> ioctl dev). This would correspond to the use of an auxiliary bus to manage local
>> endpoint creations.
>>
>> We could therefore consider an RPMsg name service based on an RPmsg device. This
>> RPMsg device would register a kind of "RPMsg service endpoint" driver on the
>> auxiliary rpmsg_ioctl bus.
>> The rpmsg_ctrl will be used to instantiate the endpoints for this RPMsg device.
>> on user application request the rpmsg_ctrl will call the appropriate auxiliary
>> device to create an endpoint.
>>
>> If we consider that one objective of this series is to allow application to
>> initiate the communication with the remote processor, so to be able to initiate
>> the service (ns announcement sent to the remote processor).
>> This implies that:
>> -either the RPMsg device has been probed first by a remote ns announcement or by
>> a Linux kernel driver using the "driver_override", to register an auxiliary
>> device. In this case an endpoint will be created associated to the RPMsg service
>> - or create a RPMsg device on first ioctl endpoint creation request, if it does
>> not exist (that could trig a NS announcement to remote processor).
>>
>> But I'm not sure that this approach would work with QCOM RPMsg backends...
>>
> 
> I don't think there is a way forward with this set without a clear understanding
> of the Glink and SMD drivers.  I have already spent a fair amount of time in the
> Glink driver and will continue on Monday with SMD.  
> 
>>>
>>> I'm out of time for today - I will continue tomorrow.
>>
>> It seems to me that the main point to step forward is to clarify the global
>> design and features of the rpmsg-ctrl.
>> Depending on the decision taken, this series could be trashed and rewritten from
>> a blank page...To not lost to much time on the series don't hesitate to limit
>> the review to the minimum.
>>
> 
> I doubt you will ever get clear guidelines on the whole solution.  I will get
> back to you once I am done with the SMD driver, which should be in the
> latter part of next week.


Thanks for your time past on this topic!

I don't expect a clear guidance but that we clarify the objective of this RPMsg
IOCTL. A first step would be sure that we are in line with the objective of the
RPMsg IOCTL.
For instance should we continue in a way to have the rpmsg_char more "rpmsg
service" generic, relying on a rpmsg_ioctl for the control part? Or should we
implement something independent (with is own API) to limit dependency with QCOM
backends constraints?
At the end, if implementing a IOCTL interface directly in the RPMsg TTY seems to
you and Bjorn simpler, I can also go on this way...

On my side I expect to find time this week to prototype a RPMSg ioctl using the
auxiliary bus to better understand involved mechanism.

Thanks,
Arnaud

> 
>> Thanks,
>> Arnaud
>>
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>> +	return NULL;
>>>> +}
>>>> +
>>>>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>>>>  				 unsigned long arg)
>>>>  {
>>>>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
>>>> -
>>>> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
>>>> +	void __user *argp = (void __user *)arg;
>>>> +	struct rpmsg_channel_info chinfo;
>>>> +	struct rpmsg_endpoint_info eptinfo;
>>>> +	struct rpmsg_device *newch;
>>>> +
>>>> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>>> +		return -EFAULT;
>>>> +
>>>> +	/*
>>>> +	 * In a frst step only the rpmsg_raw service is supported.
>>>> +	 * The override is foorced to RPMSG_RAW_SERVICE
>>>> +	 */
>>>> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
>>>> +	if (!chinfo.driver_override)
>>>> +		return -ENODEV;
>>>> +
>>>> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>>>> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>>>> +	chinfo.src = eptinfo.src;
>>>> +	chinfo.dst = eptinfo.dst;
>>>> +
>>>> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
>>>> +	if (!newch) {
>>>> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
>>>> +		return -ENXIO;
>>>> +	}
>>>>  
>>>>  	return 0;
>>>>  };
>>>> -- 
>>>> 2.17.1
>>>>
Mathieu Poirier Jan. 29, 2021, 12:13 a.m. UTC | #7
[...]

> > It seems to me that the main point to step forward is to clarify the global
> > design and features of the rpmsg-ctrl.
> > Depending on the decision taken, this series could be trashed and rewritten from
> > a blank page...To not lost to much time on the series don't hesitate to limit
> > the review to the minimum.
> > 
> 
> I doubt you will ever get clear guidelines on the whole solution.  I will get
> back to you once I am done with the SMD driver, which should be in the
> latter part of next week.
>

After looking at the rpmsg_chrdev driver, its current customers (i.e the Qcom
drivers), the rpmsg name service and considering the long term goals of this
patchset I have the following guidelines: 

1) I thought long and hard about how to split the current rpmsg_chrdev driver
between the control plane and the raw device plane and the end solution looks
much slimpler than I expected.  Exporting function rpmsg_eptdev_create() after
moving it to another file (along with other dependencies) should be all we need.
Calling rpmsg_eptdev_create() from rpmsg_ctrldev_ioctl() will automatically load
the new driver, the same way calling rpmsg_ns_register_device() from
rpmsg_probe() took care of loading the rpmsg_ns driver.

2) While keeping the control plane functionality related to
RPMSG_CREATE_EPT_IOCTL intact, introduce a new RPMSG_CREATE_DEV_IOCTL that will
allow for the instantiation of rpmsg_devices, exactly the same way a name service
announcement from a remote processor does.  I envision that code path to
eventually call rpmsg_create_channel().

3) Leave the rpmsg_channel_info structure intact and use the
rpmsg_channel_info::name to bind to a rpmsg_driver, exactly how it is currently
done for name service driver selection.  That will allow us to re-use the
current rpmsg_bus intrastructure, i.e rpmsg_bus::match(), without having to deal
with yet another bus type.  Proceeding this way gives us the opportunity to keep
the current channel name convention for other rpmch_chrdev users untouched.

4) In a prior conversation you indicated the intention of instantiating the
rpmsg_chrdev from the name service interface.  I agree with doing so but 
conjugating that with the RPMSG_CHAR kenrel define may be tricky.  I will wait
to see what you come up with.

I hope this helps.

Thanks,
Mathieu


 
> > Thanks,
> > Arnaud
> > 
> > > 
> > > Thanks,
> > > Mathieu
> > > 
> > >> +	return NULL;
> > >> +}
> > >> +
> > >>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
> > >>  				 unsigned long arg)
> > >>  {
> > >>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> > >> -
> > >> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
> > >> +	void __user *argp = (void __user *)arg;
> > >> +	struct rpmsg_channel_info chinfo;
> > >> +	struct rpmsg_endpoint_info eptinfo;
> > >> +	struct rpmsg_device *newch;
> > >> +
> > >> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> > >> +		return -EINVAL;
> > >> +
> > >> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> > >> +		return -EFAULT;
> > >> +
> > >> +	/*
> > >> +	 * In a frst step only the rpmsg_raw service is supported.
> > >> +	 * The override is foorced to RPMSG_RAW_SERVICE
> > >> +	 */
> > >> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
> > >> +	if (!chinfo.driver_override)
> > >> +		return -ENODEV;
> > >> +
> > >> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> > >> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> > >> +	chinfo.src = eptinfo.src;
> > >> +	chinfo.dst = eptinfo.dst;
> > >> +
> > >> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
> > >> +	if (!newch) {
> > >> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
> > >> +		return -ENXIO;
> > >> +	}
> > >>  
> > >>  	return 0;
> > >>  };
> > >> -- 
> > >> 2.17.1
> > >>
Arnaud Pouliquen Jan. 29, 2021, 9:45 a.m. UTC | #8
On 1/29/21 1:13 AM, Mathieu Poirier wrote:
> [...]
> 
>>> It seems to me that the main point to step forward is to clarify the global
>>> design and features of the rpmsg-ctrl.
>>> Depending on the decision taken, this series could be trashed and rewritten from
>>> a blank page...To not lost to much time on the series don't hesitate to limit
>>> the review to the minimum.
>>>
>>
>> I doubt you will ever get clear guidelines on the whole solution.  I will get
>> back to you once I am done with the SMD driver, which should be in the
>> latter part of next week.
>>
> 
> After looking at the rpmsg_chrdev driver, its current customers (i.e the Qcom
> drivers), the rpmsg name service and considering the long term goals of this
> patchset I have the following guidelines: 
> 
> 1) I thought long and hard about how to split the current rpmsg_chrdev driver
> between the control plane and the raw device plane and the end solution looks
> much slimpler than I expected.  Exporting function rpmsg_eptdev_create() after
> moving it to another file (along with other dependencies) should be all we need.
> Calling rpmsg_eptdev_create() from rpmsg_ctrldev_ioctl() will automatically load
> the new driver, the same way calling rpmsg_ns_register_device() from
> rpmsg_probe() took care of loading the rpmsg_ns driver.
> 
> 2) While keeping the control plane functionality related to
> RPMSG_CREATE_EPT_IOCTL intact, introduce a new RPMSG_CREATE_DEV_IOCTL that will
> allow for the instantiation of rpmsg_devices, exactly the same way a name service
> announcement from a remote processor does.  I envision that code path to
> eventually call rpmsg_create_channel().
> 
> 3) Leave the rpmsg_channel_info structure intact and use the
> rpmsg_channel_info::name to bind to a rpmsg_driver, exactly how it is currently
> done for name service driver selection.  That will allow us to re-use the
> current rpmsg_bus intrastructure, i.e rpmsg_bus::match(), without having to deal
> with yet another bus type.  Proceeding this way gives us the opportunity to keep
> the current channel name convention for other rpmch_chrdev users untouched.
> 
> 4) In a prior conversation you indicated the intention of instantiating the
> rpmsg_chrdev from the name service interface.  I agree with doing so but 
> conjugating that with the RPMSG_CHAR kenrel define may be tricky.  I will wait
> to see what you come up with.
> 
> I hope this helps.

Thank you for these guidelines! It need a bit of time to look at the details
(especially point 1) ), but your suggestion seems to me to be a good compromise.
I hope to come back soon with a new revision based on this point.

Regards,
Arnaud

> 
> Thanks,
> Mathieu
> 
> 
>  
>>> Thanks,
>>> Arnaud
>>>
>>>>
>>>> Thanks,
>>>> Mathieu
>>>>
>>>>> +	return NULL;
>>>>> +}
>>>>> +
>>>>>  static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
>>>>>  				 unsigned long arg)
>>>>>  {
>>>>>  	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
>>>>> -
>>>>> -	dev_info(&ctrldev->dev, "Control not yet implemented\n");
>>>>> +	void __user *argp = (void __user *)arg;
>>>>> +	struct rpmsg_channel_info chinfo;
>>>>> +	struct rpmsg_endpoint_info eptinfo;
>>>>> +	struct rpmsg_device *newch;
>>>>> +
>>>>> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>>>> +		return -EFAULT;
>>>>> +
>>>>> +	/*
>>>>> +	 * In a frst step only the rpmsg_raw service is supported.
>>>>> +	 * The override is foorced to RPMSG_RAW_SERVICE
>>>>> +	 */
>>>>> +	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
>>>>> +	if (!chinfo.driver_override)
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>>>>> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>>>>> +	chinfo.src = eptinfo.src;
>>>>> +	chinfo.dst = eptinfo.dst;
>>>>> +
>>>>> +	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
>>>>> +	if (!newch) {
>>>>> +		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
>>>>> +		return -ENXIO;
>>>>> +	}
>>>>>  
>>>>>  	return 0;
>>>>>  };
>>>>> -- 
>>>>> 2.17.1
>>>>>
diff mbox series

Patch

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 065e2e304019..8381b5b2b794 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -56,12 +56,51 @@  static int rpmsg_ctrl_dev_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static const char *rpmsg_ctrl_get_drv_name(u32 service)
+{
+	struct rpmsg_ctl_info *drv_info;
+
+	list_for_each_entry(drv_info, &rpmsg_drv_list, node) {
+		if (drv_info->ctrl->service == service)
+			return drv_info->ctrl->drv_name;
+	}
+
+	return NULL;
+}
+
 static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
 				 unsigned long arg)
 {
 	struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
-
-	dev_info(&ctrldev->dev, "Control not yet implemented\n");
+	void __user *argp = (void __user *)arg;
+	struct rpmsg_channel_info chinfo;
+	struct rpmsg_endpoint_info eptinfo;
+	struct rpmsg_device *newch;
+
+	if (cmd != RPMSG_CREATE_EPT_IOCTL)
+		return -EINVAL;
+
+	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
+		return -EFAULT;
+
+	/*
+	 * In a frst step only the rpmsg_raw service is supported.
+	 * The override is foorced to RPMSG_RAW_SERVICE
+	 */
+	chinfo.driver_override = rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
+	if (!chinfo.driver_override)
+		return -ENODEV;
+
+	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
+	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
+	chinfo.src = eptinfo.src;
+	chinfo.dst = eptinfo.dst;
+
+	newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
+	if (!newch) {
+		dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
+		return -ENXIO;
+	}
 
 	return 0;
 };