Hello,
In dw_dmac.c driver, dwc_descriptor_complete() routine, following is
mentioned before calling callback:
/*
* The API requires that no submissions are done from a
* callback, so we don't need to drop the lock here
*/
if (callback)
callback(param);
Does this hold true for dmaengine??
--
viresh
On Mon, Apr 11, 2011 at 01:25:04PM +0530, viresh kumar wrote:
>
> Hello,
>
> In dw_dmac.c driver, dwc_descriptor_complete() routine, following is
> mentioned before calling callback:
>
> /*
> * The API requires that no submissions are done from a
> * callback, so we don't need to drop the lock here
> */
> if (callback)
> callback(param);
>
> Does this hold true for dmaengine??
Not for slave devices - see Dan's reply:
http://lists.arm.linux.org.uk/lurker/message/20101223.005313.a38d7bf0.en.html
As the slave API hasn't been well documented, there's a lot of
inconsistency of behaviour between DMA engine slave implementations.
I'd suggest at least fixing slave DMA engine drivers to ensure that:
(a) the callback is always called in tasklet context
(b) the callback can submit new slave transactions (iow, the spinlock
which prep_slave_sg takes must not be held during the callback.)
The way that others solve this is to move the completed txd structures
to a local 'completed' list, and then walk this list after the spinlocks
have been dropped.
IOW, something like this:
my_tasklet()
{
INIT_LIST_HEAD(completed);
spin_lock_irqsave(my_chan->lock);
for_each_txd(my_txd, my_chan) {
if (has_completed(my_txd))
list_add_tail(my_txd->node, &completed);
}
spin_unlock_irqrestore(my_chan->lock);
list_for_each_entry_safe(my_txd, next, &completed, node) {
void *callback_param = my_txd->txd.callback_param;
void (*fn)(void *) = my_txd->txd.callback;
my_txd_free(my_chan, my_txd);
fn(callback_param);
}
}
On 04/11/2011 02:26 PM, Russell King - ARM Linux wrote:
> On Mon, Apr 11, 2011 at 01:25:04PM +0530, viresh kumar wrote:
>>
>> Hello,
>>
>> In dw_dmac.c driver, dwc_descriptor_complete() routine, following is
>> mentioned before calling callback:
>>
>> /*
>> * The API requires that no submissions are done from a
>> * callback, so we don't need to drop the lock here
>> */
>> if (callback)
>> callback(param);
>>
>> Does this hold true for dmaengine??
>
> Not for slave devices - see Dan's reply:
>
> http://lists.arm.linux.org.uk/lurker/message/20101223.005313.a38d7bf0.en.html
>
> As the slave API hasn't been well documented, there's a lot of
> inconsistency of behaviour between DMA engine slave implementations.
> I'd suggest at least fixing slave DMA engine drivers to ensure that:
>
> (a) the callback is always called in tasklet context
> (b) the callback can submit new slave transactions (iow, the spinlock
> which prep_slave_sg takes must not be held during the callback.)
>
> The way that others solve this is to move the completed txd structures
> to a local 'completed' list, and then walk this list after the spinlocks
> have been dropped.
>
> IOW, something like this:
>
> my_tasklet()
> {
> INIT_LIST_HEAD(completed);
>
> spin_lock_irqsave(my_chan->lock);
> for_each_txd(my_txd, my_chan) {
> if (has_completed(my_txd))
> list_add_tail(my_txd->node, &completed);
> }
> spin_unlock_irqrestore(my_chan->lock);
>
> list_for_each_entry_safe(my_txd, next, &completed, node) {
> void *callback_param = my_txd->txd.callback_param;
> void (*fn)(void *) = my_txd->txd.callback;
>
> my_txd_free(my_chan, my_txd);
>
> fn(callback_param);
> }
> }
Got it. Thanx.
--
viresh
On 04/11/2011 02:26 PM, Russell King - ARM Linux wrote:
> On Mon, Apr 11, 2011 at 01:25:04PM +0530, viresh kumar wrote:
>>
>> Hello,
>>
>> In dw_dmac.c driver, dwc_descriptor_complete() routine, following is
>> mentioned before calling callback:
>>
>> /*
>> * The API requires that no submissions are done from a
>> * callback, so we don't need to drop the lock here
>> */
>> if (callback)
>> callback(param);
>>
>> Does this hold true for dmaengine??
>
> Not for slave devices - see Dan's reply:
>
> http://lists.arm.linux.org.uk/lurker/message/20101223.005313.a38d7bf0.en.html
>
> As the slave API hasn't been well documented, there's a lot of
> inconsistency of behaviour between DMA engine slave implementations.
> I'd suggest at least fixing slave DMA engine drivers to ensure that:
>
> (a) the callback is always called in tasklet context
> (b) the callback can submit new slave transactions (iow, the spinlock
> which prep_slave_sg takes must not be held during the callback.)
>
> The way that others solve this is to move the completed txd structures
> to a local 'completed' list, and then walk this list after the spinlocks
> have been dropped.
>
Hello,
There is one more issue in the current DW_DMAC driver.
As most of interrupt processing is done in tasklet, spin_lock_bh() is used in almost
every routine.
Now, if some driver is calling these routines from interrupt context or with interrupt
disabled, we get KERN_WARN() messages due to following in kernel/softirq.c:
static inline void _local_bh_enable_ip(unsigned long ip)
{
WARN_ON_ONCE(in_irq() || irqs_disabled());
...
}
Should i minimize processing in tasklets, so that spin_lock_bh is not required anymore,
as in drivers/dma/amba-pl08x.c (tasklet for every channel) or is there some other way of
doing it.
Currently, drivers/tty/serial/amba-pl011.c is calling from interrupt context or with
interrupt disabled.
--
viresh
On Fri, Apr 15, 2011 at 12:15:43PM +0530, viresh kumar wrote:
> There is one more issue in the current DW_DMAC driver.
> As most of interrupt processing is done in tasklet, spin_lock_bh() is used
> in almost every routine.
As you can't use spin_lock_bh() from IRQ context (you'll get a lockdep
warning) these need to be converted to being the irqsave versions.
That's probably the easiest fix.