[PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

STALE6903d

10 messages, 5 authors, 2007-09-20 · open the first message on its own page

[PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

From: Maciej W. Rozycki <hidden>
Date: 2007-09-17 16:45:51 

 Add error messages to the probe call.

Signed-off-by: Maciej W. Rozycki <redacted>
---
 While they may rarely trigger, they may be useful when something weird is 
going on.  Also this is good style.

 Checked with checkpatch.pl and at the runtime.

 Please apply; don't be worried about the old version number.

  Maciej

patch-mips-2.6.18-20060920-pmag-ba-err-1
diff -up --recursive --new-file linux-mips-2.6.18-20060920.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.18-20060920/drivers/video/pmag-ba-fb.c
--- linux-mips-2.6.18-20060920.macro/drivers/video/pmag-ba-fb.c	2006-12-16 16:45:08.000000000 +0000
+++ linux-mips-2.6.18-20060920/drivers/video/pmag-ba-fb.c	2006-12-16 16:45:23.000000000 +0000
@@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
 	resource_size_t start, len;
 	struct fb_info *info;
 	struct pmagbafb_par *par;
+	int err = 0;
 
 	info = framebuffer_alloc(sizeof(struct pmagbafb_par), dev);
-	if (!info)
+	if (!info) {
+		printk(KERN_ERR "%s: Cannot allocate memory\n", dev->bus_id);
 		return -ENOMEM;
+	}
 
 	par = info->par;
 	dev_set_drvdata(dev, info);
 
-	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0)
+	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
+		printk(KERN_ERR "%s: Cannot allocate color map\n",
+		       dev->bus_id);
+		err = -ENOMEM;
 		goto err_alloc;
+	}
 
 	info->fbops = &pmagbafb_ops;
 	info->fix = pmagbafb_fix;
@@ -166,28 +173,41 @@ static int __init pmagbafb_probe(struct 
 	/* Request the I/O MEM resource.  */
 	start = tdev->resource.start;
 	len = tdev->resource.end - start + 1;
-	if (!request_mem_region(start, len, dev->bus_id))
+	if (!request_mem_region(start, len, dev->bus_id)) {
+		printk(KERN_ERR "%s: Cannot reserve FB region\n", dev->bus_id);
+		err = -EBUSY;
 		goto err_cmap;
+	}
 
 	/* MMIO mapping setup.  */
 	info->fix.mmio_start = start;
 	par->mmio = ioremap_nocache(info->fix.mmio_start, info->fix.mmio_len);
-	if (!par->mmio)
+	if (!par->mmio) {
+		printk(KERN_ERR "%s: Cannot map MMIO\n", dev->bus_id);
+		err = -ENOMEM;
 		goto err_resource;
+	}
 	par->dac = par->mmio + PMAG_BA_BT459;
 
 	/* Frame buffer mapping setup.  */
 	info->fix.smem_start = start + PMAG_BA_FBMEM;
 	info->screen_base = ioremap_nocache(info->fix.smem_start,
 					    info->fix.smem_len);
-	if (!info->screen_base)
+	if (!info->screen_base) {
+		printk(KERN_ERR "%s: Cannot map FB\n", dev->bus_id);
+		err = -ENOMEM;
 		goto err_mmio_map;
+	}
 	info->screen_size = info->fix.smem_len;
 
 	pmagbafb_erase_cursor(info);
 
-	if (register_framebuffer(info) < 0)
+	err = register_framebuffer(info);
+	if (err < 0) {
+		printk(KERN_ERR "%s: Cannot register framebuffer\n",
+		       dev->bus_id);
 		goto err_smem_map;
+	}
 
 	get_device(dev);

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

From: Maciej W. Rozycki <hidden>
Date: 2007-09-18 12:19:28 

 Add error messages to the probe call.

Signed-off-by: Maciej W. Rozycki <redacted>
---
 While they may rarely trigger, they may be useful when something weird is 
going on.  Also this is good style.

 This is an updated version that addresses an issue raised by Mariusz 
Kozlowski for the sibling patch.  Checked with checkpatch.pl.

 Please apply.

  Maciej

patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
--- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
+++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
@@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
 	resource_size_t start, len;
 	struct fb_info *info;
 	struct pmagbafb_par *par;
+	int err = 0;
 
 	info = framebuffer_alloc(sizeof(struct pmagbafb_par), dev);
-	if (!info)
+	if (!info) {
+		printk(KERN_ERR "%s: Cannot allocate memory\n", dev->bus_id);
 		return -ENOMEM;
+	}
 
 	par = info->par;
 	dev_set_drvdata(dev, info);
 
-	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0)
+	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
+		printk(KERN_ERR "%s: Cannot allocate color map\n",
+		       dev->bus_id);
+		err = -ENOMEM;
 		goto err_alloc;
