akpm@xxxxxxxxxxxxxxxxxxxx wrote:
> From: Atsushi Nemoto <anemo@xxxxxxxxxxxxx>
>
> 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.
>
> Acked-by: Remy Bohmer <linux@xxxxxxxxxx>
> Cc: Haavard Skinnemoen <hskinnemoen@xxxxxxxxx>
> Cc: Marc Pignat <marc.pignat@xxxxxxx>
> Signed-off-by: Atsushi Nemoto <anemo@xxxxxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
I think I already ACKed this. However...
> diff -puN drivers/serial/atmel_serial.c~atmel_serial-might-lose-modem-status-change drivers/serial/atmel_serial.c
> --- a/drivers/serial/atmel_serial.c~atmel_serial-might-lose-modem-status-change
> +++ a/drivers/serial/atmel_serial.c
> @@ -877,6 +877,9 @@ static int atmel_startup(struct uart_por
> }
> }
>
> + /* Save current CSR for comparison in atmel_tasklet_func() */
> + atmel_port->irq_status_prev = UART_GET_CSR(port);
> +
...Itai Levi pointed out that we need to initialize
atmel_port->irq_status as well here. His analysis is as follows:
> Regarding the second part of the patch (which resets irq_status_prev),
> it turns out that both versions of the patch (mine and Atsushi's)
> still leave enough room for faulty behavior when opening the port.
>
> This is because we are not resetting both irq_status_prev and
> irq_status in atmel_startup() to CSR, which leads faulty behavior in
> the following sequences:
>
> First case:
> 1. closing the port while CTS line = 1 (TX not allowed)
> 2. setting CTS line = 0 (TX allowed)
> 3. opening the port
> 4. transmitting one char
> 5. Cannot transmit more chars, although CTS line is 0
>
> Second case:
> 1. closing the port while CTS line = 0 (TX allowed)
> 2. setting CTS line = 1 (TX not allowed)
> 3. opening the port
> 4. receiving some chars
> 5. Now we can transmit, although CTS line is 1
>
> This reason for this is that the tasklet is scheduled as a result of
> TX or RX interrupts (not a status change!), in steps 4 above. Inside
> the tasklet, the atmel_port->irq_status (which holds the value from
> the previous session) is compared to atmel_port->irq_status_prev.
> Hence, a status-change of the CTS line is faultily detected.
>
> Both cases were verified on 9260 hardware.
>
> The solution for this, is to reset both irq_status_prev and irq_status
> to the value of CSR in atmel_startup().
>
> Here is my updated patch (you can ignore the first part which you
> already accepted):
--- linux-2.6.27.6/drivers/serial/atmel_serial.c 2008-11-13 19:56:21.000000000 +0200
+++ my/drivers/serial/atmel_serial.c 2009-01-08 11:32:50.000000000 +0200
@@ -579,7 +579,7 @@ static void atmel_tx_dma(struct uart_por
- if (!uart_circ_empty(xmit)) {
+ if (!uart_circ_empty(xmit) && !uart_tx_stopped(port)) {
dma_sync_single_for_device(port->dev,
pdc->dma_addr,
pdc->dma_size,
@@ -805,6 +805,9 @@ static int atmel_startup(struct uart_por
*/
UART_PUT_IDR(port, -1);
+ atmel_port->irq_status_prev = UART_GET_CSR(port);
+ atmel_port->irq_status = atmel_port->irq_status_prev;
+
/*
* Allocate the IRQ
*/
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
