Re: Linux 3.11-rc4
On Sun, Aug 4, 2013 at 4:09 PM, Linus Torvalds wrote: > It's that time of the week again.. > > "You apply 339 patches, what do you get > Another week older and deeper in debt > Saint Peter don't you call me 'cause I can't go > I owe my soul to the company store" > > I had hoped things would start calming down, but rc4 is pretty much > exactly the same size as rc3 was. That said, the patches seem a bit > more spread out, and less interesting - which is a good thing. Boring > is good. Let's keep it that way, and try to make for fewer patches for > -rc5, ok? Because we are past half-way now, and I really want to see > just fixes. > > We've got some arch updates (arm, parisc), but most of this is drivers > (mostly networking, usb and some drm updates). There's also some core > networking changes. And the printk code movement looks big if you > don't do git renames (ie like the patches I upload). I found a regression while running all v3.11-rcX kernels; Starcract II through wine crashes. The culprit is fab840f (ptrace: PTRACE_DETACH should do flush_ptrace_hw_breakpoint(child)), I revert that commit and there's no crash. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] ptrace: PTRACE_DETACH should do flush_ptrace_hw_breakpoint(child)
On Mon, May 13, 2013 at 10:17 AM, Oleg Nesterov wrote: > Change ptrace_detach() to call flush_ptrace_hw_breakpoint(child). > This frees the slots for non-ptrace PERF_TYPE_BREAKPOINT users, and > this ensures that the tracee won't be killed by SIGTRAP triggered by > the active breakpoints. There is something wrong with this patch. I've bisected an issue I've seen in v3.11-rcx kernels while running Starcraft II through wine, it's crashes 100% of the time, I revert this patch, and there's no crash any more. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sun, Aug 4, 2013 at 9:19 AM, Rafael J. Wysocki wrote: > On Sunday, August 04, 2013 01:42:49 AM Felipe Contreras wrote: >> Personally I think there are better ways to fix the code for the >> synthetic case than what you patch does, which will also make _BQC >> work. That can be discussed later though, the one-liner is simple, and >> it works. > > So, let's assume that the one-liner goes into 3.11 and work further with that > assumption. > > How would you address the sythetic case (on top of the one-liner)? I would write and read two values instead of one. The code is trying to check if _BQC is always returning the maximum, and if you try with two values you can be absolutely certain if that's happening or not; it doesn't even matter which values you choose. Even in the synthetic case that only has two values the check would work correctly and detect that _BQC works correctly (or not). In my machine I think the issue is slightly different, I think _BCM is failing, at least until enabling the _DOS thing, but at the end of the day it's the same thing for the check; _BQC is always returning the same value, and the code above will find that out, regardless of which values are tested. For my particular machine though, I think it's more interesting to find out why _BCM is failing before _DOS, and why efaa14c made it work. If that is actually the case. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sun, Aug 4, 2013 at 9:14 AM, Rafael J. Wysocki wrote: > On Sunday, August 04, 2013 01:54:21 AM Felipe Contreras wrote: >> But we cannot achieve either of those for v3.11, the only >> possibilities seem to be either a) revert efaa14c, or b) keep it and >> apply my patch. Anything else doesn't seem to be a possible or >> sensible option, and I vote for b). > > I've already said I'm going to do that for 3.11. I was just explaining why reverting efaa14c was an alternative, but it appears I'm wasting my breath. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sat, Aug 3, 2013 at 8:47 PM, Aaron Lu wrote: > On Sun, Aug 4, 2013 at 6:20 AM, Felipe Contreras > wrote: >> On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki wrote: >>> Do we still need to revert commit efaa14c if this patch is applied? >> >> I guess not. At least in this machine changing the backlight works >> correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10 >> didn't work at all. I cannot see how it would affect negatively other >> machines. > > That commit makes hotkey emit notifications, and it's not the > problem of "booting into a black screen", that problem is due to > broken _BQC. The broken _BQC has been there for quite some time, hasn't it? Either way, without efaa14c, changing the backlight doesn't work at all either way, so there's no black screen, because there cannot be. > BTW, the efaa14c will also make screen off at level 0 according > to Felipe, who consider this is a bug. But since it is required to > let firmware emit notifications on hotkey press, I think user will > want it. With or without efaa14c, level 0 makes the screen off, or at least it would, if the control worked at all. So efaa14c is one step forward, but two back, my patch removes the two steps back, but we are still not at the level of what my blacklisting patch does, for that we would need to fix two issues: 1. Fix the retrieval of the last level at boot 2. Fix level 0 (yes, I consider that a regression) But we cannot achieve either of those for v3.11, the only possibilities seem to be either a) revert efaa14c, or b) keep it and apply my patch. Anything else doesn't seem to be a possible or sensible option, and I vote for b). -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sat, Aug 3, 2013 at 8:18 PM, Aaron Lu wrote: > On 08/03/2013 07:34 PM, Rafael J. Wysocki wrote: >> On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote: >>> On 08/03/2013 07:47 AM, Rafael J. Wysocki wrote: >>>> On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote: >>>>> If the _BCL package is descending, the first level (br->levels[2]) will >>>>> be 0, and if the number of levels matches the number of steps, we might >>>>> confuse a returned level to mean the index. >>>>> >>>>> For example: >>>>> >>>>> current_level = max_level = 100 >>>>> test_level = 0 >>>>> returned level = 100 >>>>> >>>>> In this case 100 means the level, not the index, and _BCM failed. But if >>>>> the _BCL package is descending, the index of level 0 is also 100, so we >>>>> assume _BQC is indexed, when it's not. >>>>> >>>>> This causes all _BQC calls to return bogus values causing weird behavior >>>>> from the user's perspective. For example: xbacklight -set 10; xbacklight >>>>> -set 20; would flash to 90% and then slowly down to the desired level >>>>> (20). >>>>> >>>>> The solution is simple; test anything other than the first level (e.g. >>>>> 1). >>>>> >>>>> Signed-off-by: Felipe Contreras >>>> >>>> Looks reasonable. >>>> >>>> Aaron, what do you think? >>> >>> Yes, the patch is correct, but I still prefer my own version :-) >>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 >>> >>> In case you want to take mine and mine needs refresh, please let me know >>> and I can do the re-base, thanks. >> >> Well, I prefer simpler, unless there's a good reason to use more complicated. >> >> Why exactly do you think your version is better? > > As explained here: > https://lkml.org/lkml/2013/8/2/81 > https://lkml.org/lkml/2013/8/2/112 > > And for the demo broken _BQC, mine patch will disable _BQC while still > make the backlight work, and this patch here is testing the max > brightness level and may fail. Yes, but that problem can *only* happen in such a simplified _BCL, which is very very unlikely. Still, it would make sense to fix the code for that case. However, we can fix the problem first for the real known cases with my simple one-liner patch that can even be merged for v3.11, and *later* fix the issue for the synthetic unlikely case. Personally I think there are better ways to fix the code for the synthetic case than what you patch does, which will also make _BQC work. That can be discussed later though, the one-liner is simple, and it works. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sat, Aug 3, 2013 at 8:18 PM, Aaron Lu aaron@gmail.com wrote: On 08/03/2013 07:34 PM, Rafael J. Wysocki wrote: On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote: On 08/03/2013 07:47 AM, Rafael J. Wysocki wrote: On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote: If the _BCL package is descending, the first level (br-levels[2]) will be 0, and if the number of levels matches the number of steps, we might confuse a returned level to mean the index. For example: current_level = max_level = 100 test_level = 0 returned level = 100 In this case 100 means the level, not the index, and _BCM failed. But if the _BCL package is descending, the index of level 0 is also 100, so we assume _BQC is indexed, when it's not. This causes all _BQC calls to return bogus values causing weird behavior from the user's perspective. For example: xbacklight -set 10; xbacklight -set 20; would flash to 90% and then slowly down to the desired level (20). The solution is simple; test anything other than the first level (e.g. 1). Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Looks reasonable. Aaron, what do you think? Yes, the patch is correct, but I still prefer my own version :-) https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 In case you want to take mine and mine needs refresh, please let me know and I can do the re-base, thanks. Well, I prefer simpler, unless there's a good reason to use more complicated. Why exactly do you think your version is better? As explained here: https://lkml.org/lkml/2013/8/2/81 https://lkml.org/lkml/2013/8/2/112 And for the demo broken _BQC, mine patch will disable _BQC while still make the backlight work, and this patch here is testing the max brightness level and may fail. Yes, but that problem can *only* happen in such a simplified _BCL, which is very very unlikely. Still, it would make sense to fix the code for that case. However, we can fix the problem first for the real known cases with my simple one-liner patch that can even be merged for v3.11, and *later* fix the issue for the synthetic unlikely case. Personally I think there are better ways to fix the code for the synthetic case than what you patch does, which will also make _BQC work. That can be discussed later though, the one-liner is simple, and it works. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sat, Aug 3, 2013 at 8:47 PM, Aaron Lu aaron@gmail.com wrote: On Sun, Aug 4, 2013 at 6:20 AM, Felipe Contreras felipe.contre...@gmail.com wrote: On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki r...@sisk.pl wrote: Do we still need to revert commit efaa14c if this patch is applied? I guess not. At least in this machine changing the backlight works correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10 didn't work at all. I cannot see how it would affect negatively other machines. That commit makes hotkey emit notifications, and it's not the problem of booting into a black screen, that problem is due to broken _BQC. The broken _BQC has been there for quite some time, hasn't it? Either way, without efaa14c, changing the backlight doesn't work at all either way, so there's no black screen, because there cannot be. BTW, the efaa14c will also make screen off at level 0 according to Felipe, who consider this is a bug. But since it is required to let firmware emit notifications on hotkey press, I think user will want it. With or without efaa14c, level 0 makes the screen off, or at least it would, if the control worked at all. So efaa14c is one step forward, but two back, my patch removes the two steps back, but we are still not at the level of what my blacklisting patch does, for that we would need to fix two issues: 1. Fix the retrieval of the last level at boot 2. Fix level 0 (yes, I consider that a regression) But we cannot achieve either of those for v3.11, the only possibilities seem to be either a) revert efaa14c, or b) keep it and apply my patch. Anything else doesn't seem to be a possible or sensible option, and I vote for b). -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sun, Aug 4, 2013 at 9:14 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Sunday, August 04, 2013 01:54:21 AM Felipe Contreras wrote: But we cannot achieve either of those for v3.11, the only possibilities seem to be either a) revert efaa14c, or b) keep it and apply my patch. Anything else doesn't seem to be a possible or sensible option, and I vote for b). I've already said I'm going to do that for 3.11. I was just explaining why reverting efaa14c was an alternative, but it appears I'm wasting my breath. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sun, Aug 4, 2013 at 9:19 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Sunday, August 04, 2013 01:42:49 AM Felipe Contreras wrote: Personally I think there are better ways to fix the code for the synthetic case than what you patch does, which will also make _BQC work. That can be discussed later though, the one-liner is simple, and it works. So, let's assume that the one-liner goes into 3.11 and work further with that assumption. How would you address the sythetic case (on top of the one-liner)? I would write and read two values instead of one. The code is trying to check if _BQC is always returning the maximum, and if you try with two values you can be absolutely certain if that's happening or not; it doesn't even matter which values you choose. Even in the synthetic case that only has two values the check would work correctly and detect that _BQC works correctly (or not). In my machine I think the issue is slightly different, I think _BCM is failing, at least until enabling the _DOS thing, but at the end of the day it's the same thing for the check; _BQC is always returning the same value, and the code above will find that out, regardless of which values are tested. For my particular machine though, I think it's more interesting to find out why _BCM is failing before _DOS, and why efaa14c made it work. If that is actually the case. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 11/13] ptrace: PTRACE_DETACH should do flush_ptrace_hw_breakpoint(child)
On Mon, May 13, 2013 at 10:17 AM, Oleg Nesterov o...@redhat.com wrote: Change ptrace_detach() to call flush_ptrace_hw_breakpoint(child). This frees the slots for non-ptrace PERF_TYPE_BREAKPOINT users, and this ensures that the tracee won't be killed by SIGTRAP triggered by the active breakpoints. There is something wrong with this patch. I've bisected an issue I've seen in v3.11-rcx kernels while running Starcraft II through wine, it's crashes 100% of the time, I revert this patch, and there's no crash any more. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Linux 3.11-rc4
On Sun, Aug 4, 2013 at 4:09 PM, Linus Torvalds torva...@linux-foundation.org wrote: It's that time of the week again.. You apply 339 patches, what do you get Another week older and deeper in debt Saint Peter don't you call me 'cause I can't go I owe my soul to the company store I had hoped things would start calming down, but rc4 is pretty much exactly the same size as rc3 was. That said, the patches seem a bit more spread out, and less interesting - which is a good thing. Boring is good. Let's keep it that way, and try to make for fewer patches for -rc5, ok? Because we are past half-way now, and I really want to see just fixes. We've got some arch updates (arm, parisc), but most of this is drivers (mostly networking, usb and some drm updates). There's also some core networking changes. And the printk code movement looks big if you don't do git renames (ie like the patches I upload). I found a regression while running all v3.11-rcX kernels; Starcract II through wine crashes. The culprit is fab840f (ptrace: PTRACE_DETACH should do flush_ptrace_hw_breakpoint(child)), I revert that commit and there's no crash. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Sat, Aug 3, 2013 at 6:27 PM, Igor Gnatenko wrote: > On Sat, 2013-08-03 at 15:21 -0500, Felipe Contreras wrote: >> On Sat, Aug 3, 2013 at 11:08 AM, Igor Gnatenko >> wrote: >> >> > I am opposed to this patch. On ThinkPad X230 I had problems with it. >> > Felipe, come over to dark side. They have cookies. >> >> And v3.11-rc3 works's fine out-of-the box? >> > > rc2 work's fine. Since rc3 Rafael reverted patch and I think backlight > broken again (I don't have time to check rc3 yet). You think? We are talking about this patch [1]. Did you apply that patch on top of rc3 and booted without any arguments? If you did, what problems *exactly* did you have? [1] https://bugzilla.kernel.org/attachment.cgi?id=107084=diff=patch==1=raw -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sat, Aug 3, 2013 at 5:38 PM, Rafael J. Wysocki wrote: > On Saturday, August 03, 2013 05:20:33 PM Felipe Contreras wrote: >> On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki wrote: >> > On Saturday, August 03, 2013 03:24:16 PM Felipe Contreras wrote: >> >> On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki wrote: >> >> > On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote: >> >> >> >> >> Yes, the patch is correct, but I still prefer my own version :-) >> >> >> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 >> >> >> >> >> >> In case you want to take mine and mine needs refresh, please let me >> >> >> know >> >> >> and I can do the re-base, thanks. >> >> > >> >> > Well, I prefer simpler, unless there's a good reason to use more >> >> > complicated. >> >> >> >> Note that these are not exclusionary; his patch can be applied on top >> >> of mine. I don't think his patch is needed though. >> > >> > OK >> > >> > Do we still need to revert commit efaa14c if this patch is applied? >> >> I guess not. At least in this machine changing the backlight works >> correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10 >> didn't work at all. I cannot see how it would affect negatively other >> machines. >> >> That being said, the blacklisting is still needed, because 1. the >> level is not preserved between boots, and 2. level 0 turns off the >> screen, which I personally consider a regression. >> >> At least it boots to level 100 instead of 0. > > OK > > I'll push this patch to Linus for -rc5 then without the revert of commit > commit efaa14c. That's all I'm going to do for 3.11 in the ACPI video > area at this point. That seems fair. > As far as the blacklisting is concerned, I still have the blacklist of > your Asus machine queued up for 3.12. Since you're claiming that it > doesn't have any side effects on that machine, I think I can apply it. > > However, for other machines to be added to that blacklist I need to see > requests from their users with confirmation that there are no visible side > effects there. Good, that's the purpose of bug 60682. https://bugzilla.kernel.org/show_bug.cgi?id=60682 -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Sat, Aug 3, 2013 at 5:21 PM, Rafael J. Wysocki wrote: > On Saturday, August 03, 2013 05:06:10 PM Felipe Contreras wrote: >> On Sat, Aug 3, 2013 at 4:59 PM, Rafael J. Wysocki wrote: > > [...] > >> > Whatever you are thinking you will achieve this way, it doesn't work. >> >> It is the reality: v3.7 is broken, v3.8 is broken, v3.9 is broken, >> v3.10 is broken, v3.11 is going to be broken, v3.12 will probably be >> broken too, and perhaps even v3.13. > > Be precise and say "backlight control on a number of machines in broken in > those kernels". Yes, it is. It needs to be fixed. Not necessarily your way, > though. "My" way, can be done *today*. "Your" way can be done whenever it's ready. >> Who benefits from this? > > Clearly, no one. > > And who benefits from your "crusade"? Who benefits from yours? It's all very simple; we ask the owners of these machines if acpi_osi="!Windows 2012" makes things work better, if they do, they go into the blacklist, if not, they don't. That would fix the problems for these machines *today*. If later it turns out there are other issues introduced, they get removed, just like you removed the intel_backlight switch patch. Igor Gnatenko said he had problems, but didn't mention which problem. In bug #51231, tons of people reported that things improved for ThinkPad X230. And when the proper fix is done, everyone will be happy and we can drop the blacklist. Everybody benefits this way. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki wrote: > On Saturday, August 03, 2013 03:24:16 PM Felipe Contreras wrote: >> On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki wrote: >> > On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote: >> >> >> Yes, the patch is correct, but I still prefer my own version :-) >> >> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 >> >> >> >> In case you want to take mine and mine needs refresh, please let me know >> >> and I can do the re-base, thanks. >> > >> > Well, I prefer simpler, unless there's a good reason to use more >> > complicated. >> >> Note that these are not exclusionary; his patch can be applied on top >> of mine. I don't think his patch is needed though. > > OK > > Do we still need to revert commit efaa14c if this patch is applied? I guess not. At least in this machine changing the backlight works correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10 didn't work at all. I cannot see how it would affect negatively other machines. That being said, the blacklisting is still needed, because 1. the level is not preserved between boots, and 2. level 0 turns off the screen, which I personally consider a regression. At least it boots to level 100 instead of 0. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Sat, Aug 3, 2013 at 4:59 PM, Rafael J. Wysocki wrote: > On Saturday, August 03, 2013 03:20:22 PM Felipe Contreras wrote: >> On Sat, Aug 3, 2013 at 6:54 AM, Rafael J. Wysocki wrote: >> > On Friday, August 02, 2013 08:48:09 PM Felipe Contreras wrote: >> >> >> Yes, that's fine, either the revert, or the patch I mentioned, or >> >> something else, but something has to be done, and it was better to do >> >> it in v3.11-rc4 than in v3.11-rc5, because that change itself can >> >> cause further problems. >> > >> > A revert can be done in -rc5 just fine. If we don't have a working fix >> > this >> > week, I'll do the revert. >> >> I think you are waiting for miracles. But whatever. >> >> >> Let's do a though experiment, let's say you are right, and they can >> >> survive the 6 months it would take you to find the "perfect" solution, >> >> say in v3.13. What's wrong with having a partial solution in v3.12? If >> >> the blacklisting doesn't work properly (there's absolutely no evidence >> >> for that), then you revert it on v3.12.1. >> >> >> >> What's wrong with that approach? >> > >> > If the blacklisting leads to problems, they may not be reported in the 3.12 >> > time frame, but much later. For example because people won't realize that >> > the problems are caused by the blacklisting until much much later. And >> > then >> > we'll be in a spot where whatever we do will break things for someone. >> >> The key word is *may*, you don't *know*. Why do you insist in >> committing this reification fallacy? >> >> This threat is not real, it's theoretical. > > I believe it is real. That is called wishful thinking. Believing so doesn't make it so. > Igor has told you that already, hasn't he? He said he "had problems", that doesn't mean much. What kind of problems? Did he have those problems in v3.10? Does v3.11-rc3 work correctly? Did he boot without any boot arguments? "I had problems" is almost meaningless. >> But let's suppose you are right, and there are issues, and those don't >> get reported in v3.12. That is actually GOOD, if people don't report >> issues, it means the issues are not that big, or not even *there*. > > You don't even realize how wrong you are. The issues that aren't visible > in testing from the start are often much *worse* than those we can see > immediately, because usually they are much more difficult to fix and they > cause much more pain overall as a result. How convenient. In other words; there's absolutely no empirical way to prove you wrong. It's like trying to prove that your invisible pink unicorn doesn't exist. If we don't find evidence of problems, that is evidence that there are no problems. And even if there are "invisible" "worse" issues, it doesn't matter, because you will fix them properly in six months, right? I'd say, fix the visible (aka. real) issues, don't worry about the invisible (aka. imaginary) ones. Worry when they are visible. >> And no, we won't be in a spot where whatever we do will break things, >> because if the intel backlight driver works correctly, that solution >> would work for everyone. And if it doesn't, we should stay with what >> works best. >> >> > And we had situations like that in the past, which is the source of my >> > concern. >> > You obviously don't have that experience, or you won't be so eager to >> > inflict >> > the blacklisting on everyone. >> > >> > Anyway, as you know, but conveniently don't mention, I asked some >> > experienced >> > people for opinions about that. If they agree with you, we will add the >> > blacklist. If they don't, we won't add it. >> >> Again, screw the users. We are stuck with broken backlight for several >> more months to come. Great. > > Whatever you are thinking you will achieve this way, it doesn't work. It is the reality: v3.7 is broken, v3.8 is broken, v3.9 is broken, v3.10 is broken, v3.11 is going to be broken, v3.12 will probably be broken too, and perhaps even v3.13. Who benefits from this? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups
On Sat, Aug 3, 2013 at 6:38 AM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 08:34:29 PM Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 7:07 PM, Rafael J. Wysocki wrote: >> > On Friday, August 02, 2013 12:52:18 PM Felipe Contreras wrote: >> >> On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki wrote: >> >> > On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote: >> >> >> On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu wrote: >> >> >> > On 08/02/2013 07:43 AM, Felipe Contreras wrote: >> >> >> >> Signed-off-by: Felipe Contreras >> >> >> > >> >> >> > Please add change log explaining what you have changed. >> >> >> > It seems that the patch modify comment style only, some add a space >> >> >> > and >> >> >> > some change spaces to tab, is it the case? >> >> >> >> >> >> The commit message already explains what the change is; trivial >> >> >> cosmetic cleanups. Cosmetic means it's completely superficial. >> >> > >> >> > And I have a rule not to apply patches without changelogs. So either >> >> > I'll >> >> > need to write it for you, or can you add one pretty please? >> >> >> >> The commit message is right there. Maybe Jiri can apply it then, if >> >> not, then stay happy with your untidy code. >> > >> > First of all, I didn't say I wouldn't apply the patch, did I? >> > >> > Second, I asked you *nicely* to add a changelog so that I don't need to >> > write >> > it for you. >> > >> > I don't know what made it difficult to understand. >> > >> > Anyway, I ask everyone to write changelogs and nobody has had any problems >> > with >> > that so far. I don't see why I should avoid asking you to follow the rules >> > that everybody else is asked to follow. If those rules are too difficult >> > for >> > you to follow, I'm sorry. >> >> The patch has a commit message that describes exactly what it does. > > No, it doesn't describe it exactly. You're contradicting facts. > >> Unless there is valid feedback I will not send another version. >> >> To me, a valid criticism to the commit message would be: "I read X, >> but I thought it would do Y". For example; "I didn't expect the patch >> to do white-space cleanups", but I think that's exactly what people >> expect when they read "trivial costmetic cleanups'. > > If what you're saying was correct, then it would be sufficient to use a > "this patch fixes a bug" commit message for every bug fix, but quite evidently > that is not the case. No, it wouldn't be sufficient, take a look a the Corbert's list you yourself mentioned: * the original motivation for the work is quickly forgotten "this patch fixes a bug" doesn't describe the motivation. * Andrew Morton also famously pushes developers to document the reasons explaining why a patch was written, including the user-visible effects of any bugs fixed The reason for the patch is not documented, nor the user-visible effects. * Kernel developers do not like having to reverse engineer the intent of a patch years after the fact. The intent of the patch is not mentioned. That is completely different with my patch. Personally I like to answer these questions: What is the patch is doing (motivation)? What is the current problem? What is the change? What are the side-effects? All those are clear with "trivial costmetic cleanups", they are not with "this patch fixes a bug". I think you are committing a hasty generalization fallacy. Not all patches are the same. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki wrote: > On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote: >> Yes, the patch is correct, but I still prefer my own version :-) >> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 >> >> In case you want to take mine and mine needs refresh, please let me know >> and I can do the re-base, thanks. > > Well, I prefer simpler, unless there's a good reason to use more complicated. Note that these are not exclusionary; his patch can be applied on top of mine. I don't think his patch is needed though. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Sat, Aug 3, 2013 at 11:08 AM, Igor Gnatenko wrote: > I am opposed to this patch. On ThinkPad X230 I had problems with it. > Felipe, come over to dark side. They have cookies. And v3.11-rc3 works's fine out-of-the box? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Sat, Aug 3, 2013 at 6:54 AM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 08:48:09 PM Felipe Contreras wrote: >> Yes, that's fine, either the revert, or the patch I mentioned, or >> something else, but something has to be done, and it was better to do >> it in v3.11-rc4 than in v3.11-rc5, because that change itself can >> cause further problems. > > A revert can be done in -rc5 just fine. If we don't have a working fix this > week, I'll do the revert. I think you are waiting for miracles. But whatever. >> Let's do a though experiment, let's say you are right, and they can >> survive the 6 months it would take you to find the "perfect" solution, >> say in v3.13. What's wrong with having a partial solution in v3.12? If >> the blacklisting doesn't work properly (there's absolutely no evidence >> for that), then you revert it on v3.12.1. >> >> What's wrong with that approach? > > If the blacklisting leads to problems, they may not be reported in the 3.12 > time frame, but much later. For example because people won't realize that > the problems are caused by the blacklisting until much much later. And then > we'll be in a spot where whatever we do will break things for someone. The key word is *may*, you don't *know*. Why do you insist in committing this reification fallacy? This threat is not real, it's theoretical. But let's suppose you are right, and there are issues, and those don't get reported in v3.12. That is actually GOOD, if people don't report issues, it means the issues are not that big, or not even *there*. And no, we won't be in a spot where whatever we do will break things, because if the intel backlight driver works correctly, that solution would work for everyone. And if it doesn't, we should stay with what works best. > And we had situations like that in the past, which is the source of my > concern. > You obviously don't have that experience, or you won't be so eager to inflict > the blacklisting on everyone. > > Anyway, as you know, but conveniently don't mention, I asked some experienced > people for opinions about that. If they agree with you, we will add the > blacklist. If they don't, we won't add it. Again, screw the users. We are stuck with broken backlight for several more months to come. Great. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Sat, Aug 3, 2013 at 6:54 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 08:48:09 PM Felipe Contreras wrote: Yes, that's fine, either the revert, or the patch I mentioned, or something else, but something has to be done, and it was better to do it in v3.11-rc4 than in v3.11-rc5, because that change itself can cause further problems. A revert can be done in -rc5 just fine. If we don't have a working fix this week, I'll do the revert. I think you are waiting for miracles. But whatever. Let's do a though experiment, let's say you are right, and they can survive the 6 months it would take you to find the perfect solution, say in v3.13. What's wrong with having a partial solution in v3.12? If the blacklisting doesn't work properly (there's absolutely no evidence for that), then you revert it on v3.12.1. What's wrong with that approach? If the blacklisting leads to problems, they may not be reported in the 3.12 time frame, but much later. For example because people won't realize that the problems are caused by the blacklisting until much much later. And then we'll be in a spot where whatever we do will break things for someone. The key word is *may*, you don't *know*. Why do you insist in committing this reification fallacy? This threat is not real, it's theoretical. But let's suppose you are right, and there are issues, and those don't get reported in v3.12. That is actually GOOD, if people don't report issues, it means the issues are not that big, or not even *there*. And no, we won't be in a spot where whatever we do will break things, because if the intel backlight driver works correctly, that solution would work for everyone. And if it doesn't, we should stay with what works best. And we had situations like that in the past, which is the source of my concern. You obviously don't have that experience, or you won't be so eager to inflict the blacklisting on everyone. Anyway, as you know, but conveniently don't mention, I asked some experienced people for opinions about that. If they agree with you, we will add the blacklist. If they don't, we won't add it. Again, screw the users. We are stuck with broken backlight for several more months to come. Great. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Sat, Aug 3, 2013 at 11:08 AM, Igor Gnatenko i.gnatenko.br...@gmail.com wrote: I am opposed to this patch. On ThinkPad X230 I had problems with it. Felipe, come over to dark side. They have cookies. And v3.11-rc3 works's fine out-of-the box? -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote: Yes, the patch is correct, but I still prefer my own version :-) https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 In case you want to take mine and mine needs refresh, please let me know and I can do the re-base, thanks. Well, I prefer simpler, unless there's a good reason to use more complicated. Note that these are not exclusionary; his patch can be applied on top of mine. I don't think his patch is needed though. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups
On Sat, Aug 3, 2013 at 6:38 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 08:34:29 PM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 7:07 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 12:52:18 PM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote: On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu aaron@gmail.com wrote: On 08/02/2013 07:43 AM, Felipe Contreras wrote: Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Please add change log explaining what you have changed. It seems that the patch modify comment style only, some add a space and some change spaces to tab, is it the case? The commit message already explains what the change is; trivial cosmetic cleanups. Cosmetic means it's completely superficial. And I have a rule not to apply patches without changelogs. So either I'll need to write it for you, or can you add one pretty please? The commit message is right there. Maybe Jiri can apply it then, if not, then stay happy with your untidy code. First of all, I didn't say I wouldn't apply the patch, did I? Second, I asked you *nicely* to add a changelog so that I don't need to write it for you. I don't know what made it difficult to understand. Anyway, I ask everyone to write changelogs and nobody has had any problems with that so far. I don't see why I should avoid asking you to follow the rules that everybody else is asked to follow. If those rules are too difficult for you to follow, I'm sorry. The patch has a commit message that describes exactly what it does. No, it doesn't describe it exactly. You're contradicting facts. Unless there is valid feedback I will not send another version. To me, a valid criticism to the commit message would be: I read X, but I thought it would do Y. For example; I didn't expect the patch to do white-space cleanups, but I think that's exactly what people expect when they read trivial costmetic cleanups'. If what you're saying was correct, then it would be sufficient to use a this patch fixes a bug commit message for every bug fix, but quite evidently that is not the case. No, it wouldn't be sufficient, take a look a the Corbert's list you yourself mentioned: * the original motivation for the work is quickly forgotten this patch fixes a bug doesn't describe the motivation. * Andrew Morton also famously pushes developers to document the reasons explaining why a patch was written, including the user-visible effects of any bugs fixed The reason for the patch is not documented, nor the user-visible effects. * Kernel developers do not like having to reverse engineer the intent of a patch years after the fact. The intent of the patch is not mentioned. That is completely different with my patch. Personally I like to answer these questions: What is the patch is doing (motivation)? What is the current problem? What is the change? What are the side-effects? All those are clear with trivial costmetic cleanups, they are not with this patch fixes a bug. I think you are committing a hasty generalization fallacy. Not all patches are the same. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Sat, Aug 3, 2013 at 4:59 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Saturday, August 03, 2013 03:20:22 PM Felipe Contreras wrote: On Sat, Aug 3, 2013 at 6:54 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 08:48:09 PM Felipe Contreras wrote: Yes, that's fine, either the revert, or the patch I mentioned, or something else, but something has to be done, and it was better to do it in v3.11-rc4 than in v3.11-rc5, because that change itself can cause further problems. A revert can be done in -rc5 just fine. If we don't have a working fix this week, I'll do the revert. I think you are waiting for miracles. But whatever. Let's do a though experiment, let's say you are right, and they can survive the 6 months it would take you to find the perfect solution, say in v3.13. What's wrong with having a partial solution in v3.12? If the blacklisting doesn't work properly (there's absolutely no evidence for that), then you revert it on v3.12.1. What's wrong with that approach? If the blacklisting leads to problems, they may not be reported in the 3.12 time frame, but much later. For example because people won't realize that the problems are caused by the blacklisting until much much later. And then we'll be in a spot where whatever we do will break things for someone. The key word is *may*, you don't *know*. Why do you insist in committing this reification fallacy? This threat is not real, it's theoretical. I believe it is real. That is called wishful thinking. Believing so doesn't make it so. Igor has told you that already, hasn't he? He said he had problems, that doesn't mean much. What kind of problems? Did he have those problems in v3.10? Does v3.11-rc3 work correctly? Did he boot without any boot arguments? I had problems is almost meaningless. But let's suppose you are right, and there are issues, and those don't get reported in v3.12. That is actually GOOD, if people don't report issues, it means the issues are not that big, or not even *there*. You don't even realize how wrong you are. The issues that aren't visible in testing from the start are often much *worse* than those we can see immediately, because usually they are much more difficult to fix and they cause much more pain overall as a result. How convenient. In other words; there's absolutely no empirical way to prove you wrong. It's like trying to prove that your invisible pink unicorn doesn't exist. If we don't find evidence of problems, that is evidence that there are no problems. And even if there are invisible worse issues, it doesn't matter, because you will fix them properly in six months, right? I'd say, fix the visible (aka. real) issues, don't worry about the invisible (aka. imaginary) ones. Worry when they are visible. And no, we won't be in a spot where whatever we do will break things, because if the intel backlight driver works correctly, that solution would work for everyone. And if it doesn't, we should stay with what works best. And we had situations like that in the past, which is the source of my concern. You obviously don't have that experience, or you won't be so eager to inflict the blacklisting on everyone. Anyway, as you know, but conveniently don't mention, I asked some experienced people for opinions about that. If they agree with you, we will add the blacklist. If they don't, we won't add it. Again, screw the users. We are stuck with broken backlight for several more months to come. Great. Whatever you are thinking you will achieve this way, it doesn't work. It is the reality: v3.7 is broken, v3.8 is broken, v3.9 is broken, v3.10 is broken, v3.11 is going to be broken, v3.12 will probably be broken too, and perhaps even v3.13. Who benefits from this? -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Saturday, August 03, 2013 03:24:16 PM Felipe Contreras wrote: On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote: Yes, the patch is correct, but I still prefer my own version :-) https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 In case you want to take mine and mine needs refresh, please let me know and I can do the re-base, thanks. Well, I prefer simpler, unless there's a good reason to use more complicated. Note that these are not exclusionary; his patch can be applied on top of mine. I don't think his patch is needed though. OK Do we still need to revert commit efaa14c if this patch is applied? I guess not. At least in this machine changing the backlight works correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10 didn't work at all. I cannot see how it would affect negatively other machines. That being said, the blacklisting is still needed, because 1. the level is not preserved between boots, and 2. level 0 turns off the screen, which I personally consider a regression. At least it boots to level 100 instead of 0. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Sat, Aug 3, 2013 at 5:21 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Saturday, August 03, 2013 05:06:10 PM Felipe Contreras wrote: On Sat, Aug 3, 2013 at 4:59 PM, Rafael J. Wysocki r...@sisk.pl wrote: [...] Whatever you are thinking you will achieve this way, it doesn't work. It is the reality: v3.7 is broken, v3.8 is broken, v3.9 is broken, v3.10 is broken, v3.11 is going to be broken, v3.12 will probably be broken too, and perhaps even v3.13. Be precise and say backlight control on a number of machines in broken in those kernels. Yes, it is. It needs to be fixed. Not necessarily your way, though. My way, can be done *today*. Your way can be done whenever it's ready. Who benefits from this? Clearly, no one. And who benefits from your crusade? Who benefits from yours? It's all very simple; we ask the owners of these machines if acpi_osi=!Windows 2012 makes things work better, if they do, they go into the blacklist, if not, they don't. That would fix the problems for these machines *today*. If later it turns out there are other issues introduced, they get removed, just like you removed the intel_backlight switch patch. Igor Gnatenko said he had problems, but didn't mention which problem. In bug #51231, tons of people reported that things improved for ThinkPad X230. And when the proper fix is done, everyone will be happy and we can drop the blacklist. Everybody benefits this way. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Sat, Aug 3, 2013 at 5:38 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Saturday, August 03, 2013 05:20:33 PM Felipe Contreras wrote: On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Saturday, August 03, 2013 03:24:16 PM Felipe Contreras wrote: On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote: Yes, the patch is correct, but I still prefer my own version :-) https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 In case you want to take mine and mine needs refresh, please let me know and I can do the re-base, thanks. Well, I prefer simpler, unless there's a good reason to use more complicated. Note that these are not exclusionary; his patch can be applied on top of mine. I don't think his patch is needed though. OK Do we still need to revert commit efaa14c if this patch is applied? I guess not. At least in this machine changing the backlight works correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10 didn't work at all. I cannot see how it would affect negatively other machines. That being said, the blacklisting is still needed, because 1. the level is not preserved between boots, and 2. level 0 turns off the screen, which I personally consider a regression. At least it boots to level 100 instead of 0. OK I'll push this patch to Linus for -rc5 then without the revert of commit commit efaa14c. That's all I'm going to do for 3.11 in the ACPI video area at this point. That seems fair. As far as the blacklisting is concerned, I still have the blacklist of your Asus machine queued up for 3.12. Since you're claiming that it doesn't have any side effects on that machine, I think I can apply it. However, for other machines to be added to that blacklist I need to see requests from their users with confirmation that there are no visible side effects there. Good, that's the purpose of bug 60682. https://bugzilla.kernel.org/show_bug.cgi?id=60682 -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Sat, Aug 3, 2013 at 6:27 PM, Igor Gnatenko i.gnatenko.br...@gmail.com wrote: On Sat, 2013-08-03 at 15:21 -0500, Felipe Contreras wrote: On Sat, Aug 3, 2013 at 11:08 AM, Igor Gnatenko i.gnatenko.br...@gmail.com wrote: I am opposed to this patch. On ThinkPad X230 I had problems with it. Felipe, come over to dark side. They have cookies. And v3.11-rc3 works's fine out-of-the box? rc2 work's fine. Since rc3 Rafael reverted patch and I think backlight broken again (I don't have time to check rc3 yet). You think? We are talking about this patch [1]. Did you apply that patch on top of rc3 and booted without any arguments? If you did, what problems *exactly* did you have? [1] https://bugzilla.kernel.org/attachment.cgi?id=107084action=diffcontext=patchcollapsed=headers=1format=raw -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Fri, Aug 2, 2013 at 5:21 PM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 04:31:37 PM Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 4:21 PM, Rafael J. Wysocki wrote: >> > On Friday, August 02, 2013 02:12:49 PM Felipe Contreras wrote: >> >> >> You forgot this patch: >> >> >> >> https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next=3706231332d57072e0e2c0e59975443f3f18e673 >> >> >> >> Or do you think it's fine to boot these machines into a black screen? >> > >> > Seriously, what's wrong with you?! >> > >> > I didn't forget about it, I just didn't include it into this particular >> > pull request. >> > >> > And I'm not even sure I will push it for 3.11, because I prefer to revert >> > efaa14c for 3.11 if that's necessary to make your broken box work as >> > before. >> >> The issue happens in more than just "my broken box", and yes, >> reverting that patch would help (in more than just my box), in the >> sense that at least Linux won't boot into a black screen. >> >> But the backlight control still wouldn't work, as it hasn't worked >> since v3.7, possibly in many ASUS laptops, for that you need more than >> just reverting efaa14c. > > Yes, last time it worked in 3.6 and in particular it doesn't work in 3.10. > My current goal is bring things back to the 3.10 state first, possibly without > introducing any new problems, because we're kind of late in the cycle. > That's better done by reverting stuff known to have introduced problems in > the first place and not by doing things that may introduce more of them. > > And your blacklisting patch has potential to introduce problems. Your goal is > to bring backlight control to the 3.6 state on that particular machine, but > the blacklist is done at the *system* level and very well may affect more > things than just backlight. You may not see any problems resulting from it > and you may not care even if there are some, but other users of it may use > different user space, for example, and may see problems that you're not > seeing. > > That's why I'd very much prefer to do the revert at this point. Yes, that's fine, either the revert, or the patch I mentioned, or something else, but something has to be done, and it was better to do it in v3.11-rc4 than in v3.11-rc5, because that change itself can cause further problems. >> > Well, perhaps I just won't push it at all so that you actually can go and >> > complain to Linus about that ... >> >> That is very responsible from you. Screw the users, right? > > No, that's not my goal, sorry for disappointing you. > > The problem is that I'm not really convinced about the validity of the > blacklisting approach to begin with. As I said, the blacklisting is done > on the system level and the goal is to work around backlight control problems. > That sounds like a sledgehammer approach to me, which I don't really like. > If the blacklisting was more targeted, done at the video driver level etc., > I wouldn't really have any concerns about it, but that's not the case. > > And since people evidently could live for over 6 months with the backlight > control problems, maybe they'll survive some more time still and allow us to > find a better approach? They probably can survive without Linux at all, that doesn't mean we are doing our job. Let's do a though experiment, let's say you are right, and they can survive the 6 months it would take you to find the "perfect" solution, say in v3.13. What's wrong with having a partial solution in v3.12? If the blacklisting doesn't work properly (there's absolutely no evidence for that), then you revert it on v3.12.1. What's wrong with that approach? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups
On Fri, Aug 2, 2013 at 7:07 PM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 12:52:18 PM Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki wrote: >> > On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote: >> >> On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu wrote: >> >> > On 08/02/2013 07:43 AM, Felipe Contreras wrote: >> >> >> Signed-off-by: Felipe Contreras >> >> > >> >> > Please add change log explaining what you have changed. >> >> > It seems that the patch modify comment style only, some add a space and >> >> > some change spaces to tab, is it the case? >> >> >> >> The commit message already explains what the change is; trivial >> >> cosmetic cleanups. Cosmetic means it's completely superficial. >> > >> > And I have a rule not to apply patches without changelogs. So either I'll >> > need to write it for you, or can you add one pretty please? >> >> The commit message is right there. Maybe Jiri can apply it then, if >> not, then stay happy with your untidy code. > > First of all, I didn't say I wouldn't apply the patch, did I? > > Second, I asked you *nicely* to add a changelog so that I don't need to write > it for you. > > I don't know what made it difficult to understand. > > Anyway, I ask everyone to write changelogs and nobody has had any problems > with > that so far. I don't see why I should avoid asking you to follow the rules > that everybody else is asked to follow. If those rules are too difficult for > you to follow, I'm sorry. The patch has a commit message that describes exactly what it does. Unless there is valid feedback I will not send another version. To me, a valid criticism to the commit message would be: "I read X, but I thought it would do Y". For example; "I didn't expect the patch to do white-space cleanups", but I think that's exactly what people expect when they read "trivial costmetic cleanups'. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Fri, Aug 2, 2013 at 8:19 PM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 08:07:37 PM Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 8:16 PM, Rafael J. Wysocki wrote: >> > On Friday, August 02, 2013 08:04:52 PM Felipe Contreras wrote: >> >> On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki wrote: >> >> > On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote: >> >> >> If the _BCL package is descending, the first level (br->levels[2]) will >> >> >> be 0, and if the number of levels matches the number of steps, we might >> >> >> confuse a returned level to mean the index. >> >> >> >> >> >> For example: >> >> >> >> >> >> current_level = max_level = 100 >> >> >> test_level = 0 >> >> >> returned level = 100 >> >> >> >> >> >> In this case 100 means the level, not the index, and _BCM failed. But >> >> >> if >> >> >> the _BCL package is descending, the index of level 0 is also 100, so we >> >> >> assume _BQC is indexed, when it's not. >> >> >> >> >> >> This causes all _BQC calls to return bogus values causing weird >> >> >> behavior >> >> >> from the user's perspective. For example: xbacklight -set 10; >> >> >> xbacklight >> >> >> -set 20; would flash to 90% and then slowly down to the desired level >> >> >> (20). >> >> >> >> >> >> The solution is simple; test anything other than the first level (e.g. >> >> >> 1). >> >> >> >> >> >> Signed-off-by: Felipe Contreras >> >> > >> >> > Looks reasonable. >> >> > >> >> > Aaron, what do you think? >> >> >> >> Aaron has a similar patch does many more checks. I think we should add >> >> more checks, but I think those should go into a separate patch. >> >> >> >> This patch alone fixes a real problem, which is rather urgent to fix, >> >> and I did it this way so it's trivial to review and merge. >> > >> > And I still would like to know the Aaron's opinion, what's wrong with that? >> >> Nothing. What's wrong with my clarification? > > You're not Aaron. :-) I can clarify and comment without your permission. All you can do is disregard my comments, but others might find them useful, including Aaron. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] acpi: video: trivial style cleanups
On Fri, Aug 2, 2013 at 7:01 PM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 12:56:09 PM Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 9:09 AM, Rafael J. Wysocki wrote: >> > On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote: >> >> On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu wrote: >> >> > On 08/02/2013 07:44 AM, Felipe Contreras wrote: >> >> >> Signed-off-by: Felipe Contreras >> >> > >> >> > Change log please. >> >> >> >> You mean a commit message? >> > >> > No. He meant the part that goes between the subject and the signoff. >> > This is called a change log (or changelog). >> >> Not in Git lingo. >> >> % man git commit >> >> "Though not required, it’s a good idea to begin the commit message >> with a single short (less than 50 character) line summarizing the >> change, followed by a blank line and then a more thorough >> description." > > Please go and read this: https://lwn.net/Articles/560392/ OK, I've read it, and he doesn't seem to have a problem with trivial patches: "For the most trivial patches, the one-line summary might suffice" This patch falls into that category. > Now, you may still argue that your patches fall into the "add missing include > of foo.h" category, but it does several different things: > - fixes some whitespace, > - fixes a couple of static variable initializations, > - removes some braces, > - changes the placement of some lables (some of them unnecessarily). All of these fall under the category of coding style fixes. Jonathan Colbert lists the reasons for a detailed commit message description: * the original motivation for the work is quickly forgotten The motivation is clear; cleanup the code. * Andrew Morton also famously pushes developers to document the reasons explaining why a patch was written, including the user-visible effects of any bugs fixed It's clear; the reason for the cleanup is that the code is not clean. There's no user-visible effects of code cleanups. * Kernel developers do not like having to reverse engineer the intent of a patch years after the fact. The intent is clear; cleanup. > It would be simply *nice* to write what it does in the changelog so that > people reading the git log don't have to look deeper to see what changes the > author meant as "trivial style cleanups". Each time I've looked into a cleanup patch, I look at the code, never the description. Mistakes can be made in the cleanup, the description won't change that. I have landed these one-line-commit-message trivial cleanups in the kernel before. Never have I seen questions of; "what did this cleanup patch tried to do?", or; "the commit message says you fixed code-style A, but you also did whitespace changes, was that intended?" Be honest; if you apply this patch, nobody is going to see it ever again. > That's just a matter of making it easier to work with you for other people, > but maybe you just want to be difficult to work with in the first place? I add a proper description to the patches that deserve them. This patch is not one of them. I update the description according to the feedback to the patches that deserve that. This patch is not one of them. No user is ever going to be affected by this patch, I don't care that much if you apply it or not, it's not important. It's not worth my time to add a description that nobody is ever going to read, so if you are going to drop it, go ahead. If every time somebody provides a trivial cleanup patch you are going to ask people to describe what each little obvious change it does, it's no wonder the code doesn't get cleaned up. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Fri, Aug 2, 2013 at 8:16 PM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 08:04:52 PM Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki wrote: >> > On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote: >> >> If the _BCL package is descending, the first level (br->levels[2]) will >> >> be 0, and if the number of levels matches the number of steps, we might >> >> confuse a returned level to mean the index. >> >> >> >> For example: >> >> >> >> current_level = max_level = 100 >> >> test_level = 0 >> >> returned level = 100 >> >> >> >> In this case 100 means the level, not the index, and _BCM failed. But if >> >> the _BCL package is descending, the index of level 0 is also 100, so we >> >> assume _BQC is indexed, when it's not. >> >> >> >> This causes all _BQC calls to return bogus values causing weird behavior >> >> from the user's perspective. For example: xbacklight -set 10; xbacklight >> >> -set 20; would flash to 90% and then slowly down to the desired level >> >> (20). >> >> >> >> The solution is simple; test anything other than the first level (e.g. >> >> 1). >> >> >> >> Signed-off-by: Felipe Contreras >> > >> > Looks reasonable. >> > >> > Aaron, what do you think? >> >> Aaron has a similar patch does many more checks. I think we should add >> more checks, but I think those should go into a separate patch. >> >> This patch alone fixes a real problem, which is rather urgent to fix, >> and I did it this way so it's trivial to review and merge. > > And I still would like to know the Aaron's opinion, what's wrong with that? Nothing. What's wrong with my clarification? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote: >> If the _BCL package is descending, the first level (br->levels[2]) will >> be 0, and if the number of levels matches the number of steps, we might >> confuse a returned level to mean the index. >> >> For example: >> >> current_level = max_level = 100 >> test_level = 0 >> returned level = 100 >> >> In this case 100 means the level, not the index, and _BCM failed. But if >> the _BCL package is descending, the index of level 0 is also 100, so we >> assume _BQC is indexed, when it's not. >> >> This causes all _BQC calls to return bogus values causing weird behavior >> from the user's perspective. For example: xbacklight -set 10; xbacklight >> -set 20; would flash to 90% and then slowly down to the desired level >> (20). >> >> The solution is simple; test anything other than the first level (e.g. >> 1). >> >> Signed-off-by: Felipe Contreras > > Looks reasonable. > > Aaron, what do you think? Aaron has a similar patch does many more checks. I think we should add more checks, but I think those should go into a separate patch. This patch alone fixes a real problem, which is rather urgent to fix, and I did it this way so it's trivial to review and merge. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: i915 backlight
On Fri, Aug 2, 2013 at 6:35 PM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 01:58:55 PM Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki wrote: >> > On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote: >> >> I think it's pretty obvious that for the time being we need to >> >> blacklist a ton of machines so they boot without this OSI. In fact, in >> >> might make sense to simply remove the OSI completely for all machines >> >> (for now). >> > >> > That would have made sense 6 months ago, but not today. >> >> Today, like 6 months ago these machines remain broken, and it will be >> the same tomorrow, presumably on v3.11, and at least v3.12 as well. > > Can you possibly look at things from a bit broader perspective than just the > broken backlight? > > [I'm talking about "simply removing the OSI completely for all machines" if > that's not clear.] > > The problem is that for the last 6 months the kernel has responded to > OSI(Windows 2012) with a "yes" and now, after that time, you want it to > make a U turn and start saying "no" even though that may cause problems to > happen on other people's machines. That's simply irresponsible. The difference is we *know* *for sure* responding "Windows 2012" with a yes causes problems, on the other hand you *think* responding with a no, *might* cause problems. The only reason it would make sense to remain in the current situation is that if *knew* that switching to no would cause problems, but to my knowledge such problems are not real, merely theoretical. We have had proven brokedness for four releases (almost five now), if you disable it for v3.12 and it turns out it causes even more problems, then you enable it back again for v3.13, or even v3.12.1. The only way to know for certain is to go ahead and try it. But we know it's going to be all right, because v3.6 was all right. >> > The reason is that you don't really know what's affected by that and I'm >> > pretty sure it's not only backlight. >> >> I haven't heard a single comment that says acpi_osi="!Windows 2012" >> breaks other things. OTOH everybody is saying it fixes the backlight >> problem (if indeed it's the same problem). >> >> Are you claiming that those users are wrong? > > No, they are saying what they see and they are the people having the backlight > problem in the first place. You have no data from people for whom things work > now. The data comes from v3.6, who complained? Anyway, it's easy to find out; just disable it for *one release*. >> > So no, we won't do that. >> >> Yeah, because that would fix the backlight problems, not tomorrow, or >> several months from now, *today*. Geez, who would want that? >> >> Here is the patch to fix the problem, *today*. > > It doesn't "fix" anything. It just creates a blacklist of systems where > acpi_osi="!Windows 2012" happens to help with the backlight control problem. >From the user's point of view; right now it doesn't work, after the patch it does. That is a fix. > You don't even know why exactly it happens to work on those machines in the > first place and you don't know what is affected by that apart from backlight > (you can't be sure that nothing is affected in particular). I have a fairly good idea of the *real* problems involved with the backlight. The other problems you speak of are hypothetical, and yes, I don't know if there will be more problems, but neither do you. This is an argument from ignorance fallacy, and it's easy to solve; let's try it for one release and see how it goes. Then we would *know*. >> https://bugzilla.kernel.org/show_bug.cgi?id=60682 >> >> This is what we should do: >> >> 1) Improve that blacklist list >> 2) Fix the Intel driver issues >> 3) Enable your patch that uses the Intel driver instead >> 4) Remove that patch >> >> Anything else is not be good for the users. > > Actually, the users can easily put the acpi_osi="!Windows 2012" into the > kernel command line (which is what they have been doing already for some > time I suppose). The kernel should just work out-of-the-box. You can't expect every user out there to put all sorts of quirks into their boot command, that's why we have quirks in the kernel in the first place. > However, if we add the blacklist to the kernel, that will > mean we kind of give up fixing the backlight control for them (because they > won't have any incentive to test anything else then). No, it doesn't mean that. You should not break systems willingly and knowingly only to create incentives to the developers.
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Fri, Aug 2, 2013 at 4:21 PM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 02:12:49 PM Felipe Contreras wrote: >> You forgot this patch: >> >> https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next=3706231332d57072e0e2c0e59975443f3f18e673 >> >> Or do you think it's fine to boot these machines into a black screen? > > Seriously, what's wrong with you?! > > I didn't forget about it, I just didn't include it into this particular > pull request. > > And I'm not even sure I will push it for 3.11, because I prefer to revert > efaa14c for 3.11 if that's necessary to make your broken box work as before. The issue happens in more than just "my broken box", and yes, reverting that patch would help (in more than just my box), in the sense that at least Linux won't boot into a black screen. But the backlight control still wouldn't work, as it hasn't worked since v3.7, possibly in many ASUS laptops, for that you need more than just reverting efaa14c. > Well, perhaps I just won't push it at all so that you actually can go and > complain to Linus about that ... That is very responsible from you. Screw the users, right? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: i915 backlight
On Fri, Aug 2, 2013 at 3:11 PM, Josep Lladonosa wrote: > "Before" means with previous kernels that worked with > > GRUB_CMDLINE_LINUX="acpi_osi=Linux acpi_backlight=vendor" That's probably a different issue. You would need to bisect the problem. > I have not checked this issue with acpi_osi="!Windows 2012". Please do. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: i915 backlight
On Fri, Aug 2, 2013 at 3:03 PM, Josep Lladonosa wrote: > With this setup, something has happened: in xorg, when screen goes to > screensaver and after, enters into Standby mode, when I press a key, > it keeps black and, to recover screen, I have to adjust brightness > manually (by increasing), as if it didn't remember previous value to > standby mode. > > This was something that before didn't happen. You mean with acpi_osi="!Windows 2012"? And when you say "before", what do you mean? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] acpi: video: improve quirk check
If the _BCL package is descending, the first level (br->levels[2]) will be 0, and if the number of levels matches the number of steps, we might confuse a returned level to mean the index. For example: current_level = max_level = 100 test_level = 0 returned level = 100 In this case 100 means the level, not the index, and _BCM failed. But if the _BCL package is descending, the index of level 0 is also 100, so we assume _BQC is indexed, when it's not. This causes all _BQC calls to return bogus values causing weird behavior from the user's perspective. For example: xbacklight -set 10; xbacklight -set 20; would flash to 90% and then slowly down to the desired level (20). The solution is simple; test anything other than the first level (e.g. 1). Signed-off-by: Felipe Contreras --- On top of this we might want to test yet another value, because br->levels[3] might be the current value (although very unlikely). drivers/acpi/video.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 0ec434d..e1284b8 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -689,7 +689,7 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device, * Some systems always report current brightness level as maximum * through _BQC, we need to test another value for them. */ - test_level = current_level == max_level ? br->levels[2] : max_level; + test_level = current_level == max_level ? br->levels[3] : max_level; result = acpi_video_device_lcd_set_level(device, test_level); if (result) -- 1.8.4.rc1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Fri, Aug 2, 2013 at 9:29 AM, Rafael J. Wysocki wrote: > --- > > Colin Cross (1): > freezer: set PF_SUSPEND_TASK flag on tasks that call freeze_processes > > Lan Tianyu (1): > ACPI / battery: Fix parsing _BIX return value > > Rafael J. Wysocki (3): > Revert "cpuidle: Quickly notice prediction failure in general case" > Revert "cpuidle: Quickly notice prediction failure for repeat mode" > cpufreq: Fix cpufreq driver module refcount balance after suspend/resume > > --- You forgot this patch: https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next=3706231332d57072e0e2c0e59975443f3f18e673 Or do you think it's fine to boot these machines into a black screen? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: i915 backlight
On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa wrote: >> > Hello, >> > >> > I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to >> > change to this parameter to the kernel boot: >> > >> > >> > GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\"" >> >> I think it's pretty obvious that for the time being we need to >> blacklist a ton of machines so they boot without this OSI. In fact, in >> might make sense to simply remove the OSI completely for all machines >> (for now). > > That would have made sense 6 months ago, but not today. Today, like 6 months ago these machines remain broken, and it will be the same tomorrow, presumably on v3.11, and at least v3.12 as well. > The reason is that you don't really know what's affected by that and I'm > pretty sure it's not only backlight. I haven't heard a single comment that says acpi_osi="!Windows 2012" breaks other things. OTOH everybody is saying it fixes the backlight problem (if indeed it's the same problem). Are you claiming that those users are wrong? > So no, we won't do that. Yeah, because that would fix the backlight problems, not tomorrow, or several months from now, *today*. Geez, who would want that? Here is the patch to fix the problem, *today*. https://bugzilla.kernel.org/show_bug.cgi?id=60682 This is what we should do: 1) Improve that blacklist list 2) Fix the Intel driver issues 3) Enable your patch that uses the Intel driver instead 4) Remove that patch Anything else is not be good for the users. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] acpi: video: trivial style cleanups
On Fri, Aug 2, 2013 at 9:09 AM, Rafael J. Wysocki wrote: > On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote: >> On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu wrote: >> > On 08/02/2013 07:44 AM, Felipe Contreras wrote: >> >> Signed-off-by: Felipe Contreras >> > >> > Change log please. >> >> You mean a commit message? > > No. He meant the part that goes between the subject and the signoff. > This is called a change log (or changelog). Not in Git lingo. % man git commit "Though not required, it’s a good idea to begin the commit message with a single short (less than 50 character) line summarizing the change, followed by a blank line and then a more thorough description." >> That's what it's called in Git lingo, and >> it's right there: >> >> acpi: video: trivial style cleanups > > And that's not sufficient, because it doesn't explain *what* cleanups are > being made. Yes it does; *style* cleanups (coding-style). -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups
On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki wrote: > On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote: >> On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu wrote: >> > On 08/02/2013 07:43 AM, Felipe Contreras wrote: >> >> Signed-off-by: Felipe Contreras >> > >> > Please add change log explaining what you have changed. >> > It seems that the patch modify comment style only, some add a space and >> > some change spaces to tab, is it the case? >> >> The commit message already explains what the change is; trivial >> cosmetic cleanups. Cosmetic means it's completely superficial. > > And I have a rule not to apply patches without changelogs. So either I'll > need to write it for you, or can you add one pretty please? The commit message is right there. Maybe Jiri can apply it then, if not, then stay happy with your untidy code. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Fri, Aug 2, 2013 at 3:25 AM, Aaron Lu wrote:
> On 08/02/2013 04:14 PM, Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 3:06 AM, Aaron Lu wrote:
>>> On 08/02/2013 03:59 PM, Felipe Contreras wrote:
>>>> On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu wrote:
>>>>> On 08/02/2013 02:44 PM, Felipe Contreras wrote:
>>>>
>>>>>> The initial _BCM commands don't work, so the level remains at 100%.
>>>>>> Since the level is max_level, acpi_video_bqc_quirk() tries with the
>>>>>> first level, which is 0, and 0 happens to be the index of 100.
>>>>>>
>>>>>> So _BQC is returning 100, which is not the index of 0 (what we tested
>>>>>> for), but actually 100.
>>>>>>
>>>>>> I think the current code is correct, but acpi_video_bqc_quirk() should
>>>>>> be testing br->levels[3], or anything other than 0/100 which can be
>>>>>> easily confused.
>>>>>>
>>>>>> If so, the code would find that _BQC doesn't work on this machine (in
>>>>>> win8 mode)... at least initially. My guess is that it only starts to
>>>>>> work after acpi_video_bus_start_devices() is called.
>>>>>>
>>>>>> Forcing br->flags._BQC_use_index = 0 seems to work.
>>>>>
>>>>> Seems ASUS machines tend to have this issue:
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=56711
>>>>
>>>> I don't see any real solution for the ACPI driver.
>>>>
>>>>> I have a patch to enhance the quirk some time ago:
>>>>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>>>>
>>>> I think this is unnecessarily complicated; the comment makes it clear
>>>
>>> For your system, yes it is unnecessarily complicated. But since this is
>>> a quirk, it better solves as many potential problems as possible, or we
>>> would simply use a DMI entry to do the quirk.
>>
>> The only difference between my patch and yours is that your patch
>> checks that br->level[i] is not the current level, but that check is
>> not necessary. If _BQC always returns the max level, all we need to do
>
> _BQC does not always returns the max level.
>
>> is pick another value, any other value, and br->level[3] works just
>> fine.
>
> For a _BCL only having 4 elements { 100, 40, 40, 100 }, the br->levels[3]
> will be the max level. The example here may be too crazy to be true, but
> since we are dealing with firmware, I tend to believe anything could
> happen.
That can be fixed easily by checking test_level == current_level and
do the same your patch does, but I actually don't think we should do
that. It might make sense to test two values instead of only one, that
way we can we properly test _BQC, while your patch simply assumes it
doesn't work, even though it might.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Fri, Aug 2, 2013 at 3:06 AM, Aaron Lu wrote: > On 08/02/2013 03:59 PM, Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu wrote: >>> On 08/02/2013 02:44 PM, Felipe Contreras wrote: >> >>>> The initial _BCM commands don't work, so the level remains at 100%. >>>> Since the level is max_level, acpi_video_bqc_quirk() tries with the >>>> first level, which is 0, and 0 happens to be the index of 100. >>>> >>>> So _BQC is returning 100, which is not the index of 0 (what we tested >>>> for), but actually 100. >>>> >>>> I think the current code is correct, but acpi_video_bqc_quirk() should >>>> be testing br->levels[3], or anything other than 0/100 which can be >>>> easily confused. >>>> >>>> If so, the code would find that _BQC doesn't work on this machine (in >>>> win8 mode)... at least initially. My guess is that it only starts to >>>> work after acpi_video_bus_start_devices() is called. >>>> >>>> Forcing br->flags._BQC_use_index = 0 seems to work. >>> >>> Seems ASUS machines tend to have this issue: >>> https://bugzilla.kernel.org/show_bug.cgi?id=52951 >>> https://bugzilla.kernel.org/show_bug.cgi?id=56711 >> >> I don't see any real solution for the ACPI driver. >> >>> I have a patch to enhance the quirk some time ago: >>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 >> >> I think this is unnecessarily complicated; the comment makes it clear > > For your system, yes it is unnecessarily complicated. But since this is > a quirk, it better solves as many potential problems as possible, or we > would simply use a DMI entry to do the quirk. The only difference between my patch and yours is that your patch checks that br->level[i] is not the current level, but that check is not necessary. If _BQC always returns the max level, all we need to do is pick another value, any other value, and br->level[3] works just fine. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu wrote: > On 08/02/2013 02:44 PM, Felipe Contreras wrote: >> The initial _BCM commands don't work, so the level remains at 100%. >> Since the level is max_level, acpi_video_bqc_quirk() tries with the >> first level, which is 0, and 0 happens to be the index of 100. >> >> So _BQC is returning 100, which is not the index of 0 (what we tested >> for), but actually 100. >> >> I think the current code is correct, but acpi_video_bqc_quirk() should >> be testing br->levels[3], or anything other than 0/100 which can be >> easily confused. >> >> If so, the code would find that _BQC doesn't work on this machine (in >> win8 mode)... at least initially. My guess is that it only starts to >> work after acpi_video_bus_start_devices() is called. >> >> Forcing br->flags._BQC_use_index = 0 seems to work. > > Seems ASUS machines tend to have this issue: > https://bugzilla.kernel.org/show_bug.cgi?id=52951 > https://bugzilla.kernel.org/show_bug.cgi?id=56711 I don't see any real solution for the ACPI driver. > I have a patch to enhance the quirk some time ago: > https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 I think this is unnecessarily complicated; the comment makes it clear that the purpose is to find out if _BQC is working, and this does the trick: >From 2bfa401b0a50fcde292ac0eb60cb6f857caf2fc6 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Fri, 2 Aug 2013 02:27:44 -0500 Subject: [PATCH] acpi: video: improve quirk check If the _BCL package is descending, the first level (br->levels[2]) will be 0, and if the number of levels matches the number of steps, we might confuse a returned level to mean the index. For example: current_level = max_level = 100 test_level = 0 returned level = 100 In this case 100 means the level, not the index, and _BCM failed. But if the _BCL package is descending, the index of level 0 is also 100, so we assume _BQC is indexed, when it's not. The solution is simple; test anything other than the first level (e.g. 1). Signed-off-by: Felipe Contreras --- drivers/acpi/video.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 0ec434d..e1284b8 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -689,7 +689,7 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device, * Some systems always report current brightness level as maximum * through _BQC, we need to test another value for them. */ - test_level = current_level == max_level ? br->levels[2] : max_level; + test_level = current_level == max_level ? br->levels[3] : max_level; result = acpi_video_device_lcd_set_level(device, test_level); if (result) -- 1.8.3.4 -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: i915 backlight
On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa wrote: > Hello, > > I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to > change to this parameter to the kernel boot: > > > GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\"" I think it's pretty obvious that for the time being we need to blacklist a ton of machines so they boot without this OSI. In fact, in might make sense to simply remove the OSI completely for all machines (for now). -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Thu, Aug 1, 2013 at 11:59 PM, Aaron Lu wrote: > On 08/02/2013 12:50 PM, Felipe Contreras wrote: >> On Thu, Aug 1, 2013 at 11:30 PM, Aaron Lu wrote: >>> Please see acpi_video_bqc_quirk, we set _BQC_use_index by revert the >>> level on a reversed _BCL, so we will need to revert level here too. >> >> I cannot parse that sentence, but nothing needs to change there; to >> find out if _BQC is using an index, we need to see if the returned >> value is the index of the level we are looking for, and to find that >> out we need the original list of levels, which can be found by >> reverting the already reverted list. If this wasn't the case there > > Yes, but instead of reverting the already reverted list, we revert the > returned value to get the correct index in the reverted list. But your > patch removes the revert, is it correct? It removes the revert in acpi_video_bqc_value_to_level(), not acpi_video_bqc_quirk(). If I remove both reverts it doesn't work on this machine, however, I think it works by accident. The initial _BCM commands don't work, so the level remains at 100%. Since the level is max_level, acpi_video_bqc_quirk() tries with the first level, which is 0, and 0 happens to be the index of 100. So _BQC is returning 100, which is not the index of 0 (what we tested for), but actually 100. I think the current code is correct, but acpi_video_bqc_quirk() should be testing br->levels[3], or anything other than 0/100 which can be easily confused. If so, the code would find that _BQC doesn't work on this machine (in win8 mode)... at least initially. My guess is that it only starts to work after acpi_video_bus_start_devices() is called. Forcing br->flags._BQC_use_index = 0 seems to work. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups
On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote: On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu aaron@gmail.com wrote: On 08/02/2013 07:43 AM, Felipe Contreras wrote: Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Please add change log explaining what you have changed. It seems that the patch modify comment style only, some add a space and some change spaces to tab, is it the case? The commit message already explains what the change is; trivial cosmetic cleanups. Cosmetic means it's completely superficial. And I have a rule not to apply patches without changelogs. So either I'll need to write it for you, or can you add one pretty please? The commit message is right there. Maybe Jiri can apply it then, if not, then stay happy with your untidy code. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] acpi: video: trivial style cleanups
On Fri, Aug 2, 2013 at 9:09 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote: On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu aaron@gmail.com wrote: On 08/02/2013 07:44 AM, Felipe Contreras wrote: Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Change log please. You mean a commit message? No. He meant the part that goes between the subject and the signoff. This is called a change log (or changelog). Not in Git lingo. % man git commit Though not required, it’s a good idea to begin the commit message with a single short (less than 50 character) line summarizing the change, followed by a blank line and then a more thorough description. That's what it's called in Git lingo, and it's right there: acpi: video: trivial style cleanups And that's not sufficient, because it doesn't explain *what* cleanups are being made. Yes it does; *style* cleanups (coding-style). -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: i915 backlight
On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa jllad...@gmail.com wrote: Hello, I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to change to this parameter to the kernel boot: GRUB_CMDLINE_LINUX=acpi_osi=\!Windows 2012\ I think it's pretty obvious that for the time being we need to blacklist a ton of machines so they boot without this OSI. In fact, in might make sense to simply remove the OSI completely for all machines (for now). That would have made sense 6 months ago, but not today. Today, like 6 months ago these machines remain broken, and it will be the same tomorrow, presumably on v3.11, and at least v3.12 as well. The reason is that you don't really know what's affected by that and I'm pretty sure it's not only backlight. I haven't heard a single comment that says acpi_osi=!Windows 2012 breaks other things. OTOH everybody is saying it fixes the backlight problem (if indeed it's the same problem). Are you claiming that those users are wrong? So no, we won't do that. Yeah, because that would fix the backlight problems, not tomorrow, or several months from now, *today*. Geez, who would want that? Here is the patch to fix the problem, *today*. https://bugzilla.kernel.org/show_bug.cgi?id=60682 This is what we should do: 1) Improve that blacklist list 2) Fix the Intel driver issues 3) Enable your patch that uses the Intel driver instead 4) Remove that patch Anything else is not be good for the users. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Fri, Aug 2, 2013 at 9:29 AM, Rafael J. Wysocki r...@sisk.pl wrote: --- Colin Cross (1): freezer: set PF_SUSPEND_TASK flag on tasks that call freeze_processes Lan Tianyu (1): ACPI / battery: Fix parsing _BIX return value Rafael J. Wysocki (3): Revert cpuidle: Quickly notice prediction failure in general case Revert cpuidle: Quickly notice prediction failure for repeat mode cpufreq: Fix cpufreq driver module refcount balance after suspend/resume --- You forgot this patch: https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-nextid=3706231332d57072e0e2c0e59975443f3f18e673 Or do you think it's fine to boot these machines into a black screen? -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] acpi: video: improve quirk check
If the _BCL package is descending, the first level (br-levels[2]) will be 0, and if the number of levels matches the number of steps, we might confuse a returned level to mean the index. For example: current_level = max_level = 100 test_level = 0 returned level = 100 In this case 100 means the level, not the index, and _BCM failed. But if the _BCL package is descending, the index of level 0 is also 100, so we assume _BQC is indexed, when it's not. This causes all _BQC calls to return bogus values causing weird behavior from the user's perspective. For example: xbacklight -set 10; xbacklight -set 20; would flash to 90% and then slowly down to the desired level (20). The solution is simple; test anything other than the first level (e.g. 1). Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- On top of this we might want to test yet another value, because br-levels[3] might be the current value (although very unlikely). drivers/acpi/video.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 0ec434d..e1284b8 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -689,7 +689,7 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device, * Some systems always report current brightness level as maximum * through _BQC, we need to test another value for them. */ - test_level = current_level == max_level ? br-levels[2] : max_level; + test_level = current_level == max_level ? br-levels[3] : max_level; result = acpi_video_device_lcd_set_level(device, test_level); if (result) -- 1.8.4.rc1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: i915 backlight
On Fri, Aug 2, 2013 at 3:03 PM, Josep Lladonosa jllad...@gmail.com wrote: With this setup, something has happened: in xorg, when screen goes to screensaver and after, enters into Standby mode, when I press a key, it keeps black and, to recover screen, I have to adjust brightness manually (by increasing), as if it didn't remember previous value to standby mode. This was something that before didn't happen. You mean with acpi_osi=!Windows 2012? And when you say before, what do you mean? -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: i915 backlight
On Fri, Aug 2, 2013 at 3:11 PM, Josep Lladonosa jllad...@gmail.com wrote: Before means with previous kernels that worked with GRUB_CMDLINE_LINUX=acpi_osi=Linux acpi_backlight=vendor That's probably a different issue. You would need to bisect the problem. I have not checked this issue with acpi_osi=!Windows 2012. Please do. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Fri, Aug 2, 2013 at 4:21 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 02:12:49 PM Felipe Contreras wrote: You forgot this patch: https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-nextid=3706231332d57072e0e2c0e59975443f3f18e673 Or do you think it's fine to boot these machines into a black screen? Seriously, what's wrong with you?! I didn't forget about it, I just didn't include it into this particular pull request. And I'm not even sure I will push it for 3.11, because I prefer to revert efaa14c for 3.11 if that's necessary to make your broken box work as before. The issue happens in more than just my broken box, and yes, reverting that patch would help (in more than just my box), in the sense that at least Linux won't boot into a black screen. But the backlight control still wouldn't work, as it hasn't worked since v3.7, possibly in many ASUS laptops, for that you need more than just reverting efaa14c. Well, perhaps I just won't push it at all so that you actually can go and complain to Linus about that ... That is very responsible from you. Screw the users, right? -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: i915 backlight
On Fri, Aug 2, 2013 at 6:35 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 01:58:55 PM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote: I think it's pretty obvious that for the time being we need to blacklist a ton of machines so they boot without this OSI. In fact, in might make sense to simply remove the OSI completely for all machines (for now). That would have made sense 6 months ago, but not today. Today, like 6 months ago these machines remain broken, and it will be the same tomorrow, presumably on v3.11, and at least v3.12 as well. Can you possibly look at things from a bit broader perspective than just the broken backlight? [I'm talking about simply removing the OSI completely for all machines if that's not clear.] The problem is that for the last 6 months the kernel has responded to OSI(Windows 2012) with a yes and now, after that time, you want it to make a U turn and start saying no even though that may cause problems to happen on other people's machines. That's simply irresponsible. The difference is we *know* *for sure* responding Windows 2012 with a yes causes problems, on the other hand you *think* responding with a no, *might* cause problems. The only reason it would make sense to remain in the current situation is that if *knew* that switching to no would cause problems, but to my knowledge such problems are not real, merely theoretical. We have had proven brokedness for four releases (almost five now), if you disable it for v3.12 and it turns out it causes even more problems, then you enable it back again for v3.13, or even v3.12.1. The only way to know for certain is to go ahead and try it. But we know it's going to be all right, because v3.6 was all right. The reason is that you don't really know what's affected by that and I'm pretty sure it's not only backlight. I haven't heard a single comment that says acpi_osi=!Windows 2012 breaks other things. OTOH everybody is saying it fixes the backlight problem (if indeed it's the same problem). Are you claiming that those users are wrong? No, they are saying what they see and they are the people having the backlight problem in the first place. You have no data from people for whom things work now. The data comes from v3.6, who complained? Anyway, it's easy to find out; just disable it for *one release*. So no, we won't do that. Yeah, because that would fix the backlight problems, not tomorrow, or several months from now, *today*. Geez, who would want that? Here is the patch to fix the problem, *today*. It doesn't fix anything. It just creates a blacklist of systems where acpi_osi=!Windows 2012 happens to help with the backlight control problem. From the user's point of view; right now it doesn't work, after the patch it does. That is a fix. You don't even know why exactly it happens to work on those machines in the first place and you don't know what is affected by that apart from backlight (you can't be sure that nothing is affected in particular). I have a fairly good idea of the *real* problems involved with the backlight. The other problems you speak of are hypothetical, and yes, I don't know if there will be more problems, but neither do you. This is an argument from ignorance fallacy, and it's easy to solve; let's try it for one release and see how it goes. Then we would *know*. https://bugzilla.kernel.org/show_bug.cgi?id=60682 This is what we should do: 1) Improve that blacklist list 2) Fix the Intel driver issues 3) Enable your patch that uses the Intel driver instead 4) Remove that patch Anything else is not be good for the users. Actually, the users can easily put the acpi_osi=!Windows 2012 into the kernel command line (which is what they have been doing already for some time I suppose). The kernel should just work out-of-the-box. You can't expect every user out there to put all sorts of quirks into their boot command, that's why we have quirks in the kernel in the first place. However, if we add the blacklist to the kernel, that will mean we kind of give up fixing the backlight control for them (because they won't have any incentive to test anything else then). No, it doesn't mean that. You should not break systems willingly and knowingly only to create incentives to the developers. When things are ready, you enable the fix again, if they break, you disable it again. At no point in time does it make sense to retain code that we know breaks user-experience. That said this is a controverisal matter and we evidently don't agree with each other. I have my reasons, you have your arguments and it doesn't look like any of us is likely to change his mind, so why don't we do what's normally done in such cases: Why don't we ask others? Matthew, Aaron, Rui, what do you think about this? Should we create
Re: [PATCH] acpi: video: improve quirk check
On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote: If the _BCL package is descending, the first level (br-levels[2]) will be 0, and if the number of levels matches the number of steps, we might confuse a returned level to mean the index. For example: current_level = max_level = 100 test_level = 0 returned level = 100 In this case 100 means the level, not the index, and _BCM failed. But if the _BCL package is descending, the index of level 0 is also 100, so we assume _BQC is indexed, when it's not. This causes all _BQC calls to return bogus values causing weird behavior from the user's perspective. For example: xbacklight -set 10; xbacklight -set 20; would flash to 90% and then slowly down to the desired level (20). The solution is simple; test anything other than the first level (e.g. 1). Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Looks reasonable. Aaron, what do you think? Aaron has a similar patch does many more checks. I think we should add more checks, but I think those should go into a separate patch. This patch alone fixes a real problem, which is rather urgent to fix, and I did it this way so it's trivial to review and merge. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Fri, Aug 2, 2013 at 8:16 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 08:04:52 PM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote: If the _BCL package is descending, the first level (br-levels[2]) will be 0, and if the number of levels matches the number of steps, we might confuse a returned level to mean the index. For example: current_level = max_level = 100 test_level = 0 returned level = 100 In this case 100 means the level, not the index, and _BCM failed. But if the _BCL package is descending, the index of level 0 is also 100, so we assume _BQC is indexed, when it's not. This causes all _BQC calls to return bogus values causing weird behavior from the user's perspective. For example: xbacklight -set 10; xbacklight -set 20; would flash to 90% and then slowly down to the desired level (20). The solution is simple; test anything other than the first level (e.g. 1). Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Looks reasonable. Aaron, what do you think? Aaron has a similar patch does many more checks. I think we should add more checks, but I think those should go into a separate patch. This patch alone fixes a real problem, which is rather urgent to fix, and I did it this way so it's trivial to review and merge. And I still would like to know the Aaron's opinion, what's wrong with that? Nothing. What's wrong with my clarification? -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] acpi: video: trivial style cleanups
On Fri, Aug 2, 2013 at 7:01 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 12:56:09 PM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 9:09 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote: On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu aaron@gmail.com wrote: On 08/02/2013 07:44 AM, Felipe Contreras wrote: Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Change log please. You mean a commit message? No. He meant the part that goes between the subject and the signoff. This is called a change log (or changelog). Not in Git lingo. % man git commit Though not required, it’s a good idea to begin the commit message with a single short (less than 50 character) line summarizing the change, followed by a blank line and then a more thorough description. Please go and read this: https://lwn.net/Articles/560392/ OK, I've read it, and he doesn't seem to have a problem with trivial patches: For the most trivial patches, the one-line summary might suffice This patch falls into that category. Now, you may still argue that your patches fall into the add missing include of foo.h category, but it does several different things: - fixes some whitespace, - fixes a couple of static variable initializations, - removes some braces, - changes the placement of some lables (some of them unnecessarily). All of these fall under the category of coding style fixes. Jonathan Colbert lists the reasons for a detailed commit message description: * the original motivation for the work is quickly forgotten The motivation is clear; cleanup the code. * Andrew Morton also famously pushes developers to document the reasons explaining why a patch was written, including the user-visible effects of any bugs fixed It's clear; the reason for the cleanup is that the code is not clean. There's no user-visible effects of code cleanups. * Kernel developers do not like having to reverse engineer the intent of a patch years after the fact. The intent is clear; cleanup. It would be simply *nice* to write what it does in the changelog so that people reading the git log don't have to look deeper to see what changes the author meant as trivial style cleanups. Each time I've looked into a cleanup patch, I look at the code, never the description. Mistakes can be made in the cleanup, the description won't change that. I have landed these one-line-commit-message trivial cleanups in the kernel before. Never have I seen questions of; what did this cleanup patch tried to do?, or; the commit message says you fixed code-style A, but you also did whitespace changes, was that intended? Be honest; if you apply this patch, nobody is going to see it ever again. That's just a matter of making it easier to work with you for other people, but maybe you just want to be difficult to work with in the first place? I add a proper description to the patches that deserve them. This patch is not one of them. I update the description according to the feedback to the patches that deserve that. This patch is not one of them. No user is ever going to be affected by this patch, I don't care that much if you apply it or not, it's not important. It's not worth my time to add a description that nobody is ever going to read, so if you are going to drop it, go ahead. If every time somebody provides a trivial cleanup patch you are going to ask people to describe what each little obvious change it does, it's no wonder the code doesn't get cleaned up. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: improve quirk check
On Fri, Aug 2, 2013 at 8:19 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 08:07:37 PM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 8:16 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 08:04:52 PM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote: If the _BCL package is descending, the first level (br-levels[2]) will be 0, and if the number of levels matches the number of steps, we might confuse a returned level to mean the index. For example: current_level = max_level = 100 test_level = 0 returned level = 100 In this case 100 means the level, not the index, and _BCM failed. But if the _BCL package is descending, the index of level 0 is also 100, so we assume _BQC is indexed, when it's not. This causes all _BQC calls to return bogus values causing weird behavior from the user's perspective. For example: xbacklight -set 10; xbacklight -set 20; would flash to 90% and then slowly down to the desired level (20). The solution is simple; test anything other than the first level (e.g. 1). Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Looks reasonable. Aaron, what do you think? Aaron has a similar patch does many more checks. I think we should add more checks, but I think those should go into a separate patch. This patch alone fixes a real problem, which is rather urgent to fix, and I did it this way so it's trivial to review and merge. And I still would like to know the Aaron's opinion, what's wrong with that? Nothing. What's wrong with my clarification? You're not Aaron. :-) I can clarify and comment without your permission. All you can do is disregard my comments, but others might find them useful, including Aaron. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups
On Fri, Aug 2, 2013 at 7:07 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 12:52:18 PM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote: On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu aaron@gmail.com wrote: On 08/02/2013 07:43 AM, Felipe Contreras wrote: Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Please add change log explaining what you have changed. It seems that the patch modify comment style only, some add a space and some change spaces to tab, is it the case? The commit message already explains what the change is; trivial cosmetic cleanups. Cosmetic means it's completely superficial. And I have a rule not to apply patches without changelogs. So either I'll need to write it for you, or can you add one pretty please? The commit message is right there. Maybe Jiri can apply it then, if not, then stay happy with your untidy code. First of all, I didn't say I wouldn't apply the patch, did I? Second, I asked you *nicely* to add a changelog so that I don't need to write it for you. I don't know what made it difficult to understand. Anyway, I ask everyone to write changelogs and nobody has had any problems with that so far. I don't see why I should avoid asking you to follow the rules that everybody else is asked to follow. If those rules are too difficult for you to follow, I'm sorry. The patch has a commit message that describes exactly what it does. Unless there is valid feedback I will not send another version. To me, a valid criticism to the commit message would be: I read X, but I thought it would do Y. For example; I didn't expect the patch to do white-space cleanups, but I think that's exactly what people expect when they read trivial costmetic cleanups'. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] ACPI and power management fixes for v3.11-rc4
On Fri, Aug 2, 2013 at 5:21 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 04:31:37 PM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 4:21 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 02:12:49 PM Felipe Contreras wrote: You forgot this patch: https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-nextid=3706231332d57072e0e2c0e59975443f3f18e673 Or do you think it's fine to boot these machines into a black screen? Seriously, what's wrong with you?! I didn't forget about it, I just didn't include it into this particular pull request. And I'm not even sure I will push it for 3.11, because I prefer to revert efaa14c for 3.11 if that's necessary to make your broken box work as before. The issue happens in more than just my broken box, and yes, reverting that patch would help (in more than just my box), in the sense that at least Linux won't boot into a black screen. But the backlight control still wouldn't work, as it hasn't worked since v3.7, possibly in many ASUS laptops, for that you need more than just reverting efaa14c. Yes, last time it worked in 3.6 and in particular it doesn't work in 3.10. My current goal is bring things back to the 3.10 state first, possibly without introducing any new problems, because we're kind of late in the cycle. That's better done by reverting stuff known to have introduced problems in the first place and not by doing things that may introduce more of them. And your blacklisting patch has potential to introduce problems. Your goal is to bring backlight control to the 3.6 state on that particular machine, but the blacklist is done at the *system* level and very well may affect more things than just backlight. You may not see any problems resulting from it and you may not care even if there are some, but other users of it may use different user space, for example, and may see problems that you're not seeing. That's why I'd very much prefer to do the revert at this point. Yes, that's fine, either the revert, or the patch I mentioned, or something else, but something has to be done, and it was better to do it in v3.11-rc4 than in v3.11-rc5, because that change itself can cause further problems. Well, perhaps I just won't push it at all so that you actually can go and complain to Linus about that ... That is very responsible from you. Screw the users, right? No, that's not my goal, sorry for disappointing you. The problem is that I'm not really convinced about the validity of the blacklisting approach to begin with. As I said, the blacklisting is done on the system level and the goal is to work around backlight control problems. That sounds like a sledgehammer approach to me, which I don't really like. If the blacklisting was more targeted, done at the video driver level etc., I wouldn't really have any concerns about it, but that's not the case. And since people evidently could live for over 6 months with the backlight control problems, maybe they'll survive some more time still and allow us to find a better approach? They probably can survive without Linux at all, that doesn't mean we are doing our job. Let's do a though experiment, let's say you are right, and they can survive the 6 months it would take you to find the perfect solution, say in v3.13. What's wrong with having a partial solution in v3.12? If the blacklisting doesn't work properly (there's absolutely no evidence for that), then you revert it on v3.12.1. What's wrong with that approach? -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Thu, Aug 1, 2013 at 11:59 PM, Aaron Lu aaron...@intel.com wrote: On 08/02/2013 12:50 PM, Felipe Contreras wrote: On Thu, Aug 1, 2013 at 11:30 PM, Aaron Lu aaron...@intel.com wrote: Please see acpi_video_bqc_quirk, we set _BQC_use_index by revert the level on a reversed _BCL, so we will need to revert level here too. I cannot parse that sentence, but nothing needs to change there; to find out if _BQC is using an index, we need to see if the returned value is the index of the level we are looking for, and to find that out we need the original list of levels, which can be found by reverting the already reverted list. If this wasn't the case there Yes, but instead of reverting the already reverted list, we revert the returned value to get the correct index in the reverted list. But your patch removes the revert, is it correct? It removes the revert in acpi_video_bqc_value_to_level(), not acpi_video_bqc_quirk(). If I remove both reverts it doesn't work on this machine, however, I think it works by accident. The initial _BCM commands don't work, so the level remains at 100%. Since the level is max_level, acpi_video_bqc_quirk() tries with the first level, which is 0, and 0 happens to be the index of 100. So _BQC is returning 100, which is not the index of 0 (what we tested for), but actually 100. I think the current code is correct, but acpi_video_bqc_quirk() should be testing br-levels[3], or anything other than 0/100 which can be easily confused. If so, the code would find that _BQC doesn't work on this machine (in win8 mode)... at least initially. My guess is that it only starts to work after acpi_video_bus_start_devices() is called. Forcing br-flags._BQC_use_index = 0 seems to work. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: i915 backlight
On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa jllad...@gmail.com wrote: Hello, I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to change to this parameter to the kernel boot: GRUB_CMDLINE_LINUX=acpi_osi=\!Windows 2012\ I think it's pretty obvious that for the time being we need to blacklist a ton of machines so they boot without this OSI. In fact, in might make sense to simply remove the OSI completely for all machines (for now). -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu aaron...@intel.com wrote: On 08/02/2013 02:44 PM, Felipe Contreras wrote: The initial _BCM commands don't work, so the level remains at 100%. Since the level is max_level, acpi_video_bqc_quirk() tries with the first level, which is 0, and 0 happens to be the index of 100. So _BQC is returning 100, which is not the index of 0 (what we tested for), but actually 100. I think the current code is correct, but acpi_video_bqc_quirk() should be testing br-levels[3], or anything other than 0/100 which can be easily confused. If so, the code would find that _BQC doesn't work on this machine (in win8 mode)... at least initially. My guess is that it only starts to work after acpi_video_bus_start_devices() is called. Forcing br-flags._BQC_use_index = 0 seems to work. Seems ASUS machines tend to have this issue: https://bugzilla.kernel.org/show_bug.cgi?id=52951 https://bugzilla.kernel.org/show_bug.cgi?id=56711 I don't see any real solution for the ACPI driver. I have a patch to enhance the quirk some time ago: https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 I think this is unnecessarily complicated; the comment makes it clear that the purpose is to find out if _BQC is working, and this does the trick: From 2bfa401b0a50fcde292ac0eb60cb6f857caf2fc6 Mon Sep 17 00:00:00 2001 From: Felipe Contreras felipe.contre...@gmail.com Date: Fri, 2 Aug 2013 02:27:44 -0500 Subject: [PATCH] acpi: video: improve quirk check If the _BCL package is descending, the first level (br-levels[2]) will be 0, and if the number of levels matches the number of steps, we might confuse a returned level to mean the index. For example: current_level = max_level = 100 test_level = 0 returned level = 100 In this case 100 means the level, not the index, and _BCM failed. But if the _BCL package is descending, the index of level 0 is also 100, so we assume _BQC is indexed, when it's not. The solution is simple; test anything other than the first level (e.g. 1). Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- drivers/acpi/video.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 0ec434d..e1284b8 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -689,7 +689,7 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device, * Some systems always report current brightness level as maximum * through _BQC, we need to test another value for them. */ - test_level = current_level == max_level ? br-levels[2] : max_level; + test_level = current_level == max_level ? br-levels[3] : max_level; result = acpi_video_device_lcd_set_level(device, test_level); if (result) -- 1.8.3.4 -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Fri, Aug 2, 2013 at 3:06 AM, Aaron Lu aaron...@intel.com wrote: On 08/02/2013 03:59 PM, Felipe Contreras wrote: On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu aaron...@intel.com wrote: On 08/02/2013 02:44 PM, Felipe Contreras wrote: The initial _BCM commands don't work, so the level remains at 100%. Since the level is max_level, acpi_video_bqc_quirk() tries with the first level, which is 0, and 0 happens to be the index of 100. So _BQC is returning 100, which is not the index of 0 (what we tested for), but actually 100. I think the current code is correct, but acpi_video_bqc_quirk() should be testing br-levels[3], or anything other than 0/100 which can be easily confused. If so, the code would find that _BQC doesn't work on this machine (in win8 mode)... at least initially. My guess is that it only starts to work after acpi_video_bus_start_devices() is called. Forcing br-flags._BQC_use_index = 0 seems to work. Seems ASUS machines tend to have this issue: https://bugzilla.kernel.org/show_bug.cgi?id=52951 https://bugzilla.kernel.org/show_bug.cgi?id=56711 I don't see any real solution for the ACPI driver. I have a patch to enhance the quirk some time ago: https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9 I think this is unnecessarily complicated; the comment makes it clear For your system, yes it is unnecessarily complicated. But since this is a quirk, it better solves as many potential problems as possible, or we would simply use a DMI entry to do the quirk. The only difference between my patch and yours is that your patch checks that br-level[i] is not the current level, but that check is not necessary. If _BQC always returns the max level, all we need to do is pick another value, any other value, and br-level[3] works just fine. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Fri, Aug 2, 2013 at 3:25 AM, Aaron Lu aaron...@intel.com wrote:
On 08/02/2013 04:14 PM, Felipe Contreras wrote:
On Fri, Aug 2, 2013 at 3:06 AM, Aaron Lu aaron...@intel.com wrote:
On 08/02/2013 03:59 PM, Felipe Contreras wrote:
On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu aaron...@intel.com wrote:
On 08/02/2013 02:44 PM, Felipe Contreras wrote:
The initial _BCM commands don't work, so the level remains at 100%.
Since the level is max_level, acpi_video_bqc_quirk() tries with the
first level, which is 0, and 0 happens to be the index of 100.
So _BQC is returning 100, which is not the index of 0 (what we tested
for), but actually 100.
I think the current code is correct, but acpi_video_bqc_quirk() should
be testing br-levels[3], or anything other than 0/100 which can be
easily confused.
If so, the code would find that _BQC doesn't work on this machine (in
win8 mode)... at least initially. My guess is that it only starts to
work after acpi_video_bus_start_devices() is called.
Forcing br-flags._BQC_use_index = 0 seems to work.
Seems ASUS machines tend to have this issue:
https://bugzilla.kernel.org/show_bug.cgi?id=52951
https://bugzilla.kernel.org/show_bug.cgi?id=56711
I don't see any real solution for the ACPI driver.
I have a patch to enhance the quirk some time ago:
https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
I think this is unnecessarily complicated; the comment makes it clear
For your system, yes it is unnecessarily complicated. But since this is
a quirk, it better solves as many potential problems as possible, or we
would simply use a DMI entry to do the quirk.
The only difference between my patch and yours is that your patch
checks that br-level[i] is not the current level, but that check is
not necessary. If _BQC always returns the max level, all we need to do
_BQC does not always returns the max level.
is pick another value, any other value, and br-level[3] works just
fine.
For a _BCL only having 4 elements { 100, 40, 40, 100 }, the br-levels[3]
will be the max level. The example here may be too crazy to be true, but
since we are dealing with firmware, I tend to believe anything could
happen.
That can be fixed easily by checking test_level == current_level and
do the same your patch does, but I actually don't think we should do
that. It might make sense to test two values instead of only one, that
way we can we properly test _BQC, while your patch simply assumes it
doesn't work, even though it might.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] acpi: video: remove unused code
_BCM_use_index and _BCL_use_index are never used and probably never will.
Signed-off-by: Felipe Contreras
---
drivers/acpi/video.c | 12
1 file changed, 12 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 0ec434d..1a04dfe 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -185,8 +185,6 @@ struct acpi_video_device_cap {
struct acpi_video_brightness_flags {
u8 _BCL_no_ac_battery_levels:1; /* no AC/Battery levels in _BCL */
u8 _BCL_reversed:1; /* _BCL package is in a reversed order*/
- u8 _BCL_use_index:1;/* levels in _BCL are index values */
- u8 _BCM_use_index:1;/* input of _BCM is an index value */
u8 _BQC_use_index:1;/* _BQC returns an index value */
};
@@ -806,16 +804,6 @@ acpi_video_init_brightness(struct acpi_video_device
*device)
br->count = count;
device->brightness = br;
- /* Check the input/output of _BQC/_BCL/_BCM */
- if ((max_level < 100) && (max_level <= (count - 2)))
- br->flags._BCL_use_index = 1;
-
- /*
-* _BCM is always consistent with _BCL,
-* at least for all the laptops we have ever seen.
-*/
- br->flags._BCM_use_index = br->flags._BCL_use_index;
-
/* _BQC uses INDEX while _BCL uses VALUE in some laptops */
br->curr = level = max_level;
--
1.8.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] acpi: video: remove unused code
_BCM_use_index is never used and probably never will.
Signed-off-by: Felipe Contreras
---
drivers/acpi/video.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 0ec434d..54e2d4d 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -186,7 +186,6 @@ struct acpi_video_brightness_flags {
u8 _BCL_no_ac_battery_levels:1; /* no AC/Battery levels in _BCL */
u8 _BCL_reversed:1; /* _BCL package is in a reversed order*/
u8 _BCL_use_index:1;/* levels in _BCL are index values */
- u8 _BCM_use_index:1;/* input of _BCM is an index value */
u8 _BQC_use_index:1;/* _BQC returns an index value */
};
@@ -810,12 +809,6 @@ acpi_video_init_brightness(struct acpi_video_device
*device)
if ((max_level < 100) && (max_level <= (count - 2)))
br->flags._BCL_use_index = 1;
- /*
-* _BCM is always consistent with _BCL,
-* at least for all the laptops we have ever seen.
-*/
- br->flags._BCM_use_index = br->flags._BCL_use_index;
-
/* _BQC uses INDEX while _BCL uses VALUE in some laptops */
br->curr = level = max_level;
--
1.8.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Thu, Aug 1, 2013 at 11:30 PM, Aaron Lu wrote:
> On 08/02/2013 12:11 PM, Felipe Contreras wrote:
>> On Thu, Aug 1, 2013 at 9:03 PM, Aaron Lu wrote:
>>> On 08/02/2013 07:34 AM, Felipe Contreras wrote:
>>>> Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
>>>> index values) assumed that bl->levels were not reverted, but at this
>>>> point they already are, so there's no need to revert them yet again.
>>>
>>> When acpi_video_bqc_value_to_level is called, bl->levels is not
>>> reverted.
>>
>> This is the code that turns br->flags._BCL_reversed on:
>>
>> if (max_level == br->levels[2]) {
>> br->flags._BCL_reversed = 1;
>> sort(>levels[2], count - 2, sizeof(br->levels[2]),
>> acpi_video_cmp_level, NULL);
>> }
>>
>> Now tell me how br->flags._BCL_reversed can be on, and the br->levels
>> *not* reverted.
>
> Oh yes, it is reverted to be in increase order, so it is not in reverse
> order. I'm a little confused by these words.
br->levels is always ascending, but that doesn't mean _BCL is.
> Please see acpi_video_bqc_quirk, we set _BQC_use_index by revert the
> level on a reversed _BCL, so we will need to revert level here too.
I cannot parse that sentence, but nothing needs to change there; to
find out if _BQC is using an index, we need to see if the returned
value is the index of the level we are looking for, and to find that
out we need the original list of levels, which can be found by
reverting the already reverted list. If this wasn't the case there
would not be any need for the _BCL_reversed flag.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] acpi: video: trivial style cleanups
On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu wrote: > On 08/02/2013 07:44 AM, Felipe Contreras wrote: >> Signed-off-by: Felipe Contreras > > Change log please. You mean a commit message? That's what it's called in Git lingo, and it's right there: acpi: video: trivial style cleanups >> --- >> drivers/acpi/video.c | 90 >> +++- >> 1 file changed, 40 insertions(+), 50 deletions(-) >> >> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c >> index f021bf4..b07573f 100644 >> --- a/drivers/acpi/video.c >> +++ b/drivers/acpi/video.c >> @@ -88,7 +88,7 @@ module_param(allow_duplicates, bool, 0644); >> static bool use_bios_initial_backlight = 1; >> module_param(use_bios_initial_backlight, bool, 0644); >> >> -static int register_count = 0; >> +static int register_count; > > This isn't a style clean up. It is. There's no difference before and after, the only difference is the style. ERROR: do not initialise statics to 0 or NULL #91: FILE: acpi/video.c:91: +static int register_count = 0; -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups
On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu wrote: > On 08/02/2013 07:43 AM, Felipe Contreras wrote: >> Signed-off-by: Felipe Contreras > > Please add change log explaining what you have changed. > It seems that the patch modify comment style only, some add a space and > some change spaces to tab, is it the case? The commit message already explains what the change is; trivial cosmetic cleanups. Cosmetic means it's completely superficial. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Thu, Aug 1, 2013 at 9:03 PM, Aaron Lu wrote:
> On 08/02/2013 07:34 AM, Felipe Contreras wrote:
>> Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
>> index values) assumed that bl->levels were not reverted, but at this
>> point they already are, so there's no need to revert them yet again.
>
> When acpi_video_bqc_value_to_level is called, bl->levels is not
> reverted.
This is the code that turns br->flags._BCL_reversed on:
if (max_level == br->levels[2]) {
br->flags._BCL_reversed = 1;
sort(>levels[2], count - 2, sizeof(br->levels[2]),
acpi_video_cmp_level, NULL);
}
Now tell me how br->flags._BCL_reversed can be on, and the br->levels
*not* reverted.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/3] acpi: video: trivial cleanups
Hi, Here's a few patches to cleanup the code-style of acpi/video.c. Some were found by checkpatch, and some just make sense. Felipe Contreras (3): acpi: video: trivial costmetic cleanups acpi: video: trivial style cleanups acpi: video: remove unnecessary casting drivers/acpi/video.c | 210 +-- 1 file changed, 103 insertions(+), 107 deletions(-) -- 1.8.3.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] acpi: video: trivial style cleanups
Signed-off-by: Felipe Contreras
---
drivers/acpi/video.c | 90 +++-
1 file changed, 40 insertions(+), 50 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index f021bf4..b07573f 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -88,7 +88,7 @@ module_param(allow_duplicates, bool, 0644);
static bool use_bios_initial_backlight = 1;
module_param(use_bios_initial_backlight, bool, 0644);
-static int register_count = 0;
+static int register_count;
static int acpi_video_bus_add(struct acpi_device *device);
static int acpi_video_bus_remove(struct acpi_device *device);
static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
@@ -247,7 +247,7 @@ static int acpi_video_get_brightness(struct
backlight_device *bd)
* The first two entries are special - see page 575
* of the ACPI spec 3.0
*/
- return i-2;
+ return i - 2;
}
return 0;
}
@@ -304,11 +304,11 @@ video_set_cur_state(struct thermal_cooling_device
*cooling_dev, unsigned long st
struct acpi_video_device *video = acpi_driver_data(device);
int level;
- if ( state >= video->brightness->count - 2)
+ if (state >= video->brightness->count - 2)
return -EINVAL;
state = video->brightness->count - state;
- level = video->brightness->levels[state -1];
+ level = video->brightness->levels[state - 1];
return acpi_video_device_lcd_set_level(video, level);
}
@@ -349,7 +349,7 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device
*device,
return 0;
- err:
+err:
kfree(buffer.pointer);
return status;
@@ -550,7 +550,7 @@ acpi_video_device_lcd_get_level_current(struct
acpi_video_device *device,
if (device->brightness->levels[i] == *level) {
device->brightness->curr = *level;
return 0;
- }
+ }
/*
* BQC returned an invalid level.
* Stop using it.
@@ -892,16 +892,13 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
{
acpi_handle h_dummy1;
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_ADR",
_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_ADR",
_dummy1)))
device->cap._ADR = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCL",
_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCL",
_dummy1)))
device->cap._BCL = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCM",
_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCM",
_dummy1)))
device->cap._BCM = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle,"_BQC",_dummy1)))
+ if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BQC",
_dummy1)))
device->cap._BQC = 1;
else if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCQ",
_dummy1))) {
@@ -909,9 +906,8 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
device->cap._BCQ = 1;
}
- if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_DDC",
_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_DDC",
_dummy1)))
device->cap._DDC = 1;
- }
if (acpi_video_init_brightness(device))
return;
@@ -922,7 +918,7 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
acpi_handle acpi_parent;
struct device *parent = NULL;
int result;
- static int count = 0;
+ static int count;
char *name;
name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
@@ -1006,24 +1002,18 @@ static void acpi_video_bus_find_cap(struct
acpi_video_bus *video)
{
acpi_handle h_dummy1;
- if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOS",
_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOS",
_dummy1)))
video->cap._DOS = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOD",
_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get
[PATCH 1/3] acpi: video: trivial costmetic cleanups
Signed-off-by: Felipe Contreras
---
drivers/acpi/video.c | 114 +++
1 file changed, 61 insertions(+), 53 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 0ec434d..f021bf4 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1,5 +1,5 @@
/*
- * video.c - ACPI Video Driver ($Revision:$)
+ * video.c - ACPI Video Driver
*
* Copyright (C) 2004 Luming Yu
* Copyright (C) 2004 Bruno Ducrot
@@ -118,26 +118,26 @@ struct acpi_video_bus_flags {
};
struct acpi_video_bus_cap {
- u8 _DOS:1; /*Enable/Disable output switching */
- u8 _DOD:1; /*Enumerate all devices attached to display
adapter */
- u8 _ROM:1; /*Get ROM Data */
- u8 _GPD:1; /*Get POST Device */
- u8 _SPD:1; /*Set POST Device */
- u8 _VPO:1; /*Video POST Options */
+ u8 _DOS:1; /* Enable/Disable output switching */
+ u8 _DOD:1; /* Enumerate all devices attached to display
adapter */
+ u8 _ROM:1; /* Get ROM Data */
+ u8 _GPD:1; /* Get POST Device */
+ u8 _SPD:1; /* Set POST Device */
+ u8 _VPO:1; /* Video POST Options */
u8 reserved:2;
};
struct acpi_video_device_attrib {
u32 display_index:4;/* A zero-based instance of the Display */
- u32 display_port_attachment:4; /*This field differentiates the display
type */
- u32 display_type:4; /*Describe the specific type in use */
- u32 vendor_specific:4; /*Chipset Vendor Specific */
- u32 bios_can_detect:1; /*BIOS can detect the device */
- u32 depend_on_vga:1;/*Non-VGA output device whose power is related
to
+ u32 display_port_attachment:4; /* This field differentiates the
display type */
+ u32 display_type:4; /* Describe the specific type in use */
+ u32 vendor_specific:4; /* Chipset Vendor Specific */
+ u32 bios_can_detect:1; /* BIOS can detect the device */
+ u32 depend_on_vga:1;/* Non-VGA output device whose power is related
to
the VGA device. */
- u32 pipe_id:3; /*For VGA multiple-head devices. */
- u32 reserved:10;/*Must be 0 */
- u32 device_id_scheme:1; /*Device ID Scheme */
+ u32 pipe_id:3; /* For VGA multiple-head devices. */
+ u32 reserved:10;/* Must be 0 */
+ u32 device_id_scheme:1; /* Device ID Scheme */
};
struct acpi_video_enumerated_device {
@@ -174,17 +174,17 @@ struct acpi_video_device_flags {
};
struct acpi_video_device_cap {
- u8 _ADR:1; /*Return the unique ID */
- u8 _BCL:1; /*Query list of brightness control levels
supported */
- u8 _BCM:1; /*Set the brightness level */
+ u8 _ADR:1; /* Return the unique ID */
+ u8 _BCL:1; /* Query list of brightness control levels
supported */
+ u8 _BCM:1; /* Set the brightness level */
u8 _BQC:1; /* Get current brightness level */
u8 _BCQ:1; /* Some buggy BIOS uses _BCQ instead of _BQC */
- u8 _DDC:1; /*Return the EDID for this device */
+ u8 _DDC:1; /* Return the EDID for this device */
};
struct acpi_video_brightness_flags {
u8 _BCL_no_ac_battery_levels:1; /* no AC/Battery levels in _BCL */
- u8 _BCL_reversed:1; /* _BCL package is in a reversed order*/
+ u8 _BCL_reversed:1; /* _BCL package is in a reversed order
*/
u8 _BCL_use_index:1;/* levels in _BCL are index values */
u8 _BCM_use_index:1;/* input of _BCM is an index value */
u8 _BQC_use_index:1;/* _BQC returns an index value */
@@ -231,7 +231,7 @@ static int acpi_video_get_next_level(struct
acpi_video_device *device,
static int acpi_video_switch_brightness(struct acpi_video_device *device,
int event);
-/*backlight device sysfs support*/
+/* backlight device sysfs support */
static int acpi_video_get_brightness(struct backlight_device *bd)
{
unsigned long long cur_level;
@@ -243,8 +243,10 @@ static int acpi_video_get_brightness(struct
backlight_device *bd)
return -EINVAL;
for (i = 2; i < vd->brightness->count; i++) {
if (vd->brightness->levels[i] == cur_level)
- /* The first two entries are special - see page 575
- of the ACPI spec 3.0 */
+ /*
+* The first two entries are special - see page 575
+* of the ACPI spec 3.0
+*/
return i-2;
}
return 0;
@@ -316,9 +318,11 @@ stat
[PATCH 3/3] acpi: video: remove unnecessary casting
Signed-off-by: Felipe Contreras
---
drivers/acpi/video.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index b07573f..11bafbe 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -236,8 +236,7 @@ static int acpi_video_get_brightness(struct
backlight_device *bd)
{
unsigned long long cur_level;
int i;
- struct acpi_video_device *vd =
- (struct acpi_video_device *)bl_get_data(bd);
+ struct acpi_video_device *vd = bl_get_data(bd);
if (acpi_video_device_lcd_get_level_current(vd, _level, false))
return -EINVAL;
@@ -255,8 +254,7 @@ static int acpi_video_get_brightness(struct
backlight_device *bd)
static int acpi_video_set_brightness(struct backlight_device *bd)
{
int request_level = bd->props.brightness + 2;
- struct acpi_video_device *vd =
- (struct acpi_video_device *)bl_get_data(bd);
+ struct acpi_video_device *vd = bl_get_data(bd);
return acpi_video_device_lcd_set_level(vd,
vd->brightness->levels[request_level]);
--
1.8.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESSION/PATCH] acpi: blacklist win8 OSI for ASUS Zenbok Prime UX31A
On Tue, Jul 30, 2013 at 7:11 PM, Felipe Contreras wrote: > It is already broken in v3.11-rc3, in fact I just booted that to try > it out and it booted with the screen completely black (fortunately I > knew exactly what to type to change that). > > Also, each time I change the > backlight level from X, the screen blinks as if going 100%, 0%, and > then the desired level. This one is a bug in the ACPI driver. Here's the fix: http://article.gmane.org/gmane.linux.kernel/1536959 Now instead of booting with 0% (off), it boots with 100%, which is still not ideal, but way better. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] acpi: video: fix reversed indexed BQC
Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
index values) assumed that bl->levels were not reverted, but at this
point they already are, so there's no need to revert them yet again.
Signed-off-by: Felipe Contreras
---
drivers/acpi/video.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 0ec434d..b27c049 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -499,19 +499,10 @@ acpi_video_bqc_value_to_level(struct acpi_video_device
*device,
{
unsigned long long level;
- if (device->brightness->flags._BQC_use_index) {
- /*
-* _BQC returns an index that doesn't account for
-* the first 2 items with special meaning, so we need
-* to compensate for that by offsetting ourselves
-*/
- if (device->brightness->flags._BCL_reversed)
- bqc_value = device->brightness->count - 3 - bqc_value;
-
+ if (device->brightness->flags._BQC_use_index)
level = device->brightness->levels[bqc_value + 2];
- } else {
+ else
level = bqc_value;
- }
level += bqc_offset_aml_bug_workaround;
--
1.8.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESSION/PATCH] acpi: blacklist win8 OSI for ASUS Zenbok Prime UX31A
On Tue, Jul 30, 2013 at 9:22 PM, Aaron Lu wrote: > On 07/31/2013 10:07 AM, Felipe Contreras wrote: >> On Tue, Jul 30, 2013 at 8:36 PM, Aaron Lu wrote: >>> On 07/31/2013 08:11 AM, Felipe Contreras wrote: >>>> On Tue, Jul 30, 2013 at 6:13 PM, Rafael J. Wysocki wrote: >>>>> Yes, that will break backlight on your system and *then* you can complain >>>>> to >>>>> Linus if you wish. >>>> >>>> It is already broken in v3.11-rc3, in fact I just booted that to try >>>> it out and it booted with the screen completely black (fortunately I >>>> knew exactly what to type to change that). >>> >>> That is bad, can you please file a bug for that? I'll need to take a >>> look at your ACPI tables, thanks. >> >> File a bug where? > > https://bugzilla.kernel.org/ > For component, please choose ACPI->Power_Video. Here you go: https://bugzilla.kernel.org/show_bug.cgi?id=60677 >>>> Apparently this commit also needs to be reverted: efaa14c (ACPI / >>>> video: no automatic brightness changes by win8-compatible firmware). >>>> In this machine it makes the backlight work again (without >>>> acpi_osi="!Windows 2012"), but by doing so the ACPI driver also turns >>>> off the screen completely at level 0. Also, each time I change the >>> >>> So with rc3 and commit efaa14c reverted, when you set level 0 to ACPI >>> video's backlight interface, the screen will be off now? And this is not >>> the case in 3.10, right? >> >> No, setting level 0 turns it off if efaa14c is *not* reverted. In 3.10 >> 0 doesn't turn it off. > > AFAIK, Win8 firmware will check a flag to decide what to do on hotkey > press and backlight brightness level change. Common behavior is: > If that flag is set: > - on hotkey press, send out event; > - on brightness level change, use GPU driver's interface to change > backlight level following operation region spec. > If that flag is not set: > - on hotkey press, adjust the brightness level without sending out > event; > - on brightness level change, use EC to change the brightness level. > > The said commit sets the flag, so it seems with the flag set now, > it is possible the firmware interface will also give a screen off > result. But since we want to have notification event, we will have to > set that flag I think. Probably, but changing the brightness should still work even without the flag, right? It doesn't work here. Setting the flag makes it work, but not properly. >>> Please attach acpidump output to the to be opened bugzilla page, thanks. >> >> I looked at the code in the DCDT, it appears to me that they store >> different levels depending on the OSI version, so I don't think the >> problem is in the ACPI driver. Yet, the issue remains there. > > It's good to know what is the problem, so I would like to see the table. > Most of the bugs I solved in ACPI video driver's backlight control are > caused by firmware, so yes, it's very unlikely a bug of the ACPI video > driver itself. I hope you meant the output of this: https://aur.archlinux.org/packages/acpidump/ -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESSION/PATCH] acpi: blacklist win8 OSI for ASUS Zenbok Prime UX31A
On Thu, Aug 1, 2013 at 1:01 PM, Matthew Garrett wrote: > On Thu, Aug 01, 2013 at 12:50:18PM -0500, Felipe Contreras wrote: >> On Thu, Aug 1, 2013 at 12:42 PM, Matthew Garrett wrote: >> > An interface that describes reality is better than one that doesn't. But >> > hey, feel free to disagree and post a patch for the ABI docs. >> >> An interface that is useful to the user is better than one that is not. > > The interface is useful. Not 100% useful. If you are going to claim that 99% true is not true, then useful but inconsistent is not useful. I think it is useful, and I think it can be consistent if we want it to. > There are plenty of machines out there that > disable the backlight at minimum brightness setting (see every Apple, > for example), and assuming otherwise has always been incorrect. But, > like I said, send the patch. The key word in "every Apple" is *every*; what level 0 does in every apple is consistent. If 0 should turn off the screen, it should do so on all Linux machines. I will investigate and prepare an update to the documentation and the quirks to do so. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESSION/PATCH] acpi: blacklist win8 OSI for ASUS Zenbok Prime UX31A
On Thu, Aug 1, 2013 at 12:42 PM, Matthew Garrett wrote: > On Thu, Aug 01, 2013 at 12:37:04PM -0500, Felipe Contreras wrote: >> On Wed, Jul 31, 2013 at 1:47 PM, Matthew Garrett wrote: >> > If we can't make an interface 100% consistent, we shouldn't pretend that >> > the interface is 100% consistent. We can't, and so we don't. Setting a >> > backlight value of 0 may turn the screen off, and userspace needs to >> > deal with that. >> >> This is insanity; we can never guarantee 100% of anything. >> >> Better is better. And 99% is better than 90% > > An interface that describes reality is better than one that doesn't. But > hey, feel free to disagree and post a patch for the ABI docs. An interface that is useful to the user is better than one that is not. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESSION/PATCH] acpi: blacklist win8 OSI for ASUS Zenbok Prime UX31A
On Wed, Jul 31, 2013 at 1:47 PM, Matthew Garrett wrote: > On Wed, Jul 31, 2013 at 01:07:57PM -0500, Felipe Contreras wrote: >> If we can make the software behave consistently for 99% of the >> machines out there instead of only 90%, that's better. > > If we can't make an interface 100% consistent, we shouldn't pretend that > the interface is 100% consistent. We can't, and so we don't. Setting a > backlight value of 0 may turn the screen off, and userspace needs to > deal with that. This is insanity; we can never guarantee 100% of anything. Better is better. And 99% is better than 90% % git grep quirks | wc -l 1585 Moreover, Linux already does quirks, and when there are quirks it means there's no 100% guarantee of the thing working as it should; hence the need for quirks, which is never complete, never 100%. Anyway, screw the users, right? All you care about is that the code looks good to you. If we care about the users, we would provide a consistent interface, where 0 means the same thing on all the backlight drivers. If all we can do is provide this consistency 99% of the time through quirks, that is the way to go. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESSION/PATCH] acpi: blacklist win8 OSI for ASUS Zenbok Prime UX31A
On Wed, Jul 31, 2013 at 1:47 PM, Matthew Garrett mj...@srcf.ucam.org wrote: On Wed, Jul 31, 2013 at 01:07:57PM -0500, Felipe Contreras wrote: If we can make the software behave consistently for 99% of the machines out there instead of only 90%, that's better. If we can't make an interface 100% consistent, we shouldn't pretend that the interface is 100% consistent. We can't, and so we don't. Setting a backlight value of 0 may turn the screen off, and userspace needs to deal with that. This is insanity; we can never guarantee 100% of anything. Better is better. And 99% is better than 90% % git grep quirks | wc -l 1585 Moreover, Linux already does quirks, and when there are quirks it means there's no 100% guarantee of the thing working as it should; hence the need for quirks, which is never complete, never 100%. Anyway, screw the users, right? All you care about is that the code looks good to you. If we care about the users, we would provide a consistent interface, where 0 means the same thing on all the backlight drivers. If all we can do is provide this consistency 99% of the time through quirks, that is the way to go. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESSION/PATCH] acpi: blacklist win8 OSI for ASUS Zenbok Prime UX31A
On Thu, Aug 1, 2013 at 12:42 PM, Matthew Garrett mj...@srcf.ucam.org wrote: On Thu, Aug 01, 2013 at 12:37:04PM -0500, Felipe Contreras wrote: On Wed, Jul 31, 2013 at 1:47 PM, Matthew Garrett mj...@srcf.ucam.org wrote: If we can't make an interface 100% consistent, we shouldn't pretend that the interface is 100% consistent. We can't, and so we don't. Setting a backlight value of 0 may turn the screen off, and userspace needs to deal with that. This is insanity; we can never guarantee 100% of anything. Better is better. And 99% is better than 90% An interface that describes reality is better than one that doesn't. But hey, feel free to disagree and post a patch for the ABI docs. An interface that is useful to the user is better than one that is not. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESSION/PATCH] acpi: blacklist win8 OSI for ASUS Zenbok Prime UX31A
On Thu, Aug 1, 2013 at 1:01 PM, Matthew Garrett mj...@srcf.ucam.org wrote: On Thu, Aug 01, 2013 at 12:50:18PM -0500, Felipe Contreras wrote: On Thu, Aug 1, 2013 at 12:42 PM, Matthew Garrett mj...@srcf.ucam.org wrote: An interface that describes reality is better than one that doesn't. But hey, feel free to disagree and post a patch for the ABI docs. An interface that is useful to the user is better than one that is not. The interface is useful. Not 100% useful. If you are going to claim that 99% true is not true, then useful but inconsistent is not useful. I think it is useful, and I think it can be consistent if we want it to. There are plenty of machines out there that disable the backlight at minimum brightness setting (see every Apple, for example), and assuming otherwise has always been incorrect. But, like I said, send the patch. The key word in every Apple is *every*; what level 0 does in every apple is consistent. If 0 should turn off the screen, it should do so on all Linux machines. I will investigate and prepare an update to the documentation and the quirks to do so. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESSION/PATCH] acpi: blacklist win8 OSI for ASUS Zenbok Prime UX31A
On Tue, Jul 30, 2013 at 9:22 PM, Aaron Lu aaron@gmail.com wrote: On 07/31/2013 10:07 AM, Felipe Contreras wrote: On Tue, Jul 30, 2013 at 8:36 PM, Aaron Lu aaron@gmail.com wrote: On 07/31/2013 08:11 AM, Felipe Contreras wrote: On Tue, Jul 30, 2013 at 6:13 PM, Rafael J. Wysocki r...@sisk.pl wrote: Yes, that will break backlight on your system and *then* you can complain to Linus if you wish. It is already broken in v3.11-rc3, in fact I just booted that to try it out and it booted with the screen completely black (fortunately I knew exactly what to type to change that). That is bad, can you please file a bug for that? I'll need to take a look at your ACPI tables, thanks. File a bug where? https://bugzilla.kernel.org/ For component, please choose ACPI-Power_Video. Here you go: https://bugzilla.kernel.org/show_bug.cgi?id=60677 Apparently this commit also needs to be reverted: efaa14c (ACPI / video: no automatic brightness changes by win8-compatible firmware). In this machine it makes the backlight work again (without acpi_osi=!Windows 2012), but by doing so the ACPI driver also turns off the screen completely at level 0. Also, each time I change the So with rc3 and commit efaa14c reverted, when you set level 0 to ACPI video's backlight interface, the screen will be off now? And this is not the case in 3.10, right? No, setting level 0 turns it off if efaa14c is *not* reverted. In 3.10 0 doesn't turn it off. AFAIK, Win8 firmware will check a flag to decide what to do on hotkey press and backlight brightness level change. Common behavior is: If that flag is set: - on hotkey press, send out event; - on brightness level change, use GPU driver's interface to change backlight level following operation region spec. If that flag is not set: - on hotkey press, adjust the brightness level without sending out event; - on brightness level change, use EC to change the brightness level. The said commit sets the flag, so it seems with the flag set now, it is possible the firmware interface will also give a screen off result. But since we want to have notification event, we will have to set that flag I think. Probably, but changing the brightness should still work even without the flag, right? It doesn't work here. Setting the flag makes it work, but not properly. Please attach acpidump output to the to be opened bugzilla page, thanks. I looked at the code in the DCDT, it appears to me that they store different levels depending on the OSI version, so I don't think the problem is in the ACPI driver. Yet, the issue remains there. It's good to know what is the problem, so I would like to see the table. Most of the bugs I solved in ACPI video driver's backlight control are caused by firmware, so yes, it's very unlikely a bug of the ACPI video driver itself. I hope you meant the output of this: https://aur.archlinux.org/packages/acpidump/ -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] acpi: video: fix reversed indexed BQC
Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
index values) assumed that bl-levels were not reverted, but at this
point they already are, so there's no need to revert them yet again.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
drivers/acpi/video.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 0ec434d..b27c049 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -499,19 +499,10 @@ acpi_video_bqc_value_to_level(struct acpi_video_device
*device,
{
unsigned long long level;
- if (device-brightness-flags._BQC_use_index) {
- /*
-* _BQC returns an index that doesn't account for
-* the first 2 items with special meaning, so we need
-* to compensate for that by offsetting ourselves
-*/
- if (device-brightness-flags._BCL_reversed)
- bqc_value = device-brightness-count - 3 - bqc_value;
-
+ if (device-brightness-flags._BQC_use_index)
level = device-brightness-levels[bqc_value + 2];
- } else {
+ else
level = bqc_value;
- }
level += bqc_offset_aml_bug_workaround;
--
1.8.3.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [REGRESSION/PATCH] acpi: blacklist win8 OSI for ASUS Zenbok Prime UX31A
On Tue, Jul 30, 2013 at 7:11 PM, Felipe Contreras felipe.contre...@gmail.com wrote: It is already broken in v3.11-rc3, in fact I just booted that to try it out and it booted with the screen completely black (fortunately I knew exactly what to type to change that). Also, each time I change the backlight level from X, the screen blinks as if going 100%, 0%, and then the desired level. This one is a bug in the ACPI driver. Here's the fix: http://article.gmane.org/gmane.linux.kernel/1536959 Now instead of booting with 0% (off), it boots with 100%, which is still not ideal, but way better. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] acpi: video: remove unnecessary casting
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
drivers/acpi/video.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index b07573f..11bafbe 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -236,8 +236,7 @@ static int acpi_video_get_brightness(struct
backlight_device *bd)
{
unsigned long long cur_level;
int i;
- struct acpi_video_device *vd =
- (struct acpi_video_device *)bl_get_data(bd);
+ struct acpi_video_device *vd = bl_get_data(bd);
if (acpi_video_device_lcd_get_level_current(vd, cur_level, false))
return -EINVAL;
@@ -255,8 +254,7 @@ static int acpi_video_get_brightness(struct
backlight_device *bd)
static int acpi_video_set_brightness(struct backlight_device *bd)
{
int request_level = bd-props.brightness + 2;
- struct acpi_video_device *vd =
- (struct acpi_video_device *)bl_get_data(bd);
+ struct acpi_video_device *vd = bl_get_data(bd);
return acpi_video_device_lcd_set_level(vd,
vd-brightness-levels[request_level]);
--
1.8.3.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] acpi: video: trivial style cleanups
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
drivers/acpi/video.c | 90 +++-
1 file changed, 40 insertions(+), 50 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index f021bf4..b07573f 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -88,7 +88,7 @@ module_param(allow_duplicates, bool, 0644);
static bool use_bios_initial_backlight = 1;
module_param(use_bios_initial_backlight, bool, 0644);
-static int register_count = 0;
+static int register_count;
static int acpi_video_bus_add(struct acpi_device *device);
static int acpi_video_bus_remove(struct acpi_device *device);
static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
@@ -247,7 +247,7 @@ static int acpi_video_get_brightness(struct
backlight_device *bd)
* The first two entries are special - see page 575
* of the ACPI spec 3.0
*/
- return i-2;
+ return i - 2;
}
return 0;
}
@@ -304,11 +304,11 @@ video_set_cur_state(struct thermal_cooling_device
*cooling_dev, unsigned long st
struct acpi_video_device *video = acpi_driver_data(device);
int level;
- if ( state = video-brightness-count - 2)
+ if (state = video-brightness-count - 2)
return -EINVAL;
state = video-brightness-count - state;
- level = video-brightness-levels[state -1];
+ level = video-brightness-levels[state - 1];
return acpi_video_device_lcd_set_level(video, level);
}
@@ -349,7 +349,7 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device
*device,
return 0;
- err:
+err:
kfree(buffer.pointer);
return status;
@@ -550,7 +550,7 @@ acpi_video_device_lcd_get_level_current(struct
acpi_video_device *device,
if (device-brightness-levels[i] == *level) {
device-brightness-curr = *level;
return 0;
- }
+ }
/*
* BQC returned an invalid level.
* Stop using it.
@@ -892,16 +892,13 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
{
acpi_handle h_dummy1;
- if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle, _ADR,
h_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle, _ADR,
h_dummy1)))
device-cap._ADR = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle, _BCL,
h_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle, _BCL,
h_dummy1)))
device-cap._BCL = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle, _BCM,
h_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle, _BCM,
h_dummy1)))
device-cap._BCM = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle,_BQC,h_dummy1)))
+ if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle, _BQC,
h_dummy1)))
device-cap._BQC = 1;
else if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle, _BCQ,
h_dummy1))) {
@@ -909,9 +906,8 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
device-cap._BCQ = 1;
}
- if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle, _DDC,
h_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(device-dev-handle, _DDC,
h_dummy1)))
device-cap._DDC = 1;
- }
if (acpi_video_init_brightness(device))
return;
@@ -922,7 +918,7 @@ static void acpi_video_device_find_cap(struct
acpi_video_device *device)
acpi_handle acpi_parent;
struct device *parent = NULL;
int result;
- static int count = 0;
+ static int count;
char *name;
name = kasprintf(GFP_KERNEL, acpi_video%d, count);
@@ -1006,24 +1002,18 @@ static void acpi_video_bus_find_cap(struct
acpi_video_bus *video)
{
acpi_handle h_dummy1;
- if (ACPI_SUCCESS(acpi_get_handle(video-device-handle, _DOS,
h_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(video-device-handle, _DOS,
h_dummy1)))
video-cap._DOS = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(video-device-handle, _DOD,
h_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(video-device-handle, _DOD,
h_dummy1)))
video-cap._DOD = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(video-device-handle, _ROM,
h_dummy1))) {
+ if (ACPI_SUCCESS(acpi_get_handle(video-device-handle, _ROM,
h_dummy1)))
video-cap._ROM = 1;
- }
- if (ACPI_SUCCESS(acpi_get_handle(video-device-handle, _GPD
[PATCH 0/3] acpi: video: trivial cleanups
Hi, Here's a few patches to cleanup the code-style of acpi/video.c. Some were found by checkpatch, and some just make sense. Felipe Contreras (3): acpi: video: trivial costmetic cleanups acpi: video: trivial style cleanups acpi: video: remove unnecessary casting drivers/acpi/video.c | 210 +-- 1 file changed, 103 insertions(+), 107 deletions(-) -- 1.8.3.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] acpi: video: trivial costmetic cleanups
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
drivers/acpi/video.c | 114 +++
1 file changed, 61 insertions(+), 53 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 0ec434d..f021bf4 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1,5 +1,5 @@
/*
- * video.c - ACPI Video Driver ($Revision:$)
+ * video.c - ACPI Video Driver
*
* Copyright (C) 2004 Luming Yu luming...@intel.com
* Copyright (C) 2004 Bruno Ducrot duc...@poupinou.org
@@ -118,26 +118,26 @@ struct acpi_video_bus_flags {
};
struct acpi_video_bus_cap {
- u8 _DOS:1; /*Enable/Disable output switching */
- u8 _DOD:1; /*Enumerate all devices attached to display
adapter */
- u8 _ROM:1; /*Get ROM Data */
- u8 _GPD:1; /*Get POST Device */
- u8 _SPD:1; /*Set POST Device */
- u8 _VPO:1; /*Video POST Options */
+ u8 _DOS:1; /* Enable/Disable output switching */
+ u8 _DOD:1; /* Enumerate all devices attached to display
adapter */
+ u8 _ROM:1; /* Get ROM Data */
+ u8 _GPD:1; /* Get POST Device */
+ u8 _SPD:1; /* Set POST Device */
+ u8 _VPO:1; /* Video POST Options */
u8 reserved:2;
};
struct acpi_video_device_attrib {
u32 display_index:4;/* A zero-based instance of the Display */
- u32 display_port_attachment:4; /*This field differentiates the display
type */
- u32 display_type:4; /*Describe the specific type in use */
- u32 vendor_specific:4; /*Chipset Vendor Specific */
- u32 bios_can_detect:1; /*BIOS can detect the device */
- u32 depend_on_vga:1;/*Non-VGA output device whose power is related
to
+ u32 display_port_attachment:4; /* This field differentiates the
display type */
+ u32 display_type:4; /* Describe the specific type in use */
+ u32 vendor_specific:4; /* Chipset Vendor Specific */
+ u32 bios_can_detect:1; /* BIOS can detect the device */
+ u32 depend_on_vga:1;/* Non-VGA output device whose power is related
to
the VGA device. */
- u32 pipe_id:3; /*For VGA multiple-head devices. */
- u32 reserved:10;/*Must be 0 */
- u32 device_id_scheme:1; /*Device ID Scheme */
+ u32 pipe_id:3; /* For VGA multiple-head devices. */
+ u32 reserved:10;/* Must be 0 */
+ u32 device_id_scheme:1; /* Device ID Scheme */
};
struct acpi_video_enumerated_device {
@@ -174,17 +174,17 @@ struct acpi_video_device_flags {
};
struct acpi_video_device_cap {
- u8 _ADR:1; /*Return the unique ID */
- u8 _BCL:1; /*Query list of brightness control levels
supported */
- u8 _BCM:1; /*Set the brightness level */
+ u8 _ADR:1; /* Return the unique ID */
+ u8 _BCL:1; /* Query list of brightness control levels
supported */
+ u8 _BCM:1; /* Set the brightness level */
u8 _BQC:1; /* Get current brightness level */
u8 _BCQ:1; /* Some buggy BIOS uses _BCQ instead of _BQC */
- u8 _DDC:1; /*Return the EDID for this device */
+ u8 _DDC:1; /* Return the EDID for this device */
};
struct acpi_video_brightness_flags {
u8 _BCL_no_ac_battery_levels:1; /* no AC/Battery levels in _BCL */
- u8 _BCL_reversed:1; /* _BCL package is in a reversed order*/
+ u8 _BCL_reversed:1; /* _BCL package is in a reversed order
*/
u8 _BCL_use_index:1;/* levels in _BCL are index values */
u8 _BCM_use_index:1;/* input of _BCM is an index value */
u8 _BQC_use_index:1;/* _BQC returns an index value */
@@ -231,7 +231,7 @@ static int acpi_video_get_next_level(struct
acpi_video_device *device,
static int acpi_video_switch_brightness(struct acpi_video_device *device,
int event);
-/*backlight device sysfs support*/
+/* backlight device sysfs support */
static int acpi_video_get_brightness(struct backlight_device *bd)
{
unsigned long long cur_level;
@@ -243,8 +243,10 @@ static int acpi_video_get_brightness(struct
backlight_device *bd)
return -EINVAL;
for (i = 2; i vd-brightness-count; i++) {
if (vd-brightness-levels[i] == cur_level)
- /* The first two entries are special - see page 575
- of the ACPI spec 3.0 */
+ /*
+* The first two entries are special - see page 575
+* of the ACPI spec 3.0
+*/
return i-2
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Thu, Aug 1, 2013 at 9:03 PM, Aaron Lu aaron@gmail.com wrote:
On 08/02/2013 07:34 AM, Felipe Contreras wrote:
Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
index values) assumed that bl-levels were not reverted, but at this
point they already are, so there's no need to revert them yet again.
When acpi_video_bqc_value_to_level is called, bl-levels is not
reverted.
This is the code that turns br-flags._BCL_reversed on:
if (max_level == br-levels[2]) {
br-flags._BCL_reversed = 1;
sort(br-levels[2], count - 2, sizeof(br-levels[2]),
acpi_video_cmp_level, NULL);
}
Now tell me how br-flags._BCL_reversed can be on, and the br-levels
*not* reverted.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups
On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu aaron@gmail.com wrote: On 08/02/2013 07:43 AM, Felipe Contreras wrote: Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Please add change log explaining what you have changed. It seems that the patch modify comment style only, some add a space and some change spaces to tab, is it the case? The commit message already explains what the change is; trivial cosmetic cleanups. Cosmetic means it's completely superficial. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] acpi: video: trivial style cleanups
On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu aaron@gmail.com wrote: On 08/02/2013 07:44 AM, Felipe Contreras wrote: Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Change log please. You mean a commit message? That's what it's called in Git lingo, and it's right there: acpi: video: trivial style cleanups --- drivers/acpi/video.c | 90 +++- 1 file changed, 40 insertions(+), 50 deletions(-) diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index f021bf4..b07573f 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -88,7 +88,7 @@ module_param(allow_duplicates, bool, 0644); static bool use_bios_initial_backlight = 1; module_param(use_bios_initial_backlight, bool, 0644); -static int register_count = 0; +static int register_count; This isn't a style clean up. It is. There's no difference before and after, the only difference is the style. ERROR: do not initialise statics to 0 or NULL #91: FILE: acpi/video.c:91: +static int register_count = 0; -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi: video: fix reversed indexed BQC
On Thu, Aug 1, 2013 at 11:30 PM, Aaron Lu aaron...@intel.com wrote:
On 08/02/2013 12:11 PM, Felipe Contreras wrote:
On Thu, Aug 1, 2013 at 9:03 PM, Aaron Lu aaron@gmail.com wrote:
On 08/02/2013 07:34 AM, Felipe Contreras wrote:
Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
index values) assumed that bl-levels were not reverted, but at this
point they already are, so there's no need to revert them yet again.
When acpi_video_bqc_value_to_level is called, bl-levels is not
reverted.
This is the code that turns br-flags._BCL_reversed on:
if (max_level == br-levels[2]) {
br-flags._BCL_reversed = 1;
sort(br-levels[2], count - 2, sizeof(br-levels[2]),
acpi_video_cmp_level, NULL);
}
Now tell me how br-flags._BCL_reversed can be on, and the br-levels
*not* reverted.
Oh yes, it is reverted to be in increase order, so it is not in reverse
order. I'm a little confused by these words.
br-levels is always ascending, but that doesn't mean _BCL is.
Please see acpi_video_bqc_quirk, we set _BQC_use_index by revert the
level on a reversed _BCL, so we will need to revert level here too.
I cannot parse that sentence, but nothing needs to change there; to
find out if _BQC is using an index, we need to see if the returned
value is the index of the level we are looking for, and to find that
out we need the original list of levels, which can be found by
reverting the already reverted list. If this wasn't the case there
would not be any need for the _BCL_reversed flag.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
