lkml.org 
[lkml]   [2021]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 04/16] rpmsg: ctrl: implement the ioctl function to create device
    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
    > >>

    \
     
     \ /
      Last update: 2021-01-22 22:03    [W:4.117 / U:0.844 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site