[PATCH V1 0/3] rpmsg and glink signaling api support

[PATCH V1 0/3] rpmsg and glink signaling api support

[Deepak Kumar Singh]

This patch series provides support for serial like signals (DTR, RTS etc)
over glink. Clients on local host can use this interface for sending and 
receiving these signals to and from modem for flow control purpose.

Deepak Kumar Singh (3):
  rpmsg: core: Add signal API support
  rpmsg: glink: Add support to handle signals command
  rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

 drivers/rpmsg/qcom_glink_native.c | 75 +++++++++++++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_char.c        | 43 ++++++++++++++++++++++
 drivers/rpmsg/rpmsg_core.c        | 21 +++++++++++
 drivers/rpmsg/rpmsg_internal.h    |  2 ++
 include/linux/rpmsg.h             | 15 ++++++++
 5 files changed, 156 insertions(+)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Deepak Kumar Singh]

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@codeaurora.org>
---
 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);
+
 /*
  * 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

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

[Stephen Boyd]

Quoting Deepak Kumar Singh (2021-09-30 08:32:01)
> 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.

s/apis/APIs/

>
> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
[...]
> 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

s/searial/serial/

> + * @ept:       the rpmsg endpoint
> + * @enable:    enable or disable serial flow control
> + *
> + * Returns 0 on success and an appropriate error value on failure.

Use 'Return:' instead of Returns.

> + */
> +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);
> +
>  /*
>   * match a rpmsg channel with a channel info struct.
>   * this is used to make sure we're not creating rpmsg devices for channels

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

[Mathieu Poirier]

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@codeaurora.org>
> ---
>  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.

Moreover this interface is case specific and doesn't reflect the generic nature
found in ept->sig_cb.

>  /*
>   * 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
> 

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

[Bjorn Andersson]

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@codeaurora.org>
> > ---
> >  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.

> 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.

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.


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.

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
> > 

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

[Arnaud POULIQUEN]

Hello,

On 10/16/21 7:01 AM, 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@codeaurora.org>
>>> ---
>>>  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.

That would be the equivalent of 2 additional virtqueues in virtio RPmsg backend
- two virtqueues for the stream,
- two virtqueues for the control
Right?

> 
>> 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.
> 
> 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.
> 
> 
> 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.

yes this feature is something that would improve the rpmsg protocol.this could
also be interesting to pause some services on suspend.
If I well remember we also spoke about QOS, with possibility to define allocated
bandwidth per service.

In proposed implementation the signaling is managed in RPMsg backend. This means
that the backend has to be aware about the signaling defined in a service. In
some other term the signaling is fixed by the backend, and this patchset would
fix the signaling for all the backend, right?
In this case shouldn't it be part of the rpmsg core?

Then to be able to transfer signaling to the remote processor based on RPMsg
protocol
I suppose that the signaling has to be encapsulated in a RPMsg sent by an
endpoint to a remote endpoint.

How to do you keep compatibility with the legacy (no flow control)?

What about associating a control endpoint with a channel?
A channel could contain:
- a default data ept (the exiting one)
- a default control endpoint (the new one).

We could extend the ns announcement mechanism to notify the control endpoint to
a remote processor...

Regards,
Arnaud

> 
> 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
>>>

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

[Bjorn Andersson]

On Mon 18 Oct 02:00 PDT 2021, Arnaud POULIQUEN wrote:

> Hello,
> 
> On 10/16/21 7:01 AM, 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@codeaurora.org>
> >>> ---
> >>>  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.
> 
> That would be the equivalent of 2 additional virtqueues in virtio RPmsg backend
> - two virtqueues for the stream,
> - two virtqueues for the control
> Right?
> 

Imagine rather the equivalent of struct rpmsg_hdr carrying a "type"
field to specify if the payload is data, channel open, close, open_ack,
close_ack and flow control commands.

I.e. only "data" will be passed up to the client, the other types of
messages are interpreted internally.

> > 
> >> 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.
> > 
> > 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.
> > 
> > 
> > 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.
> 
> yes this feature is something that would improve the rpmsg protocol.this could
> also be interesting to pause some services on suspend.
> If I well remember we also spoke about QOS, with possibility to define allocated
> bandwidth per service.
> 
> In proposed implementation the signaling is managed in RPMsg backend. This means
> that the backend has to be aware about the signaling defined in a service. In
> some other term the signaling is fixed by the backend, and this patchset would
> fix the signaling for all the backend, right?

Correct, it's the responsibility of the backend to exchange the flow
control signals.

> In this case shouldn't it be part of the rpmsg core?
> 

In the case of GLINK flow control is inband control messages, for SMD
it's status bits in a fixed located "per-channel information struct".
And when we have talked about introducing this for virtio we have toyed
with the idea of creating a single control channel to exchange them.

So I don't think we have the ability to push the implementation itself
into the core, but I think if we define a small set of well defined
operations (such as flow on/off), it should be fairly straight forward
to implement these in the backend.

> Then to be able to transfer signaling to the remote processor based on RPMsg
> protocol
> I suppose that the signaling has to be encapsulated in a RPMsg sent by an
> endpoint to a remote endpoint.
> 
> How to do you keep compatibility with the legacy (no flow control)?
> 

In the case of the two Qualcomm protocols we simply have been ignoring
them upstream and Qualcomm have carried downstream patches adding this
functionality.

This has worked out okay, because in addition to the explicit flow
control there's above described data packets are also governed by a
sliding window-like flow control. So no single channel is able to
completely saturate the link.

But for certain applications explicit flow control is necessary.

> What about associating a control endpoint with a channel?
> A channel could contain:
> - a default data ept (the exiting one)
> - a default control endpoint (the new one).

As both GLINK and SMD has these operations defined in the protocol
itself and the firmware is already written, I don't think we can come up
with something completely generic.

> 
> We could extend the ns announcement mechanism to notify the control endpoint to
> a remote processor...
> 

But I think these are good suggestions for how this functionality could
be implemented in the virtio backend.

Or one could borrow the design from GLINK and make use of the "reserved"
filed in struct rpmsg_hdr?

Regards,
Bjorn

> Regards,
> Arnaud
> 
> > 
> > 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
> >>>

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

[Mathieu Poirier]

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@codeaurora.org>
> > > ---
> > >  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 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.  

> 
> 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.

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
> > > 

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

[Bjorn Andersson]

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@codeaurora.org>
> > > > ---
> > > >  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?

> > 
> > 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
> > > > 

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

[Mathieu Poirier]

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@codeaurora.org>
> > > > > ---
> > > > >  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
> > > > > 

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

Hi Deepak,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.15]
[cannot apply to next-20211106]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Deepak-Kumar-Singh/rpmsg-core-Add-signal-API-support/20211001-003225
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 02d5e016800d082058b3d3b7c3ede136cdc6ddcb
config: arm-randconfig-c002-20211001 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 962e503cc8bc411f7523cc393acae8aae425b1c4)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/b1743368f111ecd56fb976dd6dea19d041e3ad4e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Deepak-Kumar-Singh/rpmsg-core-Add-signal-API-support/20211001-003225
        git checkout b1743368f111ecd56fb976dd6dea19d041e3ad4e
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/bluetooth/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/bluetooth/btqcomsmd.c:9:
>> include/linux/rpmsg.h:307:1: error: expected identifier or '('
   {
   ^
   1 error generated.


vim +307 include/linux/rpmsg.h

   305	
   306	static inline int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable);
 > 307	{
   308		/* This shouldn't be possible */
   309		WARN_ON(1);
   310	
   311		return -ENXIO;
   312	}
   313	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33848 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2415 bytes --]

