Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Vadim Tsozik]

Greg,
I apologize for the delay. Finally I managed to configure sendmail to relay messages via my ISP SMTP (Too bad, my ISP blocks port 25).
Below you can find patch generated off your 2.6.37-rc7 usb branch. Please let me know if you have any problem to apply this.
Thank you in advance,
 Vadim.

Added mct_u232_ioctl (implements TIOCMIWAIT command), mct_u232_get_icount (implements TIOCGICOUNT command) and mct_u232_msr_to_icount functions. MCT U232 P9 is one of a few usb to serail adapters which converts USB +/-5v voltage levels to COM +/-15 voltages. So it can also power COM interfaced devices. This makes it very usable for legacy COM interfaced data-acquisition hardware. I tested new implementation with AWARE Electronics RM-60 radiation meter, which sends pulse via RNG COM line whenever new particle is registered.

Signed-off-by: Vadim Tsozik <tsozik@yahoo.com>

---

Patch below is based on linux-2.6.37-rc7

--- usb/serial/mct_u232.c.orig	2010-12-26 16:54:16.660294924 -0500
+++ usb/serial/mct_u232.c	2010-12-27 15:29:30.895141755 -0500
@@ -78,6 +78,8 @@
 #include <asm/unaligned.h>
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
+#include <linux/serial.h>
+#include <linux/ioctl.h>
 #include "mct_u232.h"
 
 /*
@@ -104,6 +106,10 @@ static void mct_u232_break_ctl(struct tt
 static int  mct_u232_tiocmget(struct tty_struct *tty, struct file *file);
 static int  mct_u232_tiocmset(struct tty_struct *tty, struct file *file,
 			unsigned int set, unsigned int clear);
+static int  mct_u232_ioctl(struct tty_struct *tty, struct file *file,
+			unsigned int cmd, unsigned long arg);
+static int  mct_u232_get_icount(struct tty_struct *tty,
+			struct serial_icounter_struct *icount);
 static void mct_u232_throttle(struct tty_struct *tty);
 static void mct_u232_unthrottle(struct tty_struct *tty);
 
@@ -150,9 +156,10 @@ static struct usb_serial_driver mct_u232
 	.tiocmset =	     mct_u232_tiocmset,
 	.attach =	     mct_u232_startup,
 	.release =	     mct_u232_release,
+	.ioctl =             mct_u232_ioctl,
+	.get_icount =        mct_u232_get_icount,
 };
 
-
 struct mct_u232_private {
 	spinlock_t lock;
 	unsigned int	     control_state; /* Modem Line Setting (TIOCM) */
@@ -160,6 +167,9 @@ struct mct_u232_private {
 	unsigned char	     last_lsr;      /* Line Status Register */
 	unsigned char	     last_msr;      /* Modem Status Register */
 	unsigned int	     rx_flags;      /* Throttling flags */
+	struct async_icount  icount;
+	wait_queue_head_t    msr_wait;	/* for handling sleeping while waiting
+						for msr change to happen */
 };
 
 #define THROTTLED		0x01
@@ -386,6 +396,20 @@ static int mct_u232_get_modem_stat(struc
 	return rc;
 } /* mct_u232_get_modem_stat */
 
