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

[Bjorn Andersson <andersson@kernel.org>]

On Tue, Jan 03, 2023 at 02:50:13PM +0100, Arnaud POULIQUEN wrote:
> Hello,
> 
> On 12/27/22 16:32, Bjorn Andersson wrote:
> > On Wed, Dec 21, 2022 at 05:12:22PM +0100, Arnaud POULIQUEN wrote:
> >> Hello,
> >>
> >> On 12/7/22 14:04, Sarannya S wrote:
> >>> Some transports like Glink support the state notifications between
> >>> clients using flow control signals similar to serial protocol signals.
> >>> Local glink client drivers can send and receive flow control status
> >>> to glink clients running on remote processors.
> >>>
> >>> Add APIs to support sending and receiving of flow control status by
> >>> rpmsg clients.
> >>>
> >>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
> >>> Signed-off-by: Sarannya S <quic_sarannya@quicinc.com>
> >>> ---
> >>>  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 d6dde00e..77aeba0 100644
> >>> --- a/drivers/rpmsg/rpmsg_core.c
> >>> +++ b/drivers/rpmsg/rpmsg_core.c
> >>> @@ -331,6 +331,25 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
> >>>  EXPORT_SYMBOL(rpmsg_trysend_offchannel);
> >>>  
> >>>  /**
> >>> + * rpmsg_set_flow_control() - sets/clears serial flow control signals
> >>> + * @ept:	the rpmsg endpoint
> >>> + * @enable:	enable or disable serial flow control
> >>
> >> What does it mean "enable and disable serial flow control"?
> >> Do you speak about the flow control feature or the data flow itself?
> >>
> > 
> > Good point, the purpose of the boolean is to "request throttling of the
> > incoming data flow".
> > 
> >> I guess it is the activation/deactivation of the data stream
> >> regarding Bjorn's comment in V1:
> >>
> >> "I therefore 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."
> >>
> >> For me this is the software flow control:
> >> https://en.wikipedia.org/wiki/Software_flow_control
> >>
> >> I would suggest not limiting the control only to activation/deactivation but to
> >> offer more flexibility in terms of services. replace the boolean by a bitmap
> >> would allow to extend it later.
> >>
> >> For instance by introducing 2 definitions:
> >>
> >> /* RPMSG pause transmission request:
> >>  * sent to the remote endpoint to request to suspend its transmission */
> >>  */
> >> #define RPMSG_FC_PT_REQ  (1 << 0)
> > 
> > enable = true
> > 
> >>
> >> /* RPMSG resume transmission request
> >>  * sent to the remote endpoint to allow to resume its transmission
> >>  */
> >> #define RPMSG_FC_RT_REQ  (1 << 1)
> >>
> > 
> > enable = false
> 
> Do you mean that it should be only one definition? If yes you are right
> only one definition is sufficient for the pause/resume
> 

Yes, I envision this being used for cases such as rpmsg_char being able
to send a "I already have 1MB of data in my sk_buf_head queue, please
don't send me any more data for now".

> > 
> >> Then we could add (in a next step) some other flow controls such as
> >> /* RPMSG pause transmission information
> >>  * Sent to the remote endpoint to inform that no more data will be sent
> >>  * until the reception of RPMSG_FC_RT_INFO
> >>  */
> >> #define RPMSG_FC_PT_INFO  (1 << 16)
> >> #define RPMSG_FC_RT_INFO  (1 << 16)
> >>
> > 
> > I presume you're looking for a usage pattern where the client would send
> > this to the remote and then the flow control mechanism would be used for
> > the remote end to request more data.
> > 
> > I find Deepak's (adjusted) proposal to be generic and to the point, and
> > your proposal builds unnecessary "flexibility" into this same mechanism.
> > 
> > If you have a rpmsg protocol where the client is expected to sit
> > waiting, and upon a request from the remote side send another piece of
> > data, why don't you just build this into the application protocol?  That
> > way your application would work over both transports with and without
> > flow control...
> > 
> > 
> > Perhaps I'm misunderstanding what you're asking for?
> 
> With the RPMSG_FC_PT_INFO example I had in mind the possibility to implement PM
> runtime.
> 

Which device/part are you going to runtime PM suspend using this?

> But my main point here is to allow to extend the flow control in future.
> or instance an comment in OpenAMP PR part [1] was:
> 
> "ON/OFF info isn't enough in the advanced flow control since the additional info
> is required(e.g. the slide window, round trip delay, congestion etc..)."
> 
> [1]https://github.com/OpenAMP/open-amp/pull/394#discussion_r878363627

We don't have a way to apply back pressure today, so I have a hard time
imagining the use cases and the implementation of such advanced flow
control.

Reading your proposal again, I don't think that's flow control, that's a
mechanism for requesting notifications. Either way, the mechanism seems
orthogonal to rpmsg_set_flow_control() - even if they were implemented
using the same mechanism in the underlying transport.

> 
> Using a @enable boolean would imply to create new ops if someone want to extend
> the flow control (to keep legacy compatibility). Using a bit map for the
> parameter could ease a future extension.
> 

This is a kernel-internal API, a boolean "flow or now flow" is
sufficient for what Qualcomm is asking and the ioctl is the only new
external mechanism introduced.

I have no concerns extending or altering this as the use cases appear.

Regards,
Bjorn