Hi Deepak,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.15]
[cannot apply to next-20211109]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Deepak-Kumar-Singh/rpmsg-core-Add-signal-API-support/20211001-003225
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 02d5e016800d082058b3d3b7c3ede136cdc6ddcb
config: arm-randconfig-c002-20211001 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b1743368f111ecd56fb976dd6dea19d041e3ad4e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Deepak-Kumar-Singh/rpmsg-core-Add-signal-API-support/20211001-003225
        git checkout b1743368f111ecd56fb976dd6dea19d041e3ad4e
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash drivers/bluetooth/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/bluetooth/btqcomsmd.c:9:
>> include/linux/rpmsg.h:307:1: error: expected identifier or '(' before '{' token
     307 | {
         | ^
   In file included from drivers/bluetooth/btqcomsmd.c:9:
   include/linux/rpmsg.h:306:19: warning: 'rpmsg_set_flow_control' declared 'static' but never defined [-Wunused-function]
     306 | static inline int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable);
         |                   ^~~~~~~~~~~~~~~~~~~~~~


vim +307 include/linux/rpmsg.h

   305	
   306	static inline int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable);
 > 307	{
   308		/* This shouldn't be possible */
   309		WARN_ON(1);
   310	
   311		return -ENXIO;
   312	}
   313	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33848 bytes --]

[PATCH 1/1] soc: qcom: smp2p: add feature negotiation and ssr ack feature support

[Deepak Kumar Singh]

This patch adds feature negotiation and ssr ack feature between
local and remote host. Local host can negotiate on common features
supported with remote host.
---
 drivers/soc/qcom/smp2p.c | 151 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 104 insertions(+), 47 deletions(-)

diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index 38585a7..1c6ad1c 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -14,7 +14,6 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/pm_wakeirq.h>
 #include <linux/regmap.h>
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
@@ -41,8 +40,11 @@
 #define SMP2P_MAX_ENTRY_NAME 16
 
 #define SMP2P_FEATURE_SSR_ACK 0x1
+#define SMP2P_FLAGS_RESTART_DONE_BIT 0
+#define SMP2P_FLAGS_RESTART_ACK_BIT 1
 
 #define SMP2P_MAGIC 0x504d5324
