[PATCH] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[PATCH] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Ginger Li]

tegra_dma_tx_status() reads tdc->status before taking tdc->vc.lock, and
tegra_dma_isr() snapshots tdc->dma_desc before taking the same lock.  All
writers of these fields (tegra_dma_start(), tegra_dma_xfer_complete(),
tegra_dma_pause(), tegra_dma_resume() and tegra_dma_terminate_all()) update
them under tdc->vc.lock.

The unlocked read in tegra_dma_tx_status() can therefore report DMA_PAUSED
for a transfer that has already completed, and the unlocked read in
tegra_dma_isr() can pick up a tdc->dma_desc which is being cleared
concurrently, in which case the handler keeps using a stale descriptor
pointer.

Read both fields while holding tdc->vc.lock.

Fixes: ee17028 ("dmaengine: tegra: Add tegra gpcdma driver")
Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
---
 drivers/dma/tegra186-gpc-dma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c
--- a/drivers/dma/tegra186-gpc-dma.c
+++ b/drivers/dma/tegra186-gpc-dma.c
@@ -606,7 +606,7 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id
 static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
 {
 	struct tegra_dma_channel *tdc = dev_id;
-	struct tegra_dma_desc *dma_desc = tdc->dma_desc;
+	struct tegra_dma_desc *dma_desc;
 	struct tegra_dma_sg_req *sg_req;
 	u32 status;
 
@@ -619,6 +619,7 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id
 	}
 
 	spin_lock(&tdc->vc.lock);
+	dma_desc = tdc->dma_desc;
 	status = tdc_read(tdc, tdc->regs->status);
 	if (!(status & TEGRA_GPCDMA_STATUS_ISE_EOC))
 		goto irq_done;
@@ -786,10 +787,10 @@ static enum dma_status tegra_dma_tx_status(struct dma_
 	if (ret == DMA_COMPLETE)
 		return ret;
 
+	spin_lock_irqsave(&tdc->vc.lock, flags);
 	if (tdc->status == DMA_PAUSED)
 		ret = DMA_PAUSED;
 
-	spin_lock_irqsave(&tdc->vc.lock, flags);
 	vd = vchan_find_desc(&tdc->vc, cookie);
 	if (vd) {
 		dma_desc = vd_to_tegra_dma_desc(vd);
-- 
2.43.0

Re: [PATCH] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Markus Elfring]

> Fixes: ee17028 ("dmaengine: tegra: Add tegra gpcdma driver")

Why did you specify a short hash for such a tag?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.3-rc4#n157

Regards,
Markus

[PATCH v2] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Ginger Li]

tegra_dma_tx_status() reads tdc->status before taking tdc->vc.lock, and
tegra_dma_isr() snapshots tdc->dma_desc before taking the same lock.  All
writers of these fields (tegra_dma_start(), tegra_dma_xfer_complete(),
tegra_dma_pause(), tegra_dma_resume() and tegra_dma_terminate_all()) update
them under tdc->vc.lock.

The unlocked read in tegra_dma_tx_status() can therefore report DMA_PAUSED
for a transfer that has already completed, and the unlocked read in
tegra_dma_isr() can pick up a tdc->dma_desc which is being cleared
concurrently, in which case the handler keeps using a stale descriptor
pointer.

Read both fields while holding tdc->vc.lock.

Fixes: ee17028009d4 ("dmaengine: tegra: Add tegra gpcdma driver")
Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
---
v2:
 - Fixes: expanded to 12 hex digits.
---
 drivers/dma/tegra186-gpc-dma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c
--- a/drivers/dma/tegra186-gpc-dma.c
+++ b/drivers/dma/tegra186-gpc-dma.c
@@ -606,7 +606,7 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id
 static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
 {
 	struct tegra_dma_channel *tdc = dev_id;
-	struct tegra_dma_desc *dma_desc = tdc->dma_desc;
+	struct tegra_dma_desc *dma_desc;
 	struct tegra_dma_sg_req *sg_req;
 	u32 status;
 
@@ -619,6 +619,7 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id
 	}
 
 	spin_lock(&tdc->vc.lock);
+	dma_desc = tdc->dma_desc;
 	status = tdc_read(tdc, tdc->regs->status);
 	if (!(status & TEGRA_GPCDMA_STATUS_ISE_EOC))
 		goto irq_done;
@@ -786,10 +787,10 @@ static enum dma_status tegra_dma_tx_status(struct dma_
 	if (ret == DMA_COMPLETE)
 		return ret;
 
+	spin_lock_irqsave(&tdc->vc.lock, flags);
 	if (tdc->status == DMA_PAUSED)
 		ret = DMA_PAUSED;
 
-	spin_lock_irqsave(&tdc->vc.lock, flags);
 	vd = vchan_find_desc(&tdc->vc, cookie);
 	if (vd) {
 		dma_desc = vd_to_tegra_dma_desc(vd);
-- 
2.43.0

Re: [PATCH v2] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Markus Elfring]

…
> Read both fields while holding tdc->vc.lock.
…
> ---
>  drivers/dma/tegra186-gpc-dma.c | 5 +++--…

Was such a data processing concern detected with the help of any
source code analysis tools?

Regards,
Markus

Re: [PATCH v2] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Ginger]

