Look Ma, da kernel is b0rken

Look Ma, da kernel is b0rken

[Andreas Mohr]

Hi,

drivers/pnp/pnpacpi/core.c: In function 'ispnpidacpi':
drivers/pnp/pnpacpi/core.c:65:2: warning: logical 'or' of collectively
exhaustive tests is always true [-Wlogical-op]
drivers/pnp/pnpacpi/core.c:66:2: warning: logical 'or' of collectively
exhaustive tests is always true [-Wlogical-op]
drivers/pnp/pnpacpi/core.c:67:2: warning: logical 'or' of collectively
exhaustive tests is always true [-Wlogical-op]


That's already the second less enticing -Wlogical-op issue
which was discovered by accident during less than two days
of my happy(?) activity of kernel suspend breakage bisection.


Why oh why, as a rather *very* critical piece of software,
can't the kernel use sufficiently aggressive warning levels *by default*??
IMHO it's simply NOT ACCEPTABLE to have such sloppiness creep into
the daily bandwagon of kernel development life
(or should I say: being mandated to creep in?).

Result: whichever default warning level you set
*will* end up as The New Normal,
and all those warnings which then remain able to rear their ugly heads
according to the chosen default level
will be fixed by the community eventually,
and *most others won't* (or at least not in time).

The amount of warnings spewn by make W=3 (or even W=2) is simply
shocking IMNSHO.
And there can always be an argument that most of such warnings
are fixable. If not directly (e.g. because analysis of that warning type
is partially unreliable), then by actively reworking code
into something slightly different.

So, unless there are very hard and *justified* reasons
for keeping builds at such lame-*ss defaults
(such as automated compliance test runs which may not fail -
but in such cases one could argue that *those* uses
should then be required to manually lessen *their* warning level settings),
I would strongly vote for having a hard discussion about the status
quo.


As a somewhat aggravating comment, please note that this warning
actually seems to date back to 1da177e (initial repository build)
according to blame on that file.

Andreas Mohr

P.S.: sorry for the subject line ;)

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Wed, Dec 05, 2012 at 08:09:01AM +0100, Andreas Mohr wrote:
> Hi,
> 
> drivers/pnp/pnpacpi/core.c: In function 'ispnpidacpi':
> drivers/pnp/pnpacpi/core.c:65:2: warning: logical 'or' of collectively
> exhaustive tests is always true [-Wlogical-op]
> drivers/pnp/pnpacpi/core.c:66:2: warning: logical 'or' of collectively
> exhaustive tests is always true [-Wlogical-op]
> drivers/pnp/pnpacpi/core.c:67:2: warning: logical 'or' of collectively
> exhaustive tests is always true [-Wlogical-op]
> 
> 
> That's already the second less enticing -Wlogical-op issue
> which was discovered by accident during less than two days
> of my happy(?) activity of kernel suspend breakage bisection.
> 
> 
> Why oh why, as a rather *very* critical piece of software,
> can't the kernel use sufficiently aggressive warning levels *by default*??
> IMHO it's simply NOT ACCEPTABLE to have such sloppiness creep into
> the daily bandwagon of kernel development life
> (or should I say: being mandated to creep in?).
> 
> Result: whichever default warning level you set
> *will* end up as The New Normal,
> and all those warnings which then remain able to rear their ugly heads
> according to the chosen default level
> will be fixed by the community eventually,
> and *most others won't* (or at least not in time).
> 
> The amount of warnings spewn by make W=3 (or even W=2) is simply
> shocking IMNSHO.
> And there can always be an argument that most of such warnings
> are fixable. If not directly (e.g. because analysis of that warning type
> is partially unreliable), then by actively reworking code
> into something slightly different.

Can we all relax ourselves first?

Now, I admit that ispnpidacpi() is a rather clumsy way of testing
string sanity but whatever, it is there.

Then, we added W= exactly for inquisitive and adventurous people like
yourself to build kernel compilation units with, to *verify* the
*sanity* of the compiler's error messages (yes, gcc has been wrong a lot
in the past too) and fix them.

So why not write a patch that fixes this, *test* it correctly and then
submit it to the proper maintainer instead of ranting about it without
any constructive help and thus piss off everybody on the ML?

And yes, bugs happen and this is the reality of it - programmers are
people and they make mistakes too. So I think helping out fix bugs is a
much more constructive way of spending your time instead of pissing off
the only people who can actually help you with your problem.

