If we don't disable the transmitter in atmel_stop_tx, the DMA buffer
continues to send data until it is emptied.
This cause problems with the flow control (CTS is asserted and data are
still sent).
So, disabling the transmitter in atmel_stop_tx is a sane thing to do.
Tested on at91sam9g35-cm(DMA)
Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC)
Cc: <[email protected]> (beware, this won't apply before 4.3)
Signed-off-by: Richard Genoud <[email protected]>
---
drivers/tty/serial/atmel_serial.c | 11 +++++++++++
1 file changed, 11 insertions(+)
NB: this is not for the 4.10 merge window, I'm just sending it now to
have some comments if someone is againts it.
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 168b10cad47b..f9d42de5ab2d 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port)
/* disable PDC transmit */
atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS);
}
+
+ /*
+ * Disable the transmitter.
+ * This is mandatory when DMA is used, otherwise the DMA buffer
+ * is fully transmitted.
+ */
+ atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
+
/* Disable interrupts */
atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
@@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port)
/* Enable interrupts */
atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask);
+
+ /* re-enable the transmitter */
+ atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
}
/*
Le 13/12/2016 ? 17:27, Richard Genoud a ?crit :
> If we don't disable the transmitter in atmel_stop_tx, the DMA buffer
> continues to send data until it is emptied.
> This cause problems with the flow control (CTS is asserted and data are
> still sent).
>
> So, disabling the transmitter in atmel_stop_tx is a sane thing to do.
>
> Tested on at91sam9g35-cm(DMA)
> Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC)
>
> Cc: <[email protected]> (beware, this won't apply before 4.3)
> Signed-off-by: Richard Genoud <[email protected]>
> ---
> drivers/tty/serial/atmel_serial.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> NB: this is not for the 4.10 merge window, I'm just sending it now to
> have some comments if someone is againts it.
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 168b10cad47b..f9d42de5ab2d 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port)
> /* disable PDC transmit */
> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS);
> }
> +
> + /*
> + * Disable the transmitter.
> + * This is mandatory when DMA is used, otherwise the DMA buffer
> + * is fully transmitted.
> + */
> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
> +
> /* Disable interrupts */
> atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
>
> @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port)
>
> /* Enable interrupts */
> atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask);
> +
> + /* re-enable the transmitter */
> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
> }
>
> /*
Hi Richard,
I've just discovered that I have some weird behavior with this patch. On
current Linus' tree, with sama5d2 + DMA, I see some garbage characters
coming out of the console when I try to stop my system (reboot/halt) [1].
Moreover, and I do understand that it's not the problem right here, when
applied on our linux-4.4-at91 branch (our vendor tree actually), it
hangs the boot process as it seems that a burst of open/close of the
serial port happens while starting the rootfs. It's definitively my own
problem, but it can bring light to what we are seeing on Mainline...
I think that we may need to flush the DMA channel in this
atmel_stop_tx() function.
Best regards,
[1] If you want to test, you need to apply this patch for eMMC BTW:
https://patchwork.kernel.org/patch/9617489/
--
Nicolas Ferre
2017-03-15 12:37 GMT+01:00 Nicolas Ferre <[email protected]>:
> Le 13/12/2016 à 17:27, Richard Genoud a écrit :
>> If we don't disable the transmitter in atmel_stop_tx, the DMA buffer
>> continues to send data until it is emptied.
>> This cause problems with the flow control (CTS is asserted and data are
>> still sent).
>>
>> So, disabling the transmitter in atmel_stop_tx is a sane thing to do.
>>
>> Tested on at91sam9g35-cm(DMA)
>> Tested for regressions on sama5d2-xplained(Fifo) and at91sam9g20ek(PDC)
>>
>> Cc: <[email protected]> (beware, this won't apply before 4.3)
>> Signed-off-by: Richard Genoud <[email protected]>
>> ---
>> drivers/tty/serial/atmel_serial.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> NB: this is not for the 4.10 merge window, I'm just sending it now to
>> have some comments if someone is againts it.
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 168b10cad47b..f9d42de5ab2d 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -481,6 +481,14 @@ static void atmel_stop_tx(struct uart_port *port)
>> /* disable PDC transmit */
>> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS);
>> }
>> +
>> + /*
>> + * Disable the transmitter.
>> + * This is mandatory when DMA is used, otherwise the DMA buffer
>> + * is fully transmitted.
>> + */
>> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXDIS);
>> +
>> /* Disable interrupts */
>> atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
>>
>> @@ -513,6 +521,9 @@ static void atmel_start_tx(struct uart_port *port)
>>
>> /* Enable interrupts */
>> atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask);
>> +
>> + /* re-enable the transmitter */
>> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
>> }
>>
>> /*
>
> Hi Richard,
Hi !
>
> I've just discovered that I have some weird behavior with this patch. On
> current Linus' tree, with sama5d2 + DMA, I see some garbage characters
> coming out of the console when I try to stop my system (reboot/halt) [1].
Yes, I've also seen that on my board, it was on my todo list.
(Although I didn't know this patch was the culprit !)
>
> Moreover, and I do understand that it's not the problem right here, when
> applied on our linux-4.4-at91 branch (our vendor tree actually), it
> hangs the boot process as it seems that a burst of open/close of the
> serial port happens while starting the rootfs. It's definitively my own
> problem, but it can bring light to what we are seeing on Mainline...
aïe !
This is clearly something we'll have to understand !
> I think that we may need to flush the DMA channel in this
> atmel_stop_tx() function.
>
yes, I'll look into that.
> Best regards,
>
> [1] If you want to test, you need to apply this patch for eMMC BTW:
> https://patchwork.kernel.org/patch/9617489/
>
> --
> Nicolas Ferre
Thanks !
Richard.
A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA
from transmitting in stop_tx") is that the console can be called with
TX path disabled. Then the system would hang trying to push charecters
out in atmel_console_putchar().
Signed-off-by: Nicolas Ferre <[email protected]>
Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting
in stop_tx")
Cc: stable <[email protected]> # 4.4+
---
Hi Richard,
I found this to fix the problem with system hang in my linux-4.4-at91 branch
(in the atmel_console_putchar() waiting loop actually). I'm open to more
insignt.
As we cannot figure out if this bit is set or not, I didn't preserve the
current status...
Regards,
drivers/tty/serial/atmel_serial.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index dcebb28ffbc4..7372dbdb7a4c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2483,6 +2483,9 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
pdc_tx = atmel_uart_readl(port, ATMEL_PDC_PTSR) & ATMEL_PDC_TXTEN;
atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS);
+ /* Make sure that tx path is actually able to send characters */
+ atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
+
uart_console_write(port, s, count, atmel_console_putchar);
/*
--
2.9.0
On 15/03/2017 16:29, Nicolas Ferre wrote:
> A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA
> from transmitting in stop_tx") is that the console can be called with
> TX path disabled. Then the system would hang trying to push charecters
> out in atmel_console_putchar().
>
> Signed-off-by: Nicolas Ferre <[email protected]>
> Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting
> in stop_tx")
> Cc: stable <[email protected]> # 4.4+
> ---
> Hi Richard,
>
> I found this to fix the problem with system hang in my linux-4.4-at91 branch
> (in the atmel_console_putchar() waiting loop actually). I'm open to more
> insignt.
> As we cannot figure out if this bit is set or not, I didn't preserve the
> current status...
>
> Regards,
So, I'm guessing that you may see some lines/characters printed twice on
the screen, don't you ?
> drivers/tty/serial/atmel_serial.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index dcebb28ffbc4..7372dbdb7a4c 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2483,6 +2483,9 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
> pdc_tx = atmel_uart_readl(port, ATMEL_PDC_PTSR) & ATMEL_PDC_TXTEN;
> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS);
>
> + /* Make sure that tx path is actually able to send characters */
> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
> +
> uart_console_write(port, s, count, atmel_console_putchar);
>
> /*
>
Richard.
Le 15/03/2017 à 17:19, Richard Genoud a écrit :
> On 15/03/2017 16:29, Nicolas Ferre wrote:
>> A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA
>> from transmitting in stop_tx") is that the console can be called with
>> TX path disabled. Then the system would hang trying to push charecters
>> out in atmel_console_putchar().
>>
>> Signed-off-by: Nicolas Ferre <[email protected]>
>> Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting
>> in stop_tx")
>> Cc: stable <[email protected]> # 4.4+
>> ---
>> Hi Richard,
>>
>> I found this to fix the problem with system hang in my linux-4.4-at91 branch
>> (in the atmel_console_putchar() waiting loop actually). I'm open to more
>> insignt.
>> As we cannot figure out if this bit is set or not, I didn't preserve the
>> current status...
>>
>> Regards,
>
> So, I'm guessing that you may see some lines/characters printed twice on
> the screen, don't you ?
Well, actually, I don't think so because the repetitions that I see are
probably due to open/close/open/close/re-open/... of the serial console
itself.
Same with the line "random: udevd: uninitialized urandom read (16 bytes
read, 21 bits of entropy available)", they happen at different moment in
time => the printk log timestamping seem to indicate that they are
different.
Here is my log:
http://code.bulix.org/bok23n-123056
Regards,
>> drivers/tty/serial/atmel_serial.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index dcebb28ffbc4..7372dbdb7a4c 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -2483,6 +2483,9 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
>> pdc_tx = atmel_uart_readl(port, ATMEL_PDC_PTSR) & ATMEL_PDC_TXTEN;
>> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS);
>>
>> + /* Make sure that tx path is actually able to send characters */
>> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
>> +
>> uart_console_write(port, s, count, atmel_console_putchar);
>>
>> /*
>>
>
> Richard.
>
--
Nicolas Ferre
2017-03-15 17:56 GMT+01:00 Nicolas Ferre <[email protected]>:
> Le 15/03/2017 à 17:19, Richard Genoud a écrit :
>> On 15/03/2017 16:29, Nicolas Ferre wrote:
>>> A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA
>>> from transmitting in stop_tx") is that the console can be called with
>>> TX path disabled. Then the system would hang trying to push charecters
>>> out in atmel_console_putchar().
>>>
>>> Signed-off-by: Nicolas Ferre <[email protected]>
>>> Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting
>>> in stop_tx")
>>> Cc: stable <[email protected]> # 4.4+
>>> ---
>>> Hi Richard,
>>>
>>> I found this to fix the problem with system hang in my linux-4.4-at91 branch
>>> (in the atmel_console_putchar() waiting loop actually). I'm open to more
>>> insignt.
>>> As we cannot figure out if this bit is set or not, I didn't preserve the
>>> current status...
>>>
>>> Regards,
>>
>> So, I'm guessing that you may see some lines/characters printed twice on
>> the screen, don't you ?
>
> Well, actually, I don't think so because the repetitions that I see are
> probably due to open/close/open/close/re-open/... of the serial console
> itself.
>
> Same with the line "random: udevd: uninitialized urandom read (16 bytes
> read, 21 bits of entropy available)", they happen at different moment in
> time => the printk log timestamping seem to indicate that they are
> different.
Hi Nicolas,
It seems that the problem is between atmel_tx_dma() and its callback
atmel_complete_tx_dma().
At some point, atmel_tx_dma() is called, does the job, and then, just
before the callback is called, the xmit->head and xmit->tail pointers
are set to zero (by uart_flush_buffer())
So, when atmel_complete_tx_dma() is called, it does:
xmit->tail += atmel_port->tx_len;
not knowing that the head and tail pointers have been reseted.
=> it's like there's (UART_XMIT_SIZE - atmel_port->tx_len) characters to
transmit on the serial line.
PS: I can trigger this bug by holding down the d key at login and then
ctrl - basically, a ctrl-d just after sending text - with a rate success
of about 1/5 :)
Could you try this patch to see if it corrects also your system hang ?
(The patch is small, but the bug hunt was a headache :))
[PATCH] tty/serial: atmel: fix race condition (TX+DMA)
If uart_flush_buffer() is called between atmel_tx_dma() and
atmel_complete_tx_dma(), the circular buffer has been cleared, but not
atmel_port->tx_len.
That leads to a circular buffer overflow (dumping (UART_XMIT_SIZE -
atmel_port->tx_len) bytes).
Signed-off-by: Richard Genoud <[email protected]>
---
drivers/tty/serial/atmel_serial.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/tty/serial/atmel_serial.c
b/drivers/tty/serial/atmel_serial.c
index 833d3d80446f..89552157e334 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1934,6 +1934,11 @@ static void atmel_flush_buffer(struct uart_port
*port)
atmel_uart_writel(port, ATMEL_PDC_TCR, 0);
atmel_port->pdc_tx.ofs = 0;
}
+ /*
+ * in uart_flush_buffer(), the xmit circular buffer has just
+ * been cleared, so we have to reset tx_len accordingly.
+ */
+ atmel_port->tx_len = 0;
}
/*
Le 17/03/2017 à 16:11, Richard Genoud a écrit :
> 2017-03-15 17:56 GMT+01:00 Nicolas Ferre <[email protected]>:
>> Le 15/03/2017 à 17:19, Richard Genoud a écrit :
>>> On 15/03/2017 16:29, Nicolas Ferre wrote:
>>>> A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA
>>>> from transmitting in stop_tx") is that the console can be called with
>>>> TX path disabled. Then the system would hang trying to push charecters
>>>> out in atmel_console_putchar().
>>>>
>>>> Signed-off-by: Nicolas Ferre <[email protected]>
>>>> Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting
>>>> in stop_tx")
>>>> Cc: stable <[email protected]> # 4.4+
>>>> ---
>>>> Hi Richard,
>>>>
>>>> I found this to fix the problem with system hang in my linux-4.4-at91 branch
>>>> (in the atmel_console_putchar() waiting loop actually). I'm open to more
>>>> insignt.
>>>> As we cannot figure out if this bit is set or not, I didn't preserve the
>>>> current status...
>>>>
>>>> Regards,
>>>
>>> So, I'm guessing that you may see some lines/characters printed twice on
>>> the screen, don't you ?
>>
>> Well, actually, I don't think so because the repetitions that I see are
>> probably due to open/close/open/close/re-open/... of the serial console
>> itself.
>>
>> Same with the line "random: udevd: uninitialized urandom read (16 bytes
>> read, 21 bits of entropy available)", they happen at different moment in
>> time => the printk log timestamping seem to indicate that they are
>> different.
> Hi Nicolas,
>
> It seems that the problem is between atmel_tx_dma() and its callback
> atmel_complete_tx_dma().
>
> At some point, atmel_tx_dma() is called, does the job, and then, just
> before the callback is called, the xmit->head and xmit->tail pointers
> are set to zero (by uart_flush_buffer())
> So, when atmel_complete_tx_dma() is called, it does:
> xmit->tail += atmel_port->tx_len;
> not knowing that the head and tail pointers have been reseted.
> => it's like there's (UART_XMIT_SIZE - atmel_port->tx_len) characters to
> transmit on the serial line.
>
> PS: I can trigger this bug by holding down the d key at login and then
> ctrl - basically, a ctrl-d just after sending text - with a rate success
> of about 1/5 :)
>
> Could you try this patch to see if it corrects also your system hang ?
Just tried, it doesn't fix the system hang.
But it seems to solve the issue that I had while halting the system: a
kind of flush of a previous buffer in the console.
So, I think it does solve something.
So, with both my patch and yours:
Tested-by: Nicolas Ferre <[email protected]>
Regards,
> (The patch is small, but the bug hunt was a headache :))
>
> [PATCH] tty/serial: atmel: fix race condition (TX+DMA)
>
> If uart_flush_buffer() is called between atmel_tx_dma() and
> atmel_complete_tx_dma(), the circular buffer has been cleared, but not
> atmel_port->tx_len.
> That leads to a circular buffer overflow (dumping (UART_XMIT_SIZE -
> atmel_port->tx_len) bytes).
>
> Signed-off-by: Richard Genoud <[email protected]>
> ---
> drivers/tty/serial/atmel_serial.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/tty/serial/atmel_serial.c
> b/drivers/tty/serial/atmel_serial.c
> index 833d3d80446f..89552157e334 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -1934,6 +1934,11 @@ static void atmel_flush_buffer(struct uart_port
> *port)
> atmel_uart_writel(port, ATMEL_PDC_TCR, 0);
> atmel_port->pdc_tx.ofs = 0;
> }
> + /*
> + * in uart_flush_buffer(), the xmit circular buffer has just
> + * been cleared, so we have to reset tx_len accordingly.
> + */
> + atmel_port->tx_len = 0;
> }
>
> /*
>
--
Nicolas Ferre
On 15/03/2017 16:29, Nicolas Ferre wrote:
> A side effect of 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA
> from transmitting in stop_tx") is that the console can be called with
> TX path disabled. Then the system would hang trying to push charecters
> out in atmel_console_putchar().
>
> Signed-off-by: Nicolas Ferre <[email protected]>
> Fixes: 89d8232411a8 ("tty/serial: atmel_serial: BUG: stop DMA from transmitting
> in stop_tx")
> Cc: stable <[email protected]> # 4.4+
> ---
> Hi Richard,
>
> I found this to fix the problem with system hang in my linux-4.4-at91 branch
> (in the atmel_console_putchar() waiting loop actually). I'm open to more
> insignt.
> As we cannot figure out if this bit is set or not, I didn't preserve the
> current status...
>
> Regards,
>
> drivers/tty/serial/atmel_serial.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index dcebb28ffbc4..7372dbdb7a4c 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -2483,6 +2483,9 @@ static void atmel_console_write(struct console *co, const char *s, u_int count)
> pdc_tx = atmel_uart_readl(port, ATMEL_PDC_PTSR) & ATMEL_PDC_TXTEN;
> atmel_uart_writel(port, ATMEL_PDC_PTCR, ATMEL_PDC_TXTDIS);
>
> + /* Make sure that tx path is actually able to send characters */
> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN);
> +
> uart_console_write(port, s, count, atmel_console_putchar);
>
> /*
>
Acked-by: Richard Genoud <[email protected]>