Yes, it was detected by a static analyzer that I built. I will update
the patch to reflect that.

Sincerely,
Ginger

On Wed, Sep 30, 2026 at 9:05 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> …
> > Read both fields while holding tdc->vc.lock.
> …
> > ---
> >  drivers/dma/tegra186-gpc-dma.c | 5 +++--…
>
> Was such a data processing concern detected with the help of any
> source code analysis tools?
>
> Regards,
> Markus

Re: [v2] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Markus Elfring]

> Yes, it was detected by a static analyzer that I built.

Would you like to share further background information for such a development tool?


>                                                         I will update
> the patch to reflect that.

Were any more of your contributions supported by advanced source code analysis tools?

Regards,
Markus

[PATCH v3] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Ginger Li]

My static analyzer detected a potential issue in the dmag engine.
tegra_dma_tx_status() reads tdc->status before taking tdc->vc.lock, and
tegra_dma_isr() snapshots tdc->dma_desc before taking the same lock.  All
writers of these fields (tegra_dma_start(), tegra_dma_xfer_complete(),
tegra_dma_pause(), tegra_dma_resume() and tegra_dma_terminate_all()) update
them under tdc->vc.lock.

The unlocked read in tegra_dma_tx_status() can therefore report DMA_PAUSED
for a transfer that has already completed, and the unlocked read in
tegra_dma_isr() can pick up a tdc->dma_desc which is being cleared
concurrently, in which case the handler keeps using a stale descriptor
pointer.

Read both fields while holding tdc->vc.lock.

Fixes: ee17028009d4 ("dmaengine: tegra: Add tegra gpcdma driver")
Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
---
v3:
 - Acknowledge the usage of static analysis in detecting this problem.
---
 drivers/dma/tegra186-gpc-dma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c
--- a/drivers/dma/tegra186-gpc-dma.c
+++ b/drivers/dma/tegra186-gpc-dma.c
@@ -606,7 +606,7 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id
 static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
 {
 	struct tegra_dma_channel *tdc = dev_id;
-	struct tegra_dma_desc *dma_desc = tdc->dma_desc;
+	struct tegra_dma_desc *dma_desc;
 	struct tegra_dma_sg_req *sg_req;
 	u32 status;
 
@@ -619,6 +619,7 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id
 	}
 
 	spin_lock(&tdc->vc.lock);
+	dma_desc = tdc->dma_desc;
 	status = tdc_read(tdc, tdc->regs->status);
 	if (!(status & TEGRA_GPCDMA_STATUS_ISE_EOC))
 		goto irq_done;
@@ -786,10 +787,10 @@ static enum dma_status tegra_dma_tx_status(struct dma_
 	if (ret == DMA_COMPLETE)
 		return ret;
 
+	spin_lock_irqsave(&tdc->vc.lock, flags);
 	if (tdc->status == DMA_PAUSED)
 		ret = DMA_PAUSED;
 
-	spin_lock_irqsave(&tdc->vc.lock, flags);
 	vd = vchan_find_desc(&tdc->vc, cookie);
 	if (vd) {
 		dma_desc = vd_to_tegra_dma_desc(vd);
-- 
2.43.0

Re: [PATCH v3] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Markus Elfring]

> My static analyzer detected a potential issue in the dmag engine.

                                                       DMA?


…
> ---
> v3:
>  - Acknowledge the usage of static analysis in detecting this problem.
> ---
>  drivers/dma/tegra186-gpc-dma.c | 5 +++--…

Under which circumstances will additional background information become available?

Regards,
Markus

Re: [PATCH v3] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Ginger]

On Wed, Sep 30, 2026 at 9:25 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > My static analyzer detected a potential issue in the dmag engine.
>
>                                                        DMA?
>
>
> …
> > ---
> > v3:
> >  - Acknowledge the usage of static analysis in detecting this problem.
> > ---
> >  drivers/dma/tegra186-gpc-dma.c | 5 +++--…
>
> Under which circumstances will additional background information become available?
>

The static analyzer is a research-based one, built upon the LLVM IR.
The analysis is founded
on a symbolic model (not an ML model) for concurrency bugs. And due to
the anonymity
requirements, I am afraid I cannot reveal further information.

>> Were any more of your contributions supported by advanced source code analysis tools?

If you meant AI tools, then at this stage, no. For this patch, I have
used AI tools to help phrase
the patch better, but I did not use them for code analysis.

Sincerely,
Ginger

Re: [v3] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Markus Elfring]

>> Under which circumstances will additional background information become available?
> 
> The static analyzer is a research-based one, built upon the LLVM IR.
> The analysis is founded
> on a symbolic model (not an ML model) for concurrency bugs.

Thanks for such a hint.


>                                                             And due to
> the anonymity
> requirements, I am afraid I cannot reveal further information.

How does this feedback fit to other known guidance?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/researcher-guidelines.rst?h=v7.3-rc5#n5


