Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
-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: >> On Wed, 19 Sep 2007, Andrew Morton wrote: >> 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.0 + +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c 2007-09-18 10:56:51.0 + @@ -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. > > >>> 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- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
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: > > Hi Maciej, > > > > > > On Thu, 20 Sep 2007, Maciej W. Rozycki wrote: > > > > > > On Wed, 19 Sep 2007, Andrew Morton wrote: > > > > > > > > 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. > > > > > > > > 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 :-) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
Hi Maciej, On Thu, 20 Sep 2007, Maciej W. Rozycki wrote: > > On Wed, 19 Sep 2007, Andrew Morton wrote: > > > > 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.0 + > > > +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c > > > 2007-09-18 10:56:51.0 + > > > @@ -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. > > 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 :-) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
On Wed, 19 Sep 2007, Andrew Morton wrote: > > 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.0 + > > +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c > > 2007-09-18 10:56:51.0 + > > @@ -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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
* Andrew Morton <[EMAIL PROTECTED]> [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/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
* Andrew Morton [EMAIL PROTECTED] [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/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
On Wed, 19 Sep 2007, Andrew Morton wrote: 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.0 + +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c 2007-09-18 10:56:51.0 + @@ -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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
Hi Maciej, On Thu, 20 Sep 2007, Maciej W. Rozycki wrote: On Wed, 19 Sep 2007, Andrew Morton wrote: 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.0 + +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c 2007-09-18 10:56:51.0 + @@ -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. 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 :-) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
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: Hi Maciej, On Thu, 20 Sep 2007, Maciej W. Rozycki wrote: On Wed, 19 Sep 2007, Andrew Morton wrote: 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. 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 :-) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
-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: On Wed, 19 Sep 2007, Andrew Morton wrote: 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.0 + +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c 2007-09-18 10:56:51.0 + @@ -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. 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- - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
On Tue, 18 Sep 2007 13:18:34 +0100 (BST) "Maciej W. Rozycki" <[EMAIL PROTECTED]> wrote: > Add error messages to the probe call. > > Signed-off-by: Maciej W. Rozycki <[EMAIL PROTECTED]> > --- > 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.0 + > +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c 2007-09-18 > 10:56:51.0 + > @@ -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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
On Tue, 18 Sep 2007 13:18:34 +0100 (BST) Maciej W. Rozycki [EMAIL PROTECTED] wrote: Add error messages to the probe call. Signed-off-by: Maciej W. Rozycki [EMAIL PROTECTED] --- 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.0 + +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c 2007-09-18 10:56:51.0 + @@ -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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
Add error messages to the probe call.
Signed-off-by: Maciej W. Rozycki <[EMAIL PROTECTED]>
---
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.0 +
+++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c 2007-09-18
10:56:51.0 +
@@ -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(>cmap, 256, 0) < 0)
+ if (fb_alloc_cmap(>cmap, 256, 0) < 0) {
+ printk(KERN_ERR "%s: Cannot allocate color map\n",
+ dev->bus_id);
+ err = -ENOMEM;
goto err_alloc;
+ }
info->fbops = _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)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
Add error messages to the probe call.
Signed-off-by: Maciej W. Rozycki [EMAIL PROTECTED]
---
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.0 +
+++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c 2007-09-18
10:56:51.0 +
@@ -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)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
Add error messages to the probe call.
Signed-off-by: Maciej W. Rozycki <[EMAIL PROTECTED]>
---
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.0 +
+++ linux-mips-2.6.18-20060920/drivers/video/pmag-ba-fb.c 2006-12-16
16:45:23.0 +
@@ -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(>cmap, 256, 0) < 0)
+ if (fb_alloc_cmap(>cmap, 256, 0) < 0) {
+ printk(KERN_ERR "%s: Cannot allocate color map\n",
+ dev->bus_id);
+ err = -ENOMEM;
goto err_alloc;
+ }
info->fbops = _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);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics
Add error messages to the probe call.
Signed-off-by: Maciej W. Rozycki [EMAIL PROTECTED]
---
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.0 +
+++ linux-mips-2.6.18-20060920/drivers/video/pmag-ba-fb.c 2006-12-16
16:45:23.0 +
@@ -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);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