+#define SMP2P_FEATURES	SMP2P_FEATURE_SSR_ACK
 
 /**
  * struct smp2p_smem_item - in memory communication structure
@@ -113,7 +115,6 @@ struct smp2p_entry {
  * struct qcom_smp2p - device driver context
  * @dev:	device driver handle
  * @in:		pointer to the inbound smem item
- * @out:	pointer to the outbound smem item
  * @smem_items:	ids of the two smem items
  * @valid_entries: already scanned inbound entries
  * @local_pid:	processor id of the inbound edge
@@ -136,6 +137,10 @@ struct qcom_smp2p {
 
 	unsigned valid_entries;
 
+	bool ssr_ack_enabled;
+	bool ssr_ack;
+	bool open;
+
 	unsigned local_pid;
 	unsigned remote_pid;
 
@@ -163,22 +168,59 @@ static void qcom_smp2p_kick(struct qcom_smp2p *smp2p)
 	}
 }
 
-/**
- * qcom_smp2p_intr() - interrupt handler for incoming notifications
- * @irq:	unused
- * @data:	smp2p driver context
- *
- * Handle notifications from the remote side to handle newly allocated entries
- * or any changes to the state bits of existing entries.
- */
-static irqreturn_t qcom_smp2p_intr(int irq, void *data)
+static bool qcom_smp2p_check_ssr(struct qcom_smp2p *smp2p)
+{
+	struct smp2p_smem_item *in = smp2p->in;
+	bool restart;
+
+	if (!smp2p->ssr_ack_enabled)
+		return false;
+
+	restart = in->flags & BIT(SMP2P_FLAGS_RESTART_DONE_BIT);
+	if (restart == smp2p->ssr_ack)
+		return false;
+
+	return true;
+}
+
+static void qcom_smp2p_do_ssr_ack(struct qcom_smp2p *smp2p)
+{
+	struct smp2p_smem_item *out = smp2p->out;
+	u32 ack;
+	u32 val;
+
+	ack = !smp2p->ssr_ack;
+	smp2p->ssr_ack = ack;
+	ack = ack << SMP2P_FLAGS_RESTART_ACK_BIT;
+
+	val = out->flags & ~BIT(SMP2P_FLAGS_RESTART_ACK_BIT);
+	val |= ack;
+	out->flags = val;
+
+	qcom_smp2p_kick(smp2p);
+}
+
+static void qcom_smp2p_negotiate(struct qcom_smp2p *smp2p)
+{
+	struct smp2p_smem_item *out = smp2p->out;
+	struct smp2p_smem_item *in = smp2p->in;
+	u32 features;
+
+	if (in->version == out->version) {
+		features = in->features & out->features;
+		out->features = features;
+
+		if (features & SMP2P_FEATURE_SSR_ACK)
+			smp2p->ssr_ack_enabled = true;
+
+		smp2p->open = true;
+	}
+}
+
+static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
 {
 	struct smp2p_smem_item *in;
 	struct smp2p_entry *entry;
-	struct qcom_smp2p *smp2p = data;
-	unsigned smem_id = smp2p->smem_items[SMP2P_INBOUND];
-	unsigned pid = smp2p->remote_pid;
-	size_t size;
 	int irq_pin;
 	u32 status;
 	char buf[SMP2P_MAX_ENTRY_NAME];
@@ -187,18 +229,6 @@ static irqreturn_t qcom_smp2p_intr(int irq, void *data)
 
 	in = smp2p->in;
 
-	/* Acquire smem item, if not already found */
-	if (!in) {
-		in = qcom_smem_get(pid, smem_id, &size);
-		if (IS_ERR(in)) {
-			dev_err(smp2p->dev,
-				"Unable to acquire remote smp2p item\n");
-			return IRQ_HANDLED;
-		}
-
-		smp2p->in = in;
-	}
-
 	/* Match newly created entries */
 	for (i = smp2p->valid_entries; i < in->valid_entries; i++) {
 		list_for_each_entry(entry, &smp2p->inbound, node) {
@@ -210,7 +240,7 @@ static irqreturn_t qcom_smp2p_intr(int irq, void *data)
 		}
 	}
 	smp2p->valid_entries = i;
-
+	
 	/* Fire interrupts based on any value changes */
 	list_for_each_entry(entry, &smp2p->inbound, node) {
 		/* Ignore entries not yet allocated by the remote side */
@@ -237,7 +267,52 @@ static irqreturn_t qcom_smp2p_intr(int irq, void *data)
 			}
 		}
 	}
+}
+
+/**
+ * qcom_smp2p_intr() - interrupt handler for incoming notifications
+ * @irq:	unused
+ * @data:	smp2p driver context
+ *
+ * Handle notifications from the remote side to handle newly allocated entries
+ * or any changes to the state bits of existing entries.
+ */
+static irqreturn_t qcom_smp2p_intr(int irq, void *data)
+{
+	struct smp2p_smem_item *in;
+	struct qcom_smp2p *smp2p = data;
+	unsigned int smem_id = smp2p->smem_items[SMP2P_INBOUND];
+	unsigned int pid = smp2p->remote_pid;
+	size_t size;
 
+	in = smp2p->in;
+
+	/* Acquire smem item, if not already found */
+	if (!in) {
+		in = qcom_smem_get(pid, smem_id, &size);
+		if (IS_ERR(in)) {
+			dev_err(smp2p->dev,
+				"Unable to acquire remote smp2p item\n");
+			goto out;
+		}
+
+		smp2p->in = in;
+	}
+
+	if (!smp2p->open)
+		qcom_smp2p_negotiate(smp2p);
+
+	if (smp2p->open) {
+		bool do_restart;
+
+		do_restart = qcom_smp2p_check_ssr(smp2p);
+		qcom_smp2p_notify_in(smp2p);
+
+		if (do_restart)
+			qcom_smp2p_do_ssr_ack(smp2p);
+	}
+
+out:
 	return IRQ_HANDLED;
 }
 
@@ -393,6 +468,7 @@ static int qcom_smp2p_alloc_outbound_item(struct qcom_smp2p *smp2p)
 	out->remote_pid = smp2p->remote_pid;
 	out->total_entries = SMP2P_MAX_ENTRY;
 	out->valid_entries = 0;
+	out->features = SMP2P_FEATURES;
 
 	/*
 	 * Make sure the rest of the header is written before we validate the
@@ -539,26 +615,9 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
 		goto unwind_interfaces;
 	}
 
-	/*
-	 * Treat smp2p interrupt as wakeup source, but keep it disabled
-	 * by default. User space can decide enabling it depending on its
-	 * use cases. For example if remoteproc crashes and device wants
-	 * to handle it immediatedly (e.g. to not miss phone calls) it can
-	 * enable wakeup source from user space, while other devices which
-	 * do not have proper autosleep feature may want to handle it with
-	 * other wakeup events (e.g. Power button) instead waking up immediately.
-	 */
-	device_set_wakeup_capable(&pdev->dev, true);
-
-	ret = dev_pm_set_wake_irq(&pdev->dev, irq);
-	if (ret)
-		goto set_wake_irq_fail;
 
 	return 0;
 
-set_wake_irq_fail:
-	dev_pm_clear_wake_irq(&pdev->dev);
-
 unwind_interfaces:
 	list_for_each_entry(entry, &smp2p->inbound, node)
 		irq_domain_remove(entry->domain);
@@ -583,8 +642,6 @@ static int qcom_smp2p_remove(struct platform_device *pdev)
 	struct qcom_smp2p *smp2p = platform_get_drvdata(pdev);
 	struct smp2p_entry *entry;
 
-	dev_pm_clear_wake_irq(&pdev->dev);
-
 	list_for_each_entry(entry, &smp2p->inbound, node)
 		irq_domain_remove(entry->domain);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/1] soc: qcom: smp2p: add feature negotiation and ssr ack feature support

[Deepak Kumar Singh]

Please ignore this patch. This is inadvertently uploaded.

Thanks,

