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

[Tsozik <tsozik@yahoo.com>]

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

Pete,

Again many thanks for the additional review and comments. 

I addressed your second concern and added 3 locks/unlocks around places where mct_u232_port->icount struct is being read (I attached new patch to this email).

Regarding your first concern I have a slight reservation. 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 (Please refer to http://www.lammertbies.nl/comm/info/serial-uart.html for further details). So we set control state when it's raised and clear all the states which are not raised at the moment. Implementing your proposal would increase ring state reading by two times if between 2 consecutive rings another state is raised at the moment when first ring state is off. 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. In addition after I completed initial implementation I benchmarked rm60 radiation readings
 against PDM-2 readings. The differences were far below expected maximum expected error threshold.

Thank you again,
 Vadim.

--- On Sun, 12/26/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, zaitcev@redhat.com
> Date: Sunday, December 26, 2010, 10:57 PM
> On Sun, 26 Dec 2010 18:58:46 -0800
> (PST)
> Tsozik <tsozik@yahoo.com>
> wrote:
> 
> > Please let me know if there's anything else I need to
> correct before
> > it can be rolled out,
> 
> I am hardly an expert here, but I looked it up as much as I
> could,
> and this looks like a wrong way to do it:
> 
> > @@ -386,27 +396,41 @@ static int
> mct_u232_get_modem_stat(struc
> >      /* Translate Control Line
> states */
> > +    if (msr & MCT_U232_MSR_DSR) {
> >         
> *control_state |=  TIOCM_DSR;
> > +       
> icount->dsr++;
> > +    } else {
> >         
> *control_state &= ~TIOCM_DSR;
> > +    }
> 
> Unfortunately, Kerrisk's manpages do not seem to offer an
> authoritative
> definition, but the expectations across the code seems that
> the counter
> actually counts the changes. I know that some people talked
> about
> counting "interrupts", but the way it's implemented in
> trustworthy
> drivers suggests something like this:
> 
>     unsigned int old_ctl_state;
> 
>     old_ctl_state = *control_state;
>     if (msr & MCT_U232_MSR_DSR)
>         *control_state
> |=  TIOCM_DSR;
>     else
>         *control_state &=
> ~TIOCM_DSR;
>     if (old_ctl_state != *control_state)
>         icount->dsr++;
>     ...... repeat for all bits
> 
> E.g. for DSR going down too, although perhaps I'm not
> suggesting the
> best way to do it. Could you re-implement it like the above
> and
> check that your radiation counter works the same with it
> and
> when driven by a built-in serial port?
> 
> Another thing, you should take priv->lock around
> fetching of
> mct_u232_port->icount in mct_u232_ioctl. It seems
> superfluous,
> I know, because the API itself is racy, but just to be
> nice
> and doing the expected thing...
> 
> -- Pete
>


      

[-- Attachment #2: mct_u232.c.patch.submit_v2 --]
[-- Type: application/octet-stream, Size: 7161 bytes --]

From: From: Vadim Tsozik <tsozik@yahoo.com>

Added mct_u232_ioctl (implements TIOCMIWAIT command) and mct_u232_get_icount (implements TIOCGICOUNT command) 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 00:09:33.638894938 -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,27 +396,41 @@ static int mct_u232_get_modem_stat(struc
 	return rc;
 } /* mct_u232_get_modem_stat */
 
-static void mct_u232_msr_to_state(unsigned int *control_state,
-						unsigned char msr)
+static void mct_u232_msr_to_state(struct mct_u232_private *priv)
 {
+	unsigned char msr = priv->last_msr;
+	unsigned int *control_state = &priv->control_state;
+	struct async_icount *icount = &priv->icount;
+
 	/* Translate Control Line states */
-	if (msr & MCT_U232_MSR_DSR)
+	if (msr & MCT_U232_MSR_DSR) {
 		*control_state |=  TIOCM_DSR;
-	else
+		icount->dsr++;
+	} else {
 		*control_state &= ~TIOCM_DSR;
-	if (msr & MCT_U232_MSR_CTS)
+	}
+	if (msr & MCT_U232_MSR_CTS) {
 		*control_state |=  TIOCM_CTS;
-	else
+		icount->cts++;
+	} else {
 		*control_state &= ~TIOCM_CTS;
-	if (msr & MCT_U232_MSR_RI)
+	}
+	if (msr & MCT_U232_MSR_RI) {
 		*control_state |=  TIOCM_RI;
-	else
+		icount->rng++;
+	} else {
 		*control_state &= ~TIOCM_RI;
-	if (msr & MCT_U232_MSR_CD)
+	}
+	if (msr & MCT_U232_MSR_CD) {
 		*control_state |=  TIOCM_CD;
-	else
+		icount->dcd++;
+	} else {
 		*control_state &= ~TIOCM_CD;
+	}
+
 	dbg("msr_to_state: msr=0x%x ==> state=0x%x", msr, *control_state);
+
+	wake_up_interruptible(&priv->msr_wait);
 } /* mct_u232_msr_to_state */
 
 /*
@@ -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);
@@ -498,7 +523,7 @@ static int  mct_u232_open(struct tty_str
 	mct_u232_get_modem_stat(serial, &last_msr);
 	spin_lock_irqsave(&priv->lock, flags);
 	priv->last_msr = last_msr;
-	mct_u232_msr_to_state(&priv->control_state, priv->last_msr);
+	mct_u232_msr_to_state(priv);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	port->read_urb->dev = port->serial->dev;
@@ -619,7 +644,7 @@ static void mct_u232_read_int_callback(s
 	priv->last_msr = data[MCT_U232_MSR_INDEX];
 
 	/* Record Control Line states */
-	mct_u232_msr_to_state(&priv->control_state, priv->last_msr);
+	mct_u232_msr_to_state(priv);
 
 #if 0
 	/* Not yet handled. See belkin_sa.c for further information */
@@ -826,7 +851,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 +871,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;