Commit c6babed8 authored by Tudor Ambarus's avatar Tudor Ambarus Committed by Vinod Koul
Browse files

dmaengine: at_hdmac: Fix concurrency problems by removing atc_complete_all() 



atc_complete_all() had concurrency bugs, thus remove it:
1/ atc_complete_all() in its entirety was buggy, as when the atchan->queue
list (the one that contains descriptors that are not yet issued to the
hardware) contained descriptors, it fired just the first from the
atchan->queue, but moved all the desc from atchan->queue to
atchan->active_list and considered them all as fired. This could result in
calling the completion of a descriptor that was not yet issued to the
hardware.
2/ when in tasklet at atc_advance_work() time, atchan->active_list was
queried without holding the lock of the chan. This can result in
atchan->active_list concurrency problems between the tasklet and
issue_pending().

Fixes: dc78baa2 ("dmaengine: at_hdmac: new driver for the Atmel AHB DMA Controller")
Reported-by: default avatarPeter Rosin <peda@axentia.se>
Signed-off-by: default avatarTudor Ambarus <tudor.ambarus@microchip.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/lkml/13c6c9a2-6db5-c3bf-349b-4c127ad3496a@axentia.se/


Acked-by: default avatarNicolas Ferre <nicolas.ferre@microchip.com>
Link: https://lore.kernel.org/r/20221025090306.297886-1-tudor.ambarus@microchip.com
Link: https://lore.kernel.org/r/20221025090306.297886-8-tudor.ambarus@microchip.com


Signed-off-by: default avatarVinod Koul <vkoul@kernel.org>
parent 6e5ad28d

drivers/dma/at_hdmac.c

+4 −45
Original line number Diff line number Diff line
@@ -485,42 +485,6 @@ atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc) 
	dma_run_dependencies(txd);

}



/**

 * atc_complete_all - finish work for all transactions

 * @atchan: channel to complete transactions for

 *

 * Eventually submit queued descriptors if any

 *

 * Assume channel is idle while calling this function

 * Called with atchan->lock held and bh disabled

 */

static void atc_complete_all(struct at_dma_chan *atchan)

{

	struct at_desc *desc, *_desc;

	LIST_HEAD(list);

	unsigned long flags;



	dev_vdbg(chan2dev(&atchan->chan_common), "complete all\n");



	spin_lock_irqsave(&atchan->lock, flags);



	/*

	 * Submit queued descriptors ASAP, i.e. before we go through

	 * the completed ones.

	 */

	if (!list_empty(&atchan->queue))

		atc_dostart(atchan, atc_first_queued(atchan));

	/* empty active_list now it is completed */

	list_splice_init(&atchan->active_list, &list);

	/* empty queue list by moving descriptors (if any) to active_list */

	list_splice_init(&atchan->queue, &atchan->active_list);



	spin_unlock_irqrestore(&atchan->lock, flags);



	list_for_each_entry_safe(desc, _desc, &list, desc_node)

		atc_chain_complete(atchan, desc);

}



/**

 * atc_advance_work - at the end of a transaction, move forward

 * @atchan: channel where the transaction ended
@@ -528,24 +492,19 @@ static void atc_complete_all(struct at_dma_chan *atchan) 
static void atc_advance_work(struct at_dma_chan *atchan)

{

	unsigned long flags;

	int ret;



	dev_vdbg(chan2dev(&atchan->chan_common), "advance_work\n");



	spin_lock_irqsave(&atchan->lock, flags);

	ret = atc_chan_is_enabled(atchan);

	if (atc_chan_is_enabled(atchan) || list_empty(&atchan->active_list))

		return spin_unlock_irqrestore(&atchan->lock, flags);

	spin_unlock_irqrestore(&atchan->lock, flags);

	if (ret)

		return;



	if (list_empty(&atchan->active_list) ||

	    list_is_singular(&atchan->active_list))

		return atc_complete_all(atchan);



	atc_chain_complete(atchan, atc_first_active(atchan));



	/* advance work */

	spin_lock_irqsave(&atchan->lock, flags);

	if (!list_empty(&atchan->active_list))

		atc_dostart(atchan, atc_first_active(atchan));

	spin_unlock_irqrestore(&atchan->lock, flags);

}