Re: [Bug Report] SOUND: Kernel panic occurs when playback

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Morimoto-san,

On Friday 15 May 2015 08:32:39 Kuninori Morimoto wrote:
> Hi Laurent
> 
> Can I ask you about DMAEngine and spin_lock ?
> 
> > > > We are testing Linux upstream version-4.1-rc2 on Koelsch and Lager.
> > > > We found a bug(kernel panic) on SOUND driver when Playback a sound
> > > > file.
> > > > 
> > > > The kernel panic logs is showed as below:
> > > > "root@linaro-nano:~# aplay audio/Track\ 10.wav
> > > > Playing WAVE 'audio/Track 10.wav' : Signed 16 bit Little Endian,
> > > > Rate 44100 Hz, Stereo
> > > > Unable to handle kernel NULL pointer dereference at virtual address
> > > > 00000048
> > > > pgd = edfac000
> > > > [00000048] *pgdn0f0831, *pte\0000000, *ppte\0000000
> > > > Internal error: Oops: 17 [#1] SMP ARM
> > > > CPU: 0 PID: 2009 Comm: aplay Not tainted 4.1.0-rc2-dirty #4
> > > > Hardware name: Generic R8A7790 (Flattened Device Tree)
> > > > ..."
> 
> In my debug, this issue happens under DMAEngine,
> and it depends on spin_lock (I think we talked this topic before...)
> but I don't know which one is wrong, DMAEngine or Sound ?
> 
> Now, sound driver is calling DMAEngine API under spin_lock_irqsave
> 
> 	spin_lock_irqsave(xxxx);
> 	...
> 	dmaengine_prep_dma_cyclic();
> 	...
> 	spin_unlock_irqrestore();
> 
> dmaengine_prep_dma_cyclic() does many things,
> and it is using spin_unlock_irq().
> It re-enabled sound interrupt before calling spin_unlock_irqrestore()
> 
> My question is which one is correct solution ??
>  - sound driver shouldn't call DMAEngine API under spin_lock_irqsave() ?
>  - DMAEngine driver should use spin_lock_irqsave() instead of
> spin_lock_irq() ?

I'm sorry to have missed this e-mail. I've now replied to your patch series 
fixing the problem.

I believe the best solution would be to refactor the DMA engine API to make 
reuse of descriptors possible. That way drivers could allocate descriptors at 
initialization time in non-atomic context, and then use them in atomic 
contexts (including interrupt handlers) without requiring reallocation.

That's a longer term problem, and we need a fix now for the sound bug, so 
switching to spin_lock_irqsave() seems fine to me.

-- 
Regards,

Laurent Pinchart