Deepak

On 9/30/2021 9:02 PM, Deepak Kumar Singh wrote:
> This patch adds feature negotiation and ssr ack feature between
> local and remote host. Local host can negotiate on common features
> supported with remote host.
> ---
>   drivers/soc/qcom/smp2p.c | 151 ++++++++++++++++++++++++++++++++---------------
>   1 file changed, 104 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index 38585a7..1c6ad1c 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -14,7 +14,6 @@
>   #include <linux/mfd/syscon.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> -#include <linux/pm_wakeirq.h>
>   #include <linux/regmap.h>
>   #include <linux/soc/qcom/smem.h>
>   #include <linux/soc/qcom/smem_state.h>
> @@ -41,8 +40,11 @@
>   #define SMP2P_MAX_ENTRY_NAME 16
>   
>   #define SMP2P_FEATURE_SSR_ACK 0x1
> +#define SMP2P_FLAGS_RESTART_DONE_BIT 0
> +#define SMP2P_FLAGS_RESTART_ACK_BIT 1
>   
>   #define SMP2P_MAGIC 0x504d5324
> +#define SMP2P_FEATURES	SMP2P_FEATURE_SSR_ACK
>   
>   /**
>    * struct smp2p_smem_item - in memory communication structure
> @@ -113,7 +115,6 @@ struct smp2p_entry {
>    * struct qcom_smp2p - device driver context
>    * @dev:	device driver handle
>    * @in:		pointer to the inbound smem item
> - * @out:	pointer to the outbound smem item
>    * @smem_items:	ids of the two smem items
>    * @valid_entries: already scanned inbound entries
>    * @local_pid:	processor id of the inbound edge
> @@ -136,6 +137,10 @@ struct qcom_smp2p {
>   
>   	unsigned valid_entries;
>   
> +	bool ssr_ack_enabled;
> +	bool ssr_ack;
> +	bool open;
> +
>   	unsigned local_pid;
>   	unsigned remote_pid;
>   
> @@ -163,22 +168,59 @@ static void qcom_smp2p_kick(struct qcom_smp2p *smp2p)
>   	}
>   }
>   
> -/**
> - * qcom_smp2p_intr() - interrupt handler for incoming notifications
> - * @irq:	unused
> - * @data:	smp2p driver context
> - *
> - * Handle notifications from the remote side to handle newly allocated entries
> - * or any changes to the state bits of existing entries.
> - */
> -static irqreturn_t qcom_smp2p_intr(int irq, void *data)
> +static bool qcom_smp2p_check_ssr(struct qcom_smp2p *smp2p)
> +{
> +	struct smp2p_smem_item *in = smp2p->in;
> +	bool restart;
> +
> +	if (!smp2p->ssr_ack_enabled)
> +		return false;
> +
> +	restart = in->flags & BIT(SMP2P_FLAGS_RESTART_DONE_BIT);
> +	if (restart == smp2p->ssr_ack)
> +		return false;
> +
> +	return true;
> +}
> +
> +static void qcom_smp2p_do_ssr_ack(struct qcom_smp2p *smp2p)
> +{
> +	struct smp2p_smem_item *out = smp2p->out;
> +	u32 ack;
> +	u32 val;
> +
> +	ack = !smp2p->ssr_ack;
> +	smp2p->ssr_ack = ack;
> +	ack = ack << SMP2P_FLAGS_RESTART_ACK_BIT;
> +
> +	val = out->flags & ~BIT(SMP2P_FLAGS_RESTART_ACK_BIT);
> +	val |= ack;
> +	out->flags = val;
> +
> +	qcom_smp2p_kick(smp2p);
> +}
> +
> +static void qcom_smp2p_negotiate(struct qcom_smp2p *smp2p)
> +{
> +	struct smp2p_smem_item *out = smp2p->out;
> +	struct smp2p_smem_item *in = smp2p->in;
> +	u32 features;
> +
> +	if (in->version == out->version) {
> +		features = in->features & out->features;
> +		out->features = features;
> +
> +		if (features & SMP2P_FEATURE_SSR_ACK)
> +			smp2p->ssr_ack_enabled = true;
> +
> +		smp2p->open = true;
> +	}
> +}
> +
> +static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)
>   {
>   	struct smp2p_smem_item *in;
>   	struct smp2p_entry *entry;
> -	struct qcom_smp2p *smp2p = data;
> -	unsigned smem_id = smp2p->smem_items[SMP2P_INBOUND];
> -	unsigned pid = smp2p->remote_pid;
> -	size_t size;
>   	int irq_pin;
>   	u32 status;
>   	char buf[SMP2P_MAX_ENTRY_NAME];
> @@ -187,18 +229,6 @@ static irqreturn_t qcom_smp2p_intr(int irq, void *data)
>   
>   	in = smp2p->in;
>   
> -	/* Acquire smem item, if not already found */
> -	if (!in) {
> -		in = qcom_smem_get(pid, smem_id, &size);
> -		if (IS_ERR(in)) {
> -			dev_err(smp2p->dev,
> -				"Unable to acquire remote smp2p item\n");
> -			return IRQ_HANDLED;
> -		}
> -
> -		smp2p->in = in;
> -	}
> -
>   	/* Match newly created entries */
>   	for (i = smp2p->valid_entries; i < in->valid_entries; i++) {
>   		list_for_each_entry(entry, &smp2p->inbound, node) {
> @@ -210,7 +240,7 @@ static irqreturn_t qcom_smp2p_intr(int irq, void *data)
>   		}
>   	}
>   	smp2p->valid_entries = i;
> -
> +	
>   	/* Fire interrupts based on any value changes */
>   	list_for_each_entry(entry, &smp2p->inbound, node) {
>   		/* Ignore entries not yet allocated by the remote side */
> @@ -237,7 +267,52 @@ static irqreturn_t qcom_smp2p_intr(int irq, void *data)
>   			}
>   		}
>   	}
> +}
> +
> +/**
> + * qcom_smp2p_intr() - interrupt handler for incoming notifications
> + * @irq:	unused
> + * @data:	smp2p driver context
> + *
> + * Handle notifications from the remote side to handle newly allocated entries
> + * or any changes to the state bits of existing entries.
> + */
> +static irqreturn_t qcom_smp2p_intr(int irq, void *data)
> +{
> +	struct smp2p_smem_item *in;
> +	struct qcom_smp2p *smp2p = data;
> +	unsigned int smem_id = smp2p->smem_items[SMP2P_INBOUND];
> +	unsigned int pid = smp2p->remote_pid;
> +	size_t size;
>   
> +	in = smp2p->in;
> +
> +	/* Acquire smem item, if not already found */
> +	if (!in) {
> +		in = qcom_smem_get(pid, smem_id, &size);
> +		if (IS_ERR(in)) {
> +			dev_err(smp2p->dev,
> +				"Unable to acquire remote smp2p item\n");
> +			goto out;
> +		}
> +
> +		smp2p->in = in;
> +	}
> +
> +	if (!smp2p->open)
> +		qcom_smp2p_negotiate(smp2p);
> +
> +	if (smp2p->open) {
> +		bool do_restart;
> +
> +		do_restart = qcom_smp2p_check_ssr(smp2p);
> +		qcom_smp2p_notify_in(smp2p);
> +
> +		if (do_restart)
> +			qcom_smp2p_do_ssr_ack(smp2p);
> +	}
> +
> +out:
>   	return IRQ_HANDLED;
>   }
>   
> @@ -393,6 +468,7 @@ static int qcom_smp2p_alloc_outbound_item(struct qcom_smp2p *smp2p)
>   	out->remote_pid = smp2p->remote_pid;
>   	out->total_entries = SMP2P_MAX_ENTRY;
>   	out->valid_entries = 0;
> +	out->features = SMP2P_FEATURES;
>   
>   	/*
>   	 * Make sure the rest of the header is written before we validate the
> @@ -539,26 +615,9 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>   		goto unwind_interfaces;
>   	}
>   
> -	/*
> -	 * Treat smp2p interrupt as wakeup source, but keep it disabled
> -	 * by default. User space can decide enabling it depending on its
> -	 * use cases. For example if remoteproc crashes and device wants
> -	 * to handle it immediatedly (e.g. to not miss phone calls) it can
> -	 * enable wakeup source from user space, while other devices which
> -	 * do not have proper autosleep feature may want to handle it with
> -	 * other wakeup events (e.g. Power button) instead waking up immediately.
> -	 */
> -	device_set_wakeup_capable(&pdev->dev, true);
> -
> -	ret = dev_pm_set_wake_irq(&pdev->dev, irq);
> -	if (ret)
> -		goto set_wake_irq_fail;
>   
>   	return 0;
>   
> -set_wake_irq_fail:
> -	dev_pm_clear_wake_irq(&pdev->dev);
> -
>   unwind_interfaces:
>   	list_for_each_entry(entry, &smp2p->inbound, node)
>   		irq_domain_remove(entry->domain);
> @@ -583,8 +642,6 @@ static int qcom_smp2p_remove(struct platform_device *pdev)
>   	struct qcom_smp2p *smp2p = platform_get_drvdata(pdev);
>   	struct smp2p_entry *entry;
>   
> -	dev_pm_clear_wake_irq(&pdev->dev);
> -
>   	list_for_each_entry(entry, &smp2p->inbound, node)
>   		irq_domain_remove(entry->domain);
>   

