by Mathieu Poirier

[permalink] [raw]

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

[...]

> > 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
> > >>

2021-01-29 12:17:50

by Arnaud Pouliquen

[permalink] [raw]

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

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
>>>>>