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