[PATCH V1 2/3] rpmsg: glink: Add support to handle signals command

[Deepak Kumar Singh]

Remote peripherals send signal notifications over glink with commandID 15.

Add support to send and receive the signal command and convert the signals
from NATIVE to TIOCM while receiving and vice versa while sending.

Signed-off-by: Chris Lew <clew@codeaurora.org>
Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
---
 drivers/rpmsg/qcom_glink_native.c | 75 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 05533c7..384fcd2 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -17,6 +17,7 @@
 #include <linux/rpmsg.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
+#include <linux/termios.h>
 #include <linux/workqueue.h>
 #include <linux/mailbox_client.h>
 
@@ -201,9 +202,16 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops;
 #define RPM_CMD_TX_DATA_CONT		12
 #define RPM_CMD_READ_NOTIF		13
 #define RPM_CMD_RX_DONE_W_REUSE		14
+#define RPM_CMD_SIGNALS			15
 
 #define GLINK_FEATURE_INTENTLESS	BIT(1)
 
+#define NATIVE_DTR_SIG			BIT(31)
+#define NATIVE_CTS_SIG			BIT(30)
+#define NATIVE_CD_SIG			BIT(29)
+#define NATIVE_RI_SIG			BIT(28)
+
+
 static void qcom_glink_rx_done_work(struct work_struct *work);
 
 static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
@@ -976,6 +984,68 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
 	return 0;
 }
 
