New IOCTL for Modem Control Lines monitoring

New IOCTL for Modem Control Lines monitoring

[Dominique Larchey-Wendling]

In kernel 2.6 (file drivers/serial/serial_core.c), there exists 2 IOCTL
for receiving signals over the modem control lines

TIOCMIWAIT  (function uart_wait_modem_status) for waiting for a change
             on the modem status lines.

TIOCGICOUNT (uart_get_count) to get the current state of the modem
              status lines.

While these IOCTL may be helpful to use the serial port as a general
serial communication device over modem control lines like in
the following (and quite old) discussion

http://www.mail-archive.com/linux-serial@vger.rutgers.edu/msg00407.html

I find a kind of "design" flaw with the 2 IOCTL for such a goal.

Indeed, it is possible that a modem status change occurs in between
ioctl(TIOCGICOUNT) and ioctl(TIOCMIWAIT) possibly locking a
communication because the last call would miss the status change
necessary to trigger a response. Indeed ioctl(TIOCMIWAIT) cannot
detect a change occurring before its call.

Even tough such an event would have a low probability to occur
if the 2 ioctl are close enough, it is still possible. This is
a race condition (outside the kernel but possibly locking some
process).

I propose the introduction of a new ioctl to solve this race,
implemented through the NEW function uart_wait_new_status associated
to a NEW ioctl.

The idea is that this new call would detect a change not compared
to the status at the beginning of the call but compared to some
previously recorded state. This way, the new ioctl would not
miss a status change, even if it occurs before the call to the
ioctl.

Could it be considered for inclusion in the kernel ?
It does not change any existing behavior. However it
does need a new ioctl name/number.

Thanks in advance for any comment

Dominique Larchey-Wendling
CNRS, France

-------------------------------------------------
Code to include in drivers/serial/serial_core.c
-------------------------------------------------

/*
  * Wait until any NEW signal is received on some selected
  * modem inputs lines (DCD,RI,DSR,CTS), compared to some
  * previously considered state.
  * If asked to wait on none, simply returns the current
  * status
  */

static int
uart_wait_new_status(struct uart_state *state, struct 
serial_icounter_struct __user *icnt)
{
	struct uart_port *port = state->port;
	DECLARE_WAITQUEUE(wait, current);
         struct serial_icounter_struct icount;
	struct uart_icount cstart, cnow;
         unsigned int mask, mctrl;
	int ret;

         if (copy_from_user(&icount, icnt, sizeof(icount)))
                 return -EFAULT;

         cstart.cts         = icount.cts;
         cstart.dsr         = icount.dsr;
         cstart.rng         = icount.rng;
         cstart.dcd         = icount.dcd;
         cstart.rx          = icount.rx;
         cstart.tx          = icount.tx;
         cstart.frame       = icount.frame;
         cstart.overrun     = icount.overrun;
         cstart.parity      = icount.parity;
         cstart.buf_overrun = icount.buf_overrun;

         /* if mask == 0 just simulate behavior of TIOCGICOUNT
          */

         mask = (unsigned) icount.reserved[0];

         /* enable Modem Status Interrupts in case not already done
          */

	spin_lock_irq(&port->lock);
	port->ops->enable_ms(port);
	spin_unlock_irq(&port->lock);

	add_wait_queue(&state->info->delta_msr_wait, &wait);

	for (;;) {
		spin_lock_irq(&port->lock);
                 mctrl = port->ops->get_mctrl(port);
		memcpy(&cnow, &port->icount, sizeof(struct uart_icount));
		spin_unlock_irq(&port->lock);

		set_current_state(TASK_INTERRUPTIBLE);

		if (((mask & TIOCM_RNG) && (cnow.rng != cstart.rng)) ||
		    ((mask & TIOCM_DSR) && (cnow.dsr != cstart.dsr)) ||
		    ((mask & TIOCM_CAR) && (cnow.dcd != cstart.dcd)) ||
		    ((mask & TIOCM_CTS) && (cnow.cts != cstart.cts)) ||
                      (mask == 0))
                 {
		    	ret = 0;
		    	break;
		}

		schedule();

		/* see if a signal did it */
		if (signal_pending(current)) {
			ret = -ERESTARTSYS;
			break;
		}

	}

	current->state = TASK_RUNNING;
	remove_wait_queue(&state->info->delta_msr_wait, &wait);

         if (ret < 0) return ret;

         /* we got our (new) state, transfer to user space
          */

         icount.cts         = cnow.cts;
	icount.dsr         = cnow.dsr;
	icount.rng         = cnow.rng;
	icount.dcd         = cnow.dcd;
	icount.rx          = cnow.rx;
	icount.tx          = cnow.tx;
	icount.frame       = cnow.frame;
	icount.overrun     = cnow.overrun;
	icount.parity      = cnow.parity;
	icount.brk         = cnow.brk;
	icount.buf_overrun = cnow.buf_overrun;
         icount.reserved[0] = mctrl;

         return copy_to_user(icnt, &icount, sizeof(icount)) ? -EFAULT : 0;
}

