From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Ohad Ben-Cohen <ohad@wizery.com>,
Mathieu Poirier <mathieu.poirier@linaro.org>,
<linux-remoteproc@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
<linux-stm32@st-md-mailman.stormreply.com>,
<julien.massot@iot.bzh>
Subject: Re: [PATCH v6 06/10] rpmsg: Introduce rpmsg_create_default_ept function
Date: Tue, 2 Nov 2021 17:56:51 +0100 [thread overview]
Message-ID: <d1352a00-bc6d-91ae-b902-622c75448e3a@foss.st.com> (raw)
In-Reply-To: <YYAlzvXns4Ejxa6S@builder.lan>
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@foss.st.com>
>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Tested-by: Julien Massot <julien.massot@iot.bzh>
>> ---
>> 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
>>
next prev parent reply other threads:[~2021-11-02 16:57 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-22 12:54 [PATCH v6 00/10] Restructure the rpmsg_char driver and introduce rpmsg_ctrl driver Arnaud Pouliquen
2021-10-22 12:54 ` [PATCH v6 01/10] rpmsg: char: Export eptdev create an destroy functions Arnaud Pouliquen
2021-11-01 16:55 ` Bjorn Andersson
2021-11-02 14:52 ` Arnaud POULIQUEN
2021-10-22 12:54 ` [PATCH v6 02/10] rpmsg: create the rpmsg class in core instead of in rpmsg char Arnaud Pouliquen
2021-11-01 16:57 ` Bjorn Andersson
2021-11-02 15:23 ` Arnaud POULIQUEN
2021-11-02 16:38 ` Bjorn Andersson
2021-11-02 17:11 ` Arnaud POULIQUEN
2021-11-03 17:48 ` Bjorn Andersson
2021-10-22 12:54 ` [PATCH v6 03/10] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl Arnaud Pouliquen
2021-11-01 17:07 ` Bjorn Andersson
2021-11-02 15:42 ` Arnaud POULIQUEN
2021-10-22 12:54 ` [PATCH v6 04/10] rpmsg: Update rpmsg_chrdev_register_device function Arnaud Pouliquen
2021-11-01 17:14 ` Bjorn Andersson
2021-10-22 12:54 ` [PATCH v6 05/10] rpmsg: char: Refactor rpmsg_chrdev_eptdev_create function Arnaud Pouliquen
2021-10-22 12:54 ` [PATCH v6 06/10] rpmsg: Introduce rpmsg_create_default_ept function Arnaud Pouliquen
2021-11-01 17:37 ` Bjorn Andersson
2021-11-02 16:56 ` Arnaud POULIQUEN [this message]
2021-11-03 16:57 ` Bjorn Andersson
2021-11-03 17:35 ` Arnaud POULIQUEN
2021-10-22 12:54 ` [PATCH v6 07/10] rpmsg: char: Add possibility to use default endpoint of the rpmsg device Arnaud Pouliquen
2021-10-22 12:54 ` [PATCH v6 08/10] rpmsg: char: Introduce the "rpmsg-raw" channel Arnaud Pouliquen
2021-10-22 12:54 ` [PATCH v6 09/10] rpmsg: ctrl: Introduce new RPMSG_CREATE/RELEASE_DEV_IOCTL controls Arnaud Pouliquen
2021-10-22 12:54 ` [PATCH v6 10/10] rpmsg: core: send a ns announcement when a default endpoint is created Arnaud Pouliquen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d1352a00-bc6d-91ae-b902-622c75448e3a@foss.st.com \
--to=arnaud.pouliquen@foss.st.com \
--cc=bjorn.andersson@linaro.org \
--cc=julien.massot@iot.bzh \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=mathieu.poirier@linaro.org \
--cc=ohad@wizery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).