Re: [PATCH] acpi-video: Fix backlight taking 2 steps on a brightness key press

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 07/16/2014 02:03 PM, Bjørn Mork wrote:
> Hans de Goede <hdegoede@redhat.com> 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@mork.no>
> 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 ?

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html