RE: New IOCTL for Modem Control Lines monitoring

[Tosoni]

Sounds like an equivalent to WaitCommEvent in Windows NT.

But there is a problem in Linux, which does not exist in Windows where the
serial port is locked by the requesting process: in Linux you would have to
associate your "recorded state" with the requesting process, because several
processes may use your new ioctl at the same time.

Or, you can say that it's a feature of your ioctl to handle only one process
:-)

Also, in Windows there is an extra feature, you can set a mask of which
signal changes you want to monitor, you might want this feature as well

Best regards
Tosoni, Acksys

> -----Message d'origine-----
> De : linux-serial-owner@vger.kernel.org
> [mailto:linux-serial-owner@vger.kernel.org]De la part de Dominique
> Larchey-Wendling
> Envoyé : jeudi 25 janvier 2007 18:10
> À : linux-serial@vger.kernel.org
> Objet : New IOCTL for Modem Control Lines monitoring
>
>
> In kernel 2.6 (file drivers/serial/serial_core.c), there
> exists 2 IOCTL
> for receiving signals over the modem control lines
>
> TIOCMIWAIT  (function uart_wait_modem_status) for waiting for a change
>              on the modem status lines.
>
> TIOCGICOUNT (uart_get_count) to get the current state of the modem
>               status lines.
>
> While these IOCTL may be helpful to use the serial port as a general
> serial communication device over modem control lines like in
> the following (and quite old) discussion
>
> http://www.mail-archive.com/linux-serial@vger.rutgers.edu/msg0
> 0407.html
>
> I find a kind of "design" flaw with the 2 IOCTL for such a goal.
>
> Indeed, it is possible that a modem status change occurs in between
> ioctl(TIOCGICOUNT) and ioctl(TIOCMIWAIT) possibly locking a
> communication because the last call would miss the status change
> necessary to trigger a response. Indeed ioctl(TIOCMIWAIT) cannot
> detect a change occurring before its call.
>
> Even tough such an event would have a low probability to occur
> if the 2 ioctl are close enough, it is still possible. This is
> a race condition (outside the kernel but possibly locking some
> process).
>
> I propose the introduction of a new ioctl to solve this race,
> implemented through the NEW function uart_wait_new_status associated
> to a NEW ioctl.
>
> The idea is that this new call would detect a change not compared
> to the status at the beginning of the call but compared to some
> previously recorded state. This way, the new ioctl would not
> miss a status change, even if it occurs before the call to the
> ioctl.
>
> Could it be considered for inclusion in the kernel ?
> It does not change any existing behavior. However it
> does need a new ioctl name/number.
>
> Thanks in advance for any comment
>
> Dominique Larchey-Wendling
> CNRS, France
>
> -------------------------------------------------
> Code to include in drivers/serial/serial_core.c
> -------------------------------------------------
>
> /*
>   * Wait until any NEW signal is received on some selected
>   * modem inputs lines (DCD,RI,DSR,CTS), compared to some
>   * previously considered state.
>   * If asked to wait on none, simply returns the current
>   * status
>   */
>
> static int
> uart_wait_new_status(struct uart_state *state, struct
> serial_icounter_struct __user *icnt)
> {
> 	struct uart_port *port = state->port;
> 	DECLARE_WAITQUEUE(wait, current);
>          struct serial_icounter_struct icount;
> 	struct uart_icount cstart, cnow;
>          unsigned int mask, mctrl;
> 	int ret;
>
>          if (copy_from_user(&icount, icnt, sizeof(icount)))
>                  return -EFAULT;
>
>          cstart.cts         = icount.cts;
>          cstart.dsr         = icount.dsr;
>          cstart.rng         = icount.rng;
>          cstart.dcd         = icount.dcd;
>          cstart.rx          = icount.rx;
>          cstart.tx          = icount.tx;
>          cstart.frame       = icount.frame;
>          cstart.overrun     = icount.overrun;
>          cstart.parity      = icount.parity;
>          cstart.buf_overrun = icount.buf_overrun;
>
>          /* if mask == 0 just simulate behavior of TIOCGICOUNT
>           */
>
>          mask = (unsigned) icount.reserved[0];
>
>          /* enable Modem Status Interrupts in case not already done
>           */
>
> 	spin_lock_irq(&port->lock);
> 	port->ops->enable_ms(port);
> 	spin_unlock_irq(&port->lock);
>
> 	add_wait_queue(&state->info->delta_msr_wait, &wait);
>
> 	for (;;) {
> 		spin_lock_irq(&port->lock);
>                  mctrl = port->ops->get_mctrl(port);
> 		memcpy(&cnow, &port->icount, sizeof(struct
> uart_icount));
> 		spin_unlock_irq(&port->lock);
>
> 		set_current_state(TASK_INTERRUPTIBLE);
>
> 		if (((mask & TIOCM_RNG) && (cnow.rng != cstart.rng)) ||
> 		    ((mask & TIOCM_DSR) && (cnow.dsr != cstart.dsr)) ||
> 		    ((mask & TIOCM_CAR) && (cnow.dcd != cstart.dcd)) ||
> 		    ((mask & TIOCM_CTS) && (cnow.cts != cstart.cts)) ||
>                       (mask == 0))
>                  {
> 		    	ret = 0;
> 		    	break;
> 		}
>
> 		schedule();
>
> 		/* see if a signal did it */
> 		if (signal_pending(current)) {
> 			ret = -ERESTARTSYS;
> 			break;
> 		}
>
> 	}
>
> 	current->state = TASK_RUNNING;
> 	remove_wait_queue(&state->info->delta_msr_wait, &wait);
>
>          if (ret < 0) return ret;
>
>          /* we got our (new) state, transfer to user space
>           */
>
>          icount.cts         = cnow.cts;
> 	icount.dsr         = cnow.dsr;
> 	icount.rng         = cnow.rng;
> 	icount.dcd         = cnow.dcd;
> 	icount.rx          = cnow.rx;
> 	icount.tx          = cnow.tx;
> 	icount.frame       = cnow.frame;
> 	icount.overrun     = cnow.overrun;
> 	icount.parity      = cnow.parity;
> 	icount.brk         = cnow.brk;
> 	icount.buf_overrun = cnow.buf_overrun;
>          icount.reserved[0] = mctrl;
>
>          return copy_to_user(icnt, &icount, sizeof(icount)) ?
> -EFAULT : 0;
> }
> -
> To unsubscribe from this list: send the line "unsubscribe
> linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: New IOCTL for Modem Control Lines monitoring

