All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alex Elder <elder@ieee.org>
To: Arnd Bergmann <arnd@kernel.org>,
	Vaibhav Agarwal <vaibhav.sr@gmail.com>,
	Mark Greer <mgreer@animalcreek.com>,
	Johan Hovold <johan@kernel.org>, Alex Elder <elder@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Nathan Chancellor <natechancellor@gmail.com>,
	Nick Desaulniers <ndesaulniers@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Coiby Xu <coiby.xu@gmail.com>,
	greybus-dev@lists.linaro.org, devel@driverdev.osuosl.org,
	linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com
Subject: Re: [PATCH] staging: greybus: fix stack size warning with UBSAN
Date: Sun, 3 Jan 2021 20:18:47 -0600	[thread overview]
Message-ID: <d9c341c9-9005-b425-9dd8-e797869bbcb0@ieee.org> (raw)
In-Reply-To: <20210103223541.2790855-1-arnd@kernel.org>

On 1/3/21 4:35 PM, Arnd Bergmann 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.

This is good, but I have a few comments.

You took out the default case, and it seems you are using
a w->type value bigger than the initialization array to
determine validity.  But there are more values defined in
the snd_soc_dapm_type enumerated type than are explicitly
listed as cases in the switch statement.  So the switch
statement no longer treats some types as invalid (such
as snd_soc_dapm_demux).  Am I missing something?

You are setting explicit names, such as "spk", "hp",
"mic", etc. in the initialization array.  But you
update the name after (struct) assigning from the
array.  I have no real objection I guess, but why
bother?  Why not just use null pointers in the
initialization array?

You change a semicolon to a comma in one spot, and you
should not have.  I'll point that out below.

I like that you got rid of the type casts, which were
apparently unnecessary.

You dropped the break in the final case in the switch
statement, but in an earlier discussion I think we
concluded that wasn't a problem.

I guess the main thing is the first thing mentioned.


Thanks.

					-Alex

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/staging/greybus/audio_topology.c | 106 ++++++++++-------------
>   1 file changed, 47 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 96b8b29fe899..c03873915c20 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("spk", gbcodec_event_spk),
> +	[snd_soc_dapm_hp]	= SND_SOC_DAPM_HP("hp", gbcodec_event_hp),
> +	[snd_soc_dapm_mic]	= SND_SOC_DAPM_MIC("mic", gbcodec_event_int_mic),

. . .

> @@ -1050,78 +1088,28 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module,
>   	strlcpy(temp_name, w->name, NAME_SIZE);
>   	snprintf(w->name, NAME_SIZE, "GB %d %s", module->dev_id, temp_name);
>   
> +	if (w->type > ARRAY_SIZE(gbaudio_widgets)) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +	*dw = gbaudio_widgets[w->type];
> +	dw->name = w->name;
> +
>   	switch (w->type) {
>   	case snd_soc_dapm_spk:
> -		*dw = (struct snd_soc_dapm_widget)
> -			SND_SOC_DAPM_SPK(w->name, gbcodec_event_spk);
>   		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);
>   		module->op_devices |= (GBAUDIO_DEVICE_OUT_WIRED_HEADSET
> -					| GBAUDIO_DEVICE_OUT_WIRED_HEADPHONE);
> +					| GBAUDIO_DEVICE_OUT_WIRED_HEADPHONE),

Please fix this (above) to preserve the original semicolon.

>   		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);
>   		module->ip_devices |= GBAUDIO_DEVICE_IN_BUILTIN_MIC;
>   		break;

. . .

WARNING: multiple messages have this Message-ID
From: Alex Elder <elder@ieee.org>
To: Arnd Bergmann <arnd@kernel.org>,
	Vaibhav Agarwal <vaibhav.sr@gmail.com>,
	Mark Greer <mgreer@animalcreek.com>,
	Johan Hovold <johan@kernel.org>, Alex Elder <elder@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Nathan Chancellor <natechancellor@gmail.com>,
	Nick Desaulniers <ndesaulniers@google.com>
