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

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

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