| Message ID | 1633015924-881-5-git-send-email-deesin@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rpmsg and glink signaling api support | expand |
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?
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 >
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); }