Add error messages to the probe call.
Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>

---

 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-&gt...
[ message continues ]

Add error messages to the probe call.
Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>

---

 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-...
[ message continues ]

On Tue, 18 Sep 2007 13:18:34 +0100 (BST)
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.

-

GCC used to complain: "`foo' might be used uninitialized..." and this is 
 Yes of course, barring the issue mentioned.  Note the message above is

not the same as: "`foo' is used uninitialized..." that would be reported 
 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

-

Hi Maciej,
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,
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_al...
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 :-)

-

Of course -- essentially GCC cannot really figure out whether all the

possible paths of execution include initialisation or not and complains 
 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 
 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

-

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

-

-----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
- --

_______________________________________
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-----
-

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?)
-

It's a driver for mips.

--

Martin Michlmayr

http://www.cyrius.com/

-