From: Arnd Bergmann arnd@arndb.de
clang warns about excessive stack usage in this driver when UBSAN is enabled:
drivers/staging/greybus/audio_topology.c:977:12: error: stack frame size of 1836 bytes in function 'gbaudio_tplg_create_widget' [-Werror,-Wframe-larger-than=]
Rework this code to no longer use compound literals for initializing the structure in each case, but instead keep the common bits in a preallocated constant array and copy them as needed.
Link: https://github.com/ClangBuiltLinux/linux/issues/1535 Link: https://lore.kernel.org/r/20210103223541.2790855-1-arnd@kernel.org/ Signed-off-by: Arnd Bergmann arnd@arndb.de [nathan: Address review comments from v1] Signed-off-by: Nathan Chancellor nathan@kernel.org ---
v1 -> v2: https://lore.kernel.org/r/20210103223541.2790855-1-arnd@kernel.org/
* Use NULL for name field in SND_DOC_DAPM_* in gbaudio_widgets (Alex).
* Do not eliminate *dw assignment within the switch cases, as invalid enum values in-between valid enum values (such as snd_soc_dapm_demux) would not be handled properly by the "if value is greater than the array size" check (Alex). This addresses a few other comments by Alex and Dan because w->type is not checked against the array's size.
Arnd, if you disagree with this approach, please let me know so that we can get this fixed in a way that everyone is happy with.
drivers/staging/greybus/audio_topology.c | 92 ++++++++++++------------ 1 file changed, 45 insertions(+), 47 deletions(-)
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c index 1e613d42d823..7f7d558b76d0 100644 --- a/drivers/staging/greybus/audio_topology.c +++ b/drivers/staging/greybus/audio_topology.c @@ -974,6 +974,44 @@ static int gbaudio_widget_event(struct snd_soc_dapm_widget *w, return ret; }
+static const struct snd_soc_dapm_widget gbaudio_widgets[] = { + [snd_soc_dapm_spk] = SND_SOC_DAPM_SPK(NULL, gbcodec_event_spk), + [snd_soc_dapm_hp] = SND_SOC_DAPM_HP(NULL, gbcodec_event_hp), + [snd_soc_dapm_mic] = SND_SOC_DAPM_MIC(NULL, gbcodec_event_int_mic), + [snd_soc_dapm_output] = SND_SOC_DAPM_OUTPUT(NULL), + [snd_soc_dapm_input] = SND_SOC_DAPM_INPUT(NULL), + [snd_soc_dapm_switch] = SND_SOC_DAPM_SWITCH_E(NULL, SND_SOC_NOPM, + 0, 0, NULL, + gbaudio_widget_event, + SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMD), + [snd_soc_dapm_pga] = SND_SOC_DAPM_PGA_E(NULL, SND_SOC_NOPM, + 0, 0, NULL, 0, + gbaudio_widget_event, + SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMD), + [snd_soc_dapm_mixer] = SND_SOC_DAPM_MIXER_E(NULL, SND_SOC_NOPM, + 0, 0, NULL, 0, + gbaudio_widget_event, + SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMD), + [snd_soc_dapm_mux] = SND_SOC_DAPM_MUX_E(NULL, SND_SOC_NOPM, + 0, 0, NULL, + gbaudio_widget_event, + SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMD), + [snd_soc_dapm_aif_in] = SND_SOC_DAPM_AIF_IN_E(NULL, NULL, 0, + SND_SOC_NOPM, 0, 0, + gbaudio_widget_event, + SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMD), + [snd_soc_dapm_aif_out] = SND_SOC_DAPM_AIF_OUT_E(NULL, NULL, 0, + SND_SOC_NOPM, 0, 0, + gbaudio_widget_event, + SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMD), +}; + static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, struct snd_soc_dapm_widget *dw, struct gb_audio_widget *w, int *w_size) @@ -1052,77 +1090,37 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module,
switch (w->type) { case snd_soc_dapm_spk: - *dw = (struct snd_soc_dapm_widget) - SND_SOC_DAPM_SPK(w->name, gbcodec_event_spk); + *dw = gbaudio_widgets[w->type]; module->op_devices |= GBAUDIO_DEVICE_OUT_SPEAKER; break; case snd_soc_dapm_hp: - *dw = (struct snd_soc_dapm_widget) - SND_SOC_DAPM_HP(w->name, gbcodec_event_hp); + *dw = gbaudio_widgets[w->type]; module->op_devices |= (GBAUDIO_DEVICE_OUT_WIRED_HEADSET | GBAUDIO_DEVICE_OUT_WIRED_HEADPHONE); module->ip_devices |= GBAUDIO_DEVICE_IN_WIRED_HEADSET; break; case snd_soc_dapm_mic: - *dw = (struct snd_soc_dapm_widget) - SND_SOC_DAPM_MIC(w->name, gbcodec_event_int_mic); + *dw = gbaudio_widgets[w->type]; module->ip_devices |= GBAUDIO_DEVICE_IN_BUILTIN_MIC; break; case snd_soc_dapm_output: - *dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_OUTPUT(w->name); - break; case snd_soc_dapm_input: - *dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_INPUT(w->name); - break; case snd_soc_dapm_switch: - *dw = (struct snd_soc_dapm_widget) - SND_SOC_DAPM_SWITCH_E(w->name, SND_SOC_NOPM, 0, 0, - widget_kctls, - gbaudio_widget_event, - SND_SOC_DAPM_PRE_PMU | - SND_SOC_DAPM_POST_PMD); - break; case snd_soc_dapm_pga: - *dw = (struct snd_soc_dapm_widget) - SND_SOC_DAPM_PGA_E(w->name, SND_SOC_NOPM, 0, 0, NULL, 0, - gbaudio_widget_event, - SND_SOC_DAPM_PRE_PMU | - SND_SOC_DAPM_POST_PMD); - break; case snd_soc_dapm_mixer: - *dw = (struct snd_soc_dapm_widget) - SND_SOC_DAPM_MIXER_E(w->name, SND_SOC_NOPM, 0, 0, NULL, - 0, gbaudio_widget_event, - SND_SOC_DAPM_PRE_PMU | - SND_SOC_DAPM_POST_PMD); - break; case snd_soc_dapm_mux: - *dw = (struct snd_soc_dapm_widget) - SND_SOC_DAPM_MUX_E(w->name, SND_SOC_NOPM, 0, 0, - widget_kctls, gbaudio_widget_event, - SND_SOC_DAPM_PRE_PMU | - SND_SOC_DAPM_POST_PMD); + *dw = gbaudio_widgets[w->type]; break; case snd_soc_dapm_aif_in: - *dw = (struct snd_soc_dapm_widget) - SND_SOC_DAPM_AIF_IN_E(w->name, w->sname, 0, - SND_SOC_NOPM, - 0, 0, gbaudio_widget_event, - SND_SOC_DAPM_PRE_PMU | - SND_SOC_DAPM_POST_PMD); - break; case snd_soc_dapm_aif_out: - *dw = (struct snd_soc_dapm_widget) - SND_SOC_DAPM_AIF_OUT_E(w->name, w->sname, 0, - SND_SOC_NOPM, - 0, 0, gbaudio_widget_event, - SND_SOC_DAPM_PRE_PMU | - SND_SOC_DAPM_POST_PMD); + *dw = gbaudio_widgets[w->type]; + dw->sname = w->sname; break; default: ret = -EINVAL; goto error; } + dw->name = w->name;
dev_dbg(module->dev, "%s: widget of type %d created\n", dw->name, dw->id);
base-commit: 42eb8fdac2fc5d62392dcfcf0253753e821a97b0
On Thu, Dec 9, 2021 at 11:52 AM Nathan Chancellor nathan@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
clang warns about excessive stack usage in this driver when UBSAN is enabled:
drivers/staging/greybus/audio_topology.c:977:12: error: stack frame size of 1836 bytes in function 'gbaudio_tplg_create_widget' [-Werror,-Wframe-larger-than=]
Rework this code to no longer use compound literals for initializing the structure in each case, but instead keep the common bits in a preallocated constant array and copy them as needed.
Link: https://github.com/ClangBuiltLinux/linux/issues/1535 Link: https://lore.kernel.org/r/20210103223541.2790855-1-arnd@kernel.org/ Signed-off-by: Arnd Bergmann arnd@arndb.de [nathan: Address review comments from v1] Signed-off-by: Nathan Chancellor nathan@kernel.org
Thanks for helping to get this across the finish line. Reviewed-by: Nick Desaulniers ndesaulniers@google.com
v1 -> v2: https://lore.kernel.org/r/20210103223541.2790855-1-arnd@kernel.org/
Use NULL for name field in SND_DOC_DAPM_* in gbaudio_widgets (Alex).
Do not eliminate *dw assignment within the switch cases, as invalid enum values in-between valid enum values (such as snd_soc_dapm_demux) would not be handled properly by the "if value is greater than the array size" check (Alex). This addresses a few other comments by Alex and Dan because w->type is not checked against the array's size.
Arnd, if you disagree with this approach, please let me know so that we can get this fixed in a way that everyone is happy with.
drivers/staging/greybus/audio_topology.c | 92 ++++++++++++------------ 1 file changed, 45 insertions(+), 47 deletions(-)
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c index 1e613d42d823..7f7d558b76d0 100644 --- a/drivers/staging/greybus/audio_topology.c +++ b/drivers/staging/greybus/audio_topology.c @@ -974,6 +974,44 @@ static int gbaudio_widget_event(struct snd_soc_dapm_widget *w, return ret; }
+static const struct snd_soc_dapm_widget gbaudio_widgets[] = {
[snd_soc_dapm_spk] = SND_SOC_DAPM_SPK(NULL, gbcodec_event_spk),[snd_soc_dapm_hp] = SND_SOC_DAPM_HP(NULL, gbcodec_event_hp),[snd_soc_dapm_mic] = SND_SOC_DAPM_MIC(NULL, gbcodec_event_int_mic),[snd_soc_dapm_output] = SND_SOC_DAPM_OUTPUT(NULL),[snd_soc_dapm_input] = SND_SOC_DAPM_INPUT(NULL),[snd_soc_dapm_switch] = SND_SOC_DAPM_SWITCH_E(NULL, SND_SOC_NOPM,0, 0, NULL,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),[snd_soc_dapm_pga] = SND_SOC_DAPM_PGA_E(NULL, SND_SOC_NOPM,0, 0, NULL, 0,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),[snd_soc_dapm_mixer] = SND_SOC_DAPM_MIXER_E(NULL, SND_SOC_NOPM,0, 0, NULL, 0,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),[snd_soc_dapm_mux] = SND_SOC_DAPM_MUX_E(NULL, SND_SOC_NOPM,0, 0, NULL,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),[snd_soc_dapm_aif_in] = SND_SOC_DAPM_AIF_IN_E(NULL, NULL, 0,SND_SOC_NOPM, 0, 0,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),[snd_soc_dapm_aif_out] = SND_SOC_DAPM_AIF_OUT_E(NULL, NULL, 0,SND_SOC_NOPM, 0, 0,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),+};
static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, struct snd_soc_dapm_widget *dw, struct gb_audio_widget *w, int *w_size) @@ -1052,77 +1090,37 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module,
switch (w->type) { case snd_soc_dapm_spk:
*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_SPK(w->name, gbcodec_event_spk);
*dw = gbaudio_widgets[w->type]; module->op_devices |= GBAUDIO_DEVICE_OUT_SPEAKER; break; case snd_soc_dapm_hp:
*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_HP(w->name, gbcodec_event_hp);
*dw = gbaudio_widgets[w->type]; module->op_devices |= (GBAUDIO_DEVICE_OUT_WIRED_HEADSET | GBAUDIO_DEVICE_OUT_WIRED_HEADPHONE); module->ip_devices |= GBAUDIO_DEVICE_IN_WIRED_HEADSET; break; case snd_soc_dapm_mic:
*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_MIC(w->name, gbcodec_event_int_mic);
*dw = gbaudio_widgets[w->type]; module->ip_devices |= GBAUDIO_DEVICE_IN_BUILTIN_MIC; break; case snd_soc_dapm_output:
*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_OUTPUT(w->name);break; case snd_soc_dapm_input:*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_INPUT(w->name);break; case snd_soc_dapm_switch:*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_SWITCH_E(w->name, SND_SOC_NOPM, 0, 0,widget_kctls,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);break; case snd_soc_dapm_pga:*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_PGA_E(w->name, SND_SOC_NOPM, 0, 0, NULL, 0,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);break; case snd_soc_dapm_mixer:*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_MIXER_E(w->name, SND_SOC_NOPM, 0, 0, NULL,0, gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);break; case snd_soc_dapm_mux:*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_MUX_E(w->name, SND_SOC_NOPM, 0, 0,widget_kctls, gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);
*dw = gbaudio_widgets[w->type]; break; case snd_soc_dapm_aif_in:
*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_AIF_IN_E(w->name, w->sname, 0,SND_SOC_NOPM,0, 0, gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);break; case snd_soc_dapm_aif_out:*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_AIF_OUT_E(w->name, w->sname, 0,SND_SOC_NOPM,0, 0, gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);
*dw = gbaudio_widgets[w->type];dw->sname = w->sname; break; default: ret = -EINVAL; goto error; }dw->name = w->name; dev_dbg(module->dev, "%s: widget of type %d created\n", dw->name, dw->id);base-commit: 42eb8fdac2fc5d62392dcfcf0253753e821a97b0
2.34.1
On 12/9/21 1:51 PM, Nathan Chancellor wrote:
From: Arnd Bergmann arnd@arndb.de
clang warns about excessive stack usage in this driver when UBSAN is enabled:
drivers/staging/greybus/audio_topology.c:977:12: error: stack frame size of 1836 bytes in function 'gbaudio_tplg_create_widget' [-Werror,-Wframe-larger-than=]
Rework this code to no longer use compound literals for initializing the structure in each case, but instead keep the common bits in a preallocated constant array and copy them as needed.
Link: https://github.com/ClangBuiltLinux/linux/issues/1535 Link: https://lore.kernel.org/r/20210103223541.2790855-1-arnd@kernel.org/ Signed-off-by: Arnd Bergmann arnd@arndb.de [nathan: Address review comments from v1] Signed-off-by: Nathan Chancellor nathan@kernel.org
This looks good to me and it seems the switch statement becomes much more concise.
One thing I would suggest is that you somehow capture in comments the fact that you're manually assigning the widget name and in a couple spots the "sname" (I don't know what that represents).
Someday these SND_SOC_DAPM*() macros could be changed, and it would be helpful to have a little guidance around this code to explain what was done here.
This isn't critical, but would improve maintainability.
We can see what others think, but either way:
Reviewed-by: Alex Elder elder@linaro.org
v1 -> v2: https://lore.kernel.org/r/20210103223541.2790855-1-arnd@kernel.org/
Use NULL for name field in SND_DOC_DAPM_* in gbaudio_widgets (Alex).
Do not eliminate *dw assignment within the switch cases, as invalid enum values in-between valid enum values (such as snd_soc_dapm_demux) would not be handled properly by the "if value is greater than the array size" check (Alex). This addresses a few other comments by Alex and Dan because w->type is not checked against the array's size.
Arnd, if you disagree with this approach, please let me know so that we can get this fixed in a way that everyone is happy with.
drivers/staging/greybus/audio_topology.c | 92 ++++++++++++------------ 1 file changed, 45 insertions(+), 47 deletions(-)
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c index 1e613d42d823..7f7d558b76d0 100644 --- a/drivers/staging/greybus/audio_topology.c +++ b/drivers/staging/greybus/audio_topology.c @@ -974,6 +974,44 @@ static int gbaudio_widget_event(struct snd_soc_dapm_widget *w, return ret; } +static const struct snd_soc_dapm_widget gbaudio_widgets[] = {
- [snd_soc_dapm_spk] = SND_SOC_DAPM_SPK(NULL, gbcodec_event_spk),
- [snd_soc_dapm_hp] = SND_SOC_DAPM_HP(NULL, gbcodec_event_hp),
- [snd_soc_dapm_mic] = SND_SOC_DAPM_MIC(NULL, gbcodec_event_int_mic),
- [snd_soc_dapm_output] = SND_SOC_DAPM_OUTPUT(NULL),
- [snd_soc_dapm_input] = SND_SOC_DAPM_INPUT(NULL),
- [snd_soc_dapm_switch] = SND_SOC_DAPM_SWITCH_E(NULL, SND_SOC_NOPM,
0, 0, NULL,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),- [snd_soc_dapm_pga] = SND_SOC_DAPM_PGA_E(NULL, SND_SOC_NOPM,
0, 0, NULL, 0,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),- [snd_soc_dapm_mixer] = SND_SOC_DAPM_MIXER_E(NULL, SND_SOC_NOPM,
0, 0, NULL, 0,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),- [snd_soc_dapm_mux] = SND_SOC_DAPM_MUX_E(NULL, SND_SOC_NOPM,
0, 0, NULL,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),- [snd_soc_dapm_aif_in] = SND_SOC_DAPM_AIF_IN_E(NULL, NULL, 0,
SND_SOC_NOPM, 0, 0,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),- [snd_soc_dapm_aif_out] = SND_SOC_DAPM_AIF_OUT_E(NULL, NULL, 0,
SND_SOC_NOPM, 0, 0,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD),+};
- static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, struct snd_soc_dapm_widget *dw, struct gb_audio_widget *w, int *w_size)
@@ -1052,77 +1090,37 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, switch (w->type) { case snd_soc_dapm_spk:
*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_SPK(w->name, gbcodec_event_spk);
*dw = gbaudio_widgets[w->type];
*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_HP(w->name, gbcodec_event_hp);
*dw = gbaudio_widgets[w->type];
*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_MIC(w->name, gbcodec_event_int_mic);
*dw = gbaudio_widgets[w->type];
*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_OUTPUT(w->name);break;*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_INPUT(w->name);break;*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_SWITCH_E(w->name, SND_SOC_NOPM, 0, 0,widget_kctls,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);break;*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_PGA_E(w->name, SND_SOC_NOPM, 0, 0, NULL, 0,gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);break;*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_MIXER_E(w->name, SND_SOC_NOPM, 0, 0, NULL,0, gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);break;*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_MUX_E(w->name, SND_SOC_NOPM, 0, 0,widget_kctls, gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);
*dw = gbaudio_widgets[w->type];
*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_AIF_IN_E(w->name, w->sname, 0,SND_SOC_NOPM,0, 0, gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);break;*dw = (struct snd_soc_dapm_widget)SND_SOC_DAPM_AIF_OUT_E(w->name, w->sname, 0,SND_SOC_NOPM,0, 0, gbaudio_widget_event,SND_SOC_DAPM_PRE_PMU |SND_SOC_DAPM_POST_PMD);
*dw = gbaudio_widgets[w->type];dw->sname = w->sname;- dw->name = w->name;
dev_dbg(module->dev, "%s: widget of type %d created\n", dw->name, dw->id);
base-commit: 42eb8fdac2fc5d62392dcfcf0253753e821a97b0
-
Alex Elder -
Nathan Chancellor -
Nick Desaulniers