HTH.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Alan Cox]

On Wed, 5 Dec 2012 15:29:35 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Wed, Dec 05, 2012 at 08:09:01AM +0100, Andreas Mohr wrote:
> > Hi,
> > 
> > drivers/pnp/pnpacpi/core.c: In function 'ispnpidacpi':
> > drivers/pnp/pnpacpi/core.c:65:2: warning: logical 'or' of collectively
> > exhaustive tests is always true [-Wlogical-op]
> > drivers/pnp/pnpacpi/core.c:66:2: warning: logical 'or' of collectively
> > exhaustive tests is always true [-Wlogical-op]
> > drivers/pnp/pnpacpi/core.c:67:2: warning: logical 'or' of collectively
> > exhaustive tests is always true [-Wlogical-op]
> > 
> > 
> > That's already the second less enticing -Wlogical-op issue
> > which was discovered by accident during less than two days

No it's not. It's been reported in bugzilla. I sent patches ages ago.
They were ignored. Coverity has had it tagged for years (and a ton more
of them you've not noticed yet)

http://article.gmane.org/gmane.linux.acpi.devel/56753/match=test_alpha

This isn't discovered, this is in the "If you stick your fingers in your
ears and hum you can't hear the screaming" category.

Alan

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Wed, Dec 05, 2012 at 03:27:56PM +0000, Alan Cox wrote:
> On Wed, 5 Dec 2012 15:29:35 +0100
> Borislav Petkov <bp@alien8.de> wrote:
> 
> > On Wed, Dec 05, 2012 at 08:09:01AM +0100, Andreas Mohr wrote:
> > > Hi,
> > > 
> > > drivers/pnp/pnpacpi/core.c: In function 'ispnpidacpi':
> > > drivers/pnp/pnpacpi/core.c:65:2: warning: logical 'or' of collectively
> > > exhaustive tests is always true [-Wlogical-op]
> > > drivers/pnp/pnpacpi/core.c:66:2: warning: logical 'or' of collectively
> > > exhaustive tests is always true [-Wlogical-op]
> > > drivers/pnp/pnpacpi/core.c:67:2: warning: logical 'or' of collectively
> > > exhaustive tests is always true [-Wlogical-op]
> > > 
> > > 
> > > That's already the second less enticing -Wlogical-op issue
> > > which was discovered by accident during less than two days
> 
> No it's not. It's been reported in bugzilla. I sent patches ages ago.
> They were ignored. Coverity has had it tagged for years (and a ton more
> of them you've not noticed yet)
> 
> http://article.gmane.org/gmane.linux.acpi.devel/56753/match=test_alpha
> 
> This isn't discovered, this is in the "If you stick your fingers in your
> ears and hum you can't hear the screaming" category.

Hillarious!

Andrew, would you please pick up Alan's patch? It clearly fixes an
ancient bug in the pnpacpi code.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Alan Cox]

> Hillarious!
> 
> Andrew, would you please pick up Alan's patch? It clearly fixes an
> ancient bug in the pnpacpi code.

And yes btw we should turn this option on in -next, and get these sort of
things out of the tree for good. More importantly it'll mean anyone
adding another one gets a whine on the spot.

Alan

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Wed, Dec 05, 2012 at 03:47:49PM +0000, Alan Cox wrote:
> > Hillarious!
> > 
> > Andrew, would you please pick up Alan's patch? It clearly fixes an
> > ancient bug in the pnpacpi code.
> 
> And yes btw we should turn this option on in -next, and get these sort of
> things out of the tree for good. More importantly it'll mean anyone
> adding another one gets a whine on the spot.

Right,

when we added W=, we thought that one use case would be for devs to
use it when build-testing patches and check whether they're adding new
warnings.

However, (1) we need additional functionality in some script to be able
to test when the build log adds new warnings

and

(2) need someone to enforce it.

I think Geert is doing something like that but I'm not sure he's even
using W=.

Actually, it would be cool if something checked for new warnings, mapped
it back to the patch in linux-next adding that warning and then nags the
patch author endlessly until it is fixed.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Andreas Mohr]

Hi,

