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

Groups > linux.kernel > #1203237

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

Show all headers | View raw

On 08/08/2015 02:28 AM, Peter Hurley wrote:
>> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
>> index 0340ee6ba970..07a11e0935e4 100644
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
>>  		return;
>>  	}
>>  
>> -	dmaengine_pause(dma->rxchan);
>> +	ret = dmaengine_pause(dma->rxchan);
>> +	if (WARN_ON_ONCE(ret))
>> +		priv->rx_dma_broken = true;
> 
> No offense, Sebastian, but it boggles my mind that anyone could defend this
> as solid api design. We're in the middle of an interrupt handler and the
> slave dma driver is /just/ telling us now that it doesn't implement this
> functionality?!!?

This is the best thing I could come up with. But to be fair: This
happens once _very_ early in the process and is 100% reproducible. The
WARN_ON should trigger user attention.

> The dmaengine api has _so much_ setup and none of it contemplates telling the
> consumer that critical functionality is missing?

Lets say it is a missing feature which was not noticed / required until
now. I have the missing functionality in patch #3. I don't see the
point in adding a lookup API for this feature which has to be
backported stable just to figure out that RX-DMA might not work.
Again: the WARN_ON triggers on the first short RX read _or_ on shutdown
of the port which is not after hours using the port.

> Even dma_get_slave_caps() returns _true_ for cmd_pause support; ok, that
> interface is pointless.
> 
> Rather than losing /critical data/ here, the interrupt handler should just
> busy-wait until dmaengine_tx_status() returns DMA_COMPLETE for the rx_cookie.

This might not happen at all. At 115200 I *never* encouraged this. Once
the FIFO is filled with less than RX-trigger size than the UART sends
the time-out interrupt and the DMA *never* completes. Even if the FIFO
reaches trigger size later. So you have to abort the DMA transfer and
read data manually from the FIFO. That is why the omap enqueues the
RX-transfer upfront (while the FIFO is still empty).

It happens *sometimes* that the UART sends a timeout interrupt and the
DMA transfer just is started and it has been only observed at 3mbaud
(but there were no tests 115200 > x > 3mbaud that I know of).

Therefor I think this is a fair fix without hiding anything.

> Regards,
> Peter Hurley

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

omap-dma + 8250_omap: fix the dmaengine_pause() Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-08-07 22:10 +0200
  [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-08-07 22:10 +0200
    Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause  is not supported Peter Hurley <peter@hurleysoftware.com> - 2015-08-08 02:30 +0200
      Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause  is not supported Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-08-08 11:10 +0200
        Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause  is not supported Vinod Koul <vinod.koul@intel.com> - 2015-08-11 12:00 +0200
      Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause  is not supported Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-08-08 11:40 +0200
        Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause  is not supported Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-08-08 12:00 +0200
        Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause  is not supported Greg KH <greg@kroah.com> - 2015-08-08 17:50 +0200
    Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is  not supported Peter Ujfalusi <peter.ujfalusi@ti.com> - 2015-08-10 14:00 +0200
      Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause  is not supported Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-08-10 14:20 +0200
      Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause  is not supported Peter Hurley <peter@hurleysoftware.com> - 2015-08-10 15:10 +0200
        Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause  is not supported Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-08-10 19:20 +0200
  [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-08-07 22:10 +0200
    Re: [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic  transfers Peter Ujfalusi <peter.ujfalusi@ti.com> - 2015-08-11 14:10 +0200
      Re: [PATCH v3 3/3] dma: omap-dma: add support for pause of  non-cyclic transfers Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-08-11 14:40 +0200
        Re: [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic  transfers Peter Ujfalusi <peter.ujfalusi@ti.com> - 2015-08-11 14:50 +0200
  [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-08-07 22:10 +0200
    Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() Peter Hurley <peter@hurleysoftware.com> - 2015-08-08 02:50 +0200
    Re: [PATCH 2/3] dma: add __must_check annotation for  dmaengine_pause() Vinod Koul <vinod.koul@intel.com> - 2015-08-11 12:00 +0200
      Re: [PATCH 2/3] dma: add __must_check annotation for  dmaengine_pause() Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-08-11 12:10 +0200
        Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2015-08-11 14:40 +0200

csiph-web