| Message ID | 1460061433-63750-7-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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; > -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)