On Wed, Dec 05, 2012 at 03:27:56PM +0000, Alan Cox wrote:
> > On Wed, Dec 05, 2012 at 08:09:01AM +0100, Andreas Mohr wrote:
> > > Hi,
> > > 
> > > drivers/pnp/pnpacpi/core.c: In function 'ispnpidacpi':
> > > drivers/pnp/pnpacpi/core.c:65:2: warning: logical 'or' of collectively
> > > exhaustive tests is always true [-Wlogical-op]
> > > drivers/pnp/pnpacpi/core.c:66:2: warning: logical 'or' of collectively
> > > exhaustive tests is always true [-Wlogical-op]
> > > drivers/pnp/pnpacpi/core.c:67:2: warning: logical 'or' of collectively
> > > exhaustive tests is always true [-Wlogical-op]
> > > 
> > > 
> > > That's already the second less enticing -Wlogical-op issue
> > > which was discovered by accident during less than two days
> 
> No it's not. It's been reported in bugzilla. I sent patches ages ago.
> They were ignored. Coverity has had it tagged for years (and a ton more
> of them you've not noticed yet)

And that additional new info bit now arriving
is supposed to improve on the total set of the general state of affairs
as I'm observing it (and thus to appease me, sufficiently), how exactly? ;)
(well, admittedly it was new to me since I didn't bother with doing
any more research after discovering that bug
after also having endured countless pages of dangerously obscuring
completely *superfluous* warning spew scrolling by)

Let's see:

- we've got this *core layer* (right?) bug
  originally appearing in <= 2.6.10 (2004, i.e. 8+ years).
- we've got a janitorial project which does not (possibly "cannot"?)
  do a sufficiently clean work (as evidenced by a metric crap ton of
  header-side woefully repeated warnings persisting over perhaps half a year
  in my observation, with W=2 or W=3)
- we've got how many dozen dozen kernel maintainers or "interested" persons
  who I'd imagine would be a sure-fire method to get such issues fixed
  in due time, and how many distributors or embedded companies
  who I'd imagine would help towards doing a quite large amount of effective
  testing/verification/housekeeping work in a controlled manner?
  (which would include regularly/mechanically keeping an eye
  on a larger set of *non-default* compiler warnings, too)
- we've got how many kernel-related tools to assist in such efforts,
  which then were not being followed sufficiently? [Coverity]
- and now your reply seems to indicate we additionally have
  a less optimal response time for such fixes, too?


If that isn't a strong indicator of having to spend some time on
rethinking possibly automated kernel development core infrastructure
or due process...


Heck, the Linux kernel isn't a "Joe's Garage works" effort -
it's a *very* widely used *very* public project, thus you'd think
with its clout and resulting manpower
it should be able to easily afford things such as near-eradication of warnings
(and thus the resulting much improved precision in *successfully* letting
mere users pinpoint single critical warnings as near-soon as they newly appear, due to a low amount of warnings even on a high warning level).


I'm afraid even with my measly very specific project/laughable manpower
I've got certain parts in place which go beyond of what the kernel
chooses to use.


As an additional factor, the C-based kernel has the convenience of
not even having to spend effort on the large set
of additional compiler warnings that the developer is gifted with in C++ space!


For gcc -Wlogical-op, it seems many articles/discussions were in 2009 time,
thus there admittedly probably wasn't such a large window of opportunity
to get this bug discovered - however that's still no excuse for the very
visible showering of simple warnings when enabling advanced levels.


Thanks to Borislav Petkov for his reply, too -
however I'd like to state that given this lack of tooling/attention
writing a mail in this more special manner was fully justified IMPHO.

Andreas Mohr

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Wed, Dec 05, 2012 at 05:39:05PM +0100, Andreas Mohr wrote:
> Thanks to Borislav Petkov for his reply, too -
> however I'd like to state that given this lack of tooling/attention
> writing a mail in this more special manner was fully justified IMPHO.

Ok, you've stated more than clearly that you're pissed with the current
situation - I think we all got that.

Now, how do you suggest we change it constructively and how are you
willing to help?

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Geert Uytterhoeven]

On Wed, Dec 5, 2012 at 4:59 PM, Borislav Petkov <bp@alien8.de> wrote:
> when we added W=, we thought that one use case would be for devs to
> use it when build-testing patches and check whether they're adding new
> warnings.
>
> However, (1) we need additional functionality in some script to be able
> to test when the build log adds new warnings
>
> and
>
> (2) need someone to enforce it.
>
> I think Geert is doing something like that but I'm not sure he's even
> using W=.

