Re: [PATCH V1 1/3] rpmsg: core: Add signal API support

From: Mathieu Poirier
Date: Thu Oct 21 2021 - 13:52:07 EST

Next message: Shuah Khan: "[PATCH] selftests: kvm: fix mismatched fclose() after popen()"
Previous message: Vlastimil Babka: "Re: [next] [dragonboard 410c] Unable to handle kernel paging request at virtual address 00000000007c4240"
In reply to: Bjorn Andersson: "Re: [PATCH V1 1/3] rpmsg: core: Add signal API support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Oct 18, 2021 at 08:05:00PM -0700, Bjorn Andersson wrote:
> On Mon 18 Oct 10:52 PDT 2021, Mathieu Poirier wrote:
>
> > On Sat, Oct 16, 2021 at 12:01:31AM -0500, Bjorn Andersson wrote:
> > > On Mon 11 Oct 13:02 CDT 2021, Mathieu Poirier wrote:
> > >
> > > > Good day Deepak,
> > > >
> > > > On Thu, Sep 30, 2021 at 09:02:01PM +0530, Deepak Kumar Singh wrote:
> > > > > Some transports like Glink support the state notifications between
> > > > > clients using signals similar to serial protocol signals.
> > > > > Local glink client drivers can send and receive signals to glink
> > > > > clients running on remote processors.
> > > > >
> > > > > Add apis to support sending and receiving of signals by rpmsg clients.
> > > > >
> > > > > Signed-off-by: Deepak Kumar Singh <deesin@xxxxxxxxxxxxxx>
> > > > > ---
> > > > > drivers/rpmsg/rpmsg_core.c | 21 +++++++++++++++++++++
> > > > > drivers/rpmsg/rpmsg_internal.h | 2 ++
> > > > > include/linux/rpmsg.h | 15 +++++++++++++++
> > > > > 3 files changed, 38 insertions(+)
> > > > >
> > > > > diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> > > > > index 9151836..5cae50c 100644
> > > > > --- a/drivers/rpmsg/rpmsg_core.c
> > > > > +++ b/drivers/rpmsg/rpmsg_core.c
> > > > > @@ -327,6 +327,24 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
> > > > > }
> > > > > EXPORT_SYMBOL(rpmsg_trysend_offchannel);
> > > > >
> > > > > +/**
> > > > > + * rpmsg_set_flow_control() - sets/clears searial flow control signals
> > > > > + * @ept: the rpmsg endpoint
> > > > > + * @enable: enable or disable serial flow control
> > > > > + *
> > > > > + * Returns 0 on success and an appropriate error value on failure.
> > > > > + */
> > > > > +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
> > > > > +{
> > > > > + if (WARN_ON(!ept))
> > > > > + return -EINVAL;
> > > > > + if (!ept->ops->set_flow_control)
> > > > > + return -ENXIO;
> > > > > +
> > > > > + return ept->ops->set_flow_control(ept, enable);
> > > > > +}
> > > > > +EXPORT_SYMBOL(rpmsg_set_flow_control);
> > > > > +
> > > >
> > > > I'm looking at this patchset as the introduction of an out-of-bound
> > > > control interface. But looking at the implementation of the GLINK's
> > > > set_flow_control() the data is sent in-band, making me perplexed about
> > > > introducing a new rpmsg_endpoint_ops for something that could be done
> > > > from user space. Especially when user space is triggering the message
> > > > with an ioctl in patch 3.
> > > >
> > >
> > > GLINK is built around one fifo per processor pair, similar to a
> > > virtqueue. So the signal request is muxed in the same pipe as data
> > > requests, but the signal goes alongside data request, not within them.
> > >
> >
> > I reflected more on this and I can see scenarios where sending control flow
> > messages alongside other data packet could be the only solution. How the signal
> > is implemented is a platform specific choice. I believe the same kind of
> > delivery mechanism implemented by kick() functions would be the best way to go
> > but if that isn't possible then in-band, as suggested in this patchset, is
> > better than nothing.
> >
> > > > Moreover this interface is case specific and doesn't reflect the
> > > > generic nature found in ept->sig_cb.
> > > >
> > >
> > > The previous proposal from Deepak was to essentially expose the normal
> > > tty flags all the way down to the rpmsg driver. But I wasn't sure how
> > > those various flags should be interpreted in the typical rpmsg driver.
> >
> > That is interesting. I was hoping to keep the user level signal interfaces
> > generic and let the drivers do as they please with them. I see your point
> > though and this might be one of those cases where there isn't a right or wrong
> > answer.
> >
>
> I'm definitely in favor of something generic, my objection was simply to
> inherit the tty interface as that generic thing.
>
> If nothing else I myself have a hard time understanding the actual
> meaning of those bits and tend to have to look them up every time.
>
> > >
> > > I therefor asked Deepak to change it so the rpmsg api would contain a
> > > single "pause incoming data"/"resume incoming data" - given that this is
> > > a wish that we've seen in a number of discussions.
> > >
> >
> > This will work for as long as we have a single usecase for it, i.e flow control.
> > I fear things will quickly get out of hands when more messages are needed, hence
> > the idea of keeping things as generic as possible.
> >
>
> Do you have any other types of signals in mind?
>

