Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

[Dan Carpenter]

Hello Jyri Sarha,

The patch a75a053f1eef: "ASoC: davinci-mcasp: Set rule constraints if
implicit BCLK divider is used" from Mar 20, 2015, leads to the
following Smatch warning:

	sound/soc/davinci/davinci-mcasp.c:1152 davinci_mcasp_startup()
	error: buffer overflow 'mcasp->ruledata' 2 <= 2

sound/soc/davinci/davinci-mcasp.c
  1138  
  1139          /*
  1140           * Limit the maximum allowed channels for the first stream:
  1141           * number of serializers for the direction * tdm slots per serializer
  1142           */
  1143          if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
  1144                  dir = TX_MODE;
  1145          else
  1146                  dir = RX_MODE;

dir is either 1 or 2.

  1147  
  1148          for (i = 0; i < mcasp->num_serializer; i++) {
  1149                  if (mcasp->serial_dir[i] == dir)
  1150                          max_channels++;
  1151          }
  1152          mcasp->ruledata[dir].serializers = max_channels;
                       ^^^^^^^^^^^^^
->ruledata[] has only two elements so ->ruledata[2] is beyond the end of
the array.

  1153          max_channels *= mcasp->tdm_slots;

regards,
dan carpenter

[PATCH] ASoC: davinci-mcasp: Index ruledata in drvdata with substream->stream

[Jyri Sarha]

The serializer direction definitions runs from 1 to 2, which does not
suite the purpose. The substream->stream is perfect for the purpose
and should have been used from the beginning.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 sound/soc/davinci/davinci-mcasp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 76156d18..0b6b1b2 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -1149,7 +1149,7 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream,
 		if (mcasp->serial_dir[i] == dir)
 			max_channels++;
 	}
-	mcasp->ruledata[dir].serializers = max_channels;
+	mcasp->ruledata[substream->stream].serializers = max_channels;
 	max_channels *= mcasp->tdm_slots;
 	/*
 	 * If the already active stream has less channels than the calculated
@@ -1172,7 +1172,7 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream,
 	if (mcasp->bclk_master && mcasp->bclk_div == 0 && mcasp->sysclk_freq) {
 		int ret;
 
-		mcasp->ruledata[dir].mcasp = mcasp;
+		mcasp->ruledata[substream->stream].mcasp = mcasp;
 
 		ret = snd_pcm_hw_rule_add(substream->runtime, 0,
 					  SNDRV_PCM_HW_PARAM_RATE,
-- 
1.9.1

Re: [PATCH] ASoC: davinci-mcasp: Index ruledata in drvdata with substream->stream

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 268 bytes --]

On Fri, Mar 27, 2015 at 11:47:51AM +0200, Jyri Sarha wrote:
> The serializer direction definitions runs from 1 to 2, which does not
> suite the purpose. The substream->stream is perfect for the purpose
> and should have been used from the beginning.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

[Takashi Iwai]

At Mon, 13 Apr 2015 16:58:20 +0300,
Dan Carpenter wrote:
> 
> On Mon, Apr 13, 2015 at 04:32:24PM +0300, Jyri Sarha wrote:
> > On 04/13/15 15:16, Dan Carpenter wrote:
> > >Thanks for looking at this.  I haven't double checked your analysis, but
> > >512 means the stack is 2k and that's too high.  I think there is
> > >supposed to be a build warning if we use more than 1k of stack in a
> > >function?
> > >
> > >It's better to allocate it with kmalloc() and free it at the end.
> > >
> > >regards,
> > >dan carpenter
> > >
> > 
> > The function may get called hundreds or even thousands of times when
> > an audio stream is started. How heavy is kmalloc/free cycle?
> > 
> > Using kmalloc just feels a bit stupid since in any remotely feasible
> > case the maximum number of channels is 8. But maybe I just should
> > not care.
> 
> I'm a total newbie to this specific so I can't really advise you.  I
> did find the limit where GCC prints a warning:
> 
> home/kisskb/slave/src/net/rds/iw_rdma.c: warning: the frame size of 1052 bytes is larger than 1024 bytes [-Wframe-larger-than=]: 200:1 =>
> 
> Since this could go up to 2k that's pretty huge.  One idea is to just
> say:
> 
> 	int small_buffer[64];
> 	int size = ci->max - ci->min + 1;
> 	int *list = &small_buffer;
> 
> 	if (size > 64)
> 		list = kmalloc();
> 
> 	...
> 
> free:
> 	if (size > 64)
> 		kfree(list);
> 
> If you've looked at the call trees which call this function and we can't
> actually run out of space then that's probably fine too.  I don't think
> there are any arches which still use 4k stacks.

The code there needs just to calculate min and max values instead of
the whole discrete value list.  The code has really much room to
optimize.  A totally untested patch is below.


Takashi

---
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index bb4b78eada58..68a1ae801fb8 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -1095,33 +1095,30 @@ static int davinci_mcasp_hw_rule_channels(struct snd_pcm_hw_params *params,
 	int rate = params_rate(params);
 	int max_chan_per_wire = rd->mcasp->tdm_slots < ci->max ?
 		rd->mcasp->tdm_slots : ci->max;
-	unsigned int list[ci->max - ci->min + 1];
-	int c1, c, count = 0;
+	struct snd_interval range;
+	int c1;
 
+	snd_interval_any(&range);
+	range.min = range.max = 0;
 	for (c1 = ci->min; c1 <= max_chan_per_wire; c1++) {
 		uint bclk_freq = c1*sbits*rate;
 		int ppm;
 
 		davinci_mcasp_calc_clk_div(rd->mcasp, bclk_freq, &ppm);
 		if (abs(ppm) < DAVINCI_MAX_RATE_ERROR_PPM) {
+			if (!range.min)
+				range.min = c1;
+			range.max = c1;
 			/* If we can use all tdm_slots, we can put any
 			   amount of channels to remaining wires as
 			   long as they fit in. */
