| Message ID | 20201222105726.16906-5-arnaud.pouliquen@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series | introduce generic IOCTL interface for RPMsg channels management | expand |
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 >
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 >>
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 >
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 >>
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 > >>
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 >>>>
[...] > > 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 > > >>
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 --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; };
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(-)