From: Tudor Ambarus <tudor.ambarus@microchip.com> To: <vkoul@kernel.org>, <peda@axentia.se>, <du@axentia.se>, <regressions@leemhuis.info> Cc: <ludovic.desroches@microchip.com>, <maciej.sosnowski@intel.com>, <tudor.ambarus@microchip.com>, <dan.j.williams@intel.com>, <nicolas.ferre@microchip.com>, <mripard@kernel.org>, <torfl6749@gmail.com>, <linux-kernel@vger.kernel.org>, <dmaengine@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <stable@vger.kernel.org> Subject: [PATCH 13/33] dmaengine: at_hdmac: Fix concurrency problems by removing atc_complete_all() Date: Sat, 20 Aug 2022 15:56:57 +0300 [thread overview] Message-ID: <20220820125717.588722-14-tudor.ambarus@microchip.com> (raw) In-Reply-To: <20220820125717.588722-1-tudor.ambarus@microchip.com> 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: dc78baa2b90b ("dmaengine: at_hdmac: new driver for the Atmel AHB DMA Controller") Reported-by: Peter Rosin <peda@axentia.se> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> Cc: stable@vger.kernel.org Link: https://lore.kernel.org/lkml/13c6c9a2-6db5-c3bf-349b-4c127ad3496a@axentia.se/ --- drivers/dma/at_hdmac.c | 49 ++++-------------------------------------- 1 file changed, 4 insertions(+), 45 deletions(-) diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c index aeb241832c52..10424a6fcbbf 100644 --- a/drivers/dma/at_hdmac.c +++ b/drivers/dma/at_hdmac.c @@ -920,42 +920,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->dma_chan), "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 @@ -963,25 +927,20 @@ 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->dma_chan), "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); - atc_dostart(atchan, atc_first_active(atchan)); + if (!list_empty(&atchan->active_list)) + atc_dostart(atchan, atc_first_active(atchan)); spin_unlock_irqrestore(&atchan->lock, flags); } -- 2.25.1
WARNING: multiple messages have this Message-ID (diff)
From: Tudor Ambarus <tudor.ambarus@microchip.com> To: <vkoul@kernel.org>, <peda@axentia.se>, <du@axentia.se>, <regressions@leemhuis.info> Cc: tudor.ambarus@microchip.com, linux-kernel@vger.kernel.org, maciej.sosnowski@intel.com, mripard@kernel.org, torfl6749@gmail.com, ludovic.desroches@microchip.com, stable@vger.kernel.org, dmaengine@vger.kernel.org, dan.j.williams@intel.com, linux-arm-kernel@lists.infradead.org Subject: [PATCH 13/33] dmaengine: at_hdmac: Fix concurrency problems by removing atc_complete_all() Date: Sat, 20 Aug 2022 15:56:57 +0300 [thread overview] Message-ID: <20220820125717.588722-14-tudor.ambarus@microchip.com> (raw) In-Reply-To: <20220820125717.588722-1-tudor.ambarus@microchip.com> 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: dc78baa2b90b ("dmaengine: at_hdmac: new driver for the Atmel AHB DMA Controller") Reported-by: Peter Rosin <peda@axentia.se> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> Cc: stable@vger.kernel.org Link: https://lore.kernel.org/lkml/13c6c9a2-6db5-c3bf-349b-4c127ad3496a@axentia.se/ --- drivers/dma/at_hdmac.c | 49 ++++-------------------------------------- 1 file changed, 4 insertions(+), 45 deletions(-) diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c index aeb241832c52..10424a6fcbbf 100644 --- a/drivers/dma/at_hdmac.c +++ b/drivers/dma/at_hdmac.c @@ -920,42 +920,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->dma_chan), "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 @@ -963,25 +927,20 @@ 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->dma_chan), "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); - atc_dostart(atchan, atc_first_active(atchan)); + if (!list_empty(&atchan->active_list)) + atc_dostart(atchan, atc_first_active(atchan)); spin_unlock_irqrestore(&atchan->lock, flags); } -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-08-20 12:58 UTC|newest] Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-20 12:56 [PATCH 00/33] dmaengine: at_hdmac: Fix concurrency bugs and then convert to virt-dma Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 01/33] dmaengine: at_hdmac: Keep register definitions and structures private to at_hdmac.c Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 02/33] dmaengine: at_hdmac: Use bitfield access macros Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 03/33] dmaengine: at_hdmac: Rename "dma_common" to "dma_device" Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 04/33] dmaengine: at_hdmac: Rename "chan_common" to "dma_chan" Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 05/33] dmaengine: at_hdmac: Remove unused member of at_dma_chan Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 06/33] dmaengine: at_hdmac: Return dma_cookie_status()'s ret code when txstate is NULL Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 07/33] dmaengine: at_hdmac: Fix at_lli struct definition Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-10-19 16:38 ` Vinod Koul 2022-10-19 16:38 ` Vinod Koul 2022-10-20 7:08 ` Tudor.Ambarus 2022-10-20 7:08 ` Tudor.Ambarus 2022-08-20 12:56 ` [PATCH 08/33] dmaengine: at_hdmac: Don't start transactions at tx_submit level Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 09/33] dmaengine: at_hdmac: Start transfer for cyclic channels in issue_pending Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 10/33] dmaengine: at_hdmac: Fix premature completion of desc " Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 11/33] dmaengine: at_hdmac: Do not call the complete callback on device_terminate_all Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 12/33] dmaengine: at_hdmac: Protect atchan->status with the channel lock Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus [this message] 2022-08-20 12:56 ` [PATCH 13/33] dmaengine: at_hdmac: Fix concurrency problems by removing atc_complete_all() Tudor Ambarus 2022-08-20 12:56 ` [PATCH 14/33] dmaengine: at_hdmac: Fix concurrency over descriptor Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:56 ` [PATCH 15/33] dmaengine: at_hdmac: Free the memset buf without holding the chan lock Tudor Ambarus 2022-08-20 12:56 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 16/33] dmaengine: at_hdmac: Fix concurrency over the active list Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 17/33] dmaengine: at_hdmac: Fix descriptor handling when issuing it to hardware Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 18/33] dmaengine: at_hdmac: Fix completion of unissued descriptor in case of errors Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 19/33] dmaengine: at_hdmac: Don't allow CPU to reorder channel enable Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 20/33] dmaengine: at_hdmac: Do not print messages on console while holding the lock Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 21/33] dmaengine: at_hdmac: Fix impossible condition Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 22/33] dmaengine: at_hdmac: Pass residue by address to avoid unneccessary implicit casts Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 23/33] dmaengine: at_hdmac: s/atc_get_bytes_left/atc_get_residue Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 24/33] dmaengine: at_hdmac: Introduce atc_get_llis_residue() Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 25/33] dmaengine: at_hdmac: Remove superfluous cast Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 26/33] dmaengine: at_hdmac: Use devm_kzalloc() and struct_size() Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 27/33] dmaengine: at_hdmac: Use devm_platform_ioremap_resource Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 28/33] dmaengine: at_hdmac: Use devm_request_irq() Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-10-19 16:49 ` Vinod Koul 2022-10-19 16:49 ` Vinod Koul 2022-10-20 7:24 ` Tudor.Ambarus 2022-10-20 7:24 ` Tudor.Ambarus 2022-08-20 12:57 ` [PATCH 29/33] dmaengine: at_hdmac: Use devm_clk_get() Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 30/33] dmaengine: at_hdmac: Check return code of dma_async_device_register Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 31/33] dmaengine: at_hdmac: Use pm_ptr() Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-10-19 16:50 ` Vinod Koul 2022-10-19 16:50 ` Vinod Koul 2022-10-20 7:10 ` Tudor.Ambarus 2022-10-20 7:10 ` Tudor.Ambarus 2022-08-20 12:57 ` [PATCH 32/33] dmaengine: at_hdmac: Set include entries in alphabetic order Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus 2022-08-20 12:57 ` [PATCH 33/33] dmaengine: at_hdmac: Convert driver to use virt-dma Tudor Ambarus 2022-08-20 12:57 ` Tudor Ambarus
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220820125717.588722-14-tudor.ambarus@microchip.com \ --to=tudor.ambarus@microchip.com \ --cc=dan.j.williams@intel.com \ --cc=dmaengine@vger.kernel.org \ --cc=du@axentia.se \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=ludovic.desroches@microchip.com \ --cc=maciej.sosnowski@intel.com \ --cc=mripard@kernel.org \ --cc=nicolas.ferre@microchip.com \ --cc=peda@axentia.se \ --cc=regressions@leemhuis.info \ --cc=stable@vger.kernel.org \ --cc=torfl6749@gmail.com \ --cc=vkoul@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.