-			if (c1 == rd->mcasp->tdm_slots) {
-				for (c = c1; c <= rd->serializers*c1 &&
-					     c <= ci->max; c++)
-					list[count++] = c;
-			} else {
-				list[count++] = c1;
-			}
+			if (c1 == rd->mcasp->tdm_slots)
+				range.max = rd->serializers * c1;
 		}
 	}
-	dev_dbg(rd->mcasp->dev,
-		"%d possible channel counts (%d-%d) for %d Hz and %d sbits\n",
-		count, ci->min, ci->max, rate, sbits);
 
-	return snd_interval_list(hw_param_interval(params, rule->var),
-				 count, list, 0);
+	return snd_interval_refine(hw_param_interval(params, rule->var),
+				   &range);
 }
 
 static int davinci_mcasp_startup(struct snd_pcm_substream *substream,

Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

[Dan Carpenter]

On Mon, Apr 13, 2015 at 04:32:24PM +0300, Jyri Sarha wrote:
> On 04/13/15 15:16, Dan Carpenter wrote:
> >Thanks for looking at this.  I haven't double checked your analysis, but
> >512 means the stack is 2k and that's too high.  I think there is
> >supposed to be a build warning if we use more than 1k of stack in a
> >function?
> >
> >It's better to allocate it with kmalloc() and free it at the end.
> >
> >regards,
> >dan carpenter
> >
> 
> The function may get called hundreds or even thousands of times when
> an audio stream is started. How heavy is kmalloc/free cycle?
> 
> Using kmalloc just feels a bit stupid since in any remotely feasible
> case the maximum number of channels is 8. But maybe I just should
> not care.

I'm a total newbie to this specific so I can't really advise you.  I
did find the limit where GCC prints a warning:

home/kisskb/slave/src/net/rds/iw_rdma.c: warning: the frame size of 1052 bytes is larger than 1024 bytes [-Wframe-larger-than=]: 200:1 =>

Since this could go up to 2k that's pretty huge.  One idea is to just
say:

	int small_buffer[64];
	int size = ci->max - ci->min + 1;
	int *list = &small_buffer;

	if (size > 64)
		list = kmalloc();

	...

free:
	if (size > 64)
		kfree(list);

If you've looked at the call trees which call this function and we can't
actually run out of space then that's probably fine too.  I don't think
there are any arches which still use 4k stacks.

regards,
dan carpenter

Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

[Jyri Sarha]

On 04/13/15 15:16, Dan Carpenter wrote:
> Thanks for looking at this.  I haven't double checked your analysis, but
> 512 means the stack is 2k and that's too high.  I think there is
> supposed to be a build warning if we use more than 1k of stack in a
> function?
>
> It's better to allocate it with kmalloc() and free it at the end.
>
> regards,
> dan carpenter
>

The function may get called hundreds or even thousands of times when an 
audio stream is started. How heavy is kmalloc/free cycle?

Using kmalloc just feels a bit stupid since in any remotely feasible 
case the maximum number of channels is 8. But maybe I just should not care.

Best regards,
Jyri

Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

[Dan Carpenter]

Thanks for looking at this.  I haven't double checked your analysis, but
512 means the stack is 2k and that's too high.  I think there is
supposed to be a build warning if we use more than 1k of stack in a
function?

It's better to allocate it with kmalloc() and free it at the end.

regards,
dan carpenter

Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

[Jyri Sarha]

On 04/10/15 12:33, Dan Carpenter wrote:
> Hello Jyri Sarha,
>
> The patch a75a053f1eef: "ASoC: davinci-mcasp: Set rule constraints if
> implicit BCLK divider is used" from Mar 20, 2015, leads to the
> following Sparse warning:
>
> 	sound/soc/davinci/davinci-mcasp.c:1098:45:
> 	warning: Variable length array is used.
>
> sound/soc/davinci/davinci-mcasp.c
>    1088  static int davinci_mcasp_hw_rule_channels(struct snd_pcm_hw_params *params,
>    1089                                            struct snd_pcm_hw_rule *rule)
>    1090  {
>    1091          struct davinci_mcasp_ruledata *rd = rule->private;
>    1092          struct snd_interval *ci =
>    1093                  hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
>    1094          int sbits = params_width(params);
>    1095          int rate = params_rate(params);
>    1096          int max_chan_per_wire = rd->mcasp->tdm_slots < ci->max ?
>    1097                  rd->mcasp->tdm_slots : ci->max;
>    1098          unsigned int list[ci->max - ci->min + 1];
>                                    ^^^^^^^^^^^^^^^^^^^^^
> I wasnt able to determine where these values are capped.  The worry is
> that the kernel only has an 8k stack so if they are too high it could
> oops.  I think these values can come from the user in
> snd_pcm_hw_params_user() but I'm an newbie to the code and I didn't
> see where the limits were enforced.
>

In theory the ci->max should never be more than 512, because
static struct snd_soc_dai_driver davinci_mcasp_dai[] = {
...
			.channels_max	= 32 * 16,

should limit the maximum allowed channel index. Normally this value 
should be way smaller.

However, since there is considerable amount of code involved it is hard 
to be produce a proof of that. I could add a hard max limit of 512 - or 
even 128 - to the list size. The cases when the list size would need to 
be even close 128 are quite theoretical.

Best regards,
Jyri

>    1099          int c1, c, count = 0;
>    1100
>    1101          for (c1 = ci->min; c1 <= max_chan_per_wire; c1++) {
>    1102                  uint bclk_freq = c1*sbits*rate;
>    1103                  int ppm;
>
>
> regards,
> dan carpenter
>

Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

[Dan Carpenter]

Hello Jyri Sarha,

The patch a75a053f1eef: "ASoC: davinci-mcasp: Set rule constraints if
implicit BCLK divider is used" from Mar 20, 2015, leads to the
following Sparse warning:

	sound/soc/davinci/davinci-mcasp.c:1098:45:
	warning: Variable length array is used.

sound/soc/davinci/davinci-mcasp.c
  1088  static int davinci_mcasp_hw_rule_channels(struct snd_pcm_hw_params *params,
  1089                                            struct snd_pcm_hw_rule *rule)
  1090  {
  1091          struct davinci_mcasp_ruledata *rd = rule->private;
  1092          struct snd_interval *ci =
  1093                  hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
  1094          int sbits = params_width(params);
  1095          int rate = params_rate(params);
  1096          int max_chan_per_wire = rd->mcasp->tdm_slots < ci->max ?
  1097                  rd->mcasp->tdm_slots : ci->max;
  1098          unsigned int list[ci->max - ci->min + 1];
                                  ^^^^^^^^^^^^^^^^^^^^^
I wasnt able to determine where these values are capped.  The worry is
that the kernel only has an 8k stack so if they are too high it could
oops.  I think these values can come from the user in
snd_pcm_hw_params_user() but I'm an newbie to the code and I didn't
see where the limits were enforced.

  1099          int c1, c, count = 0;
  1100  
  1101          for (c1 = ci->min; c1 <= max_chan_per_wire; c1++) {
  1102                  uint bclk_freq = c1*sbits*rate;
  1103                  int ppm;


regards,
dan carpenter