+	}
 
 	info->fbops = &pmagbafb_ops;
 	info->fix = pmagbafb_fix;
@@ -166,28 +173,41 @@ static int __init pmagbafb_probe(struct 
 	/* Request the I/O MEM resource.  */
 	start = tdev->resource.start;
 	len = tdev->resource.end - start + 1;
-	if (!request_mem_region(start, len, dev->bus_id))
+	if (!request_mem_region(start, len, dev->bus_id)) {
+		printk(KERN_ERR "%s: Cannot reserve FB region\n", dev->bus_id);
+		err = -EBUSY;
 		goto err_cmap;
+	}
 
 	/* MMIO mapping setup.  */
 	info->fix.mmio_start = start;
 	par->mmio = ioremap_nocache(info->fix.mmio_start, info->fix.mmio_len);
-	if (!par->mmio)
+	if (!par->mmio) {
+		printk(KERN_ERR "%s: Cannot map MMIO\n", dev->bus_id);
+		err = -ENOMEM;
 		goto err_resource;
+	}
 	par->dac = par->mmio + PMAG_BA_BT459;
 
 	/* Frame buffer mapping setup.  */
 	info->fix.smem_start = start + PMAG_BA_FBMEM;
 	info->screen_base = ioremap_nocache(info->fix.smem_start,
 					    info->fix.smem_len);
-	if (!info->screen_base)
+	if (!info->screen_base) {
+		printk(KERN_ERR "%s: Cannot map FB\n", dev->bus_id);
+		err = -ENOMEM;
 		goto err_mmio_map;
+	}
 	info->screen_size = info->fix.smem_len;
 
 	pmagbafb_erase_cursor(info);
 
-	if (register_framebuffer(info) < 0)
+	err = register_framebuffer(info);
+	if (err < 0) {
+		printk(KERN_ERR "%s: Cannot register framebuffer\n",
+		       dev->bus_id);
 		goto err_smem_map;
+	}
 
 	get_device(dev);
 
@@ -211,7 +231,7 @@ err_cmap:
 
 err_alloc:
 	framebuffer_release(info);
-	return -ENXIO;
+	return err;
 }
 
 static int __exit pmagbafb_remove(struct device *dev)

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

From: Andrew Morton <akpm@linux-foundation.org>
Date: 2007-09-20 00:24:44 

On Tue, 18 Sep 2007 13:18:34 +0100 (BST)
"Maciej W. Rozycki" [off-list ref] wrote:
quoted hunk
 Add error messages to the probe call.

Signed-off-by: Maciej W. Rozycki <redacted>
---
 While they may rarely trigger, they may be useful when something weird is 
going on.  Also this is good style.

 This is an updated version that addresses an issue raised by Mariusz 
Kozlowski for the sibling patch.  Checked with checkpatch.pl.

 Please apply.

  Maciej

patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
--- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
+++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
@@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
 	resource_size_t start, len;
 	struct fb_info *info;
 	struct pmagbafb_par *par;
+	int err = 0;
This initialisation to zero is not good.

