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@xxxxxxxxxxx> > >>> Signed-off-by: Sarannya S <quic_sarannya@xxxxxxxxxxx> > >>> --- > >>> 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
