On Wednesday, July 16, 2014 04:32:36 PM Hans de Goede wrote: > Hi, > > On 07/16/2014 02:03 PM, Bjørn Mork wrote: > > Hans de Goede <hdegoede@xxxxxxxxxx> writes: > > > >> Hi All, > >> > >> Here is my cleaned-up version of Linus' patch to fix the 2 steps on a > >> brightness key press problem. > >> > >> Changes compared to Linus' version: > >> -Move the global variables to inside struct acpi_video_device > >> -Rebase on top of Rafael's linux-pm linux-next branch (so on top of the > >> revert of changing the brightness_switch_enabled default) > >> > >> I've tested this on a laptop affected by the 2 steps problem and I can confirm > >> that it fixes the 2 steps issue under normal usage. I managed to trigger the > >> race by generating a heavy io-load, as exptected hitting the race has no > >> unwanted side-effects other then taking 2 steps instead of one. > >> > >> Bjørn can you test this patch on your system and confirm that it does not > >> break things for you please ? Note that in order to apply it you first need > >> to do: "git revert 886129a8eebebec", as the revert has not yet reached > >> Linus' tree AFAIK. > > > > Tested and verified that it still works fine for me, so feel free to add > > > > Tested-by: Bjørn Mork <bjorn@xxxxxxx> > > if you like. > > > > Thanks, if there is a need for a v2 I'll add your tested-by. > > Rafael, if there is not going to be a v2, could you pick up this tested-by > then? > > > But as noted earlier, I believe this poses another risk if anyone has a > > config using bool input values: > > > > -static bool brightness_switch_enabled = 1; > > -module_param(brightness_switch_enabled, bool, 0644); > > +static int brightness_switch_enabled = -1; > > +module_param(brightness_switch_enabled, int, 0644); > > > > The only way I can see that being a problem is if someone is using > video.brightness_switch_enabled=N for reasons other then the 2 step > problem this patch fixes. That is not unthinkable though. > > > I wonder if it wouldn't be better to just redefine the behaviour of > > "brightness_switch_enabled = 1" to the new delayed response scheme and > > avoid changing the type of this parameter? > > I think that is a good idea, that would also allow to simplify the patch > somewhat more. Rafael, what is your take on this ? I prefer simpler, so why don't you do that? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
