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