I'm using whatever build output is generated by the linux-next build service.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Look Ma, da kernel is b0rken

[Andreas Mohr]

On Wed, Dec 05, 2012 at 05:44:10PM +0100, Borislav Petkov wrote:
> On Wed, Dec 05, 2012 at 05:39:05PM +0100, Andreas Mohr wrote:
> > Thanks to Borislav Petkov for his reply, too -
> > however I'd like to state that given this lack of tooling/attention
> > writing a mail in this more special manner was fully justified IMPHO.
> 
> Ok, you've stated more than clearly that you're pissed with the current
> situation - I think we all got that.

Hohumm.

> Now, how do you suggest we change it constructively and how are you
> willing to help?

IMHO that's some common "misconception" - namely that innocent observing
by-standers are fully expected to get involved in the process.
Not necessarily, however (that's what domain experts are there for -
since people are well-advised to concentrate on the activities
that they are commonly involved with/do best).

I would be willing to handle corrections in the areas of drivers
I was working on (some corrections were already queued locally).

For scripted mechanisms (e.g. usefully nagging people as stated
in another posting), someone else would have to work on it I'm afraid.

Thanks,

Andreas Mohr

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Wed, Dec 05, 2012 at 06:09:00PM +0100, Andreas Mohr wrote:
> IMHO that's some common "misconception" - namely that innocent
> observing by-standers are fully expected to get involved in the
> process. Not necessarily, however (that's what domain experts are
> there for - since people are well-advised to concentrate on the
> activities that they are commonly involved with/do best).

Well, you don't have to be an expert to fix bugs. There are all kinds
of bugs for all kinds of technical levels - you only need to be willing
to fix them and send out patches. If you're not willing to help, then
you should better keep your rant to yourself because simply passively
ranting is not making anything better and not helping to anyone.

> I would be willing to handle corrections in the areas of drivers
> I was working on (some corrections were already queued locally).

See now, that's the spirit. :)

> For scripted mechanisms (e.g. usefully nagging people as stated in
> another posting), someone else would have to work on it I'm afraid.

Yep, that won't be that easy to get up and running so we'd need someone
to spend some quality time on such a framework.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Wed, Dec 05, 2012 at 06:04:38PM +0100, Geert Uytterhoeven wrote:
> > I think Geert is doing something like that but I'm not sure he's
> > even using W=.
>
> I'm using whatever build output is generated by the linux-next build
> service.

Then most probably not. W= levels are supplied at the make command line
and I don't think they're doing that due to the sheer amount of warnings
being spewed with W=.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

Hi Alan,

On Wed, 5 Dec 2012 15:47:49 +0000 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> And yes btw we should turn this option on in -next, and get these sort of
> things out of the tree for good. More importantly it'll mean anyone
> adding another one gets a whine on the spot.

While I appreciate your confidence, I don't notice quite a few new
warnings (because there are so many of them already :-().  Is there some
reason to not turn this on in our "normal" builds?  Does it produce many
false positives?  What compiler version is required?

I also currently don't carry patches that only ever appear in linux-next
(well, not intentionally anyway).  I assume it would require a patch to
the Makefile(s) to turn this on.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Thu, Dec 06, 2012 at 07:57:21AM +1100, Stephen Rothwell wrote:
> On Wed, 5 Dec 2012 15:47:49 +0000 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > And yes btw we should turn this option on in -next, and get these sort of
> > things out of the tree for good. More importantly it'll mean anyone
> > adding another one gets a whine on the spot.
> 
> While I appreciate your confidence, I don't notice quite a few new
> warnings (because there are so many of them already :-().  Is there some
> reason to not turn this on in our "normal" builds?  Does it produce many
> false positives?

Yes, it produces a huge number of warnings which need weeding out (some
of them are false positives and some of them are simply unfixable due to
design decisions in the kernel, etc, etc):

$ make W=123 drivers/pnp/pnpacpi/core.o 2> w.log
make[1]: Nothing to be done for `all'.
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
make[1]: Nothing to be done for `relocs'.
  CALL    scripts/checksyscalls.sh
  CC      drivers/pnp/pnpacpi/core.o
$ wc w.log
  2305  11202 168011 w.log

This is 2305 lines only for one compilation unit.

So if one enables all additional warning levels (this is what "W=123"
does) your build logs will be huge.

> What compiler version is required?

Works on all compilers by checking for supported -W options - see
scripts/Makefile.build.

> I also currently don't carry patches that only ever appear in
> linux-next (well, not intentionally anyway). I assume it would require
> a patch to the Makefile(s) to turn this on.

See above.

So ideally it would be if someone would build with "W=123" and track all
new warnings appearing with each new patch in linux-next and nag the
patch author to fix it before it hits mainline. This would require a
moderate level of scripting and experimenting though. The advantage is
that with something like that we'll be able to use all -W code checking
methods implemented gcc on our code and let the compiler possibly catch
more stuff.

We simply need someone not lazy enough to write that tracking and
nagging bit :).

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Andrew Morton]

