login
Search
Signed-off-by: Steven Noonan <steven@uplinklabs.net> --- sound/core/pcm_native.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index c487025..3953bf6 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3252,7 +3252,7 @@ static int snd_pcm_fasync(int fd, struct file * file, int on) runtime = substream->runtime; err = fasync_helper(fd, file, on, &runtime->fasync); -out: + unlock_kernel(); if (err < 0) return err; -- 1.6.0.2 --
Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
drivers/mmc/host/sdhci.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e3a8133..6257677 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
{
unsigned long flags;
size_t blksize, len, chunk;
- u32 scratch;
+ u32 uninitialized_var(scratch);
u8 *buf;
DBG("PIO reading\n");
--
1.6.0.2
--
Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
drivers/serial/8250.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 356c2a2..b51e91e 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1599,7 +1599,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
static void serial_unlink_irq_chain(struct uart_8250_port *up)
{
- struct irq_info *i;
+ struct irq_info *uninitialized_var(i);
struct hlist_node *n;
struct hlist_head *h;
--
1.6.0.2
--
( Alan Cc:-ed. Seems like gcc bogosity - so your solution of using uninitialized_var() is the correct way to annotate this. ) --
Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
drivers/serial/8250.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 356c2a2..b51e91e 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1599,7 +1599,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
static void serial_unlink_irq_chain(struct uart_8250_port *up)
{
- struct irq_info *i;
+ struct irq_info *uninitialized_var(i);
struct hlist_node *n;
struct hlist_head *h;
--
1.6.0.2
--
the change is obvious to you, but it's useful to put an analysis into the changelog. Something like: serial_unlink_irq_chain() does not initialize iterator 'i', and that is correct logically because it is always initialized due to XYZ. Gcc does not realize this connection and emits a false warning. Annotate it with uninitialized_var(). and fill in XYZ. Doing such changelogs is useful to maintainers: they'll see that you havent just squashed a warning you noticed, you understood the code and determined it via review that the warning is GCC's fault, not the kernel's. with an empty changelog the maintainer will have to do this himself. (and can easily put your patch to the tail of a very long TODO list, or outright skip your patch.) Ingo --
I was kind of worried about being exceedingly verbose, but I understand your point perfectly. I'm going to resend the patch with an appropriately verbose comment. As always, I appreciate the criticism. Thank you! :) - Steven --
serial_unlink_irq_chain() does not initialize iterator 'i', and that is
correct logically because it is always initialized, either in the
hlist_for_each or in the conditional immediately after (which fires if
hlist_for_each comes up empty-handed). GCC does not realize this
connection and emits a false warning. Annotate it with uninitialized_var().
Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
drivers/serial/8250.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 356c2a2..4950ee5 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1551,7 +1551,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
{
struct hlist_head *h;
struct hlist_node *n;
- struct irq_info *i;
+ struct irq_info *uninitialized_var(i);
int ret, irq_flags = up->port.flags & UPF_SHARE_IRQ ? IRQF_SHARED : 0;
mutex_lock(&hash_mutex);
--
1.6.0.2
--
On Wed, 1 Oct 2008 01:47:07 -0700 Ok definitive NAK. gcc 4.3.0 can work this out and doesn't produce a warning. Thanks for sending the patch though (and to the gcc folks for rendering it unnecessary) Alan --
thanks for sorting it out! A CONFIG_CC_DISABLE_WARNING_ANNOTATIONS=y might be useful as well, which could be used periodically (by gcc folks) to check which warnings are hidden by annotations? Ingo --
I have yet to meet a too verbose commit log, and i've seen many - so there's basically no way you can stretch. Our problem 99% of the time is that commit logs are either not verbose at all, or are structured in a way that makes it hard to interpret them. If you think a change is too verbose, you could start using the 'Impact:' line convention we recently started using in the x86 tree. Something like: serial, 8250.c: fix warning: 'i' may be used uninitialized Impact: cleanup, fix bogus gcc warning serial_unlink_irq_chain() does not initialize iterator 'i', and that is correct logically because it is always initialized due to XYZ. Gcc does not realize this connection and emits a false warning. Annotate it with uninitialized_var(). Signed-off-by: Steven Noonan <steven@uplinklabs.net> Other good 'Impact:' tags are: Impact: fix boot crash Impact: style cleanup Impact: documentation fix it's a "see impact at a glance" kind of thing. Maintainers will still read the rest and the code as well, but the thought process is much smoother: the maintainer can concentrate on "does what the patch does meet the expectation spelled out in the changelog", instead of spending time on "what does this patch do" thinking. Ingo --
On Wed, 1 Oct 2008 01:16:57 -0700 I'd rather have the warning than that particular ugly in the source of the serial/tty code. It doesn't fit the pattern of attributes used elsewhere in the code (__unused __user etc). Alan --
On Wed, 1 Oct 2008 10:01:50 +0200 Sorry but uninitialized_var() is too ugly to be acceptable. Please use a proper __foo style notation for consistency with the rest of the kernel (Besides which last time I checked current gcc could figure that one out) Alan --
it cannot be wrapped like that via __foo style notation (which can be used for section tricks), and uninitialized_var() is the accepted upstream facility for this, it got introduced 1.5 years ago, via commit that would indeed moot the patch. Ingo --
Any particular reason that #define __used __attribute__((used)) I'll re-check that before rejecting the patch definitively just to be sure. Alan --
ah, if that works then it's a very nice idea! Ingo --
Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
drivers/mmc/host/sdhci.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e3a8133..6257677 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
{
unsigned long flags;
size_t blksize, len, chunk;
- u32 scratch;
+ u32 uninitialized_var(scratch);
u8 *buf;
DBG("PIO reading\n");
--
1.6.0.2
--
A bit of a further explanation: The variable 'scratch' is always initialized before it's used. The conditional which is responsible for initialization of 'scratch' will always evaluate 'true' when the first loop iteration occurs, and thus, it's properly initialized. GCC doesn't see this, of course, so using the uninitialized_var() macro seems to work for silencing this case. - Steven --
another small workflow suggestion: when you add explanation for the changelog it's better to just include the updated patch (dont worry about the duplication) - that way maintainers dont have to cut&slice the patch and the description toghether. [which is not just a matter of wasting time, but if you do tons of patch juggling, it becomes a real risk factor.] Ingo --
The variable 'scratch' is always initialized before it's used. The
conditional which is responsible for initialization of 'scratch' will
always evaluate 'true' when the first loop iteration occurs, and thus,
it's properly initialized. GCC doesn't see this, of course, so using
the uninitialized_var() macro seems to work for silencing this case.
Signed-off-by: Steven Noonan <steven@uplinklabs.net>
---
drivers/mmc/host/sdhci.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e3a8133..6257677 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -177,7 +177,7 @@ static void sdhci_read_block_pio(struct sdhci_host *host)
{
unsigned long flags;
size_t blksize, len, chunk;
- u32 scratch;
+ u32 uninitialized_var(scratch);
u8 *buf;
DBG("PIO reading\n");
--
1.6.0.2
--
On Wed, 1 Oct 2008 01:50:25 -0700
Queued, thanks.
Rgds
--=20
-- Pierre Ossman
WARNING: This correspondence is being monitored by the
Swedish government. Make sure your server uses encryption
for SMTP traffic and consider using PGP for end-to-end
encryption.
With which gcc version?
I'm not getting this warning with gcc 4.3, and IMHO it doesn't make
sense to clutter the source code with such workarounds for older gcc
versions (we officially support 6 years old compilers, and warning-free
compilations with all of them are not reasonably possible).
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
I've seen it on GCC 4.1 and 4.2. Since lots of distributions still haven't marked GCC >4.1 stable, it makes sense to me to kill warnings for GCC 4.1 and above. I don't know of any current distribution releases using less than GCC 4.1 at the moment. - Steven --
It will clutter our code with these workarounds forever.
And due to silencing these false warnings we will no longer get a
warning when one of them becomes a real bug.
Working on the remaining warnings that are visible with gcc 4.3 is a
worthwhile goal, but I see no point for silencing some warnings that
only occur with older gcc versions (especially as long as warnings
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
I feel like there's a logical fallacy here. Sure, we can fix GCC 4.3 warnings, but what about when GCC 4.3 becomes an "old version"? uninitialized_var and other such workarounds will still exist in the code. It seems like the logical progression of your argument should be to never fix false warnings. - Steven --
There is no logical fallacy here - getting a warning-free compilation
with the latest gcc (so that new warnings get more obvious) makes sense,
but workarounds for warnings only present with older gcc versions are
not worth the price.
If compilation with gcc 4.3 was always warning-free you might have a
point, but considering that it's unlikely that we get all warnings with
gcc 4.3 fixed in the forseeable future [1] it would make more sense to
fix one of the non-trivial warnings present with all gcc version than
clutter the code with workarounds for warnings only present with older
cu
Adrian
[1] e.g. the MCA legacy stuff gives #warning's since
more than 5 years (sic)
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
Correct. Would you be interested in sending a patch for a (default-off) debug feature that allows the disabling of all the gcc annotations? That way we can do regular sweeps to determine whether old annotations are still relevant on latest and greatest GCC. Something like CONFIG_CC_DEBUG_ALLOW_WARNINGS=y in lib/Kconfig.debug, then use that to #ifdef the uninitialized_var() include/linux/compiler-gcc[34].h? Also, please try Alan's suggestion as well: does the __attribute_ ((unused)) trick work equally well? If yes then please introduce a __annotate_initialized tag instead of the weird-looking uninitialized_var() construct. Ingo --
On Mon, 6 Oct 2008 08:30:35 +0200
How would you find the ones that are no longer needed though?
Perhaps we could make it so that uninitialized_var() itself emits a
warning. That way you could turn that option on and check that you
always have pairs of warnings.
Rgds
--=20
-- Pierre Ossman
WARNING: This correspondence is being monitored by the
Swedish government. Make sure your server uses encryption
for SMTP traffic and consider using PGP for end-to-end
encryption.
ideally gcc should not emit _any_ bogus warning. Life is too short to comb through crappy warnings and to keep in mind which ones are relevant and which ones are not. compiler-intel.h does not make use of the ok - when CONFIG_CC_DEBUG_ALLOW_WARNINGS=y - and then the work flow would be to go for single-occurance warnings and eliminate them (because their annotation is moot). Ingo --
(Cc:-ed Taskashi for this ALSA cleanup patch.) --
Signed-off-by: Steven Noonan <steven@uplinklabs.net> --- sound/core/pcm_native.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index c487025..3953bf6 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3252,7 +3252,7 @@ static int snd_pcm_fasync(int fd, struct file * file, int on) runtime = substream->runtime; err = fasync_helper(fd, file, on, &runtime->fasync); -out: + unlock_kernel(); if (err < 0) return err; -- 1.6.0.2 --
At Wed, 1 Oct 2008 09:59:21 +0200, This label is used when the debug option is enabled. So, don't apply this. thanks, --
ah, indeed:
snd_assert(substream != NULL, goto out);
if then i'd suggest to clean it up like this:
if (snd_assert(substream != NULL))
goto out;
this is how DEBUG_LOCKS_WARN_ON() is used in kernel/lockdep.c for
example. Putting code flow side-effects into debug macros looks a bit
weird.
Ingo
--
At Wed, 1 Oct 2008 10:32:24 +0200, Yeah, already a similar change was done in the latest ALSA tree :) thanks, Takashi --
