| Message ID | 1642534993-6552-4-git-send-email-quic_deesin@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rpmsg and glink signaling API support | expand |
On 1/18/22 20:43, 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 <quic_clew@quicinc.com> > Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com> > --- > drivers/rpmsg/rpmsg_char.c | 47 ++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 43 insertions(+), 4 deletions(-) > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > index b5907b8..c03a118 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> > > @@ -74,6 +75,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) > @@ -112,7 +116,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len, > skb_queue_tail(&eptdev->queue, skb); > spin_unlock(&eptdev->queue_lock); > > - /* wake up any blocking processes, waiting for new data */ > + wake_up_interruptible(&eptdev->readq); > + > + 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_interruptible(&eptdev->readq); Regarding the Glink code, the callback is used to be informed that the remote is ready to send (DSR) and to receive (CTS or DSR) So I suppose that the transmission should also be conditioned by the sig_pending That said tell me if I'm wrong but look to me that what is implemented here is the hardware flow control already managed by the TTY interface. What about using the TTY interface in this case? And What about using the "software flow control" instead? [1] [1] https://en.wikipedia.org/wiki/Software_flow_control > > return 0; > @@ -137,6 +152,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; > > @@ -155,6 +171,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); > @@ -265,6 +282,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; > @@ -274,11 +294,30 @@ 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; Could this directly be handled by the driver on open close? If application wants to suspend the link it could just close de /dev/rpmsgX. Regards, Arnaud > + case RPMSG_DESTROY_EPT_IOCTL: > + ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL); > + break; > + default: > + ret = -EINVAL; > + } > > - return rpmsg_eptdev_destroy(&eptdev->dev, NULL); > + return ret; > } > > static const struct file_operations rpmsg_eptdev_fops = {
On 3/23/2022 7:08 PM, Arnaud POULIQUEN wrote: > > On 1/18/22 20:43, 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 <quic_clew@quicinc.com> >> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com> >> --- >> drivers/rpmsg/rpmsg_char.c | 47 ++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 43 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >> index b5907b8..c03a118 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> >> >> @@ -74,6 +75,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) >> @@ -112,7 +116,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len, >> skb_queue_tail(&eptdev->queue, skb); >> spin_unlock(&eptdev->queue_lock); >> >> - /* wake up any blocking processes, waiting for new data */ >> + wake_up_interruptible(&eptdev->readq); >> + >> + 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_interruptible(&eptdev->readq); > Regarding the Glink code, the callback is used to be informed that the remote > is ready to send (DSR) and to receive (CTS or DSR) > So I suppose that the transmission should also be conditioned by the sig_pending I think client need to get signal value before starting transmission, so that it knows that it good to transmit data. Also it is not be enforced for every client. Some clients may not require to use signalling/flow control. > > That said tell me if I'm wrong but look to me that what is implemented here is the > hardware flow control already managed by the TTY interface. What about using the > TTY interface in this case? Correct. But some clients are using rpmsg char driver directly and don't go through tty interface. So we are incorporating tty like interface here(flow control). > And What about using the "software flow control" instead? [1] > > [1] https://en.wikipedia.org/wiki/Software_flow_control > >> >> return 0; >> @@ -137,6 +152,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; >> >> @@ -155,6 +171,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); >> @@ -265,6 +282,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; >> @@ -274,11 +294,30 @@ 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; > Could this directly be handled by the driver on open close? > If application wants to suspend the link it could just close de /dev/rpmsgX. All clients may not require setting flow control. > > Regards, > Arnaud > >> + case RPMSG_DESTROY_EPT_IOCTL: >> + ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL); >> + break; >> + default: >> + ret = -EINVAL; >> + } >> >> - return rpmsg_eptdev_destroy(&eptdev->dev, NULL); >> + return ret; >> } >> >> static const struct file_operations rpmsg_eptdev_fops = {
On 3/29/22 14:25, Deepak Kumar Singh wrote: > > On 3/23/2022 7:08 PM, Arnaud POULIQUEN wrote: >> >> On 1/18/22 20:43, 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 <quic_clew@quicinc.com> >>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com> >>> --- >>> drivers/rpmsg/rpmsg_char.c | 47 >>> ++++++++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 43 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >>> index b5907b8..c03a118 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> >>> @@ -74,6 +75,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) >>> @@ -112,7 +116,18 @@ static int rpmsg_ept_cb(struct rpmsg_device >>> *rpdev, void *buf, int len, >>> skb_queue_tail(&eptdev->queue, skb); >>> spin_unlock(&eptdev->queue_lock); >>> - /* wake up any blocking processes, waiting for new data */ >>> + wake_up_interruptible(&eptdev->readq); >>> + >>> + 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_interruptible(&eptdev->readq); >> Regarding the Glink code, the callback is used to be informed that the >> remote >> is ready to send (DSR) and to receive (CTS or DSR) >> So I suppose that the transmission should also be conditioned by the >> sig_pending > > I think client need to get signal value before starting transmission, so > that it knows that > > it good to transmit data. Also it is not be enforced for every client. > Some clients may not require > > to use signalling/flow control. > >> >> That said tell me if I'm wrong but look to me that what is implemented >> here is the >> hardware flow control already managed by the TTY interface. What >> about using the >> TTY interface in this case? > > Correct. But some clients are using rpmsg char driver directly and don't > go through tty interface. > > So we are incorporating tty like interface here(flow control). This is the point I would like to highlight to be sure that it is the good direction. From my windows I would say if application wants a basic link it uses the rpmsg_char, if it wants more complex link with TIOCMGET and TIOCMSET signaling it should migrate on rmsg tty as the link is now available (so implementing signaling in rpmsg_tty instead of rpmsg char). Anyway here I only share my opinion. It is not my role to give the direction. > >> And What about using the "software flow control" instead? [1] >> >> [1] https://en.wikipedia.org/wiki/Software_flow_control >> >>> return 0; >>> @@ -137,6 +152,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; >>> @@ -155,6 +171,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); >>> @@ -265,6 +282,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; >>> @@ -274,11 +294,30 @@ 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; >> Could this directly be handled by the driver on open close? >> If application wants to suspend the link it could just close de >> /dev/rpmsgX. > All clients may not require setting flow control. Agree, but this could be conditioned by rpdrv->signals, right? And this could avoid to expose controls - in open/close the rpmsg_set_flow_control would be called, - in rpmsg_eptdev_write_iter an error would be returned ( or 0) if remote side has suspended the transmission. But perhaps you need more that a ON/OFF flow control? Regards, Arnaud >> Regards, >> Arnaud >> >>> + case RPMSG_DESTROY_EPT_IOCTL: >>> + ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL); >>> + break; >>> + default: >>> + ret = -EINVAL; >>> + } >>> - return rpmsg_eptdev_destroy(&eptdev->dev, NULL); >>> + return ret; >>> } >>> static const struct file_operations rpmsg_eptdev_fops = {
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index b5907b8..c03a118 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> @@ -74,6 +75,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) @@ -112,7 +116,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len, skb_queue_tail(&eptdev->queue, skb); spin_unlock(&eptdev->queue_lock); - /* wake up any blocking processes, waiting for new data */ + wake_up_interruptible(&eptdev->readq); + + 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_interruptible(&eptdev->readq); return 0; @@ -137,6 +152,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; @@ -155,6 +171,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); @@ -265,6 +282,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; @@ -274,11 +294,30 @@ 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); + return ret; } static const struct file_operations rpmsg_eptdev_fops = {