[Dominique Larchey]

Tosoni a écrit :
> Sounds like an equivalent to WaitCommEvent in Windows NT.
> 
> But there is a problem in Linux, which does not exist in Windows where the
> serial port is locked by the requesting process: in Linux you would have to
> associate your "recorded state" with the requesting process, because several
> processes may use your new ioctl at the same time.

Yes the recorded state COULD BE recorded by the same process that makes
the call. This process can get the status by reading the OUTPUT
icount.reserved[0] field each time the ioctl(uart_wait_new_status)
returns. Several processes can wait for a change each wrt to its own
"recorded state".

> Or, you can say that it's a feature of your ioctl to handle only one process
> :-)

This restriction is not necessary I think.

> Also, in Windows there is an extra feature, you can set a mask of which
> signal changes you want to monitor, you might want this feature as well

It exists in the code. Indeed, the INPUT field icount.reserved[0]
contains the corresponding mask. If this mask is 0, then the behavior
is not to wait forever but to return immediately, given back the
current modem status in the OUTPUT icount.reserved[0] field.

Regards,

Dominique LW
-
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: New IOCTL for Modem Control Lines monitoring

[Theodore Tso]

On Thu, Jan 25, 2007 at 06:09:33PM +0100, Dominique Larchey-Wendling wrote:
> Even tough such an event would have a low probability to occur
> if the 2 ioctl are close enough, it is still possible. This is
> a race condition (outside the kernel but possibly locking some
> process).
> 
> I propose the introduction of a new ioctl to solve this race,
> implemented through the NEW function uart_wait_new_status associated
> to a NEW ioctl.
> 
> The idea is that this new call would detect a change not compared
> to the status at the beginning of the call but compared to some
> previously recorded state. This way, the new ioctl would not
> miss a status change, even if it occurs before the call to the
> ioctl.

