| Message ID | 20121205133853.770451ca.akpm@linux-foundation.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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>
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {