Re: [alsa-devel] ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used
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
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
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
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
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
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,
participants (3)
-
Dan Carpenter -
Jyri Sarha -
Takashi Iwai