Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1373726 > unrolled thread

[PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on exception

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on exception Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-04-07 22:40 +0200
    Re: [PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on  exception Peter Hurley <peter@hurleysoftware.com> - 2016-04-08 02:00 +0200
      Re: [PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on exception Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-04-08 10:10 +0200
        Re: [PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on  exception Peter Hurley <peter@hurleysoftware.com> - 2016-04-09 01:30 +0200

#1373726 — [PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on exception

If we get an exeption interrupt. i.e. UART_IIR_RLSI, stop any ongoing RX DMA
transfer otherwise it might generates more spurious interrupts and make port
unavailable anymore.

As has been seen on Intel Broxton system:
...
[  168.526281] serial8250: too much work for irq5
[  168.535908] serial8250: too much work for irq5
[  173.449464] serial8250_interrupt: 4439 callbacks suppressed
[  173.455694] serial8250: too much work for irq5
...

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dma.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 9d80bb1..b134bec 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -110,6 +110,16 @@ err:
 	return ret;
 }
 
+static void __dma_rx_stop(struct uart_8250_port *p, struct uart_8250_dma *dma)
+{
+	if (!dma->rx_running)
+		return;
+
+	dmaengine_pause(dma->rxchan);
+	__dma_rx_complete(p);
+	dmaengine_terminate_async(dma->rxchan);
+}
+
 int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 {
 	struct uart_8250_dma		*dma = p->dma;
@@ -118,17 +128,14 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 	switch (iir & 0x3f) {
 	case UART_IIR_RLSI:
 		/* 8250_core handles errors and break interrupts */
+		__dma_rx_stop(p, dma);
 		return -EIO;
 	case UART_IIR_RX_TIMEOUT:
 		/*
 		 * If RCVR FIFO trigger level was not reached, complete the
 		 * transfer and let 8250_core copy the remaining data.
 		 */
-		if (dma->rx_running) {
-			dmaengine_pause(dma->rxchan);
-			__dma_rx_complete(p);
-			dmaengine_terminate_async(dma->rxchan);
-		}
+		__dma_rx_stop(p, dma);
 		return -ETIMEDOUT;
 	default:
 		break;
-- 
2.8.0.rc3

[toc] | [next] | [standalone]

#1373837 — Re: [PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on exception

On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
> If we get an exeption interrupt. i.e. UART_IIR_RLSI, stop any ongoing RX DMA
> transfer otherwise it might generates more spurious interrupts and make port
> unavailable anymore.

Then how to know which rx byte the error is for if dma continues anyway?
What if there are multiple error bytes?


> As has been seen on Intel Broxton system:

This system shouldn't be setup for UART DMA imo.


> ...
> [  168.526281] serial8250: too much work for irq5
> [  168.535908] serial8250: too much work for irq5
> [  173.449464] serial8250_interrupt: 4439 callbacks suppressed
> [  173.455694] serial8250: too much work for irq5
> ...
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dma.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 9d80bb1..b134bec 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -110,6 +110,16 @@ err:
>  	return ret;
>  }
>  
> +static void __dma_rx_stop(struct uart_8250_port *p, struct uart_8250_dma *dma)
> +{
> +	if (!dma->rx_running)
> +		return;
> +
> +	dmaengine_pause(dma->rxchan);
> +	__dma_rx_complete(p);
> +	dmaengine_terminate_async(dma->rxchan);
> +}
> +
>  int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
>  {
>  	struct uart_8250_dma		*dma = p->dma;
> @@ -118,17 +128,14 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
>  	switch (iir & 0x3f) {
>  	case UART_IIR_RLSI:
>  		/* 8250_core handles errors and break interrupts */
> +		__dma_rx_stop(p, dma);
>  		return -EIO;
>  	case UART_IIR_RX_TIMEOUT:
>  		/*
>  		 * If RCVR FIFO trigger level was not reached, complete the
>  		 * transfer and let 8250_core copy the remaining data.
>  		 */
> -		if (dma->rx_running) {
> -			dmaengine_pause(dma->rxchan);
> -			__dma_rx_complete(p);
> -			dmaengine_terminate_async(dma->rxchan);
> -		}
> +		__dma_rx_stop(p, dma);
>  		return -ETIMEDOUT;
>  	default:
>  		break;
> 

[toc] | [prev] | [next] | [standalone]

#1374063

On Fri, Apr 8, 2016 at 2:54 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
>> If we get an exeption interrupt. i.e. UART_IIR_RLSI, stop any ongoing RX DMA
>> transfer otherwise it might generates more spurious interrupts and make port
>> unavailable anymore.
>
> Then how to know which rx byte the error is for if dma continues anyway?
> What if there are multiple error bytes?

And how should it work?
We get an interrupt during DMA, if we don't stop DMA it will be racy
with direct readings.

>
>
>> As has been seen on Intel Broxton system:
>
> This system shouldn't be setup for UART DMA imo.

Same approach is done in 8250_omap.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1374524 — Re: [PATCH v1 06/12] serial: 8250_dma: stop ongoing RX DMA on exception

On 04/08/2016 01:07 AM, Andy Shevchenko wrote:
> On Fri, Apr 8, 2016 at 2:54 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 04/07/2016 01:37 PM, Andy Shevchenko wrote:
>>> If we get an exeption interrupt. i.e. UART_IIR_RLSI, stop any ongoing RX DMA
>>> transfer otherwise it might generates more spurious interrupts and make port
>>> unavailable anymore.
>>
>> Then how to know which rx byte the error is for if dma continues anyway?
>> What if there are multiple error bytes?
> 
> And how should it work?
> We get an interrupt during DMA, if we don't stop DMA it will be racy
> with direct readings.

It makes sense to me that the ongoing DMA needs paused, flushed & terminated,
but the UART should have already aborted the DMA at the first error byte,
so it doesn't make sense to me that the DMA hardware went sideways.

Have you verified that the actual byte in error is reported as the frame/parity
byte and that error-free data is unmangled? Like with a data pattern and a logic
analyzer?


>>
>>
>>> As has been seen on Intel Broxton system:
>>
>> This system shouldn't be setup for UART DMA imo.
> 
> Same approach is done in 8250_omap.

Well, omap8250 has totally different (and possibly unnecessary) rx dma flow.

During the development of the omap8250 driver, it was discovered that the
normal 8250 rx dma flow didn't work reliably on OMAP; ie., the rx dma wouldn't
start once rx uart interrupt had already happened.

*So omap8250 sets up rx dma before any data has been received*
That's the dma that is cancelled when an RLSI interrupt is received;
on OMAP the residue is always 0.

Well, it turns out that the omap8250 rx dma flow *may* be limited to only
1 specific design, the am335x, which has a bunch of other dma issues, with
both tx and rx dma. So all that omap8250 dma handling might be going
away anyway.

IOW, omap8250 is a terrible dma model; do not use.
[Granted the current model needs some work as well; eg., using ping-pong
dma buffers to weather dmaengine descriptor completion latency).

Regards,
Peter Hurley

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web