On Thu, Mar 15, 2012 at 11:50:28AM +0100, Guennadi Liakhovetski wrote:

On Thu, 15 Mar 2012, Paul Mundt wrote:

quoted

Looks good to me. I'll apply it unless Guennadi has any other concerns.

No, I don't - my ack holds:) However, I do have some concerns regarding a 
couple of other possible issues with SCI DMA:

Ok, I've queued it up now (it was too late for -final, so we'll have to
rely on the stable backport later).

1. As I mentioned earlier, I think, sci_start_tx() should always be called 
under the port spinlock to get consistent ->cookie_tx and ->chan_tx 
values. This is the case, when called from serial core as 
->start_tx() from most locations, but not from uart_handle_cts_change(), 
which is an exported function. It is also called internally in the SCI 
driver itself several times - with no locking. This might need fixing, 
especially in sci_tx_dma_release().

The bulk of the uart_handle_cts_change() callers do so with the lock
held, the only exception seems to be a few drivers that call it directly
from their interrupt handlers.

At first glance, the sci_tx/rx_dma_release() cases will probably need a
bit of reordering given that dma_release_channel() is taking a list
mutex, but I don't see too many issues otherwise. Having said that,
the whole DMA enable/disable path could probably be split up a bit with
individual toggle logic pushed down in to ->start/stop_tx as well as
->stop_rx for some finer-grained control. This would at least help make
the locking a bit more obvious.

2. The DMA Tx work might need to be cancelled from time to time... E.g., 
when DMA is disabled to switch to PIO, or when shutting down the port.

Yes, this is something that needs to be done. I've tried to use the
driver as a module before in the past, and it does blow up rather
spectacularly in the remove case, this is hardly limited to the DMA
case though (and is also not a configuration most people are going to
ever really use, which is why we've largely ignored it thus far). The
PIO<->DMA transition on the other hand we're far more likely to hit,
especially if we end up exposing something like a userspace knob for
enabling/disabling arbitrarily for testing.

Most of the DMA cancelling looks it should be pretty easy to do via
->flush_buffer, unless I'm missing something. The amba-pl011.c driver
does just this for the dmaengine case.