On Wed, 5 Dec 2012 16:31:21 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Wed, Dec 05, 2012 at 03:27:56PM +0000, Alan Cox wrote:
> > On Wed, 5 Dec 2012 15:29:35 +0100
> > Borislav Petkov <bp@alien8.de> wrote:
> > 
> > > On Wed, Dec 05, 2012 at 08:09:01AM +0100, Andreas Mohr wrote:
> > > > Hi,
> > > > 
> > > > drivers/pnp/pnpacpi/core.c: In function 'ispnpidacpi':
> > > > drivers/pnp/pnpacpi/core.c:65:2: warning: logical 'or' of collectively
> > > > exhaustive tests is always true [-Wlogical-op]
> > > > drivers/pnp/pnpacpi/core.c:66:2: warning: logical 'or' of collectively
> > > > exhaustive tests is always true [-Wlogical-op]
> > > > drivers/pnp/pnpacpi/core.c:67:2: warning: logical 'or' of collectively
> > > > exhaustive tests is always true [-Wlogical-op]
> > > > 
> > > > 
> > > > That's already the second less enticing -Wlogical-op issue
> > > > which was discovered by accident during less than two days
> > 
> > No it's not. It's been reported in bugzilla. I sent patches ages ago.
> > They were ignored. Coverity has had it tagged for years (and a ton more
> > of them you've not noticed yet)
> > 
> > http://article.gmane.org/gmane.linux.acpi.devel/56753/match=test_alpha
> > 
> > This isn't discovered, this is in the "If you stick your fingers in your
> > ears and hum you can't hear the screaming" category.
> 
> Hillarious!
> 
> Andrew, would you please pick up Alan's patch? It clearly fixes an
> ancient bug in the pnpacpi code.
> 

Bjorn had a review comment which appears to remain unaddressed:

: The original is definitely broken.
: 
: I think the corrected test allows PNP IDs containing '@', which
: doesn't appear legal per sec 6.1.5 of the ACPI 5.0 spec.  Should this
: be
: 
: +       if (!('A' <= (c) && (c) <= 'Z')) \
: 
: instead?

Also, the original patch is missing a signed-off-by.  Here's what I
have queued:

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Subject: pnpacpi: fix incorrect TEST_ALPHA() test

TEST_ALPHA() is broken and always returns 0.

[akpm@linux-foundation.org: return false for '@' as well, per Bjorn]
Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Andreas Mohr <andi@lisas.de>
Cc: Li Shaohua <shaohua.li@intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/pnp/pnpacpi/core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN drivers/pnp/pnpacpi/core.c~pnpacpi-fix-incorrect-test_alpha-test drivers/pnp/pnpacpi/core.c
--- a/drivers/pnp/pnpacpi/core.c~pnpacpi-fix-incorrect-test_alpha-test
+++ a/drivers/pnp/pnpacpi/core.c
@@ -58,7 +58,7 @@ static inline int __init is_exclusive_de
 	if (!(('0' <= (c) && (c) <= '9') || ('A' <= (c) && (c) <= 'F'))) \
 		return 0
 #define TEST_ALPHA(c) \
-	if (!('@' <= (c) || (c) <= 'Z')) \
+	if (!('A' <= (c) && (c) <= 'Z')) \
 		return 0
 static int __init ispnpidacpi(const char *id)
 {
_

Re: Look Ma, da kernel is b0rken

[Alan Cox]

> While I appreciate your confidence, I don't notice quite a few new
> warnings (because there are so many of them already :-().  Is there some
> reason to not turn this on in our "normal" builds?  Does it produce many
> false positives?  What compiler version is required?

I've not seen any false positives from it yet. Unlike some of the
variable related ones it seems pretty solid.

> I also currently don't carry patches that only ever appear in linux-next
> (well, not intentionally anyway).  I assume it would require a patch to
> the Makefile(s) to turn this on.

Yes. I guess it belongs to the build scripts maintainers ?

Alan

Re: Look Ma, da kernel is b0rken

[Alan Cox]

On Wed, 5 Dec 2012 22:12:45 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Dec 06, 2012 at 07:57:21AM +1100, Stephen Rothwell wrote:
> > On Wed, 5 Dec 2012 15:47:49 +0000 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > > And yes btw we should turn this option on in -next, and get these sort of
> > > things out of the tree for good. More importantly it'll mean anyone
> > > adding another one gets a whine on the spot.
> > 
> > While I appreciate your confidence, I don't notice quite a few new
> > warnings (because there are so many of them already :-().  Is there some
> > reason to not turn this on in our "normal" builds?  Does it produce many
> > false positives?
> 
> Yes, it produces a huge number of warnings which need weeding out (some
> of them are false positives and some of them are simply unfixable due to
> design decisions in the kernel, etc, etc):
> 
> $ make W=123 drivers/pnp/pnpacpi/core.o 2> w.log

I was just talking about the always true/always false stuff !

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Wed, Dec 05, 2012 at 09:41:14PM +0000, Alan Cox wrote:
> I was just talking about the always true/always false stuff !

That's -Wlogical-op and not on by default. You can enable it with -W=2.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Wed, Dec 05, 2012 at 01:38:53PM -0800, Andrew Morton wrote:
> Also, the original patch is missing a signed-off-by. Here's what I
> have queued:

Thanks, looks good.

<snip the good-looking patch>

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Thu, Dec 06, 2012 at 12:38:34AM +0100, Rafael J. Wysocki wrote:
> The last patch you sent wasn't ignored.  You got this comment from Bjorn:
> 
> http://thread.gmane.org/gmane.linux.acpi.devel/56753
> 
> that you didn't respond to, so I'm still unsure what to do with that patch.

Andrew picked it up incorporating Bjorn's comment so you can drop it, I'd say.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Rafael J. Wysocki]

On Wednesday, December 05, 2012 03:27:56 PM Alan Cox wrote:
> On Wed, 5 Dec 2012 15:29:35 +0100
> Borislav Petkov <bp@alien8.de> wrote:
> 
> > On Wed, Dec 05, 2012 at 08:09:01AM +0100, Andreas Mohr wrote:
> > > Hi,
> > > 
> > > drivers/pnp/pnpacpi/core.c: In function 'ispnpidacpi':
> > > drivers/pnp/pnpacpi/core.c:65:2: warning: logical 'or' of collectively
> > > exhaustive tests is always true [-Wlogical-op]
> > > drivers/pnp/pnpacpi/core.c:66:2: warning: logical 'or' of collectively
> > > exhaustive tests is always true [-Wlogical-op]
> > > drivers/pnp/pnpacpi/core.c:67:2: warning: logical 'or' of collectively
> > > exhaustive tests is always true [-Wlogical-op]
> > > 
> > > 
> > > That's already the second less enticing -Wlogical-op issue
> > > which was discovered by accident during less than two days
> 
> No it's not. It's been reported in bugzilla. I sent patches ages ago.
> They were ignored.

The last patch you sent wasn't ignored.  You got this comment from Bjorn:

http://thread.gmane.org/gmane.linux.acpi.devel/56753

that you didn't respond to, so I'm still unsure what to do with that patch.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: Look Ma, da kernel is b0rken

[Alan Cox]

> The last patch you sent wasn't ignored.  You got this comment from Bjorn:
> 
> http://thread.gmane.org/gmane.linux.acpi.devel/56753

Ok that one never made it to me for some reason.

> that you didn't respond to, so I'm still unsure what to do with that patch.

I think Bjorn is correct.

Alan

Re: Look Ma, da kernel is b0rken

[Rafael J. Wysocki]

On Thursday, December 06, 2012 02:04:58 PM Alan Cox wrote:
> > The last patch you sent wasn't ignored.  You got this comment from Bjorn:
> > 
> > http://thread.gmane.org/gmane.linux.acpi.devel/56753
> 
> Ok that one never made it to me for some reason.
> 
> > that you didn't respond to, so I'm still unsure what to do with that patch.
> 
> I think Bjorn is correct.

OK, thanks.

Boris says that Andrew took the patch with the Bjorn's change.

Boris, is the patch in -mm somewhere?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Thu, Dec 06, 2012 at 09:56:09PM +0100, Rafael J. Wysocki wrote:
> Boris, is the patch in -mm somewhere?

It should be under the name: pnpacpi-fix-incorrect-test_alpha-test.patch

At least this is what Andrew's scripts said.

HTH.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Thu, Dec 06, 2012 at 10:21:32PM +0100, Rafael J. Wysocki wrote:
> On Thursday, December 06, 2012 09:59:56 PM Borislav Petkov wrote:
> > On Thu, Dec 06, 2012 at 09:56:09PM +0100, Rafael J. Wysocki wrote:
> > > Boris, is the patch in -mm somewhere?
> > 
> > It should be under the name: pnpacpi-fix-incorrect-test_alpha-test.patch
> > 
> > At least this is what Andrew's scripts said.
> 
> Found it, thanks!
> 
> I put it into the ACPI tree so that Andrew doesn't have to carry it.

Ok, CCed so that he can take it out.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Rafael J. Wysocki]

On Thursday, December 06, 2012 09:59:56 PM Borislav Petkov wrote:
> On Thu, Dec 06, 2012 at 09:56:09PM +0100, Rafael J. Wysocki wrote:
> > Boris, is the patch in -mm somewhere?
> 
> It should be under the name: pnpacpi-fix-incorrect-test_alpha-test.patch
> 
> At least this is what Andrew's scripts said.

Found it, thanks!

I put it into the ACPI tree so that Andrew doesn't have to carry it.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: Look Ma, da kernel is b0rken

[Andreas Mohr]

Hi,

On Wed, Dec 05, 2012 at 01:38:53PM -0800, Andrew Morton wrote:
> Bjorn had a review comment which appears to remain unaddressed:
> 
> : The original is definitely broken.
> : 
> : I think the corrected test allows PNP IDs containing '@', which
> : doesn't appear legal per sec 6.1.5 of the ACPI 5.0 spec.  Should this
> : be
> : 
> : +       if (!('A' <= (c) && (c) <= 'Z')) \
> : 
> : instead?

I hate having to rain on the parade again ;)
I just developed some doubts, by accident again,
by getting dangerously near sound/isa/als100.c snd_als100_pnpids:
there are many IDs with '@' embedded (at least in this ISA-based PnP code),
thus I guess that code may have had its justification (unless ACPI 5.0
is clearly fully authoritative for this space and thus '@' does not have any
business there any more).

Dito e.g. isa/cmi8330.c.

Hmm, anyone deeply familiar with ISA PnP ID magic? :)

