Re: [PATCH v6 06/10] rpmsg: Introduce rpmsg_create_default_ept function

[Arnaud POULIQUEN]

On 11/3/21 5:57 PM, Bjorn Andersson wrote:
> On Tue 02 Nov 11:56 CDT 2021, Arnaud POULIQUEN wrote:
> 
>>
>>
>> On 11/1/21 6:37 PM, Bjorn Andersson wrote:
>>> On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:
>>>
>>>> By providing a callback in the rpmsg_driver structure, the rpmsg devices
>>>> can be probed with a default endpoint created.
>>>>
>>>> In this case, it is not possible to associated to this endpoint private data
>>>> that could allow the driver to retrieve the context.
>>>>
>>>> This helper function allows rpmsg drivers to create a default endpoint
>>>> on runtime with an associated private context.
>>>>
>>>> For example, a driver might create a context structure on the probe and
>>>> want to provide that context as private data for the default rpmsg
>>>> callback.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
>>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>>>> Tested-by: Julien Massot <julien.massot@xxxxxxx>
>>>> ---
>>>>  drivers/rpmsg/rpmsg_core.c | 51 ++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/rpmsg.h      | 13 ++++++++++
>>>>  2 files changed, 64 insertions(+)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>>>> index 53162038254d..92557c49d460 100644
>>>> --- a/drivers/rpmsg/rpmsg_core.c
>>>> +++ b/drivers/rpmsg/rpmsg_core.c
>>>> @@ -132,6 +132,57 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
>>>>  }
>>>>  EXPORT_SYMBOL(rpmsg_destroy_ept);
>>>>  
>>>> +/**
>>>> + * rpmsg_create_default_ept() - create a default rpmsg_endpoint for a rpmsg device
>>>> + * @rpdev: rpmsg channel device
>>>> + * @cb: rx callback handler
>>>> + * @priv: private data for the driver's use
>>>> + * @chinfo: channel_info with the local rpmsg address to bind with @cb
>>>> + *
>>>> + * On register_rpmsg_driver if no callback is provided in the rpmsg_driver structure,
>>>> + * no endpoint is created when the device is probed by the rpmsg bus.
>>>> + *
>>>> + * This function returns a pointer to the default endpoint if already created or creates
>>>> + * an endpoint and assign it as the default endpoint of the rpmsg device.
>>>
>>> But if the driver didn't specify a callback, when would this ever
>>> happen?
>>
>> Not sure to understand your point here...
>> Do you mean that something is missing in description such as:
>>  * On register_rpmsg_driver if no callback is provided in the rpmsg_driver
>>  * structure, no endpoint is created when the device is probed by the rpmsg bus.
>>  * The rpmsg driver can call rpmsg_create_default_ept during or after its
>>  * probing to register a default endpoint with an associated callback and @priv
>>  * context.
>>
> 
> I was referring specifically to the case of rpmsg_create_default_ept()
> being called on a rpmsg_device that already has a rpdev->ept.
> 
> Afaict this would either be because the driver did specify a callback or
> because the driver didn't but has already called
> rpmsg_create_default_ept().
> 
> Both cases sounds like invalid usage.
> 
>>>
>>>> + *
>>>> + * Drivers should provide their @rpdev channel (so the new endpoint would belong
>>>> + * to the same remote processor their channel belongs to), an rx callback
>>>> + * function, an optional private data (which is provided back when the
>>>> + * rx callback is invoked), and an address they want to bind with the
>>>> + * callback. If @addr is RPMSG_ADDR_ANY, then rpmsg_create_ept will
>>>> + * dynamically assign them an available rpmsg address (drivers should have
>>>> + * a very good reason why not to always use RPMSG_ADDR_ANY here).
>>>> + *
>>>> + * Returns a pointer to the endpoint on success, or NULL on error.
>>>
>>> Correct kerneldoc is "Return: ..."
>>
>> I will update this
>>
>>>
>>>> + */
>>>> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
>>>> +						rpmsg_rx_cb_t cb, void *priv,
>>>> +						struct rpmsg_channel_info chinfo)
>>>> +{
>>>> +	struct rpmsg_endpoint *ept;
>>>> +
>>>> +	if (WARN_ON(!rpdev))
>>>> +		return NULL;
>>>> +
>>>> +	/* It does not make sense to create a default endpoint without a callback. */
>>>> +	if (!cb)
>>>> +		return NULL;
>>>> +
>>>> +	if (rpdev->ept)
>>>> +		return rpdev->ept;
>>>
>>> How does the caller know if they should call rpmsg_destroy_ept() on the
>>> returned ept or not?
>>
>> This case is probably a bug. What about replacing the condition by
>> if(WARN_ON(rpdev->ept))?
>>
> 
> Right, I don't think it will be possible for the client driver to do the
> right thing based on this logic.
> 
>>>
>>>> +
>>>> +	ept = rpdev->ops->create_ept(rpdev, cb, priv, chinfo);
>>>> +	if (!ept)
>>>> +		return NULL;
>>>> +
>>>> +	/* Assign the new endpoint as default endpoint */
>>>> +	rpdev->ept = ept;
>>>> +	rpdev->src = ept->addr;
>>>> +
>>>> +	return ept;
>>>> +}
>>>> +EXPORT_SYMBOL(rpmsg_create_default_ept);
>>>> +
>>>>  /**
>>>>   * rpmsg_send() - send a message across to the remote processor
>>>>   * @ept: the rpmsg endpoint
>>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>> index 6fe51549d931..b071ac17ff78 100644
>>>> --- a/include/linux/rpmsg.h
>>>> +++ b/include/linux/rpmsg.h
>>>> @@ -172,6 +172,9 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *);
>>>>  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *,
>>>>  					rpmsg_rx_cb_t cb, void *priv,
>>>>  					struct rpmsg_channel_info chinfo);
>>>> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
>>>
>>> Is there ever a case where someone outside drivers/rpmsg/ should call
>>> this function?
>>
>> A rpmsg service driver could call it to generate the ns announcement after
>> the probe (for instance on a sysfs open).
>> (Please have a look to [PATCH v6 10/10] rpmsg: core: send a ns announcement when
>> a default endpoint is created)
>>
> 
> I'm still not convinced that it's correct to do NS only for primary
> endpoints.
> 
> In particular looking down the path where you are instantiating services
> on the Linux side; e.g. what if you want your driver to probe on some
> control channel but have the actual data flow on a separate channel
> (something I believe Loic talked about earlier).
> 
> How would the remote side know about that second endpoint if the NS
> doesn't announce it?

Right, today it is a limitation of the announcement mechanism. The service is
linked to rpmsg_dev_probe and rpmsg_dev_remove (except if the backend implements
an announcement on create_ept & destroy_ept ops).

The rpmsg_create_default_ept follows this behavior.
As result of this patch, the limitation will exist in both directions.

Another limitation is an ack of the NS announcement to provide the associated
remote address.

Extending rpmsg_ns.c to add new messages could solve these limitations, but that
would be a separate topic.

Regards,
Arnaud

> 
> Regards,
> Bjorn
> 
>> Thanks,
>> Arnaud
>>
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> +						rpmsg_rx_cb_t cb, void *priv,
>>>> +						struct rpmsg_channel_info chinfo);
>>>>  
>>>>  int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
>>>>  int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
>>>> @@ -236,6 +239,16 @@ static inline struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev
>>>>  	return NULL;
>>>>  }
>>>>  
>>>> +static inline struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
>>>> +							      rpmsg_rx_cb_t cb, void *priv,
>>>> +							      struct rpmsg_channel_info chinfo)
>>>> +{
>>>> +	/* This shouldn't be possible */
>>>> +	WARN_ON(1);
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>>  static inline int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
>>>>  {
>>>>  	/* This shouldn't be possible */
>>>> -- 
>>>> 2.17.1
>>>>