Re: [PATCH V7 1/3] rpmsg: core: Add signal API support
From: Arnaud POULIQUEN
Date: Thu Jun 15 2023 - 12:20:38 EST
On 6/15/23 16:50, Bjorn Andersson wrote:
> On Thu, Jun 15, 2023 at 11:01:14AM +0200, Arnaud POULIQUEN wrote:
>>
>>
>> On 6/14/23 17:54, Bjorn Andersson wrote:
>>> On Sat, Apr 22, 2023 at 04:12:05PM +0530, Sarannya S wrote:
>>>> From: Deepak Kumar Singh <quic_deesin@xxxxxxxxxxx>
>>>>
>>>> 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 a2207c0..e8bbe05 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: pause/resume incoming data flow
>>>
>>> As shown in the discussion, it's still not clear what true/false means.
>>> Also, let's try to clarify that it's a request for the other side to do
>>> something:
>>>
>>> * rpmsg_set_flow_control() - request remote to pause/resume transmission
>>> * ...
>>> * @enable: flow restricted
>>> * ...
>>>
>>>
>>> PS. There's a stray space at the end of the line.
>>
>> The notion of flow restricted seems to me also ambiguous. It does
>> not specify if the stream is limited in term of bandwidth or stopped.
>>
>> What about using XON/XOFF as specified in software flow control[1]
>>
>> XOFF Pause transmission
>> XON Resume transmission
>>
>> or simply pause/resume definitions
>>
>
> I agree, that's still ambiguous.
>
> I was concerned about expressing it such that the reader would assume
> that calling this means there will be no more data coming, but there
> might be things in the queues etc. Expressing it in terms of the state
> of transmission is clearer.
>
>
> /*
> * rpmsg_set_flow_control() - request remote to pause/resume transmission
> ...
> * @enable: Pause transmission
> ...
> */
>
> Does that sound okay and clear to you?
Much better! I still have a nitpicking point :)
What about replacing @enable variable by @pause to align the variable with the
usage?
/*
* rpmsg_set_flow_control() - request remote to pause/resume transmission
...
* @pause: set to 1 to pause transmission, to 0 to resume the transmission
...
*/
Thanks,
Arnaud
>
> Thanks,
> Bjorn
>
>> [1]https://en.wikipedia.org/wiki/Software_flow_control
>>
>> Regards,
>> Arnaud
>>
>>>
>>>> + * @dst: destination address of the endpoint
>>>> + *
>>>> + * Return: 0 on success and an appropriate error value on failure.
>>>> + */
>>>> +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable, u32 dst)
>>>> +{
>>>> + if (WARN_ON(!ept))
>>>> + return -EINVAL;
>>>> + if (!ept->ops->set_flow_control)
>>>> + return -ENXIO;
>>>> +
>>>> + return ept->ops->set_flow_control(ept, enable, dst);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(rpmsg_set_flow_control);
>>>> +
>>>> +/**
>>>> * rpmsg_get_mtu() - get maximum transmission buffer size for sending message.
>>>> * @ept: the rpmsg endpoint
>>>> *
>>>> @@ -539,6 +558,8 @@ static int rpmsg_dev_probe(struct device *dev)
>>>>
>>>> rpdev->ept = ept;
>>>> rpdev->src = ept->addr;
>>>> +
>>>> + ept->flow_cb = rpdrv->flowcontrol;
>>>> }
>>>>
>>>> err = rpdrv->probe(rpdev);
>>>> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
>>>> index 39b646d..b6efd3e 100644
>>>> --- a/drivers/rpmsg/rpmsg_internal.h
>>>> +++ b/drivers/rpmsg/rpmsg_internal.h
>>>> @@ -55,6 +55,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
>>>> * @get_mtu: see @rpmsg_get_mtu(), optional
>>>> *
>>>> * Indirection table for the operations that a rpmsg backend should implement.
>>>> @@ -75,6 +76,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, u32 dst);
>>>> ssize_t (*get_mtu)(struct rpmsg_endpoint *ept);
>>>> };
>>>>
>>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>> index 523c98b..a0e9d38 100644
>>>> --- a/include/linux/rpmsg.h
>>>> +++ b/include/linux/rpmsg.h
>>>> @@ -64,12 +64,14 @@ struct rpmsg_device {
>>>> };
>>>>
>>>> typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
>>>> +typedef int (*rpmsg_flowcontrol_cb_t)(struct rpmsg_device *, void *, bool);
>>>>
>>>> /**
>>>> * 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
>>>> + * @flow_cb: remote flow control callback handler
>>>> * @cb_lock: must be taken before accessing/changing @cb
>>>> * @addr: local rpmsg address
>>>> * @priv: private data for the driver's use
>>>> @@ -92,6 +94,7 @@ struct rpmsg_endpoint {
>>>> struct rpmsg_device *rpdev;
>>>> struct kref refcount;
>>>> rpmsg_rx_cb_t cb;
>>>> + rpmsg_flowcontrol_cb_t flow_cb;
>>>> struct mutex cb_lock;
>>>> u32 addr;
>>>> void *priv;
>>>> @@ -106,6 +109,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
>>>> + * @flowcontrol: invoked when remote side flow control status is received
>>>
>>> s/status/request/
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> */
>>>> struct rpmsg_driver {
>>>> struct device_driver drv;
>>>> @@ -113,6 +117,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 (*flowcontrol)(struct rpmsg_device *, void *, bool);
>>>> };
>>>>
>>>> static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val)
>>>> @@ -192,6 +197,8 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>>>>
>>>> ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept);
>>>>
>>>> +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable, u32 dst);
>>>> +
>>>> #else
>>>>
>>>> static inline int rpmsg_register_device_override(struct rpmsg_device *rpdev,
>>>> @@ -316,6 +323,14 @@ static inline ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept)
>>>> return -ENXIO;
>>>> }
>>>>
>>>> +static inline int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable, u32 dst)
>>>> +{
>>>> + /* 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
>>>>