Andreas Mohr

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Fri, Dec 07, 2012 at 05:52:18PM +0100, Andreas Mohr wrote:
> I hate having to rain on the parade again ;) I just developed
> some doubts, by accident again, by getting dangerously near
> sound/isa/als100.c snd_als100_pnpids: there are many IDs with '@'
> embedded (at least in this ISA-based PnP code), thus I guess that
> code may have had its justification (unless ACPI 5.0 is clearly fully
> authoritative for this space and thus '@' does not have any business
> there any more).
>
> Dito e.g. isa/cmi8330.c.
>
> Hmm, anyone deeply familiar with ISA PnP ID magic? :)

Even if this is violating the ACPI spec, any fix for this needs to be
tested on the hardware (and I can very well imagine that the hardware
might be violating the spec too, nothing new here).

So even if you had a fix, you need to run it on the hardware to verify
that it actually works.

So the actual practical question turns into: do you have such hardware
to verify your or anyone else's fix on?

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Andreas Mohr]

Hi,

On Fri, Dec 07, 2012 at 06:44:05PM +0100, Borislav Petkov wrote:
> On Fri, Dec 07, 2012 at 05:52:18PM +0100, Andreas Mohr wrote:
> > Hmm, anyone deeply familiar with ISA PnP ID magic? :)
> 
> Even if this is violating the ACPI spec, any fix for this needs to be
> tested on the hardware (and I can very well imagine that the hardware
> might be violating the spec too, nothing new here).
> 
> So even if you had a fix, you need to run it on the hardware to verify
> that it actually works.

