* Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used @ 2015-04-10 9:33 Dan Carpenter 2015-04-13 11:39 ` Jyri Sarha 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2015-04-10 9:33 UTC (permalink / raw) To: jsarha; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used 2015-04-10 9:33 ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used Dan Carpenter @ 2015-04-13 11:39 ` Jyri Sarha 2015-04-13 12:16 ` Dan Carpenter 0 siblings, 1 reply; 7+ messages in thread From: Jyri Sarha @ 2015-04-13 11:39 UTC (permalink / raw) To: Dan Carpenter; +Cc: Ujfalusi, Peter, alsa-devel, Mark Brown 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used 2015-04-13 11:39 ` Jyri Sarha @ 2015-04-13 12:16 ` Dan Carpenter 2015-04-13 13:32 ` Jyri Sarha 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2015-04-13 12:16 UTC (permalink / raw) To: Jyri Sarha; +Cc: Ujfalusi, Peter, alsa-devel, Mark Brown 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used 2015-04-13 12:16 ` Dan Carpenter @ 2015-04-13 13:32 ` Jyri Sarha 2015-04-13 13:58 ` Dan Carpenter 0 siblings, 1 reply; 7+ messages in thread From: Jyri Sarha @ 2015-04-13 13:32 UTC (permalink / raw) To: Dan Carpenter; +Cc: Ujfalusi, Peter, alsa-devel, Mark Brown 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used 2015-04-13 13:32 ` Jyri Sarha @ 2015-04-13 13:58 ` Dan Carpenter 2015-04-13 14:22 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2015-04-13 13:58 UTC (permalink / raw) To: Jyri Sarha; +Cc: Ujfalusi, Peter, alsa-devel, Mark Brown 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used 2015-04-13 13:58 ` Dan Carpenter @ 2015-04-13 14:22 ` Takashi Iwai 0 siblings, 0 replies; 7+ messages in thread From: Takashi Iwai @ 2015-04-13 14:22 UTC (permalink / raw) To: Dan Carpenter; +Cc: Ujfalusi, Peter, alsa-devel, Mark Brown, Jyri Sarha 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, ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used @ 2015-03-27 6:20 Dan Carpenter 0 siblings, 0 replies; 7+ messages in thread From: Dan Carpenter @ 2015-03-27 6:20 UTC (permalink / raw) To: jsarha; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-04-13 14:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-10 9:33 ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used Dan Carpenter 2015-04-13 11:39 ` Jyri Sarha 2015-04-13 12:16 ` Dan Carpenter 2015-04-13 13:32 ` Jyri Sarha 2015-04-13 13:58 ` Dan Carpenter 2015-04-13 14:22 ` Takashi Iwai -- strict thread matches above, loose matches on Subject: below -- 2015-03-27 6:20 Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.