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