+/**
+ * qcom_glink_set_flow_control() - convert a signal cmd to wire format and
+ * 				   transmit
+ * @ept:	Rpmsg endpoint for channel.
+ * @enable:	True/False - enable or disable flow control
+ *
+ * Return: 0 on success or standard Linux error code.
+ */
+static int qcom_glink_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
+{
+	struct glink_channel *channel = to_glink_channel(ept);
+	struct qcom_glink *glink = channel->glink;
+	struct glink_msg msg;
+	u32 sigs = 0;
+
+	/**
+	 * convert signals from TIOCM to NATIVE
+	 * sigs = TIOCM_DTR|TIOCM_RTS
+	 */
+	if (enable)
+		sigs |= (NATIVE_DTR_SIG | NATIVE_CTS_SIG);
+	else
+		sigs |= (~(NATIVE_DTR_SIG | NATIVE_CTS_SIG));
+
+	msg.cmd = cpu_to_le16(RPM_CMD_SIGNALS);
+	msg.param1 = cpu_to_le16(channel->lcid);
+	msg.param2 = cpu_to_le32(sigs);
+
+	return qcom_glink_tx(glink, &msg, sizeof(msg), NULL, 0, true);
+}
+
+static int qcom_glink_handle_signals(struct qcom_glink *glink,
+				     unsigned int rcid, unsigned int sigs)
+{
+	struct glink_channel *channel;
+	unsigned long flags;
+
+	spin_lock_irqsave(&glink->idr_lock, flags);
+	channel = idr_find(&glink->rcids, rcid);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
+	if (!channel) {
+		dev_err(glink->dev, "signal for non-existing channel\n");
+		return -EINVAL;
+	}
+
+	/* convert signals from NATIVE to TIOCM */
+	if (sigs & NATIVE_DTR_SIG)
+		sigs |= TIOCM_DSR;
+	if (sigs & NATIVE_CTS_SIG)
+		sigs |= TIOCM_CTS;
+	if (sigs & NATIVE_CD_SIG)
+		sigs |= TIOCM_CD;
+	if (sigs & NATIVE_RI_SIG)
+		sigs |= TIOCM_RI;
+	sigs &= 0x0fff;
+
+	if (channel->ept.sig_cb)
+		channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv, sigs);
+
+	return 0;
+}
+
 static irqreturn_t qcom_glink_native_intr(int irq, void *data)
 {
 	struct qcom_glink *glink = data;
@@ -1037,6 +1107,10 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
 			qcom_glink_handle_intent_req_ack(glink, param1, param2);
 			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
 			break;
+		case RPM_CMD_SIGNALS:
+			qcom_glink_handle_signals(glink, param1, param2);
+			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
+			break;
 		default:
 			dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
 			ret = -EINVAL;
@@ -1382,6 +1456,7 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
 	.sendto = qcom_glink_sendto,
 	.trysend = qcom_glink_trysend,
 	.trysendto = qcom_glink_trysendto,
+	.set_flow_control = qcom_glink_set_flow_control,
 };
 
 static void qcom_glink_rpdev_release(struct device *dev)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 2/3] rpmsg: glink: Add support to handle signals command

[Stephen Boyd]

Quoting Deepak Kumar Singh (2021-09-30 08:32:03)
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 05533c7..384fcd2 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -976,6 +984,68 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
>         return 0;
>  }
>
> +/**
> + * qcom_glink_set_flow_control() - convert a signal cmd to wire format and
> + *                                transmit
> + * @ept:       Rpmsg endpoint for channel.
> + * @enable:    True/False - enable or disable flow control
> + *
> + * Return: 0 on success or standard Linux error code.
> + */
> +static int qcom_glink_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
> +{
> +       struct glink_channel *channel = to_glink_channel(ept);
> +       struct qcom_glink *glink = channel->glink;
> +       struct glink_msg msg;
> +       u32 sigs = 0;

Drop assignment.

> +
> +       /**
> +        * convert signals from TIOCM to NATIVE
> +        * sigs = TIOCM_DTR|TIOCM_RTS
> +        */
> +       if (enable)
> +               sigs |= (NATIVE_DTR_SIG | NATIVE_CTS_SIG);

sigs = NATIVE_DTR_SIG | NATIVE_CTS_SIG;

> +       else
> +               sigs |= (~(NATIVE_DTR_SIG | NATIVE_CTS_SIG));

sigs = ~(NATIVE_DTR_SIG | NATIVE_CTS_SIG);

> +
> +       msg.cmd = cpu_to_le16(RPM_CMD_SIGNALS);
> +       msg.param1 = cpu_to_le16(channel->lcid);
> +       msg.param2 = cpu_to_le32(sigs);
> +
> +       return qcom_glink_tx(glink, &msg, sizeof(msg), NULL, 0, true);
> +}
> +
> +static int qcom_glink_handle_signals(struct qcom_glink *glink,
> +                                    unsigned int rcid, unsigned int sigs)
> +{
> +       struct glink_channel *channel;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&glink->idr_lock, flags);

irqsave can be skipped because this is only called from an irq handler.

> +       channel = idr_find(&glink->rcids, rcid);
> +       spin_unlock_irqrestore(&glink->idr_lock, flags);
> +       if (!channel) {
> +               dev_err(glink->dev, "signal for non-existing channel\n");
> +               return -EINVAL;
> +       }
> +
> +       /* convert signals from NATIVE to TIOCM */
> +       if (sigs & NATIVE_DTR_SIG)
> +               sigs |= TIOCM_DSR;
> +       if (sigs & NATIVE_CTS_SIG)
> +               sigs |= TIOCM_CTS;
> +       if (sigs & NATIVE_CD_SIG)
> +               sigs |= TIOCM_CD;
> +       if (sigs & NATIVE_RI_SIG)
> +               sigs |= TIOCM_RI;
> +       sigs &= 0x0fff;

Is this to only keep TIOCM_* bits? Maybe make this an ORed together mask
of those bits so we know what 0xfff means.

