On Thu, Mar 15, 2012 at 11:50:28AM +0100, Guennadi Liakhovetski wrote: > On Thu, 15 Mar 2012, Paul Mundt wrote: > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
