Re: [PATCH v7 05/10] dmaengine: dw-edma: Add channel delegation helpers

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Data race on `chan->irq_mode` between channel delegation/reclaim and the hardirq handler.
--

--- Patch [5]: [PATCH v7 05/10] dmaengine: dw-edma: Add channel delegation helpers ---
commit ada234f341417a6397309d554f4e6b955a5ce106
Author: Koichiro Den <[email protected]>

dmaengine: dw-edma: Add channel delegation helpers

This commit introduces helper functions to delegate endpoint-local
DesignWare eDMA channels to a remote host. It adds dw_edma_delegate_chan()
to switch an idle channel to remote interrupt routing, and
dw_edma_reclaim_chan() to restore the channel's default routing.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index d214df55da3c3..1582ecfc2965f 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -1303,6 +1303,45 @@ int dw_edma_remove(struct dw_edma_chip *chip)
>  }
>  EXPORT_SYMBOL_GPL(dw_edma_remove);
>  
> +int dw_edma_delegate_chan(struct dma_chan *dchan)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +	int ret = 0;
> +
> +	if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> +		return -EINVAL;
> +
> +	guard(spinlock_irqsave)(&chan->vc.lock);
> +
> +	if (chan->configured || chan->status != EDMA_ST_IDLE ||
> +	    chan->request != EDMA_REQ_NONE)
> +		ret = -EBUSY;
> +	else
> +		chan->irq_mode = DW_EDMA_CH_IRQ_REMOTE;

[Severity: Medium]
Does this introduce a data race on chan->irq_mode?

The irq_mode is updated here using a plain store while chan->vc.lock is held,
but it appears to be read locklessly during interrupt handling:

drivers/dma/dw-edma/dw-edma-core.h:dw_edma_core_ch_ignore_irq() {
    return chan->irq_mode == DW_EDMA_CH_IRQ_REMOTE;
}

Could the compiler optimize or tear these plain accesses, potentially causing
the interrupt handler to incorrectly ignore or process a spurious interrupt,
or trigger KCSAN warnings? Would using WRITE_ONCE() and READ_ONCE() be
appropriate here?

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dw_edma_delegate_chan);
> +
> +void dw_edma_reclaim_chan(struct dma_chan *dchan, bool quiesce)
> +{
> +	struct dw_edma_chan *chan;
> +
> +	if (!dchan)
> +		return;
> +
> +	chan = dchan2dw_edma_chan(dchan);
> +	if (quiesce && dw_edma_core_ch_quiesce(chan))
> +		dev_warn(chan->dw->chip->dev,
> +			 "failed to quiesce delegated %s channel %u\n",
> +			 chan->dir == EDMA_DIR_WRITE ? "write" : "read",
> +			 chan->id);
> +
> +	scoped_guard(spinlock_irqsave, &chan->vc.lock)
> +		chan->irq_mode = dw_edma_get_default_irq_mode(chan);

[Severity: Medium]
Similarly, does this plain store also race with the lockless read in
dw_edma_core_ch_ignore_irq()?

> +}
> +EXPORT_SYMBOL_GPL(dw_edma_reclaim_chan);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5