And that demand actually applies to both the '@' change (questionable)
and the much less disputed (obviously correct) wrong conditional fixup,
since both introduce a notable change (either large, or possibly
improper) in behaviour.

> So the actual practical question turns into: do you have such hardware
> to verify your or anyone else's fix on?

Not the ALS100 (only ALS4000 here).
I possibly have some other ISA hardware, but probably none which contains
'@' data in their PnP id struct.
The driver for the well-known case of ISDN PnP cards
does not seem to contain it.
However ISTR that CMI8330 was quite widespread (did I have one? Do I??).
For identification, see http://www.yjfy.com/C/C-Media/soundchipset/CMI8330A.htm

I'm afraid I should get an old system back up and running,
exactly for such validation work cases (and perhaps so should a select few
other developers, too).

BTW, "my" fix? I thought that everybody had come to the conclusion by now
that I merely pointed out (in no uncertain terms to boot)
that something was broken :)

Andreas Mohr

Re: Look Ma, da kernel is b0rken

[Borislav Petkov]

On Sat, Dec 08, 2012 at 08:36:34AM +0100, Andreas Mohr wrote:
> And that demand actually applies to both the '@' change (questionable)
> and the much less disputed (obviously correct) wrong conditional
> fixup, since both introduce a notable change (either large, or
> possibly improper) in behaviour.

