2008-11-12 17:16:40

by Atsushi Nemoto

Subject: atmel_serial might lose modem status change

I found a problem of handling of modem status of atmel_serial driver.

With the commit 1ecc26 ("atmel_serial: split the interrupt handler"),
handling of modem status signal was splitted into two parts. The
atmel_tasklet_func() compares new status with irq_status_prev, but
irq_status_prev is not correct if signal status was changed while the
port is closed.

Here is a sequence to cause problem:

1. Remote side sets CTS (and DSR).
2. Local side close the port.
3. Local side clears RTS and DTR.
4. Remote side clears CTS and DSR.
5. Local side reopen the port. hw_stopped becomes 1.
6. Local side sets RTS and DTR.
7. Remote side sets CTS and DSR.

Then CTS change interrupt can be received, but since CTS bit in
irq_status_prev and new status is same, uart_handle_cts_change() will
not be called (so hw_stopped will not be cleared, i.e. cannot send any
data).

I suppose irq_status_prev should be initialized at somewhere in open
sequence. Is this fix correct?

diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 4c535f7..9f3b697 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -877,6 +877,9 @@ static int atmel_startup(struct uart_port *port)
}
}

+ /* Save current CSR for comparison in atmel_tasklet_func() */
+ atmel_port->irq_status_prev = UART_GET_CSR(port);
+
/*
* Finally, enable the serial port
*/

---
Atsushi Nemoto

2008-11-12 19:04:37

by Remy Bohmer

Subject: Re: atmel_serial might lose modem status change

Hello Atsushi,

2008/11/12 Atsushi Nemoto <[email protected]>:
> I found a problem of handling of modem status of atmel_serial driver.
>
> With the commit 1ecc26 ("atmel_serial: split the interrupt handler"),
> handling of modem status signal was splitted into two parts. The
> atmel_tasklet_func() compares new status with irq_status_prev, but
> irq_status_prev is not correct if signal status was changed while the
> port is closed.
>
> Here is a sequence to cause problem:
>
> 1. Remote side sets CTS (and DSR).
> 2. Local side close the port.
> 3. Local side clears RTS and DTR.
> 4. Remote side clears CTS and DSR.
> 5. Local side reopen the port. hw_stopped becomes 1.
> 6. Local side sets RTS and DTR.
> 7. Remote side sets CTS and DSR.
>
> Then CTS change interrupt can be received, but since CTS bit in
> irq_status_prev and new status is same, uart_handle_cts_change() will
> not be called (so hw_stopped will not be cleared, i.e. cannot send any
> data).
>
> I suppose irq_status_prev should be initialized at somewhere in open
> sequence. Is this fix correct?

I cannot shoot a hole in it... So, to me it appears okay.

Kind Regards,

Remy


> diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
> index 4c535f7..9f3b697 100644
> --- a/drivers/serial/atmel_serial.c
> +++ b/drivers/serial/atmel_serial.c
> @@ -877,6 +877,9 @@ static int atmel_startup(struct uart_port *port)
> }
> }
>
> + /* Save current CSR for comparison in atmel_tasklet_func() */
> + atmel_port->irq_status_prev = UART_GET_CSR(port);
> +
> /*
> * Finally, enable the serial port
> */
>
> ---
> Atsushi Nemoto
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2009-01-05 15:20:40

by Haavard Skinnemoen

Subject: Re: atmel_serial might lose modem status change

Remy Bohmer wrote:
> > I suppose irq_status_prev should be initialized at somewhere in open
> > sequence. Is this fix correct?
>
> I cannot shoot a hole in it... So, to me it appears okay.

Looks good to me too.

Acked-by: Haavard Skinnemoen <[email protected]>