Fixed a flag reuse bug in the mtk_cqdma_tx_status() function.
Fixes: 157ae5ffd76a ("dmaengine: mediatek: Fix a possible deadlock error in mtk_cqdma_tx_status()") Cc: stable@vger.kernel.org Reported-by: kernel test robot lkp@intel.com Closes: https://lore.kernel.org/oe-kbuild-all/202505270641.MStzJUfU-lkp@intel.com/ Signed-off-by: Qiu-ji Chen chenqiuji666@gmail.com --- drivers/dma/mediatek/mtk-cqdma.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/dma/mediatek/mtk-cqdma.c b/drivers/dma/mediatek/mtk-cqdma.c index 47c8adfdc155..f7b870d2ca90 100644 --- a/drivers/dma/mediatek/mtk-cqdma.c +++ b/drivers/dma/mediatek/mtk-cqdma.c @@ -441,18 +441,19 @@ static enum dma_status mtk_cqdma_tx_status(struct dma_chan *c, struct mtk_cqdma_vdesc *cvd; struct virt_dma_desc *vd; enum dma_status ret; - unsigned long flags; + unsigned long pc_flags; + unsigned long vc_flags; size_t bytes = 0;
ret = dma_cookie_status(c, cookie, txstate); if (ret == DMA_COMPLETE || !txstate) return ret;
- spin_lock_irqsave(&cvc->pc->lock, flags); - spin_lock_irqsave(&cvc->vc.lock, flags); + spin_lock_irqsave(&cvc->pc->lock, pc_flags); + spin_lock_irqsave(&cvc->vc.lock, vc_flags); vd = mtk_cqdma_find_active_desc(c, cookie); - spin_unlock_irqrestore(&cvc->vc.lock, flags); - spin_unlock_irqrestore(&cvc->pc->lock, flags); + spin_unlock_irqrestore(&cvc->vc.lock, vc_flags); + spin_unlock_irqrestore(&cvc->pc->lock, pc_flags);
if (vd) { cvd = to_cqdma_vdesc(vd);
On 6/6/25 10:17, Qiu-ji Chen wrote:
Fixed a flag reuse bug in the mtk_cqdma_tx_status() function.
Fixes: 157ae5ffd76a ("dmaengine: mediatek: Fix a possible deadlock error in mtk_cqdma_tx_status()") Cc: stable@vger.kernel.org Reported-by: kernel test robot lkp@intel.com Closes: https://lore.kernel.org/oe-kbuild-all/202505270641.MStzJUfU-lkp@intel.com/ Signed-off-by: Qiu-ji Chen chenqiuji666@gmail.com
drivers/dma/mediatek/mtk-cqdma.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/dma/mediatek/mtk-cqdma.c b/drivers/dma/mediatek/mtk-cqdma.c index 47c8adfdc155..f7b870d2ca90 100644 --- a/drivers/dma/mediatek/mtk-cqdma.c +++ b/drivers/dma/mediatek/mtk-cqdma.c @@ -441,18 +441,19 @@ static enum dma_status mtk_cqdma_tx_status(struct dma_chan *c, struct mtk_cqdma_vdesc *cvd; struct virt_dma_desc *vd; enum dma_status ret;
- unsigned long flags;
- unsigned long pc_flags;
- unsigned long vc_flags; size_t bytes = 0;
ret = dma_cookie_status(c, cookie, txstate); if (ret == DMA_COMPLETE || !txstate) return ret;
- spin_lock_irqsave(&cvc->pc->lock, flags);
- spin_lock_irqsave(&cvc->vc.lock, flags);
- spin_lock_irqsave(&cvc->pc->lock, pc_flags);
- spin_lock_irqsave(&cvc->vc.lock, vc_flags); vd = mtk_cqdma_find_active_desc(c, cookie);
- spin_unlock_irqrestore(&cvc->vc.lock, flags);
- spin_unlock_irqrestore(&cvc->pc->lock, flags);
- spin_unlock_irqrestore(&cvc->vc.lock, vc_flags);
- spin_unlock_irqrestore(&cvc->pc->lock, pc_flags);
if (vd) { cvd = to_cqdma_vdesc(vd);
If the first spin_lock_irqsave already saved the irq flags and disabled them, would it be meaningful to actually use a simple spin_lock for the second lock ? Or rather spin_lock_nested since there is a second nested lock taken ?
Eugen
On 6/6/25 10:17, Qiu-ji Chen wrote:
Fixed a flag reuse bug in the mtk_cqdma_tx_status() function.
If the first spin_lock_irqsave already saved the irq flags and disabled them, would it be meaningful to actually use a simple spin_lock for the second lock ? Or rather spin_lock_nested since there is a second nested lock taken ?
Eugen
Hello Eugen,
Thanks for helpful suggestion. The modification has been submitted in patch v2 as discussed.
Best regards, Qiu-ji Chen
On 6/6/25 12:14, Qiu-ji Chen wrote:
On 6/6/25 10:17, Qiu-ji Chen wrote:
Fixed a flag reuse bug in the mtk_cqdma_tx_status() function.
If the first spin_lock_irqsave already saved the irq flags and disabled them, would it be meaningful to actually use a simple spin_lock for the second lock ? Or rather spin_lock_nested since there is a second nested lock taken ?
Eugen
Hello Eugen,
Thanks for helpful suggestion. The modification has been submitted in patch v2 as discussed.
Best regards, Qiu-ji Chen
You are welcome, but in fact I suggested two alternatives. Any reason you picked this one instead of the other ?
Hello Eugen,
Thank you for discussing this with me!
In this specific code scenario, the lock acquisition order is strictly fixed (e.g., pc->lock is always acquired before vc->lock). This sequence is linear and won't interleave with other code paths in a conflicting nested pattern (e.g., the pc → vc sequence never coexists with a potential vc → pc sequence). Therefore, a standard spin_lock() is sufficient to safely prevent deadlocks, and explicitly declaring a nesting level via spin_lock_nested() is unnecessary.
Additionally, using spin_lock_nested() would require specifying an extra nesting subclass parameter. This adds unnecessary complexity to the code and could adversely affect maintainability for other developers working on it in the future.
Best regards, Qiu-ji Chen
On 6/6/25 12:14, Qiu-ji Chen wrote:
On 6/6/25 10:17, Qiu-ji Chen wrote:
Fixed a flag reuse bug in the mtk_cqdma_tx_status() function.
If the first spin_lock_irqsave already saved the irq flags and disabled them, would it be meaningful to actually use a simple spin_lock for the second lock ? Or rather spin_lock_nested since there is a second nested lock taken ?
Eugen
Hello Eugen,
Thanks for helpful suggestion. The modification has been submitted in patch v2 as discussed.
Best regards, Qiu-ji Chen
You are welcome, but in fact I suggested two alternatives. Any reason you picked this one instead of the other ?
linux-stable-mirror@lists.linaro.org
-
Eugen Hristev -
Qiu-ji Chen