Message ID | 1687361648-27688-4-git-send-email-quic_sarannya@quicinc.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | *** rpmsg signaling/flowcontrol patches *** | expand |
On 6/21/23 17:34, Sarannya S wrote: > From: Chris Lew <quic_clew@quicinc.com> > > Add RPMSG_GET_OUTGOING_FLOWCONTROL and RPMSG_SET_INCOMING_FLOWCONTROL > IOCTL support for rpmsg char device nodes to get/set the low level > transport signals. > > Signed-off-by: Chris Lew <quic_clew@quicinc.com> > Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com> > Signed-off-by: Sarannya S <quic_sarannya@quicinc.com> > --- > drivers/rpmsg/rpmsg_char.c | 50 ++++++++++++++++++++++++++++++++++++++++------ > include/uapi/linux/rpmsg.h | 10 ++++++++++ > 2 files changed, 54 insertions(+), 6 deletions(-) > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > index a271fce..2cdd31e 100644 > --- a/drivers/rpmsg/rpmsg_char.c > +++ b/drivers/rpmsg/rpmsg_char.c > @@ -52,6 +52,8 @@ static DEFINE_IDA(rpmsg_minor_ida); > * @readq: wait object for incoming queue > * @default_ept: set to channel default endpoint if the default endpoint should be re-used > * on device open to prevent endpoint address update. > + * remote_flow_restricted: to indicate if the remote has requested for flow to be limited > + * remote_flow_updated: to indicate if the flow control has been requested replace tab by space after ':' > */ > struct rpmsg_eptdev { > struct device dev; > @@ -68,6 +70,8 @@ struct rpmsg_eptdev { > struct sk_buff_head queue; > wait_queue_head_t readq; > > + bool remote_flow_restricted; > + bool remote_flow_updated; > }; > > int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) > @@ -116,6 +120,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len, > return 0; > } > > +static int rpmsg_ept_flow_cb(struct rpmsg_device *rpdev, void *priv, bool enable) > +{ > + struct rpmsg_eptdev *eptdev = priv; > + > + eptdev->remote_flow_restricted = enable; > + eptdev->remote_flow_updated = true; > + > + 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); > @@ -152,6 +168,7 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp) > return -EINVAL; > } > > + ept->flow_cb = rpmsg_ept_flow_cb; > eptdev->ept = ept; > filp->private_data = eptdev; > mutex_unlock(&eptdev->ept_lock); > @@ -172,6 +189,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp) > eptdev->ept = NULL; > } > mutex_unlock(&eptdev->ept_lock); > + eptdev->remote_flow_updated = false; > > /* Discard all SKBs */ > skb_queue_purge(&eptdev->queue); > @@ -285,6 +303,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait) > if (!skb_queue_empty(&eptdev->queue)) > mask |= EPOLLIN | EPOLLRDNORM; > > + if (eptdev->remote_flow_updated) > + mask |= EPOLLPRI; > + > mutex_lock(&eptdev->ept_lock); > mask |= rpmsg_poll(eptdev->ept, filp, wait); > mutex_unlock(&eptdev->ept_lock); > @@ -297,14 +318,31 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd, > { > struct rpmsg_eptdev *eptdev = fp->private_data; > > - if (cmd != RPMSG_DESTROY_EPT_IOCTL) > - return -EINVAL; > + bool set; > + int ret; > > - /* Don't allow to destroy a default endpoint. */ > - if (eptdev->default_ept) > - return -EINVAL; > + switch (cmd) { > + case RPMSG_GET_OUTGOING_FLOWCONTROL: > + eptdev->remote_flow_updated = false; > + ret = put_user(eptdev->remote_flow_restricted, (int __user *)arg); > + break; > + case RPMSG_SET_INCOMING_FLOWCONTROL: > + set = !!arg; > + ret = rpmsg_set_flow_control(eptdev->ept, set, eptdev->chinfo.dst); > + break; > + case RPMSG_DESTROY_EPT_IOCTL: > + /* Don't allow to destroy a default endpoint. */ > + if (eptdev->default_ept) { > + ret = -EINVAL; > + break; > + } > + ret = rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL); > + break; > + default: > + ret = -EINVAL; > + } > > - return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL); > + return ret; > } > > static const struct file_operations rpmsg_eptdev_fops = { > diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h > index 1637e68..b0a6c17 100644 > --- a/include/uapi/linux/rpmsg.h > +++ b/include/uapi/linux/rpmsg.h > @@ -43,4 +43,14 @@ struct rpmsg_endpoint_info { > */ > #define RPMSG_RELEASE_DEV_IOCTL _IOW(0xb5, 0x4, struct rpmsg_endpoint_info) > > +/** > + * Set the flow control for the remote rpmsg char device. > + */ > +#define RPMSG_GET_OUTGOING_FLOWCONTROL _IOW(0xb5, 0x5, struct rpmsg_endpoint_info) > + > +/** > + * Set the flow control for the local rpmsg char device. > + */ > +#define RPMSG_SET_INCOMING_FLOWCONTROL _IOW(0xb5, 0x6, struct rpmsg_endpoint_info) Perhaps I missed something, but you use "rpmsg_endpoint_info" as argument. In rpmsg_eptdev_ioctl the argument is treated as a boolean. Seems to me that something is wrong here. regards, Arnaud > + > #endif
On Fri, Jun 23, 2023 at 11:11:03AM +0200, Arnaud POULIQUEN wrote: > On 6/21/23 17:34, Sarannya S wrote: [..] > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c [..] > > + switch (cmd) { > > + case RPMSG_GET_OUTGOING_FLOWCONTROL: > > + eptdev->remote_flow_updated = false; > > + ret = put_user(eptdev->remote_flow_restricted, (int __user *)arg); > > + break; > > + case RPMSG_SET_INCOMING_FLOWCONTROL: > > + set = !!arg; > > + ret = rpmsg_set_flow_control(eptdev->ept, set, eptdev->chinfo.dst); > > + break; > > + case RPMSG_DESTROY_EPT_IOCTL: > > + /* Don't allow to destroy a default endpoint. */ > > + if (eptdev->default_ept) { > > + ret = -EINVAL; > > + break; > > + } > > + ret = rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL); > > + break; > > + default: > > + ret = -EINVAL; > > + } > > > > - return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL); > > + return ret; > > } > > > > static const struct file_operations rpmsg_eptdev_fops = { > > diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h > > index 1637e68..b0a6c17 100644 > > --- a/include/uapi/linux/rpmsg.h > > +++ b/include/uapi/linux/rpmsg.h > > @@ -43,4 +43,14 @@ struct rpmsg_endpoint_info { > > */ > > #define RPMSG_RELEASE_DEV_IOCTL _IOW(0xb5, 0x4, struct rpmsg_endpoint_info) > > > > +/** > > + * Set the flow control for the remote rpmsg char device. We improved the wording elsewhere in the patch, to make the description of what the interface controls/exposes. This would benefit from the same attention. And this says "Set" while the ioctl says "GET". > > + */ > > +#define RPMSG_GET_OUTGOING_FLOWCONTROL _IOW(0xb5, 0x5, struct rpmsg_endpoint_info) > > + > > +/** > > + * Set the flow control for the local rpmsg char device. > > + */ > > +#define RPMSG_SET_INCOMING_FLOWCONTROL _IOW(0xb5, 0x6, struct rpmsg_endpoint_info) > > > Perhaps I missed something, but you use "rpmsg_endpoint_info" as argument. > In rpmsg_eptdev_ioctl the argument is treated as a boolean. > Seems to me that something is wrong here. > This is indeed an int in the implementation above. Regards, Bjorn > regards, > Arnaud > > > + > > #endif
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index a271fce..2cdd31e 100644 --- a/drivers/rpmsg/rpmsg_char.c +++ b/drivers/rpmsg/rpmsg_char.c @@ -52,6 +52,8 @@ static DEFINE_IDA(rpmsg_minor_ida); * @readq: wait object for incoming queue * @default_ept: set to channel default endpoint if the default endpoint should be re-used * on device open to prevent endpoint address update. + * remote_flow_restricted: to indicate if the remote has requested for flow to be limited + * remote_flow_updated: to indicate if the flow control has been requested */ struct rpmsg_eptdev { struct device dev; @@ -68,6 +70,8 @@ struct rpmsg_eptdev { struct sk_buff_head queue; wait_queue_head_t readq; + bool remote_flow_restricted; + bool remote_flow_updated; }; int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data) @@ -116,6 +120,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len, return 0; } +static int rpmsg_ept_flow_cb(struct rpmsg_device *rpdev, void *priv, bool enable) +{ + struct rpmsg_eptdev *eptdev = priv; + + eptdev->remote_flow_restricted = enable; + eptdev->remote_flow_updated = true; + + 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); @@ -152,6 +168,7 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp) return -EINVAL; } + ept->flow_cb = rpmsg_ept_flow_cb; eptdev->ept = ept; filp->private_data = eptdev; mutex_unlock(&eptdev->ept_lock); @@ -172,6 +189,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp) eptdev->ept = NULL; } mutex_unlock(&eptdev->ept_lock); + eptdev->remote_flow_updated = false; /* Discard all SKBs */ skb_queue_purge(&eptdev->queue); @@ -285,6 +303,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait) if (!skb_queue_empty(&eptdev->queue)) mask |= EPOLLIN | EPOLLRDNORM; + if (eptdev->remote_flow_updated) + mask |= EPOLLPRI; + mutex_lock(&eptdev->ept_lock); mask |= rpmsg_poll(eptdev->ept, filp, wait); mutex_unlock(&eptdev->ept_lock); @@ -297,14 +318,31 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd, { struct rpmsg_eptdev *eptdev = fp->private_data; - if (cmd != RPMSG_DESTROY_EPT_IOCTL) - return -EINVAL; + bool set; + int ret; - /* Don't allow to destroy a default endpoint. */ - if (eptdev->default_ept) - return -EINVAL; + switch (cmd) { + case RPMSG_GET_OUTGOING_FLOWCONTROL: + eptdev->remote_flow_updated = false; + ret = put_user(eptdev->remote_flow_restricted, (int __user *)arg); + break; + case RPMSG_SET_INCOMING_FLOWCONTROL: + set = !!arg; + ret = rpmsg_set_flow_control(eptdev->ept, set, eptdev->chinfo.dst); + break; + case RPMSG_DESTROY_EPT_IOCTL: + /* Don't allow to destroy a default endpoint. */ + if (eptdev->default_ept) { + ret = -EINVAL; + break; + } + ret = rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL); + break; + default: + ret = -EINVAL; + } - return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL); + return ret; } static const struct file_operations rpmsg_eptdev_fops = { diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h index 1637e68..b0a6c17 100644 --- a/include/uapi/linux/rpmsg.h +++ b/include/uapi/linux/rpmsg.h @@ -43,4 +43,14 @@ struct rpmsg_endpoint_info { */ #define RPMSG_RELEASE_DEV_IOCTL _IOW(0xb5, 0x4, struct rpmsg_endpoint_info) +/** + * Set the flow control for the remote rpmsg char device. + */ +#define RPMSG_GET_OUTGOING_FLOWCONTROL _IOW(0xb5, 0x5, struct rpmsg_endpoint_info) + +/** + * Set the flow control for the local rpmsg char device. + */ +#define RPMSG_SET_INCOMING_FLOWCONTROL _IOW(0xb5, 0x6, struct rpmsg_endpoint_info) + #endif