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

[Arnaud POULIQUEN]

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.

> 
>> + *
>> + * 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))?

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

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