Because if some error-path code forgot to do `err = -EFOO' then probe()
will return zero and the driver will leave things in half-initialised state
and will then proceed as if things had succeeded.  It will crash.

So it's better to leave this local uninitialised, because we really want to
get that compiler warning if someone forgot to set the return value.

I made that change, but am too stupid to be able to work out how to create
a config which will let me compile this thing.

akpm:/usr/src/25> grep PMAG arch/arm/configs/*
akpm:/usr/src/25> 

bah.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

From: Martin Michlmayr <hidden>
Date: 2007-09-20 06:16:41 

* Andrew Morton [off-list ref] [2007-09-19 17:24]:
I made that change, but am too stupid to be able to work out how to create
a config which will let me compile this thing.

akpm:/usr/src/25> grep PMAG arch/arm/configs/*
akpm:/usr/src/25> 
It's a driver for mips.
-- 
Martin Michlmayr
http://www.cyrius.com/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

From: Maciej W. Rozycki <hidden>
Date: 2007-09-20 12:53:03 

On Wed, 19 Sep 2007, Andrew Morton wrote:
quoted
patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
--- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
+++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
@@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
 	resource_size_t start, len;
 	struct fb_info *info;
 	struct pmagbafb_par *par;
+	int err = 0;
This initialisation to zero is not good.

Because if some error-path code forgot to do `err = -EFOO' then probe()
will return zero and the driver will leave things in half-initialised state
and will then proceed as if things had succeeded.  It will crash.
 GCC used to complain: "`foo' might be used uninitialized..." and this is 
the usual cure; let me see if this not the case anymore (I have 4.1.2).

So it's better to leave this local uninitialised, because we really want to
get that compiler warning if someone forgot to set the return value.
 Yes of course, barring the issue mentioned.  Note the message above is 
not the same as: "`foo' is used uninitialized..." that would be reported 
in the case which you are concerned of.

I made that change, but am too stupid to be able to work out how to create
a config which will let me compile this thing.

