Something broke in snd_soc_pcm_stream

Something broke in snd_soc_pcm_stream

[Timur Tabi]

Mark,

I'm testing your for-3.1 branch on the P1022DS, and I'm getting a kernel panic
because a pointer is NULL where I don't expect it to be.  In fsl_dma_new(), I
have this code:

if (dai->driver->capture.channels_min) {
	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, card->dev,
		fsl_dma_hardware.buffer_bytes_max,
		&pcm->streams[1].substream->dma_buffer);
	if (ret) {
		dev_err(card->dev, "can't alloc capture dma buffer\n");
		snd_dma_free_pages(&pcm->streams[0].substream->dma_buffer);
		return ret;
	}
}

I test for channels_min because the playback stream is registered separately
from the capture stream, so I expect fsl_dma_new() to be called twice: once to
initialize playback, and again to initialize capture.

In the past, if dai->driver->capture.channels_min was equal to some non-zero
value, then pcm->streams[1].substream was also non-NULL.  This appears no longer
to be true.  Now it appears that playback.channels_min and capture.channels_min
are both set to 2 before fsl_dma_new() is called the first time.

I can change my code to do this:

if (pcm->streams[1].substream) {
	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, card->dev,
		fsl_dma_hardware.buffer_bytes_max,
		&pcm->streams[1].substream->dma_buffer);
	if (ret) {
		dev_err(card->dev, "can't alloc capture dma buffer\n");
		snd_dma_free_pages(&pcm->streams[0].substream->dma_buffer);
		return ret;
	}
}

But before I do that, I'd like to know if this is a bug in ASoC, or just a new
behavior that I need to handle.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Something broke in snd_soc_pcm_stream

[Mark Brown]

On Tue, Jun 07, 2011 at 04:31:52PM -0500, Timur Tabi wrote:

> I can change my code to do this:

> if (pcm->streams[1].substream) {

instead of checking for channels_min.

> 	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, card->dev,
> 		fsl_dma_hardware.buffer_bytes_max,
> 		&pcm->streams[1].substream->dma_buffer);
> 	if (ret) {
> 		dev_err(card->dev, "can't alloc capture dma buffer\n");
> 		snd_dma_free_pages(&pcm->streams[0].substream->dma_buffer);
> 		return ret;
> 	}
> }

> But before I do that, I'd like to know if this is a bug in ASoC, or just a new
> behavior that I need to handle.

I don't think this is a bug in the core, relying on the having things
partially initialised in a particular order seems overly fragile -
better to check for the thing you're actually looking for.  There wasn't
a deliberate change I can think of here, though.

Re: Something broke in snd_soc_pcm_stream

[Timur Tabi]

Mark Brown wrote:
> I don't think this is a bug in the core, relying on the having things
> partially initialised in a particular order seems overly fragile -
> better to check for the thing you're actually looking for.  There wasn't
> a deliberate change I can think of here, though.

I think it's because you test the codec_dai in soc_new_pcm(), which seems weird.
 I put some printks in this function, and this is what I got:

soc_new_pcm:2132 codec_dai=e6998840
soc_new_pcm:2133 codec_dai->driver->playback.channels_min=2
soc_new_pcm:2134 codec_dai->driver->capture.channels_min=0
soc_new_pcm:2136 cpu_dai->driver->playback.channels_min=2
soc_new_pcm:2137 cpu_dai->driver->capture.channels_min=2
soc_new_pcm:2144 playback=1
soc_new_pcm:2145 capture=0
soc_new_pcm:2173 platform->driver->pcm_new=c0329fd0
soc_new_pcm:2176
fsl_dma_new:305 dai->driver->playback.channels_min=2
fsl_dma_new:306 dai->driver->capture.channels_min=2
fsl_dma_new:329 pcm=e69aae00
fsl_dma_new:330 pcm->streams[1].substream=  (null)

It just so happens that on the the P1022, the CPU does support both directions
in its DAI, but the codec (WM8776) supports only one direction per DAI.  So I
suspect that you shouldn't be testing codec_dai either, because it assumes that
the codec is the only arbiter on which direction a given dai-group supports.

Anyway, I changed my driver to test for the actual pointer, and it works.  You
might want to consider doing the same in soc_pcm_new().

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Something broke in snd_soc_pcm_stream

[Mark Brown]

On Tue, Jun 07, 2011 at 05:20:25PM -0500, Timur Tabi wrote:

> It just so happens that on the the P1022, the CPU does support both directions
> in its DAI, but the codec (WM8776) supports only one direction per DAI.  So I
> suspect that you shouldn't be testing codec_dai either, because it assumes that
> the codec is the only arbiter on which direction a given dai-group supports.

Or it just means that unidirectional CPU DAIs are so rare that nobody
got round to adding the check for them.

> Anyway, I changed my driver to test for the actual pointer, and it works.  You
> might want to consider doing the same in soc_pcm_new().

I know we've been round this loop quite a few times in the past but just
to reiterate the situation with Linux is rather different to with other
OSs - you can submit patches to *any* code in Linux, you're not restricted
to only working on your individual driver.