Re: [PATCH 3/4] dmaengine: dw-edma: Initialize IRQ data before requesting IRQs

[Koichiro Den]

On Thu, May 21, 2026 at 10:44:47AM -0400, Frank Li wrote:
> On Thu, May 21, 2026 at 11:21:52PM +0900, Koichiro Den wrote:
> > dw_edma_irq_request() passes struct dw_edma_irq to request_irq()
> > before dw_edma_channel_setup() fills the back pointer. A shared
> > interrupt can therefore enter the handler with dw_irq->dw still NULL,
> > leading to a NULL pointer dereference.
> >
> > Set the back pointer before installing each handler.
> >
> > Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> 
> Reviewed-by: Frank Li <Frank.Li@xxxxxxx>

Hi Frank,

Thanks for reviewing.

After Sashiko raised another point about patch 3, I looked into the init
ordering again. I now think patch 3 is half-baked fix, and probably not
needed in this small-fixes series.

The two Sashiko comments were about the IRQ handler being registered before
dw_edma_channel_setup():

  https://lore.kernel.org/dmaengine/20260521072453.E5AD21F00A3C@xxxxxxxxxxxxxxx/
  https://lore.kernel.org/dmaengine/20260521155834.D8DFF1F00A3C@xxxxxxxxxxxxxxx/

For current upstream, I don't think a shared IRQ alone can reach
dw_edma_done_interrupt() or dw_edma_abort_interrupt(). dw_edma_core_off() runs
before dw_edma_irq_request(), and the handler still needs DONE/ABORT status bits
before it dispatches to those callbacks.

So patch 3 only fixes part of a defensive ordering concern, and the later
Sashiko comment shows that moving irq->dw alone would be incomplete anyway.
If we want to harden this path, I think the cleaner change would be to split and
reorder the setup flow like this:

  Before:
    (1). dw_edma_irq_request()
      (1-a). allocate/populate dw->irq[] and cache MSI messages
      (1-b). request_irq()
    (2). dw_edma_channel_setup()
      (2-a). initialize channels, including vchan_init() and
             dw_edma_core_ch_config()
      (2-b). register the DMA device with dma_async_device_register()

  After:
    (1-a) -> (2-a) -> (1-b) -> (2-b)

But that is more of a cleanup/hardening change than a small pre-existing fix.
(If preferred, I can send a separate patch for that.)

So my conclusion is that only patches 2 and 4 are really needed in this series.
Patch 3 should be dropped. If a respin is needed for any resons, I will send v2
with dropping Patch 1 and 3.

Best regards,
Koichiro

> 
> >  drivers/dma/dw-edma/dw-edma-core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index c2feb3adc79f..d221e3efcb36 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -929,7 +929,6 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
> >  		else
> >  			irq->rd_mask |= BIT(chan->id);
> >
> > -		irq->dw = dw;
> >  		memcpy(&chan->msi, &irq->msi, sizeof(chan->msi));
> >
> >  		dev_vdbg(dev, "MSI:\t\tChannel %s[%u] addr=0x%.8x%.8x, data=0x%.8x\n",
> > @@ -1018,6 +1017,7 @@ static int dw_edma_irq_request(struct dw_edma *dw,
> >  	if (chip->nr_irqs == 1) {
> >  		/* Common IRQ shared among all channels */
> >  		irq = chip->ops->irq_vector(dev, 0);
> > +		dw->irq[0].dw = dw;
> >  		err = request_irq(irq, dw_edma_interrupt_common,
> >  				  IRQF_SHARED, dw->name, &dw->irq[0]);
> >  		if (err) {
> > @@ -1043,6 +1043,7 @@ static int dw_edma_irq_request(struct dw_edma *dw,
> >
> >  		for (i = 0; i < (*wr_alloc + *rd_alloc); i++) {
> >  			irq = chip->ops->irq_vector(dev, i);
> > +			dw->irq[i].dw = dw;
> >  			err = request_irq(irq,
> >  					  i < *wr_alloc ?
> >  						dw_edma_interrupt_write :
> > --
> > 2.51.0
> >