akpm:/usr/src/25> grep PMAG arch/arm/configs/*
akpm:/usr/src/25> 
 TURBOchannel is currently MIPS only:

$ grep PMAG arch/mips/configs/*
arch/mips/configs/decstation_defconfig:# CONFIG_FB_PMAG_AA is not set
arch/mips/configs/decstation_defconfig:CONFIG_FB_PMAG_BA=y
arch/mips/configs/decstation_defconfig:CONFIG_FB_PMAGB_B=y
$

 Thanks for your review.

  Maciej

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

From: Satyam Sharma <hidden>
Date: 2007-09-20 13:40:39 

Hi Maciej,


On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
On Wed, 19 Sep 2007, Andrew Morton wrote:
quoted
quoted
patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
--- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
+++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
@@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
 	resource_size_t start, len;
 	struct fb_info *info;
 	struct pmagbafb_par *par;
+	int err = 0;
This initialisation to zero is not good.

Because if some error-path code forgot to do `err = -EFOO' then probe()
will return zero and the driver will leave things in half-initialised state
and will then proceed as if things had succeeded.  It will crash.
 GCC used to complain: "`foo' might be used uninitialized..." and this is 
the usual cure; let me see if this not the case anymore (I have 4.1.2).
Even so, initializing to zero isn't quite good. You could use the
uninitialized_var() (once you've confirmed that the warning is bogus).
However, some maintainers may still nack uninitialized_var() usage,
quite legitimately.
quoted
So it's better to leave this local uninitialised, because we really want to
get that compiler warning if someone forgot to set the return value.
 Yes of course, barring the issue mentioned.  Note the message above is 
not the same as: "`foo' is used uninitialized..." that would be reported 
in the case which you are concerned of.
Firstly, "may be used uninitialized" can still be a bug.

Secondly, latest gcc is *horribly* buggy (and has been so for last several
releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501

We'd been hurling all sorts of abuses on gcc for quite long (when it fails
to detect these "false positive" cases), but now, it turns out it is quite
easy to write *genuinely* buggy code that still won't get any warnings,
neither the "is used" nor "may be used" one!

In short, there are three ways to fix these false positive warnings:

1. Do nothing, there are enough "uninitialized variable" warnings anyway,
   and hopefully, one day GCC would clean up its act.

2. Use uninitialized_var() to shut it up (only if it's genuinely bogus).

3. Do something like the following legendary patch [1]:

http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch

i.e., explicitly change the structure/logic of the function to make it
obvious enough to gcc that the variable will not be used uninitialized.


Satyam

[1] That was a funny case -- the alpha linux maintainer is also a gcc
    maintainer. Alpha even sets -Werror, so either he had to fix the
    kernel code that produced the warning, or go fix GCC to not warn
    about it -- he chose the former :-)

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

From: Maciej W. Rozycki <hidden>
Date: 2007-09-20 14:04:41 

Hi Satyam,

Firstly, "may be used uninitialized" can still be a bug.
 Of course -- essentially GCC cannot really figure out whether all the 
possible paths of execution include initialisation or not and complains 
just in case.

Secondly, latest gcc is *horribly* buggy (and has been so for last several
releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501

We'd been hurling all sorts of abuses on gcc for quite long (when it fails
to detect these "false positive" cases), but now, it turns out it is quite
easy to write *genuinely* buggy code that still won't get any warnings,
neither the "is used" nor "may be used" one!
 GCC for MIPS used to be problematic enough elsewhere I do not want to 
turn back.  Even 4.0.x generates bad code, e.g. fs/partitions/msdos.c gets 
miscompiled for the big endianness (but not for the little one!).  
Compared to that some useless warnings are negligible.  This 4.1.2 version 
has triggered no problems with the kernel yet (though I suspect it is 
still so-so -- e.g. gmp gets miscompiled; which used to be fine with 
4.0.x, oddly enough).

In short, there are three ways to fix these false positive warnings:

1. Do nothing, there are enough "uninitialized variable" warnings anyway,
   and hopefully, one day GCC would clean up its act.

2. Use uninitialized_var() to shut it up (only if it's genuinely bogus).

3. Do something like the following legendary patch [1]:

http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch

i.e., explicitly change the structure/logic of the function to make it
obvious enough to gcc that the variable will not be used uninitialized.
 Perhaps preinitialising to an error value such as -EINVAL would be of 
more sense.  This way any error paths lacking initialisation are still 
reported as errors, even though the classification might be wrong.  In 
fact more exotic one might be chosen (the glibc manual has some nice 
proposals if none of these we currently define fits) so the mistake is 
more obvious.

  Maciej

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

From: Satyam Sharma <hidden>
Date: 2007-09-20 14:04:57 


On Thu, 20 Sep 2007, Markus Gothe wrote:
GCC 4.1.2 has been stable for a long time now, maybe you better
upgrade your binutils instead...
I'd been using 4.2.1 -- I don't want to downgrade to 4.1.2. (btw from
the discussion on gcc's bugzilla it appears the bug wasn't resolved
in 4.1.2 either?)

Satyam

Satyam Sharma wrote:
quoted
Hi Maciej,


On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
quoted
On Wed, 19 Sep 2007, Andrew Morton wrote:
quoted
This initialisation to zero is not good.

Because if some error-path code forgot to do `err = -EFOO' then
probe() will return zero and the driver will leave things in
half-initialised state and will then proceed as if things had
succeeded.  It will crash.
GCC used to complain: "`foo' might be used uninitialized..." and
this is the usual cure; let me see if this not the case anymore
(I have 4.1.2).
Even so, initializing to zero isn't quite good. You could use the
uninitialized_var() (once you've confirmed that the warning is
bogus). However, some maintainers may still nack
uninitialized_var() usage, quite legitimately.
quoted
quoted
So it's better to leave this local uninitialised, because we
really want to get that compiler warning if someone forgot to
set the return value.
Yes of course, barring the issue mentioned.  Note the message
above is not the same as: "`foo' is used uninitialized..." that
would be reported in the case which you are concerned of.
Firstly, "may be used uninitialized" can still be a bug.

Secondly, latest gcc is *horribly* buggy (and has been so for last
several releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501

We'd been hurling all sorts of abuses on gcc for quite long (when
it fails to detect these "false positive" cases), but now, it turns
out it is quite easy to write *genuinely* buggy code that still
won't get any warnings, neither the "is used" nor "may be used"
one!

In short, there are three ways to fix these false positive
warnings:

1. Do nothing, there are enough "uninitialized variable" warnings
anyway, and hopefully, one day GCC would clean up its act.

2. Use uninitialized_var() to shut it up (only if it's genuinely
bogus).

3. Do something like the following legendary patch [1]:

http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch


i.e., explicitly change the structure/logic of the function to make
it obvious enough to gcc that the variable will not be used
uninitialized.


Satyam

[1] That was a funny case -- the alpha linux maintainer is also a
gcc maintainer. Alpha even sets -Werror, so either he had to fix
the kernel code that produced the warning, or go fix GCC to not
warn about it -- he chose the former :-)

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

From: Markus Gothe <hidden>
Date: 2007-09-20 14:14:32 

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

GCC 4.1.2 has been stable for a long time now, maybe you better
upgrade your binutils instead...

//Markus

Satyam Sharma wrote:
Hi Maciej,


On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
quoted
On Wed, 19 Sep 2007, Andrew Morton wrote:
quoted
quoted
patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2 diff -up
--recursive --new-file
linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c
linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c ---
linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c
2007-02-21 05:56:47.000000000 +0000 +++
linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
2007-09-18 10:56:51.000000000 +0000 @@ -147,16 +147,23 @@
static int __init pmagbafb_probe(struct resource_size_t
start, len; struct fb_info *info; struct pmagbafb_par *par; +
int err = 0;
This initialisation to zero is not good.

Because if some error-path code forgot to do `err = -EFOO' then
probe() will return zero and the driver will leave things in
half-initialised state and will then proceed as if things had
succeeded.  It will crash.
GCC used to complain: "`foo' might be used uninitialized..." and
this is the usual cure; let me see if this not the case anymore
(I have 4.1.2).
Even so, initializing to zero isn't quite good. You could use the
uninitialized_var() (once you've confirmed that the warning is
bogus). However, some maintainers may still nack
uninitialized_var() usage, quite legitimately.
quoted
quoted
So it's better to leave this local uninitialised, because we
really want to get that compiler warning if someone forgot to
set the return value.
Yes of course, barring the issue mentioned.  Note the message
above is not the same as: "`foo' is used uninitialized..." that
would be reported in the case which you are concerned of.
Firstly, "may be used uninitialized" can still be a bug.

Secondly, latest gcc is *horribly* buggy (and has been so for last
several releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501

We'd been hurling all sorts of abuses on gcc for quite long (when
it fails to detect these "false positive" cases), but now, it turns
out it is quite easy to write *genuinely* buggy code that still
won't get any warnings, neither the "is used" nor "may be used"
one!

In short, there are three ways to fix these false positive
warnings:

1. Do nothing, there are enough "uninitialized variable" warnings
anyway, and hopefully, one day GCC would clean up its act.

2. Use uninitialized_var() to shut it up (only if it's genuinely
bogus).

3. Do something like the following legendary patch [1]:

http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch


i.e., explicitly change the structure/logic of the function to make
it obvious enough to gcc that the variable will not be used
uninitialized.


Satyam

[1] That was a funny case -- the alpha linux maintainer is also a
gcc maintainer. Alpha even sets -Werror, so either he had to fix
the kernel code that produced the warning, or go fix GCC to not
warn about it -- he chose the former :-)

- --
_______________________________________

Mr Markus Gothe
Software Engineer

Phone: +46 (0)13 21 81 20 (ext. 1046)
Fax: +46 (0)13 21 21 15
Mobile: +46 (0)73 718 72 80
Diskettgatan 11, SE-583 35 Linköping, Sweden
www.27m.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFG8nuh6I0XmJx2NrwRCIX3AJ9c1HwLMWXV6SFb/WmJ2zFR0xbJxgCeLrIb
g7N9BsOQhRre5iDf6hFcXF0=
=VXuc
-----END PGP SIGNATURE-----

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

From: Satyam Sharma <hidden>
Date: 2007-09-20 14:21:50 


On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
 Perhaps preinitialising to an error value such as -EINVAL would be of
more sense.  This way any error paths lacking initialisation are still
reported as errors, even though the classification might be wrong.
Eeee ... at least I wouldn't prefer that. Why not simply use the
"int x = x;" trick (which is what uninitialized_var() does) -- it shuts
up the warning, and does *nothing* else. The bug will not be hidden, if
there's bad misbehaviour happening due to the bug, it will continue to
happen that way -- thus bringing our attention to it. Pre-initializing
to -EINVAL (or whatever) has the problem that when the bug actually
triggers, something unrelated might happen higher up the callchain, and
we'd be scratching our heads in a "why are we getting a -EINVAL here?"
kind of way ... worse still, we might think that this was _really_ an
EINVAL and go about debugging it ...

Plus, pre-initializing to -EINVAL (or even 0) will waste some bytes in
kernel text size, but no such overhead with uninitialized_var() :-)


Satyam
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help