Cc: devel@driverdev.osuosl.org, Arnd Bergmann <arnd@arndb.de>,
	clang-built-linux@googlegroups.com, linux-kernel@vger.kernel.org,
	Coiby Xu <coiby.xu@gmail.com>,
	greybus-dev@lists.linaro.org,
	Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [PATCH] staging: greybus: fix stack size warning with UBSAN
Date: Sun, 3 Jan 2021 20:18:47 -0600	[thread overview]
Message-ID: <d9c341c9-9005-b425-9dd8-e797869bbcb0@ieee.org> (raw)
In-Reply-To: <20210103223541.2790855-1-arnd@kernel.org>

On 1/3/21 4:35 PM, Arnd Bergmann 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.

This is good, but I have a few comments.

You took out the default case, and it seems you are using
a w->type value bigger than the initialization array to
determine validity.  But there are more values defined in
the snd_soc_dapm_type enumerated type than are explicitly
listed as cases in the switch statement.  So the switch
statement no longer treats some types as invalid (such
as snd_soc_dapm_demux).  Am I missing something?

You are setting explicit names, such as "spk", "hp",
"mic", etc. in the initialization array.  But you
update the name after (struct) assigning from the
array.  I have no real objection I guess, but why
bother?  Why not just use null pointers in the
initialization array?

You change a semicolon to a comma in one spot, and you
should not have.  I'll point that out below.

I like that you got rid of the type casts, which were
apparently unnecessary.

You dropped the break in the final case in the switch
statement, but in an earlier discussion I think we
concluded that wasn't a problem.

I guess the main thing is the first thing mentioned.


Thanks.

					-Alex

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/staging/greybus/audio_topology.c | 106 ++++++++++-------------
>   1 file changed, 47 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 96b8b29fe899..c03873915c20 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("spk", gbcodec_event_spk),
> +	[snd_soc_dapm_hp]	= SND_SOC_DAPM_HP("hp", gbcodec_event_hp),
> +	[snd_soc_dapm_mic]	= SND_SOC_DAPM_MIC("mic", gbcodec_event_int_mic),

. . .

> @@ -1050,78 +1088,28 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module,
>   	strlcpy(temp_name, w->name, NAME_SIZE);
>   	snprintf(w->name, NAME_SIZE, "GB %d %s", module->dev_id, temp_name);
>   
> +	if (w->type > ARRAY_SIZE(gbaudio_widgets)) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +	*dw = gbaudio_widgets[w->type];
> +	dw->name = w->name;
> +
>   	switch (w->type) {
>   	case snd_soc_dapm_spk:
> -		*dw = (struct snd_soc_dapm_widget)
> -			SND_SOC_DAPM_SPK(w->name, gbcodec_event_spk);
>   		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);
>   		module->op_devices |= (GBAUDIO_DEVICE_OUT_WIRED_HEADSET
> -					| GBAUDIO_DEVICE_OUT_WIRED_HEADPHONE);
> +					| GBAUDIO_DEVICE_OUT_WIRED_HEADPHONE),

Please fix this (above) to preserve the original semicolon.

>   		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);
>   		module->ip_devices |= GBAUDIO_DEVICE_IN_BUILTIN_MIC;
>   		break;

. . .
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

  reply	other threads:[~2021-01-04  2:19 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-03 22:35 [PATCH] staging: greybus: fix stack size warning with UBSAN Arnd Bergmann
2021-01-03 22:35 ` Arnd Bergmann
2021-01-04  2:18 ` Alex Elder [this message]
2021-01-04  2:18   ` Alex Elder
2021-01-04  6:22 ` Dan Carpenter
2021-01-04  6:22   ` Dan Carpenter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d9c341c9-9005-b425-9dd8-e797869bbcb0@ieee.org \
    --to=elder@ieee.org \
    --cc=arnd@arndb.de \
    --cc=arnd@kernel.org \
    --cc=clang-built-linux@googlegroups.com \
    --cc=coiby.xu@gmail.com \
    --cc=dan.carpenter@oracle.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=elder@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=greybus-dev@lists.linaro.org \
    --cc=johan@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mgreer@animalcreek.com \
    --cc=natechancellor@gmail.com \
    --cc=ndesaulniers@google.com \
    --cc=vaibhav.sr@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.