>>> Were any more of your contributions supported by advanced source code analysis tools?
> 
> If you meant AI tools, then at this stage, no. For this patch, I have
> used AI tools to help phrase
> the patch better, but I did not use them for code analysis.
Will this information trigger further collateral evolution?

Regards,
Markus

Re: [v3] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Ginger]

On Wed, Sep 30, 2026 at 9:46 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >> Under which circumstances will additional background information become available?
> >
> > The static analyzer is a research-based one, built upon the LLVM IR.
> > The analysis is founded
> > on a symbolic model (not an ML model) for concurrency bugs.
>
> Thanks for such a hint.
>
>
> >                                                             And due to
> > the anonymity
> > requirements, I am afraid I cannot reveal further information.
>
> How does this feedback fit to other known guidance?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/researcher-guidelines.rst?h=v7.3-rc5#n5
>

I can update more information about the static analyzer (i.e., it is
experimental and its reported log) in the patch.
I believe this aligns with the known guidance, but I cannot provide
the source code links.

>
> >>> Were any more of your contributions supported by advanced source code analysis tools?
> >
> > If you meant AI tools, then at this stage, no. For this patch, I have
> > used AI tools to help phrase
> > the patch better, but I did not use them for code analysis.
> Will this information trigger further collateral evolution?
>

The collateral evolution will only affect the experimental analyzer.
It does not affect the submitted patch.

Sincerely,
Ginger

Re: [v3] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Markus Elfring]

>> How does this feedback fit to other known guidance?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/researcher-guidelines.rst?h=v7.3-rc5#n5
> 
> I can update more information about the static analyzer (i.e., it is
> experimental and its reported log) in the patch.
> I believe this aligns with the known guidance,

Partly.

How many contributors would be involved in the mentioned software research so far?


>                                                but I cannot provide
> the source code links.

Do you try to follow scientific development methods?

Will publishing requirements grow accordingly?


>>>>> Were any more of your contributions supported by advanced source code analysis tools?
>>>
>>> If you meant AI tools, then at this stage, no. For this patch, I have
>>> used AI tools to help phrase
>>> the patch better, but I did not use them for code analysis.
>> Will this information trigger further collateral evolution?
> 
> The collateral evolution will only affect the experimental analyzer.
> It does not affect the submitted patch.
Will opportunities be taken better into account for improved change descriptions?

Regards,
Markus

Re: [v3] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Ginger]

On Wed, Sep 30, 2026 at 2:21 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >> How does this feedback fit to other known guidance?
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/researcher-guidelines.rst?h=v7.3-rc5#n5
> >
> > I can update more information about the static analyzer (i.e., it is
> > experimental and its reported log) in the patch.
> > I believe this aligns with the known guidance,
>
> Partly.
>
> How many contributors would be involved in the mentioned software research so far?
>

Currently, I am the only one contributing to the source code of the
analyzer. My co-workers
provide research-oriented discussions but not bug reviews/implementation.
Regarding this, how would it be reflected in the patch?

>
> >                                                but I cannot provide
> > the source code links.
>
> Do you try to follow scientific development methods?
>
> Will publishing requirements grow accordingly?
>

Yes. The source code will be made public once the research work gets published.

>
> >>>>> Were any more of your contributions supported by advanced source code analysis tools?
> >>>
> >>> If you meant AI tools, then at this stage, no. For this patch, I have
> >>> used AI tools to help phrase
> >>> the patch better, but I did not use them for code analysis.
> >> Will this information trigger further collateral evolution?
> >
> > The collateral evolution will only affect the experimental analyzer.
> > It does not affect the submitted patch.
> Will opportunities be taken better into account for improved change descriptions?
>

May I kindly ask what opportunities you are referring to? If you meant
that whether
future reports generated potentially with the help of AI tools will
credit AI tools, then
yes.

Sincerely,
Ginger

Re: [v3] dmaengine: tegra186-gpc-dma: Read channel state under the lock

[Markus Elfring]

>> How many contributors would be involved in the mentioned software research so far?
> 
> Currently, I am the only one contributing to the source code of the
> analyzer. My co-workers
> provide research-oriented discussions but not bug reviews/implementation.

Thus it seems that there is a corresponding research group somewhere.


> Regarding this, how would it be reflected in the patch?

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.3-rc5#n545


>> Do you try to follow scientific development methods?
>>
>> Will publishing requirements grow accordingly?
>>
> Yes. The source code will be made public once the research work gets published.

Would there any research grants be involved as another related information source?


>>>> Will this information trigger further collateral evolution?
>>>
>>> The collateral evolution will only affect the experimental analyzer.
>>> It does not affect the submitted patch.
>> Will opportunities be taken better into account for improved change descriptions?
>>
> 
> May I kindly ask what opportunities you are referring to?

I indicated also a possibility to avoid another typo (for example).


>                                                           If you meant
> that whether
> future reports generated potentially with the help of AI tools will
> credit AI tools, then
> yes.
* Adjustments for word wrapping

* Additional tags

* How will locking analyses evolve further?


Regards,
Markus