You're right that this is a technical flaw in TIOCMIWAIT.  When it was
originally implemented, it was done to retain compatibility with older
implementations in other OS's.

The question is why do you need such functionality?  The only way to
implement what you propose would be pass a structure containing the
previous counts into your proposed new ioctl(), which won't be well
received by the folks who have to maintain 32/64-bit translation
tables for ioctl's for use by supporting architectures that have to
support 32 and 64 bit ABI's simultaneously.  Because of this issue
some folks have proposed killing off ioctl's entirely, which is
probably not the right answer, but the fact remains that adding new
ioctl's that require passing in pointers to arbitrary data structures
is definitely not going to be well received.

So what are you actually trying to *do*?  Is this just to fix a
theoretical shortcoming?  What does your application really need to
do, and perhaps there's a another way we can address it with perhaps a
cleaner interface.

					- Ted

Re: New IOCTL for Modem Control Lines monitoring

[Dominique Larchey-Wendling]

 > The question is why do you need such functionality?  The only way to
 > implement what you propose would be pass a structure containing the
 > previous counts into your proposed new ioctl(), which won't be well
 > received by the folks who have to maintain 32/64-bit translation
 > tables for ioctl's for use by supporting architectures that have to
 > support 32 and 64 bit ABI's simultaneously.

Well I do not clearly understand the problem you refer to because
it seems to me that the new function uart_wait_new_status would not
need extra structure compared to function uart_get_count (ioctl 
TIOCGICOUNT) for example. As shown in my previous mail, the profile
of the function would be :

static int
uart_wait_new_status(struct uart_state *state, struct 
serial_icounter_struct __user *icnt)

the "icnt" argument serving as an input as well as an output.
The mask could be in icount.reserved[0]

But maybe the folks (32/64 porters and ioctl haters) you refer
to already do not like the existing ioctls and do not want to
add a new one, even if its interface is similar to TIOCGICOUNT.

 > So what are you actually trying to *do*?  Is this just to fix a
 > theoretical shortcoming?  What does your application really need to
 > do, and perhaps there's a another way we can address it with perhaps a
 > cleaner interface.

Well the application I want to improve is a "serial" (userspace) driver
for a Lacrosse Weatherstation WS 8610, communicating only through modem 
control lines. There exists a usable driver but it uses polling and
so grabs all the cpu for its processing.

- Dominique

Theodore Tso wrote:
> On Thu, Jan 25, 2007 at 06:09:33PM +0100, Dominique Larchey-Wendling wrote:
>> Even tough such an event would have a low probability to occur
>> if the 2 ioctl are close enough, it is still possible. This is
>> a race condition (outside the kernel but possibly locking some
>> process).
>>
>> I propose the introduction of a new ioctl to solve this race,
>> implemented through the NEW function uart_wait_new_status associated
>> to a NEW ioctl.
>>
>> The idea is that this new call would detect a change not compared
>> to the status at the beginning of the call but compared to some
>> previously recorded state. This way, the new ioctl would not
>> miss a status change, even if it occurs before the call to the
>> ioctl.
> 
> You're right that this is a technical flaw in TIOCMIWAIT.  When it was
> originally implemented, it was done to retain compatibility with older
> implementations in other OS's.
> 
> The question is why do you need such functionality?  The only way to
> implement what you propose would be pass a structure containing the
> previous counts into your proposed new ioctl(), which won't be well
> received by the folks who have to maintain 32/64-bit translation
> tables for ioctl's for use by supporting architectures that have to
> support 32 and 64 bit ABI's simultaneously.  Because of this issue
> some folks have proposed killing off ioctl's entirely, which is
> probably not the right answer, but the fact remains that adding new
> ioctl's that require passing in pointers to arbitrary data structures
> is definitely not going to be well received.
> 
> So what are you actually trying to *do*?  Is this just to fix a
> theoretical shortcoming?  What does your application really need to
> do, and perhaps there's a another way we can address it with perhaps a
> cleaner interface.
> 
> 					- Ted
> -
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html