Well, I think Alan's fix is obviously correct and doesn't necessarily
need confirmation. But it will have ~3 months verification time in 3.8,
just in case.

> > So the actual practical question turns into: do you have such
> > hardware to verify your or anyone else's fix on?
>
> Not the ALS100 (only ALS4000 here). I possibly have some other
> ISA hardware, but probably none which contains '@' data in their
> PnP id struct. The driver for the well-known case of ISDN PnP
> cards does not seem to contain it. However ISTR that CMI8330 was
> quite widespread (did I have one? Do I??). For identification, see
> http://www.yjfy.com/C/C-Media/soundchipset/CMI8330A.htm
>
> I'm afraid I should get an old system back up and running, exactly for
> such validation work cases (and perhaps so should a select few other
> developers, too).
>
> BTW, "my" fix? I thought that everybody had come to the conclusion by
> now that I merely pointed out (in no uncertain terms to boot) that
> something was broken :)

Ok, here's the deal: we can blubber about fixing bugs in the kernel and
whether it needs this and that forever, but at the end of the day, Linux
is scratching an itch. That's it. There are no contracts, affirmations
or whatever. IOW, if it is not itching you strong enough, you won't
scratch it.

All I'm saying is, there are bugs and bugs. If this is a bug in a piece
of hardware which no one uses anymore and it might be violating the spec
or not, whatever, who cares..., but we can't verify that anymore, then
we leave it as is. There's absolutely no reason to touch this code and
so we'll let it die a peaceful death.

Now, if your box doesn't boot or something else annoys you strong enough
(the itch), then you can try fixing it (the scratch), or involve someone
more knowledgeable if you cannot do it yourself.

In any case, if you decide to fix anything though, you better do it
right. Thus the requirement to verify the fix on a real hardware.

So to answer your initial rant: yes, like any other non-trivial piece
of software, there are bugs in the kernel too. And yes, we always try
addressing the most important ones as fast as possible. And I'm sure
you know the kernel's track record of fixing bugs in a lightning fast
manner.

The other, not-so-important, not-so-critical bugs get "delayed"
sometimes. :-)

That's the whole deal.

-- 
Regards/Gruss,
    Boris.

Re: Look Ma, da kernel is b0rken

[Alan Cox]

On Sat, 8 Dec 2012 11:52:34 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Sat, Dec 08, 2012 at 08:36:34AM +0100, Andreas Mohr wrote:
> > And that demand actually applies to both the '@' change (questionable)
> > and the much less disputed (obviously correct) wrong conditional
> > fixup, since both introduce a notable change (either large, or
> > possibly improper) in behaviour.
> 
> Well, I think Alan's fix is obviously correct and doesn't necessarily
> need confirmation. But it will have ~3 months verification time in 3.8,
> just in case.

And to answer the question '@' is used in ISAPnP. Whether it is valid is
a rather different question 8)

Alan