> +
> +       if (channel->ept.sig_cb)
> +               channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv, sigs);
> +
> +       return 0;
> +}
> +
>  static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>  {
>         struct qcom_glink *glink = data;
> @@ -1037,6 +1107,10 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>                         qcom_glink_handle_intent_req_ack(glink, param1, param2);
>                         qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>                         break;
> +               case RPM_CMD_SIGNALS:
> +                       qcom_glink_handle_signals(glink, param1, param2);

Should the return value be handled and if there isn't a channel then
treat it as a spurious irq?

> +                       qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
> +                       break;
>                 default:
>                         dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
>                         ret = -EINVAL;

Re: [PATCH V1 2/3] rpmsg: glink: Add support to handle signals command

[Matthias Kaehlcke]

On Thu, Sep 30, 2021 at 09:02:03PM +0530, Deepak Kumar Singh wrote:
> Remote peripherals send signal notifications over glink with commandID 15.
> 
> Add support to send and receive the signal command and convert the signals
> from NATIVE to TIOCM while receiving and vice versa while sending.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 75 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 05533c7..384fcd2 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
>
> ...
>
> +static int qcom_glink_handle_signals(struct qcom_glink *glink,
> +				     unsigned int rcid, unsigned int sigs)
> +{
> +	struct glink_channel *channel;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&glink->idr_lock, flags);
> +	channel = idr_find(&glink->rcids, rcid);
> +	spin_unlock_irqrestore(&glink->idr_lock, flags);
> +	if (!channel) {
> +		dev_err(glink->dev, "signal for non-existing channel\n");
> +		return -EINVAL;
> +	}
> +
> +	/* convert signals from NATIVE to TIOCM */
> +	if (sigs & NATIVE_DTR_SIG)
> +		sigs |= TIOCM_DSR;
> +	if (sigs & NATIVE_CTS_SIG)
> +		sigs |= TIOCM_CTS;
> +	if (sigs & NATIVE_CD_SIG)
> +		sigs |= TIOCM_CD;
> +	if (sigs & NATIVE_RI_SIG)
> +		sigs |= TIOCM_RI;
> +	sigs &= 0x0fff;

'sigs' is only used when the channel has a signal handler, hence you could
return before the above block if there is no signal handler and remove the
condition below.

> +
> +	if (channel->ept.sig_cb)
> +		channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv, sigs);
> +
> +	return 0;
> +}

[PATCH V1 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Deepak Kumar Singh]

Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
to get/set the low level transport signals.

Signed-off-by: Chris Lew <clew@codeaurora.org>
Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
---
 drivers/rpmsg/rpmsg_char.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 2bebc9b..60a889b 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -19,6 +19,7 @@
 #include <linux/rpmsg.h>
 #include <linux/skbuff.h>
 #include <linux/slab.h>
+#include <linux/termios.h>
 #include <linux/uaccess.h>
 #include <uapi/linux/rpmsg.h>
 
@@ -76,6 +77,9 @@ struct rpmsg_eptdev {
 	spinlock_t queue_lock;
 	struct sk_buff_head queue;
 	wait_queue_head_t readq;
+
+	u32 rsigs;
+	bool sig_pending;
 };
 
 static int rpmsg_eptdev_destroy(struct device *dev, void *data)
@@ -120,6 +124,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
 	return 0;
 }
 
+static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32 sigs)
+{
+	struct rpmsg_eptdev *eptdev = priv;
+
+	eptdev->rsigs = sigs;
+	eptdev->sig_pending = true;
+
+	/* wake up any blocking processes, waiting for signal notification */
+	wake_up_interruptible(&eptdev->readq);
+	return 0;
+}
+
 static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 {
 	struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
@@ -139,6 +155,7 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 		return -EINVAL;
 	}
 
+	ept->sig_cb = rpmsg_sigs_cb;
 	eptdev->ept = ept;
 	filp->private_data = eptdev;
 
@@ -157,6 +174,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
 		eptdev->ept = NULL;
 	}
 	mutex_unlock(&eptdev->ept_lock);
+	eptdev->sig_pending = false;
 
 	/* Discard all SKBs */
 	skb_queue_purge(&eptdev->queue);
@@ -267,6 +285,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
 	if (!skb_queue_empty(&eptdev->queue))
 		mask |= EPOLLIN | EPOLLRDNORM;
 
+	if (eptdev->sig_pending)
+		mask |= EPOLLPRI;
+
 	mask |= rpmsg_poll(eptdev->ept, filp, wait);
 
 	return mask;