+static void mct_u232_msr_to_icount(struct async_icount *icount,
+						unsigned char msr)
+{
+	/* Translate Control Line states */
+	if (msr & MCT_U232_MSR_DDSR)
+		icount->dsr++;
+	if (msr & MCT_U232_MSR_DCTS)
+		icount->cts++;
+	if (msr & MCT_U232_MSR_DRI)
+		icount->rng++;
+	if (msr & MCT_U232_MSR_DCD)
+		icount->dcd++;
+} /* mct_u232_msr_to_icount */
+
 static void mct_u232_msr_to_state(unsigned int *control_state,
 						unsigned char msr)
 {
@@ -422,6 +446,7 @@ static int mct_u232_startup(struct usb_s
 	if (!priv)
 		return -ENOMEM;
 	spin_lock_init(&priv->lock);
+	init_waitqueue_head(&priv->msr_wait);
 	usb_set_serial_port_data(serial->port[0], priv);
 
 	init_waitqueue_head(&serial->port[0]->write_wait);
@@ -621,6 +646,8 @@ static void mct_u232_read_int_callback(s
 	/* Record Control Line states */
 	mct_u232_msr_to_state(&priv->control_state, priv->last_msr);
 
+	mct_u232_msr_to_icount(&priv->icount, priv->last_msr);
+
 #if 0
 	/* Not yet handled. See belkin_sa.c for further information */
 	/* Now to report any errors */
@@ -647,6 +674,7 @@ static void mct_u232_read_int_callback(s
 		tty_kref_put(tty);
 	}
 #endif
+	wake_up_interruptible(&priv->msr_wait);
 	spin_unlock_irqrestore(&priv->lock, flags);
 exit:
 	retval = usb_submit_urb(urb, GFP_ATOMIC);
@@ -826,7 +854,6 @@ static void mct_u232_throttle(struct tty
 	}
 }
 
-
 static void mct_u232_unthrottle(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
@@ -847,6 +874,82 @@ static void mct_u232_unthrottle(struct t
 	}
 }
 
+static int  mct_u232_ioctl(struct tty_struct *tty, struct file *file,
+			unsigned int cmd, unsigned long arg)
+{
+	DEFINE_WAIT(wait);
+	struct usb_serial_port *port = tty->driver_data;
+	struct mct_u232_private *mct_u232_port = usb_get_serial_port_data(port);
+	struct async_icount cnow, cprev;
+	unsigned long flags;
+
+	dbg("%s - port %d, cmd = 0x%x", __func__, port->number, cmd);
+
+	switch (cmd) {
+
+	case TIOCMIWAIT:
+
+		dbg("%s (%d) TIOCMIWAIT", __func__,  port->number);
+
+		spin_lock_irqsave(&mct_u232_port->lock, flags);
+		cprev = mct_u232_port->icount;
+		spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+		for ( ; ; ) {
+			prepare_to_wait(&mct_u232_port->msr_wait,
+					&wait, TASK_INTERRUPTIBLE);
+			schedule();
+			finish_wait(&mct_u232_port->msr_wait, &wait);
+			/* see if a signal did it */
+			if (signal_pending(current))
+				return -ERESTARTSYS;
+			spin_lock_irqsave(&mct_u232_port->lock, flags);
+			cnow = mct_u232_port->icount;
+			spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+			if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
+			    cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
+				return -EIO; /* no change => error */
+			if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
+			    ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
+			    ((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
+			    ((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
+				return 0;
+			}
+			cprev = cnow;
+		}
+
+	}
+	return -ENOIOCTLCMD;
+}
+
+static int  mct_u232_get_icount(struct tty_struct *tty,
+			struct serial_icounter_struct *icount)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct mct_u232_private *mct_u232_port = usb_get_serial_port_data(port);
+	struct async_icount *ic = &mct_u232_port->icount;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mct_u232_port->lock, flags);
+
+	icount->cts = ic->cts;
+	icount->dsr = ic->dsr;
+	icount->rng = ic->rng;
+	icount->dcd = ic->dcd;
+	icount->rx = ic->rx;
+	icount->tx = ic->tx;
+	icount->frame = ic->frame;
+	icount->overrun = ic->overrun;
+	icount->parity = ic->parity;
+	icount->brk = ic->brk;
+	icount->buf_overrun = ic->buf_overrun;
+
+	spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+
+	dbg("%s (%d) TIOCGICOUNT RX=%d, TX=%d",
+		__func__,  port->number, icount->rx, icount->tx);
+	return 0;
+}
+
 static int __init mct_u232_init(void)
 {
 	int retval;

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Greg KH]

On Wed, Jan 12, 2011 at 10:34:53PM -0500, Vadim Tsozik wrote:
> Greg,
> I apologize for the delay. Finally I managed to configure sendmail to relay messages via my ISP SMTP (Too bad, my ISP blocks port 25).
> Below you can find patch generated off your 2.6.37-rc7 usb branch. Please let me know if you have any problem to apply this.
> Thank you in advance,

In the future, you need to create a patch that can be applied with '-p1'
option from the root of the kernel tree, right now you created it at the
root of the drivers/usb/ directory:

> --- usb/serial/mct_u232.c.orig	2010-12-26 16:54:16.660294924 -0500
> +++ usb/serial/mct_u232.c	2010-12-27 15:29:30.895141755 -0500

I edited it by hand and now applied it, sorry for the delay.

greg k-h

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Tsozik]

Greg,
 Many thanks for your help with this,
  Vadim.

--- On Fri, 2/4/11, Greg KH <greg@kroah.com> wrote:

> From: Greg KH <greg@kroah.com>
> Subject: Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions
> To: "Vadim Tsozik" <vtsozik@optimum.net>
> Cc: "Greg Kroah-Hartman" <gregkh@suse.de>, "Pete Zaitcev" <zaitcev@redhat.com>, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, "Vadim Tsozik" <tsozik@yahoo.com>
> Date: Friday, February 4, 2011, 2:35 PM
> On Wed, Jan 12, 2011 at 10:34:53PM
> -0500, Vadim Tsozik wrote:
> > Greg,
> > I apologize for the delay. Finally I managed to
> configure sendmail to relay messages via my ISP SMTP (Too
> bad, my ISP blocks port 25).
> > Below you can find patch generated off your 2.6.37-rc7
> usb branch. Please let me know if you have any problem to
> apply this.
> > Thank you in advance,
> 
> In the future, you need to create a patch that can be
> applied with '-p1'
> option from the root of the kernel tree, right now you
> created it at the
> root of the drivers/usb/ directory:
> 
> > --- usb/serial/mct_u232.c.orig   
> 2010-12-26 16:54:16.660294924 -0500
> > +++ usb/serial/mct_u232.c    2010-12-27
> 15:29:30.895141755 -0500
> 
> I edited it by hand and now applied it, sorry for the
> delay.
> 
> greg k-h
> 


      

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Vadim Tsozik]

Greg,
I apologize for the delay. Finally I managed to configure sendmail to relay messages via my ISP SMTP (Too bad, my ISP blocks port 25).
Below you can find patch generated off your 2.6.37-rc7 usb branch. Please let me know if you have any problem to apply this.
Thank you in advance,
 Vadim.

Added mct_u232_ioctl (implements TIOCMIWAIT command), mct_u232_get_icount (implements TIOCGICOUNT command) and mct_u232_msr_to_icount functions. MCT U232 P9 is one of a few usb to serail adapters which converts USB +/-5v voltage levels to COM +/-15 voltages. So it can also power COM interfaced devices. This makes it very usable for legacy COM interfaced data-acquisition hardware. I tested new implementation with AWARE Electronics RM-60 radiation meter, which sends pulse via RNG COM line whenever new particle is registered.

Signed-off-by: Vadim Tsozik <tsozik@yahoo.com>

---

Patch below is based on linux-2.6.37-rc7

--- usb/serial/mct_u232.c.orig	2010-12-26 16:54:16.660294924 -0500
+++ usb/serial/mct_u232.c	2010-12-27 15:29:30.895141755 -0500
@@ -78,6 +78,8 @@
 #include <asm/unaligned.h>
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
+#include <linux/serial.h>
+#include <linux/ioctl.h>
 #include "mct_u232.h"
 
 /*
@@ -104,6 +106,10 @@ static void mct_u232_break_ctl(struct tt
 static int  mct_u232_tiocmget(struct tty_struct *tty, struct file *file);
 static int  mct_u232_tiocmset(struct tty_struct *tty, struct file *file,
 			unsigned int set, unsigned int clear);
+static int  mct_u232_ioctl(struct tty_struct *tty, struct file *file,
+			unsigned int cmd, unsigned long arg);
+static int  mct_u232_get_icount(struct tty_struct *tty,
+			struct serial_icounter_struct *icount);
 static void mct_u232_throttle(struct tty_struct *tty);
 static void mct_u232_unthrottle(struct tty_struct *tty);
 
@@ -150,9 +156,10 @@ static struct usb_serial_driver mct_u232
 	.tiocmset =	     mct_u232_tiocmset,
 	.attach =	     mct_u232_startup,
 	.release =	     mct_u232_release,
+	.ioctl =             mct_u232_ioctl,
+	.get_icount =        mct_u232_get_icount,
 };
 
-
 struct mct_u232_private {
 	spinlock_t lock;
 	unsigned int	     control_state; /* Modem Line Setting (TIOCM) */
@@ -160,6 +167,9 @@ struct mct_u232_private {
 	unsigned char	     last_lsr;      /* Line Status Register */
 	unsigned char	     last_msr;      /* Modem Status Register */
 	unsigned int	     rx_flags;      /* Throttling flags */
+	struct async_icount  icount;
+	wait_queue_head_t    msr_wait;	/* for handling sleeping while waiting
+						for msr change to happen */
 };
 
 #define THROTTLED		0x01
@@ -386,6 +396,20 @@ static int mct_u232_get_modem_stat(struc
 	return rc;
 } /* mct_u232_get_modem_stat */
 
+static void mct_u232_msr_to_icount(struct async_icount *icount,
+						unsigned char msr)
+{
+	/* Translate Control Line states */
+	if (msr & MCT_U232_MSR_DDSR)
+		icount->dsr++;
+	if (msr & MCT_U232_MSR_DCTS)
+		icount->cts++;
+	if (msr & MCT_U232_MSR_DRI)
+		icount->rng++;
+	if (msr & MCT_U232_MSR_DCD)
+		icount->dcd++;
+} /* mct_u232_msr_to_icount */
+
 static void mct_u232_msr_to_state(unsigned int *control_state,
 						unsigned char msr)
 {
@@ -422,6 +446,7 @@ static int mct_u232_startup(struct usb_s
 	if (!priv)
 		return -ENOMEM;
 	spin_lock_init(&priv->lock);
+	init_waitqueue_head(&priv->msr_wait);
 	usb_set_serial_port_data(serial->port[0], priv);
 
 	init_waitqueue_head(&serial->port[0]->write_wait);
@@ -621,6 +646,8 @@ static void mct_u232_read_int_callback(s
 	/* Record Control Line states */
 	mct_u232_msr_to_state(&priv->control_state, priv->last_msr);
 
+	mct_u232_msr_to_icount(&priv->icount, priv->last_msr);
+
 #if 0
 	/* Not yet handled. See belkin_sa.c for further information */
 	/* Now to report any errors */
@@ -647,6 +674,7 @@ static void mct_u232_read_int_callback(s
 		tty_kref_put(tty);
 	}
 #endif
+	wake_up_interruptible(&priv->msr_wait);
 	spin_unlock_irqrestore(&priv->lock, flags);
 exit:
 	retval = usb_submit_urb(urb, GFP_ATOMIC);
@@ -826,7 +854,6 @@ static void mct_u232_throttle(struct tty
 	}
 }
 
-
 static void mct_u232_unthrottle(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
@@ -847,6 +874,82 @@ static void mct_u232_unthrottle(struct t
 	}
 }
 
+static int  mct_u232_ioctl(struct tty_struct *tty, struct file *file,
+			unsigned int cmd, unsigned long arg)
+{
+	DEFINE_WAIT(wait);
+	struct usb_serial_port *port = tty->driver_data;
+	struct mct_u232_private *mct_u232_port = usb_get_serial_port_data(port);
+	struct async_icount cnow, cprev;
+	unsigned long flags;
+
+	dbg("%s - port %d, cmd = 0x%x", __func__, port->number, cmd);
+
+	switch (cmd) {
+
+	case TIOCMIWAIT:
+
+		dbg("%s (%d) TIOCMIWAIT", __func__,  port->number);
+
+		spin_lock_irqsave(&mct_u232_port->lock, flags);
+		cprev = mct_u232_port->icount;
+		spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+		for ( ; ; ) {
+			prepare_to_wait(&mct_u232_port->msr_wait,
+					&wait, TASK_INTERRUPTIBLE);
+			schedule();
+			finish_wait(&mct_u232_port->msr_wait, &wait);
+			/* see if a signal did it */
+			if (signal_pending(current))
+				return -ERESTARTSYS;
+			spin_lock_irqsave(&mct_u232_port->lock, flags);
+			cnow = mct_u232_port->icount;
+			spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+			if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
+			    cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
+				return -EIO; /* no change => error */
+			if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
+			    ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
+			    ((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
+			    ((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
+				return 0;
+			}
+			cprev = cnow;
+		}
+
+	}
+	return -ENOIOCTLCMD;
+}
+
+static int  mct_u232_get_icount(struct tty_struct *tty,
+			struct serial_icounter_struct *icount)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct mct_u232_private *mct_u232_port = usb_get_serial_port_data(port);
+	struct async_icount *ic = &mct_u232_port->icount;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mct_u232_port->lock, flags);
+
+	icount->cts = ic->cts;
+	icount->dsr = ic->dsr;
+	icount->rng = ic->rng;
+	icount->dcd = ic->dcd;
+	icount->rx = ic->rx;
+	icount->tx = ic->tx;
+	icount->frame = ic->frame;
+	icount->overrun = ic->overrun;
+	icount->parity = ic->parity;
+	icount->brk = ic->brk;
+	icount->buf_overrun = ic->buf_overrun;
+
+	spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+
+	dbg("%s (%d) TIOCGICOUNT RX=%d, TX=%d",
+		__func__,  port->number, icount->rx, icount->tx);
+	return 0;
+}
+
 static int __init mct_u232_init(void)
 {
 	int retval;

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Greg KH]

On Mon, Dec 27, 2010 at 12:55:38PM -0800, Tsozik wrote:
> Pete,
> 
> You're absolutely right. My apology again, I re-checked values of MSR masks used to update icount counters and they all belong to upper MSR nibble, so they indeed track changes (not states). I took out counter increments from mct_u232_msr_to_state function and encapsulated them in the newly defined mct_u232_msr_to_icount function, effectively leaving the previous implementation of mct_u232_msr_to_state function intact. mct_u232_msr_to_icount uses delta nibble to track state changes. RM-60 testing showed the same levels as measured by PDM-2 in close proximity to RM-60. Please let me know if anything else needs to be corrected to roll this out,

Can you not attach the patch as a base64 attachment?  I can't apply it
as-is, because of that, sorry.

thanks,

greg k-h

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Greg KH]

On Mon, Dec 27, 2010 at 11:40:47PM -0700, Pete Zaitcev wrote:
> On Mon, 27 Dec 2010 20:04:51 -0800 (PST)
> Tsozik <tsozik@yahoo.com> wrote:
> 
> > So I ran geiger counter against /dev/ttyS0 device for 20 minutes and
> > acquired 20 measurements. Then I compared last average with last 20
> > minute measurement average acquired via mct_u232 on the laptop placed
> > nearby. The error was ~4% (rounded up).
> 
> Great, I'm ready to ack.
> 
> There's just one thing that is bugging me... I think it would be best
> if Alan Cox or Greg Kroah commented on it. The edgeport does the
> following, which we copied:
> 
> 
> 		schedule();
> 		........
> 		if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
> 		    cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
> 			return -EIO; /* no change => error */
> 		if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
> 		    ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
> 		    ((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
> 		    ((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
> 			return 0;
> 		}
> 
> So, if there was a status report, but no change to bits, the ioctl
> TIOCMIWAIT would return with -EIO. In serial_core.c, that serves
> conventional non-USB UARTs, nothing like this occurs. I am not quite
> sure what the point of doing this -EIO check is.

Odd, I don't really remember where I copied that logic from, that was a
long time ago.

> Oh and BTW, I'm wondering what is going to happen if the device is
> disconnected while an application is blocked waiting for the status
> change. The patch is not particularly bad here, it just copies
> an existing code from elsewhere.

It should handle the disconnect, if not, we should fix it.

thanks,

greg k-h

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Tsozik]

Greg,

I'm sorry to bother you again, but I'm wondering if you could comment on Pete's concern below.

Thank you in advance for your expertise on the matter,
 Vadim. 

--- On Tue, 12/28/10, Pete Zaitcev <zaitcev@redhat.com> wrote:

> From: Pete Zaitcev <zaitcev@redhat.com>
> Subject: Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions
> To: "Tsozik" <tsozik@yahoo.com>
> Cc: "Greg Kroah-Hartman" <gregkh@suse.de>, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, zaitcev@redhat.com
> Date: Tuesday, December 28, 2010, 1:40 AM
> On Mon, 27 Dec 2010 20:04:51 -0800
> (PST)
> Tsozik <tsozik@yahoo.com>
> wrote:
> 
> > So I ran geiger counter against /dev/ttyS0 device for
> 20 minutes and
> > acquired 20 measurements. Then I compared last average
> with last 20
> > minute measurement average acquired via mct_u232 on
> the laptop placed
> > nearby. The error was ~4% (rounded up).
> 
> Great, I'm ready to ack.
> 
> There's just one thing that is bugging me... I think it
> would be best
> if Alan Cox or Greg Kroah commented on it. The edgeport
> does the
> following, which we copied:
> 
> 
>         schedule();
>         ........
>         if (cnow.rng ==
> cprev.rng && cnow.dsr == cprev.dsr &&
>            
> cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
>            
> return -EIO; /* no change => error */
>         if (((arg &
> TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
>             ((arg
> & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
>             ((arg
> & TIOCM_CD)  && (cnow.dcd != cprev.dcd))
> ||
>             ((arg
> & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
>            
> return 0;
>         }
> 
> So, if there was a status report, but no change to bits,
> the ioctl
> TIOCMIWAIT would return with -EIO. In serial_core.c, that
> serves
> conventional non-USB UARTs, nothing like this occurs. I am
> not quite
> sure what the point of doing this -EIO check is.
> 
> Oh and BTW, I'm wondering what is going to happen if the
> device is
> disconnected while an application is blocked waiting for
> the status
> change. The patch is not particularly bad here, it just
> copies
> an existing code from elsewhere.
> 
> -- Pete
> 


      

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Pete Zaitcev]

On Mon, 27 Dec 2010 20:04:51 -0800 (PST)
Tsozik <tsozik@yahoo.com> wrote:

> So I ran geiger counter against /dev/ttyS0 device for 20 minutes and
> acquired 20 measurements. Then I compared last average with last 20
> minute measurement average acquired via mct_u232 on the laptop placed
> nearby. The error was ~4% (rounded up).

Great, I'm ready to ack.

There's just one thing that is bugging me... I think it would be best
if Alan Cox or Greg Kroah commented on it. The edgeport does the
following, which we copied:


		schedule();
		........
		if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
		    cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
			return -EIO; /* no change => error */
		if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
		    ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
		    ((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
		    ((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
			return 0;
		}

So, if there was a status report, but no change to bits, the ioctl
TIOCMIWAIT would return with -EIO. In serial_core.c, that serves
conventional non-USB UARTs, nothing like this occurs. I am not quite
sure what the point of doing this -EIO check is.

Oh and BTW, I'm wondering what is going to happen if the device is
disconnected while an application is blocked waiting for the status
change. The patch is not particularly bad here, it just copies
an existing code from elsewhere.

-- Pete

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Tsozik]

Pete,

Many thanks for your reply again. I actually still own an antique HP a230n (disclaimer: I got it from/with my wife and would never buy it myself). According to dmesg it's got built-in 8250-compliant serial:

[vtsozik@f15 serial]$  dmesg | grep -i serial | grep 8250
Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
serial8250: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
[vtsozik@f15 serial]$

So I ran geiger counter against /dev/ttyS0 device for 20 minutes and acquired 20 measurements. Then I compared last average with last 20 minute measurement average acquired via mct_u232 on the laptop placed nearby. The error was ~4% (rounded up). Taking in consideration that measurements were not acquired synchronously (i.e. in 2 different time slots) this is a pretty good match. 

Thank you again,
 Vadim.

--- On Mon, 12/27/10, Pete Zaitcev <zaitcev@redhat.com> wrote:

> From: Pete Zaitcev <zaitcev@redhat.com>
> Subject: Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions
> To: "Tsozik" <tsozik@yahoo.com>
> Cc: "Greg Kroah-Hartman" <gregkh@suse.de>, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, zaitcev@redhat.com
> Date: Monday, December 27, 2010, 5:12 PM
> On Mon, 27 Dec 2010 12:55:38 -0800
> (PST)
> Tsozik <tsozik@yahoo.com>
> wrote:
> 
> > +static void mct_u232_msr_to_icount(struct
> async_icount *icount,
> > +       
>            
>     unsigned char msr)
> > +{
> > +    /* Translate Control Line states
> */
> > +    if (msr & MCT_U232_MSR_DDSR)
> > +       
> icount->dsr++;
> > +    if (msr & MCT_U232_MSR_DCTS)
> > +       
> icount->cts++;
> > +    if (msr & MCT_U232_MSR_DRI)
> > +       
> icount->rng++;
> > +    if (msr & MCT_U232_MSR_DCD)
> > +       
> icount->dcd++;
> > +} /* mct_u232_msr_to_icount */
> 
> This looks good to me, but since it's a new hardware
> facility that
> we did not use previously, I'd like to make sure this works
> for you.
> 
> > mct_u232_msr_to_icount uses delta nibble to track
> state changes.
> > RM-60 testing showed the same levels as measured by
> PDM-2 in close
> > proximity to RM-60.
> 
> Right, this is good. I have just one request for you: could
> you find
> somewhere a computer with a built-in 8250-compatible serial
> port,
> attach RM-60 to it, and run your counting application
> there?
> I think your patch looks just fine, but I would like to
> make sure
> that we (e.g. mct_u232+patch) are truly compatible now.
> 
> -- Pete
> 


      

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Pete Zaitcev]

On Mon, 27 Dec 2010 12:55:38 -0800 (PST)
Tsozik <tsozik@yahoo.com> wrote:

> +static void mct_u232_msr_to_icount(struct async_icount *icount,
> +						unsigned char msr)
> +{
> +	/* Translate Control Line states */
> +	if (msr & MCT_U232_MSR_DDSR)
> +		icount->dsr++;
> +	if (msr & MCT_U232_MSR_DCTS)
> +		icount->cts++;
> +	if (msr & MCT_U232_MSR_DRI)
> +		icount->rng++;
> +	if (msr & MCT_U232_MSR_DCD)
> +		icount->dcd++;
> +} /* mct_u232_msr_to_icount */

This looks good to me, but since it's a new hardware facility that
we did not use previously, I'd like to make sure this works for you.

> mct_u232_msr_to_icount uses delta nibble to track state changes.
> RM-60 testing showed the same levels as measured by PDM-2 in close
> proximity to RM-60.

Right, this is good. I have just one request for you: could you find
somewhere a computer with a built-in 8250-compatible serial port,
attach RM-60 to it, and run your counting application there?
I think your patch looks just fine, but I would like to make sure
that we (e.g. mct_u232+patch) are truly compatible now.

-- Pete

Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Tsozik]

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

Pete,

You're absolutely right. My apology again, I re-checked values of MSR masks used to update icount counters and they all belong to upper MSR nibble, so they indeed track changes (not states). I took out counter increments from mct_u232_msr_to_state function and encapsulated them in the newly defined mct_u232_msr_to_icount function, effectively leaving the previous implementation of mct_u232_msr_to_state function intact. mct_u232_msr_to_icount uses delta nibble to track state changes. RM-60 testing showed the same levels as measured by PDM-2 in close proximity to RM-60. Please let me know if anything else needs to be corrected to roll this out,

Thank you again for your expertise,
 Vadim.

--- On Mon, 12/27/10, Pete Zaitcev <zaitcev@redhat.com> wrote:

> From: Pete Zaitcev <zaitcev@redhat.com>
> Subject: Re: [PATCH 1/1] mct_u232: added _ioctl and _get_icount functions
> To: "Tsozik" <tsozik@yahoo.com>
> Cc: "Greg Kroah-Hartman" <gregkh@suse.de>, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
> Date: Monday, December 27, 2010, 12:08 PM
> On Sun, 26 Dec 2010 22:34:23 -0800
> (PST)
> Tsozik <tsozik@yahoo.com>
> wrote:
> 
> > Usually MSR contains information about both states
> which are raised and
> > states which are changed. It looks like we use upper
> MSR nibble which
> > contains information about 4 states which were raised,
> not lower MSR
> > nibble which contains information about 4 states which
> were changed.
> 
> Sure, I know how typical UART works. Does the MCT device
> deliver the
> change states? If it does and you want to use those, go
> ahead.
> 
> > I also researched implementations under usb/serial and
> saw that all of
> > them are rely on the current states (not change
> states) to increment
> > the respective counter variables.
> 
> Here's drivers/usb/serial/io_edgeport.c:
> 
> static void handle_new_msr(struct edgeport_port *edge_port,
> __u8 newMsr)
> {
>     struct  async_icount *icount;
> 
>     dbg("%s %02x", __func__, newMsr);
> 
>     if (newMsr & (EDGEPORT_MSR_DELTA_CTS
> | EDGEPORT_MSR_DELTA_DSR |
>            
> EDGEPORT_MSR_DELTA_RI | EDGEPORT_MSR_DELTA_CD)) {
>         icount =
> &edge_port->icount;
> 
>         /* update input line
> counters */
>         if (newMsr &
> EDGEPORT_MSR_DELTA_CTS)
>            
> icount->cts++;
>         if (newMsr &
> EDGEPORT_MSR_DELTA_DSR)
>            
> icount->dsr++;
>         if (newMsr &
> EDGEPORT_MSR_DELTA_CD)
>            
> icount->dcd++;
>         if (newMsr &
> EDGEPORT_MSR_DELTA_RI)
>            
> icount->rng++;
>        
> wake_up_interruptible(&edge_port->delta_msr_wait);
>     }
> 
> Looks like it counts changes to me. And in any case, the
> gold standard
> is in drivers/serial/8250.c:
> 
> static unsigned int check_modem_status(struct
> uart_8250_port *up)
> {
>     unsigned int status = serial_in(up,
> UART_MSR);
> 
>     status |= up->msr_saved_flags;
>     up->msr_saved_flags = 0;
>     if (status & UART_MSR_ANY_DELTA
> && up->ier & UART_IER_MSI &&
>         up->port.state != NULL)
> {
>         if (status &
> UART_MSR_TERI)
>            
> up->port.icount.rng++;
>         if (status &
> UART_MSR_DDSR)
>            
> up->port.icount.dsr++;
>         if (status &
> UART_MSR_DDCD)
>            
> uart_handle_dcd_change(&up->port, status &
> UART_MSR_DCD);
>         if (status &
> UART_MSR_DCTS)
>            
> uart_handle_cts_change(&up->port, status &
> UART_MSR_CTS);
> 
>        
> wake_up_interruptible(&up->port.state->port.delta_msr_wait);
>     }
> 
> I only proposed accomplishing the same result by comparing
> states
> in place of relying on hardware change reports like both of
> the
> above do.
> 
> -- Pete
>


      

[-- Attachment #2: mct_u232.c.patch_v3 --]
[-- Type: application/octet-stream, Size: 5699 bytes --]

--- usb/serial/mct_u232.c.orig	2010-12-26 16:54:16.660294924 -0500
+++ usb/serial/mct_u232.c	2010-12-27 15:29:30.895141755 -0500
@@ -78,6 +78,8 @@
 #include <asm/unaligned.h>
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
+#include <linux/serial.h>
+#include <linux/ioctl.h>
 #include "mct_u232.h"
 
 /*
@@ -104,6 +106,10 @@ static void mct_u232_break_ctl(struct tt
 static int  mct_u232_tiocmget(struct tty_struct *tty, struct file *file);
 static int  mct_u232_tiocmset(struct tty_struct *tty, struct file *file,
 			unsigned int set, unsigned int clear);
+static int  mct_u232_ioctl(struct tty_struct *tty, struct file *file,
+			unsigned int cmd, unsigned long arg);
+static int  mct_u232_get_icount(struct tty_struct *tty,
+			struct serial_icounter_struct *icount);
 static void mct_u232_throttle(struct tty_struct *tty);
 static void mct_u232_unthrottle(struct tty_struct *tty);
 
@@ -150,9 +156,10 @@ static struct usb_serial_driver mct_u232
 	.tiocmset =	     mct_u232_tiocmset,
 	.attach =	     mct_u232_startup,
 	.release =	     mct_u232_release,
+	.ioctl =             mct_u232_ioctl,
+	.get_icount =        mct_u232_get_icount,
 };
 
-
 struct mct_u232_private {
 	spinlock_t lock;
 	unsigned int	     control_state; /* Modem Line Setting (TIOCM) */
@@ -160,6 +167,9 @@ struct mct_u232_private {
 	unsigned char	     last_lsr;      /* Line Status Register */
 	unsigned char	     last_msr;      /* Modem Status Register */
 	unsigned int	     rx_flags;      /* Throttling flags */
+	struct async_icount  icount;
+	wait_queue_head_t    msr_wait;	/* for handling sleeping while waiting
+						for msr change to happen */
 };
 
 #define THROTTLED		0x01
@@ -386,6 +396,20 @@ static int mct_u232_get_modem_stat(struc
 	return rc;
 } /* mct_u232_get_modem_stat */
 
+static void mct_u232_msr_to_icount(struct async_icount *icount,
+						unsigned char msr)
+{
+	/* Translate Control Line states */
+	if (msr & MCT_U232_MSR_DDSR)
+		icount->dsr++;
+	if (msr & MCT_U232_MSR_DCTS)
+		icount->cts++;
+	if (msr & MCT_U232_MSR_DRI)
+		icount->rng++;
+	if (msr & MCT_U232_MSR_DCD)
+		icount->dcd++;
+} /* mct_u232_msr_to_icount */
+
 static void mct_u232_msr_to_state(unsigned int *control_state,
 						unsigned char msr)
 {
@@ -422,6 +446,7 @@ static int mct_u232_startup(struct usb_s
 	if (!priv)
 		return -ENOMEM;
 	spin_lock_init(&priv->lock);
+	init_waitqueue_head(&priv->msr_wait);
 	usb_set_serial_port_data(serial->port[0], priv);
 
 	init_waitqueue_head(&serial->port[0]->write_wait);
@@ -621,6 +646,8 @@ static void mct_u232_read_int_callback(s
 	/* Record Control Line states */
 	mct_u232_msr_to_state(&priv->control_state, priv->last_msr);
 
+	mct_u232_msr_to_icount(&priv->icount, priv->last_msr);
+
 #if 0
 	/* Not yet handled. See belkin_sa.c for further information */
 	/* Now to report any errors */
@@ -647,6 +674,7 @@ static void mct_u232_read_int_callback(s
 		tty_kref_put(tty);
 	}
 #endif
+	wake_up_interruptible(&priv->msr_wait);
 	spin_unlock_irqrestore(&priv->lock, flags);
 exit:
 	retval = usb_submit_urb(urb, GFP_ATOMIC);
@@ -826,7 +854,6 @@ static void mct_u232_throttle(struct tty
 	}
 }
 
-
 static void mct_u232_unthrottle(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
@@ -847,6 +874,82 @@ static void mct_u232_unthrottle(struct t
 	}
 }
 
+static int  mct_u232_ioctl(struct tty_struct *tty, struct file *file,
+			unsigned int cmd, unsigned long arg)
+{
+	DEFINE_WAIT(wait);
+	struct usb_serial_port *port = tty->driver_data;
+	struct mct_u232_private *mct_u232_port = usb_get_serial_port_data(port);
+	struct async_icount cnow, cprev;
+	unsigned long flags;
+
+	dbg("%s - port %d, cmd = 0x%x", __func__, port->number, cmd);
+
+	switch (cmd) {
+
+	case TIOCMIWAIT:
+
+		dbg("%s (%d) TIOCMIWAIT", __func__,  port->number);
+
+		spin_lock_irqsave(&mct_u232_port->lock, flags);
+		cprev = mct_u232_port->icount;
+		spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+		for ( ; ; ) {
+			prepare_to_wait(&mct_u232_port->msr_wait,
+					&wait, TASK_INTERRUPTIBLE);
+			schedule();
+			finish_wait(&mct_u232_port->msr_wait, &wait);
+			/* see if a signal did it */
+			if (signal_pending(current))
+				return -ERESTARTSYS;
+			spin_lock_irqsave(&mct_u232_port->lock, flags);
+			cnow = mct_u232_port->icount;
+			spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+			if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
+			    cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
+				return -EIO; /* no change => error */
+			if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
+			    ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
+			    ((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
+			    ((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
+				return 0;
+			}
+			cprev = cnow;
+		}
+
+	}
+	return -ENOIOCTLCMD;
+}
+
+static int  mct_u232_get_icount(struct tty_struct *tty,
+			struct serial_icounter_struct *icount)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct mct_u232_private *mct_u232_port = usb_get_serial_port_data(port);
+	struct async_icount *ic = &mct_u232_port->icount;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mct_u232_port->lock, flags);
+
+	icount->cts = ic->cts;
+	icount->dsr = ic->dsr;
+	icount->rng = ic->rng;
+	icount->dcd = ic->dcd;
+	icount->rx = ic->rx;
+	icount->tx = ic->tx;
+	icount->frame = ic->frame;
+	icount->overrun = ic->overrun;
+	icount->parity = ic->parity;
+	icount->brk = ic->brk;
+	icount->buf_overrun = ic->buf_overrun;
+
+	spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+
+	dbg("%s (%d) TIOCGICOUNT RX=%d, TX=%d",
+		__func__,  port->number, icount->rx, icount->tx);
+	return 0;
+}
+
 static int __init mct_u232_init(void)
 {
 	int retval;