Re: [PATCH] To avoid the divide by zero error during the first execution, initialize the data type.

Re: [PATCH] To avoid the divide by zero error during the first execution, initialize the data type.

[Troy Kisky]

avm@ti.com wrote:
> From: Arun Mani <a0270733@gtcx26221.gt.design.ti.com>
> 
> Signed-off-by: Arun Mani <a0270733@gtcx26221.gt.design.ti.com>
> ---
>  sound/soc/davinci/davinci-i2s.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/davinci/davinci-i2s.c b/sound/soc/davinci/davinci-i2s.c
> index b1ea52f..61b1763 100644
> --- a/sound/soc/davinci/davinci-i2s.c
> +++ b/sound/soc/davinci/davinci-i2s.c
> @@ -104,10 +104,12 @@ enum {
>  
>  static struct davinci_pcm_dma_params davinci_i2s_pcm_out = {
>  	.name = "I2S PCM Stereo out",
> +	.data_type = 2, //Initialize the data type for playback to avoid divide by zero

You should have ".acnt = 2" also for consistency.
>  };
>  
>  static struct davinci_pcm_dma_params davinci_i2s_pcm_in = {
>  	.name = "I2S PCM Stereo in",
> +	.data_type = 2, //Initialize the data type for playback to avoid divide by zero
>  };
>  
>  struct davinci_mcbsp_dev {

And if the 1st stream is an 8-bit audio stream, will it initialize it incorrectly ???
Of course, we have formats = SNDRV_PCM_FMTBIT_S16_LE, currently so it's not an issue yet...

I don't see how data_type is not being set in davinci_i2s_hw_params
before being used in davinci_pcm_prepare.

Can prepare be called before hw_params ?

There is a "return -EINVAL" in davinci_i2s_hw_params with a
"printk(KERN_WARNING "davinci-i2s: unsupported PCM format\n")" before it.

Do you see this message in your log ?


Thanks
Troy

RE: [PATCH] To avoid the divide by zero error during the first execution, initialize the data type.

[Mani, Arun]

It indeed gets initialized in the hw_params. But the runtime dma parameters are not populated with the substream private data information that is gets in hw_params for the first time. I am not sure why. Any ideas?



Thanks,
Arun.

> -----Original Message-----
> From: Troy Kisky [mailto:troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org]
> Sent: Wednesday, September 09, 2009 4:47 PM
> To: Mani, Arun
> Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org; alsa-devel@alsa-
> project.org; Arun Mani
> Subject: Re: [PATCH] To avoid the divide by zero error during the first
> execution, initialize the data type.
> 
> avm-l0cyMroinI0@public.gmane.org wrote:
> > From: Arun Mani <a0270733-xHf0xn1KbjXtkKqbFX7TNUJdNcTp/5TL0E9HWUfgJXw@public.gmane.org>
> >
> > Signed-off-by: Arun Mani <a0270733-xHf0xn1KbjXtkKqbFX7TNUJdNcTp/5TL0E9HWUfgJXw@public.gmane.org>
> > ---
> >  sound/soc/davinci/davinci-i2s.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/sound/soc/davinci/davinci-i2s.c
> b/sound/soc/davinci/davinci-i2s.c
> > index b1ea52f..61b1763 100644
> > --- a/sound/soc/davinci/davinci-i2s.c
> > +++ b/sound/soc/davinci/davinci-i2s.c
> > @@ -104,10 +104,12 @@ enum {
> >
> >  static struct davinci_pcm_dma_params davinci_i2s_pcm_out = {
> >  	.name = "I2S PCM Stereo out",
> > +	.data_type = 2, //Initialize the data type for playback to avoid
> divide by zero
> 
> You should have ".acnt = 2" also for consistency.
> >  };
> >
> >  static struct davinci_pcm_dma_params davinci_i2s_pcm_in = {
> >  	.name = "I2S PCM Stereo in",
> > +	.data_type = 2, //Initialize the data type for playback to avoid
> divide by zero
> >  };
> >
> >  struct davinci_mcbsp_dev {
> 
> And if the 1st stream is an 8-bit audio stream, will it initialize it
> incorrectly ???
> Of course, we have formats = SNDRV_PCM_FMTBIT_S16_LE, currently so it's
> not an issue yet...
> 
> I don't see how data_type is not being set in davinci_i2s_hw_params
> before being used in davinci_pcm_prepare.
> 
> Can prepare be called before hw_params ?
> 
> There is a "return -EINVAL" in davinci_i2s_hw_params with a
> "printk(KERN_WARNING "davinci-i2s: unsupported PCM format\n")" before it.
> 
> Do you see this message in your log ?
> 
> 
> Thanks
> Troy
> 

Re: [PATCH] To avoid the divide by zero error during the first execution, initialize the data type.

[Troy Kisky]

Mani, Arun wrote:
> It indeed gets initialized in the hw_params. But the runtime dma parameters are not populated with the substream private data information that is gets in hw_params for the first time. I am not sure why. Any ideas?
> 
> 
> 
> Thanks,
> Arun.
> 

The real bug is in the routine davinci_i2s_startup.

The line

	cpu_dai->dma_data = dev->dma_params[substream->stream];


This works, as long as both streams aren't open.

playback stream
davinci_i2s_startup: P0 dma_params=c03a7680
davinci_pcm_dma_request: P0 dma_data=c03a7680
^^ saves pointer

capture stream
davinci_i2s_startup: C1 dma_params=c03a769c
^^ changes cpu_dai pointer
davinci_pcm_dma_request: C1 dma_data=c03a769c
^^ saves pointer
davinci_i2s_hw_params: data_type=2 C1 dma_params=c03a769c
^^ capture data_type=2

davinci_pcm_enqueue_dma: data_type=2 C1 c03a769c
davinci_pcm_enqueue_dma: data_type=2 C1 c03a769c

playback stream
davinci_i2s_hw_params: data_type=2 P0 dma_params=c03a769c
^^ playback dma_params is WRONG should be c03a7680

davinci_pcm_enqueue_dma: data_type=0 P0 c03a7680
^^ This uses the save dma_params value, hence data_type is still 0

Division by zero in kernel.


It seems like a lot of drivers may have this bug.

Troy