@@ -276,10 +297,32 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
 			       unsigned long arg)
 {
 	struct rpmsg_eptdev *eptdev = fp->private_data;
+	bool set;
+	u32 val;
+	int ret;
 
 	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
 		return -EINVAL;
 
+	switch (cmd) {
+	case TIOCMGET:
+		eptdev->sig_pending = false;
+		ret = put_user(eptdev->rsigs, (int __user *)arg);
+		break;
+	case TIOCMSET:
+		ret = get_user(val, (int __user *)arg);
+		if (ret)
+			break;
+		set = (val & TIOCM_DTR) ? true : false;
+		ret = rpmsg_set_flow_control(eptdev->ept, set);
+		break;
+	case RPMSG_DESTROY_EPT_IOCTL:
+		ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
 	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Stephen Boyd]

Quoting Deepak Kumar Singh (2021-09-30 08:32:04)
> Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
> to get/set the low level transport signals.
>
> Signed-off-by: Chris Lew <clew@codeaurora.org>

Is Chris the author? Because if so then there should be a From: Chris
line before the commit text starts.

> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> ---
>  drivers/rpmsg/rpmsg_char.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 2bebc9b..60a889b 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -19,6 +19,7 @@
>  #include <linux/rpmsg.h>
>  #include <linux/skbuff.h>
>  #include <linux/slab.h>
> +#include <linux/termios.h>
>  #include <linux/uaccess.h>
>  #include <uapi/linux/rpmsg.h>
>
> @@ -76,6 +77,9 @@ struct rpmsg_eptdev {
>         spinlock_t queue_lock;
>         struct sk_buff_head queue;
>         wait_queue_head_t readq;
> +
> +       u32 rsigs;
> +       bool sig_pending;
>  };
>
>  static int rpmsg_eptdev_destroy(struct device *dev, void *data)
> @@ -120,6 +124,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
>         return 0;
>  }
>
> +static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32 sigs)
> +{
> +       struct rpmsg_eptdev *eptdev = priv;
> +
> +       eptdev->rsigs = sigs;
> +       eptdev->sig_pending = true;
> +
> +       /* wake up any blocking processes, waiting for signal notification */

Comment would be better if it indicated what function these waiters are
in instead of saying what wake_up_interruptible() does. Also, what is
interruptible for?

> +       wake_up_interruptible(&eptdev->readq);

Nitpick: Add newline here.

> +       return 0;
> +}
> +
>  static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  {
>         struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
> @@ -276,10 +297,32 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>                                unsigned long arg)
>  {
>         struct rpmsg_eptdev *eptdev = fp->private_data;
> +       bool set;
> +       u32 val;
> +       int ret;
>
>         if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>                 return -EINVAL;

This should be deleted?

>
> +       switch (cmd) {
> +       case TIOCMGET:
> +               eptdev->sig_pending = false;
> +               ret = put_user(eptdev->rsigs, (int __user *)arg);
> +               break;
> +       case TIOCMSET:
> +               ret = get_user(val, (int __user *)arg);
> +               if (ret)
> +                       break;
> +               set = (val & TIOCM_DTR) ? true : false;
> +               ret = rpmsg_set_flow_control(eptdev->ept, set);
> +               break;
> +       case RPMSG_DESTROY_EPT_IOCTL:
> +               ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +       }
> +
>         return rpmsg_eptdev_destroy(&eptdev->dev, NULL);

This should be replaced with return ret?

Re: [PATCH V1 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Mathieu Poirier]

On Thu, Sep 30, 2021 at 09:02:04PM +0530, Deepak Kumar Singh wrote:
> Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
> to get/set the low level transport signals.
> 
> Signed-off-by: Chris Lew <clew@codeaurora.org>
> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> ---
>  drivers/rpmsg/rpmsg_char.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 

I would have expected this patch to be 02 rather than 03.  That way the
framework is sent in place and then used by platform code.

> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 2bebc9b..60a889b 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -19,6 +19,7 @@
>  #include <linux/rpmsg.h>
>  #include <linux/skbuff.h>
>  #include <linux/slab.h>
> +#include <linux/termios.h>
>  #include <linux/uaccess.h>
>  #include <uapi/linux/rpmsg.h>
>  
> @@ -76,6 +77,9 @@ struct rpmsg_eptdev {
>  	spinlock_t queue_lock;
>  	struct sk_buff_head queue;
>  	wait_queue_head_t readq;
> +
> +	u32 rsigs;
> +	bool sig_pending;
>  };
>  
>  static int rpmsg_eptdev_destroy(struct device *dev, void *data)
> @@ -120,6 +124,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
>  	return 0;
>  }
>  
> +static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32 sigs)
> +{
> +	struct rpmsg_eptdev *eptdev = priv;
> +
> +	eptdev->rsigs = sigs;
> +	eptdev->sig_pending = true;

If two signals are sent in a row without user space having the time to process
the first one, the second message will overwrite the first one.

> +
> +	/* wake up any blocking processes, waiting for signal notification */
> +	wake_up_interruptible(&eptdev->readq);
> +	return 0;
> +}
> +
>  static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  {
>  	struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
> @@ -139,6 +155,7 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  		return -EINVAL;
>  	}
>  
> +	ept->sig_cb = rpmsg_sigs_cb;
>  	eptdev->ept = ept;
>  	filp->private_data = eptdev;
>  
> @@ -157,6 +174,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
>  		eptdev->ept = NULL;
>  	}
>  	mutex_unlock(&eptdev->ept_lock);
> +	eptdev->sig_pending = false;
>  
>  	/* Discard all SKBs */
>  	skb_queue_purge(&eptdev->queue);
> @@ -267,6 +285,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
>  	if (!skb_queue_empty(&eptdev->queue))
>  		mask |= EPOLLIN | EPOLLRDNORM;
>  
> +	if (eptdev->sig_pending)
> +		mask |= EPOLLPRI;
> +
>  	mask |= rpmsg_poll(eptdev->ept, filp, wait);
>  
>  	return mask;
> @@ -276,10 +297,32 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>  			       unsigned long arg)
>  {
>  	struct rpmsg_eptdev *eptdev = fp->private_data;
> +	bool set;
> +	u32 val;
> +	int ret;
>  
>  	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>  		return -EINVAL;
>  
> +	switch (cmd) {
> +	case TIOCMGET:

The IOCTLs should be generic so that any kind of out-of-band signals.

> +		eptdev->sig_pending = false;
> +		ret = put_user(eptdev->rsigs, (int __user *)arg);
> +		break;
> +	case TIOCMSET:
> +		ret = get_user(val, (int __user *)arg);
> +		if (ret)
> +			break;
> +		set = (val & TIOCM_DTR) ? true : false;
> +		ret = rpmsg_set_flow_control(eptdev->ept, set);
> +		break;

But as said in patch 01, I'm not sure about the path from AP to RP.  Function
rpmsg_set_flow_control() turns into qcom_glink_tx().  In this case it should be
possible for user space to send this information as it does with other kind of
data destined for the remote processor.  At the very least the send interface
should be decoupled from the implementation specific nature of this use case.

Lastly, Arnaud has sent patches that refactor rpmsg_eptdev_ioctl().  I would
like that patchset to be dealth with before we move forward with this one.  That
way we make sure to avoid supporting features that are incompatible with each
other.

Regards,
Mathieu

> +	case RPMSG_DESTROY_EPT_IOCTL:
> +		ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
>  	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
>  }
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH V1 0/3] rpmsg and glink signaling api support

[Mathieu Poirier]

Hi Deepak,

On Thu, Sep 30, 2021 at 09:02:00PM +0530, Deepak Kumar Singh wrote:
> This patch series provides support for serial like signals (DTR, RTS etc)
> over glink. Clients on local host can use this interface for sending and 
> receiving these signals to and from modem for flow control purpose.
> 
> Deepak Kumar Singh (3):
>   rpmsg: core: Add signal API support
>   rpmsg: glink: Add support to handle signals command
>   rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support
> 
>  drivers/rpmsg/qcom_glink_native.c | 75 +++++++++++++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_char.c        | 43 ++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_core.c        | 21 +++++++++++
>  drivers/rpmsg/rpmsg_internal.h    |  2 ++
>  include/linux/rpmsg.h             | 15 ++++++++
>  5 files changed, 156 insertions(+)
> 

I have started to review this set - comments to follow next weeks.

Thanks,
Mathieu

> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>