Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755299Ab3CEJFe (ORCPT <rfc822;w@1wt.eu>);
	Tue, 5 Mar 2013 04:05:34 -0500
Received: from cantor2.suse.de ([195.135.220.15]:57576 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753536Ab3CEJFR (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 5 Mar 2013 04:05:17 -0500
Date: Tue, 05 Mar 2013 10:05:15 +0100
Message-ID: <s5h8v62gqt0.wl%tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Christine Spang <christine.spang@oracle.com>
Cc: Jaroslav Kysela <perex@perex.cz>, alsa-devel@alsa-project.org,
        Jamie Iles <jamie.iles@oracle.com>,
        Sasha Levin <sasha.levin@oracle.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Make snd_BUG_ON() always evaluate and return the conditional expression.
In-Reply-To: <1362434579-7733-1-git-send-email-christine.spang@oracle.com>
References: <1362434579-7733-1-git-send-email-christine.spang@oracle.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.2
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3803
Lines: 101

At Mon,  4 Mar 2013 17:02:59 -0500,
Christine Spang wrote:
> 
> Having snd_BUG_ON() only evaluate its conditional when CONFIG_SND_DEBUG
> is set leads to frequent bugs, since other similar macros in the kernel
> have different behavior. Let's make snd_BUG_ON() act like those macros
> so it will stop being accidentally misused.
> 
> Signed-off-by: Christine Spang <christine.spang@oracle.com>

Sounds reasonable.  The dependency on CONFIG_SND_DEBUG was for
allowing more optimization, but since we use this for more places than
expected, this change would be safer indeed.

If no one has objection, I'll apply it for 3.10 kernel.


thanks,

Takashi

> ---
>  Documentation/DocBook/writing-an-alsa-driver.tmpl | 12 +++++-------
>  include/sound/core.h                              | 24 ++++++++---------------
>  2 files changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/DocBook/writing-an-alsa-driver.tmpl b/Documentation/DocBook/writing-an-alsa-driver.tmpl
> index bd6fee2..06741e9 100644
> --- a/Documentation/DocBook/writing-an-alsa-driver.tmpl
> +++ b/Documentation/DocBook/writing-an-alsa-driver.tmpl
> @@ -6164,14 +6164,12 @@ struct _snd_pcm_runtime {
>  
>        <para>
>          The macro takes an conditional expression to evaluate.
> -	When <constant>CONFIG_SND_DEBUG</constant>, is set, the
> -	expression is actually evaluated. If it's non-zero, it shows
> -	the warning message such as
> +	When <constant>CONFIG_SND_DEBUG</constant>, is set, if the
> +	expression is non-zero, it shows the warning message such as
>  	<computeroutput>BUG? (xxx)</computeroutput>
> -	normally followed by stack trace.  It returns the evaluated
> -	value.
> -	When no <constant>CONFIG_SND_DEBUG</constant> is set, this
> -	macro always returns zero.
> +	normally followed by stack trace.
> +
> +	In both cases it returns the evaluated value.
>        </para>
>  
>      </section>
> diff --git a/include/sound/core.h b/include/sound/core.h
> index 7cede2d..a63680b 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h
> @@ -379,18 +379,10 @@ void __snd_printk(unsigned int level, const char *file, int line,
>   * snd_BUG_ON - debugging check macro
>   * @cond: condition to evaluate
>   *
> - * When CONFIG_SND_DEBUG is set, this macro evaluates the given condition,
> - * and call WARN() and returns the value if it's non-zero.
> - * 
> - * When CONFIG_SND_DEBUG is not set, this just returns zero, and the given
> - * condition is ignored.
> - *
> - * NOTE: the argument won't be evaluated at all when CONFIG_SND_DEBUG=n.
> - * Thus, don't put any statement that influences on the code behavior,
> - * such as pre/post increment, to the argument of this macro.
> - * If you want to evaluate and give a warning, use standard WARN_ON().
> + * Has the same behavior as WARN_ON when CONFIG_SND_DEBUG is set,
> + * otherwise just evaluates the conditional and returns the value.
>   */
> -#define snd_BUG_ON(cond)	WARN((cond), "BUG? (%s)\n", __stringify(cond))
> +#define snd_BUG_ON(cond)	WARN_ON((cond))
>  
>  #else /* !CONFIG_SND_DEBUG */
>  
> @@ -400,11 +392,11 @@ __printf(2, 3)
>  static inline void _snd_printd(int level, const char *format, ...) {}
>  
>  #define snd_BUG()			do { } while (0)
> -static inline int __snd_bug_on(int cond)
> -{
> -	return 0;
> -}
> -#define snd_BUG_ON(cond)	__snd_bug_on(0 && (cond))  /* always false */
> +
> +#define snd_BUG_ON(condition) ({ \
> +	int __ret_warn_on = !!(condition); \
> +	unlikely(__ret_warn_on); \
> +})
>  
>  #endif /* CONFIG_SND_DEBUG */
>  
> -- 
> 1.8.2.rc0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/