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

[Arnaud POULIQUEN]

Hello Deepak,

On 3/29/22 13:00, Deepak Kumar Singh wrote:
> 
> On 3/23/2022 4:27 PM, Arnaud POULIQUEN wrote:
>>
>> On 3/11/22 22:11, Bjorn Andersson wrote:
>>> On Tue 18 Jan 13:43 CST 2022, 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 <quic_deesin@xxxxxxxxxxx>
>>>> ---
>>>>   drivers/rpmsg/rpmsg_core.c     | 21 +++++++++++++++++++++
>>>>   drivers/rpmsg/rpmsg_internal.h |  2 ++
>>>>   include/linux/rpmsg.h          | 14 ++++++++++++++
>>>>   3 files changed, 37 insertions(+)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>>>> index d3eb600..6712418 100644
>>>> --- a/drivers/rpmsg/rpmsg_core.c
>>>> +++ b/drivers/rpmsg/rpmsg_core.c
>>>> @@ -328,6 +328,24 @@ 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
>>>> + *
>>>> + * Return: 0 on success and an appropriate error value on failure.
>>>> + */
>>>> +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
>>> This API looks nice and clean and deals with flow control.
>> seems to me ambiguous API... what does it means flow control enable?
>> Does it means that the flow control is enable or that the the local
>> endpoint is ready to receive?
> This means that local endpoint is ready to receive data.
>>
>> Could we consider here that it is more a bind/unbind of the endpoint?
> 
> Endpoint will remain bind, only data on that endpoint is not expected if
> flow control is disabled.
> 
> If flow control is enabled remote client can send data.
> 
>>>> +{
>>>> +    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);
>>>> +
>>>> +/**
>>>>    * rpmsg_get_mtu() - get maximum transmission buffer size for
>>>> sending message.
>>>>    * @ept: the rpmsg endpoint
>>>>    *
>>>> @@ -535,6 +553,9 @@ static int rpmsg_dev_probe(struct device *dev)
>>>>             rpdev->ept = ept;
>>>>           rpdev->src = ept->addr;
>>>> +
>>>> +        if (rpdrv->signals)
>> seems an useless check
> 
> Some rpmsg cleints may not want to deal with flow control and not
> provide signal callback.
> 
> In such case this check is needed.
> 

my point here is that if rpdrv->signals is NULL,
then  ept->sig_cb will be NULL with or without the check.

>>>> +            ept->sig_cb = rpdrv->signals;
>>>>       }
>>>>         err = rpdrv->probe(rpdev);
>>>> diff --git a/drivers/rpmsg/rpmsg_internal.h
>>>> b/drivers/rpmsg/rpmsg_internal.h
>>>> index b1245d3..35c2197 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
>>>>    * @get_mtu:        see @rpmsg_get_mtu(), optional
>>>>    *
>>>>    * Indirection table for the operations that a rpmsg backend
>>>> should implement.
>>>> @@ -73,6 +74,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);
>>>>       ssize_t (*get_mtu)(struct rpmsg_endpoint *ept);
>>>>   };
>>>>   diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>> index 02fa911..06d090c 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);
>>> This callback however, is still using the original low level tty
>>> signals.
>>>
>>> Is there any reason why this can't be "rpmsg_flowcontrol_cb_t" and take
>>> a boolean, so we get a clean interface in both directions?
>>>
>>> Regards,
>>> Bjorn
>>>
>>>>     /**
>>>>    * 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
>> Is it signaling for reception or transmission?
> Remote is signalling for transmission.

So that the remote side is ready to receive, right?

Perhaps "@sig_cb: remote signaling callback handler" would be
less ambiguous?

Regards,
Arnaud

>>
>> Regards,
>> Arnaud
>>
>>>>    * @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;
>>>> @@ -111,6 +114,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)
>>>> @@ -188,6 +192,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);
>>>> +
>>>>   #else
>>>>     static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
>>>> @@ -306,6 +312,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)
>>>> +{
>>>> +    /* 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 */
>>>> -- 
>>>> 2.7.4
>>>>