I currently don't have anything in mind but I know it will happen at one point,
hence hoping to proceed differently than having one ioctl per signal.

> > >
> > > Unfortunately I don't have any good suggestion for how we could
> > > implement this in the virtio backend at this time, but with the muxing
> > > of all the different channels in the same virtqueue it would be good for
> > > a driver to able to pause the inflow on a specific endpoint, to avoid
> > > stalling other communication when a driver can't receive more messages.
> >
> > Humm...
> >
> > For application to remote processor things would work the same as it does for
> > GLINK, whether the communication is done from a rpmsg_driver (as in
> > rpmsg_client_sample.c) or from user space via something like the rpmsg_char.c
> > driver.
> >
> > For remote processor to application processor the interruptions would need to
> > carry the destination address of the endpoint, which might not be possible.
> >
> > All this discussion proves that we really need to think about this before moving
> > forward, especially with Arnaud's ongoing refactoring of the rpmsg_char driver.
> >
>
> The concept of flow control comes pretty natural in both GLINK and SMD,
> given that an endpoint is the local representation of an established
> link to an entity on the other side - while in virtio rpmsg endpoints
> doesn't really have a state and a limited sense of there being something
> on the other side.
>
> So I agree that flow controlling in virtio rpmsg could have unforeseen
> consequences e.g. by a service being blocked forever because it's
> waiting for "flow resume" from an endpoint that never existed.
>
> But I believe the impact of this is that we need to accept that there
> will be cases where the flow control requests can't be fulfilled; such
> as a loose rpmsg_endpoint without a predefined dst address or when
> communicating with a remote that predates the protocol extensions that
> will be necessary.
>
> Regards,
> Bjorn
>
> > Thanks,
> > Mathieu
> >
> > >
> > > Regards,
> > > Bjorn
> > >
> > > > > /*
> > > > > * match a rpmsg channel with a channel info struct.
> > > > > * this is used to make sure we're not creating rpmsg devices for channels
> > > > > @@ -514,6 +532,9 @@ static int rpmsg_dev_probe(struct device *dev)
> > > > >
> > > > > rpdev->ept = ept;
> > > > > rpdev->src = ept->addr;
> > > > > +
> > > > > + if (rpdrv->signals)
> > > > > + ept->sig_cb = rpdrv->signals;
> > > > > }
> > > > >
> > > > > err = rpdrv->probe(rpdev);
> > > > > diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> > > > > index a76c344..dcb2ec1 100644
> > > > > --- a/drivers/rpmsg/rpmsg_internal.h
> > > > > +++ b/drivers/rpmsg/rpmsg_internal.h
> > > > > @@ -53,6 +53,7 @@ struct rpmsg_device_ops {
> > > > > * @trysendto: see @rpmsg_trysendto(), optional
> > > > > * @trysend_offchannel: see @rpmsg_trysend_offchannel(), optional
> > > > > * @poll: see @rpmsg_poll(), optional
> > > > > + * @set_flow_control: see @rpmsg_set_flow_control(), optional
> > > > > *
> > > > > * Indirection table for the operations that a rpmsg backend should implement.
> > > > > * In addition to @destroy_ept, the backend must at least implement @send and
> > > > > @@ -72,6 +73,7 @@ struct rpmsg_endpoint_ops {
> > > > > void *data, int len);
> > > > > __poll_t (*poll)(struct rpmsg_endpoint *ept, struct file *filp,
> > > > > poll_table *wait);
> > > > > + int (*set_flow_control)(struct rpmsg_endpoint *ept, bool enable);
> > > > > };
> > > > >
> > > > > struct device *rpmsg_find_device(struct device *parent,
> > > > > diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> > > > > index d97dcd0..b805c70 100644
> > > > > --- a/include/linux/rpmsg.h
> > > > > +++ b/include/linux/rpmsg.h
> > > > > @@ -62,12 +62,14 @@ struct rpmsg_device {
> > > > > };
> > > > >
> > > > > typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
> > > > > +typedef int (*rpmsg_rx_sig_t)(struct rpmsg_device *, void *, u32);
> > > > >
> > > > > /**
> > > > > * struct rpmsg_endpoint - binds a local rpmsg address to its user
> > > > > * @rpdev: rpmsg channel device
> > > > > * @refcount: when this drops to zero, the ept is deallocated
> > > > > * @cb: rx callback handler
> > > > > + * @sig_cb: rx serial signal handler
> > > > > * @cb_lock: must be taken before accessing/changing @cb
> > > > > * @addr: local rpmsg address
> > > > > * @priv: private data for the driver's use
> > > > > @@ -90,6 +92,7 @@ struct rpmsg_endpoint {
> > > > > struct rpmsg_device *rpdev;
> > > > > struct kref refcount;
> > > > > rpmsg_rx_cb_t cb;
> > > > > + rpmsg_rx_sig_t sig_cb;
> > > > > struct mutex cb_lock;
> > > > > u32 addr;
> > > > > void *priv;
> > > > > @@ -104,6 +107,7 @@ struct rpmsg_endpoint {
> > > > > * @probe: invoked when a matching rpmsg channel (i.e. device) is found
> > > > > * @remove: invoked when the rpmsg channel is removed
> > > > > * @callback: invoked when an inbound message is received on the channel
> > > > > + * @signals: invoked when a serial signal change is received on the channel
> > > > > */
> > > > > struct rpmsg_driver {
> > > > > struct device_driver drv;
> > > > > @@ -111,6 +115,7 @@ struct rpmsg_driver {
> > > > > int (*probe)(struct rpmsg_device *dev);
> > > > > void (*remove)(struct rpmsg_device *dev);
> > > > > int (*callback)(struct rpmsg_device *, void *, int, void *, u32);
> > > > > + int (*signals)(struct rpmsg_device *rpdev, void *priv, u32);
> > > > > };
> > > > >
> > > > > static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val)
> > > > > @@ -186,6 +191,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
> > > > > __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
> > > > > poll_table *wait);
> > > > >
> > > > > +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable);
> > > > > +
> > > > > #else
> > > > >
> > > > > static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
> > > > > @@ -296,6 +303,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static inline int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable);
> > > > > +{
> > > > > + /* This shouldn't be possible */
> > > > > + WARN_ON(1);
> > > > > +
> > > > > + return -ENXIO;
> > > > > +}
> > > > > +
> > > > > #endif /* IS_ENABLED(CONFIG_RPMSG) */
> > > > >
> > > > > /* use a macro to avoid include chaining to get THIS_MODULE */
> > > > > --
> > > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > > > a Linux Foundation Collaborative Project
> > > > >

Next message: Shuah Khan: "[PATCH] selftests: kvm: fix mismatched fclose() after popen()"
Previous message: Vlastimil Babka: "Re: [next] [dragonboard 410c] Unable to handle kernel paging request at virtual address 00000000007c4240"
In reply to: Bjorn Andersson: "Re: [PATCH V1 1/3] rpmsg: core: Add signal API support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]