| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 12 Jul 2023 10:21:33 +1200
From: Luke Jones <luke@...nes.dev>
To: Hans de Goede <hdegoede@...hat.com>
Cc: corentin.chary@...il.com, acpi4asus-user@...ts.sourceforge.net,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-hwmon@...r.kernel.org, markgross@...nel.org,
jdelvare@...e.com, linux@...ck-us.net
Subject: Re: [PATCH v4 1/1] platform/x86: asus-wmi: add support for ASUS
screenpad
On Tue, Jul 11 2023 at 11:42:25 +02:00:00, Hans de Goede
<hdegoede@...hat.com> wrote:
> Hi,
>
> On 7/7/23 00:23, Luke Jones wrote:
>> On Tue, 2023-07-04 at 13:16 +0200, Hans de Goede wrote:
>>> Hi Luke,
>>>
>>> On 6/30/23 06:17, Luke D. Jones wrote:
>>>> Add support for the WMI methods used to turn off and adjust the
>>>> brightness of the secondary "screenpad" device found on some high-
>>>> end
>>>> ASUS laptops like the GX650P series and others.
>>>>
>>>> These methods are utilised in a new backlight device named
>>>> asus_screenpad.
>>>>
>>>> Signed-off-by: Luke D. Jones <luke@...nes.dev>
>>>
>>> Thank you for your work on this. I have one small change request
>>> and then v5 should be ready for merging, see me inline comment
>>> below.
>>>
>>>> ---
>>>> drivers/platform/x86/asus-wmi.c | 128
>>>> +++++++++++++++++++++
>>>> drivers/platform/x86/asus-wmi.h | 1 +
>>>> include/linux/platform_data/x86/asus-wmi.h | 4 +
>>>> 3 files changed, 133 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/asus-wmi.c
>>>> b/drivers/platform/x86/asus-wmi.c
>>>> index 62cee13f5576..967c92ceb041 100644
>>>> --- a/drivers/platform/x86/asus-wmi.c
>>>> +++ b/drivers/platform/x86/asus-wmi.c
>>>> @@ -25,6 +25,7 @@
>>>> #include <linux/input/sparse-keymap.h>
>>>> #include <linux/kernel.h>
>>>> #include <linux/leds.h>
>>>> +#include <linux/minmax.h>
>>>> #include <linux/module.h>
>>>> #include <linux/pci.h>
>>>> #include <linux/pci_hotplug.h>
>>>> @@ -212,6 +213,7 @@ struct asus_wmi {
>>>>
>>>> struct input_dev *inputdev;
>>>> struct backlight_device *backlight_device;
>>>> + struct backlight_device *screenpad_backlight_device;
>>>> struct platform_device *platform_device;
>>>>
>>>> struct led_classdev wlan_led;
>>>> @@ -3839,6 +3841,123 @@ static int is_display_toggle(int code)
>>>> return 0;
>>>> }
>>>>
>>>> +/* Screenpad backlight
>>>> *******************************************************/
>>>> +
>>>> +static int read_screenpad_backlight_power(struct asus_wmi *asus)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = asus_wmi_get_devstate_simple(asus,
>>>> ASUS_WMI_DEVID_SCREENPAD_POWER);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + /* 1 == powered */
>>>> + return ret ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
>>>> +}
>>>> +
>>>> +static int read_screenpad_brightness(struct backlight_device *bd)
>>>> +{
>>>> + struct asus_wmi *asus = bl_get_data(bd);
>>>> + u32 retval;
>>>> + int err;
>>>> +
>>>> + err = read_screenpad_backlight_power(asus);
>>>> + if (err < 0)
>>>> + return err;
>>>> + /* The device brightness can only be read if powered, so
>>>> return stored */
>>>> + if (err == FB_BLANK_POWERDOWN)
>>>> + return asus->driver->screenpad_brightness;
>>>> +
>>>> + err = asus_wmi_get_devstate(asus,
>>>> ASUS_WMI_DEVID_SCREENPAD_LIGHT, &retval);
>>>> + if (err < 0)
>>>> + return err;
>>>> +
>>>> + return retval & ASUS_WMI_DSTS_BRIGHTNESS_MASK;
>>>> +}
>>>> +
>>>> +static int update_screenpad_bl_status(struct backlight_device
>>>> *bd)
>>>> +{
>>>> + struct asus_wmi *asus = bl_get_data(bd);
>>>> + int power, err = 0;
>>>> + u32 ctrl_param;
>>>> +
>>>> + power = read_screenpad_backlight_power(asus);
>>>> + if (power < 0)
>>>> + return power;
>>>> +
>>>> + if (bd->props.power != power) {
>>>> + if (power != FB_BLANK_UNBLANK) {
>>>> + /* Only brightness > 0 can power it back
>>>> on
>>>> */
>>>> + ctrl_param = max(1, asus->driver-
>>>>> screenpad_brightness);
>>>> + err =
>>>> asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT,
>>>> + ctrl_param,
>>>> NULL);
>>>> + } else {
>>>> + err =
>>>> asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
>>>> + }
>>>> + } else if (power == FB_BLANK_UNBLANK) {
>>>> + /* Only set brightness if powered on or we get
>>>> invalid/unsync state */
>>>> + ctrl_param = bd->props.brightness;
>>>> + err =
>>>> asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param,
>>>> NULL);
>>>> + }
>>>> +
>>>> + /* Ensure brightness is stored to turn back on with */
>>>> + asus->driver->screenpad_brightness = bd->props.brightness;
>>>> +
>>>> + return err;
>>>> +}
>>>> +
>>>> +static const struct backlight_ops asus_screenpad_bl_ops = {
>>>> + .get_brightness = read_screenpad_brightness,
>>>> + .update_status = update_screenpad_bl_status,
>>>> + .options = BL_CORE_SUSPENDRESUME,
>>>> +};
>>>> +
>>>> +static int asus_screenpad_init(struct asus_wmi *asus)
>>>> +{
>>>> + struct backlight_device *bd;
>>>> + struct backlight_properties props;
>>>> + int err, power;
>>>> + int brightness = 0;
>>>> +
>>>> + power = read_screenpad_backlight_power(asus);
>>>> + if (power < 0)
>>>> + return power;
>>>> +
>>>> + if (power != FB_BLANK_POWERDOWN) {
>>>> + err = asus_wmi_get_devstate(asus,
>>>> ASUS_WMI_DEVID_SCREENPAD_LIGHT, &brightness);
>>>> + if (err < 0)
>>>> + return err;
>>>> + }
>>>> + /* default to an acceptable min brightness on boot if too
>>>> low */
>>>> + if (brightness < 60)
>>>> + brightness = 60;
>>>
>>> If settings below 60 are no good, then the correct way to handle
>>> this is to limit the range to 0 - (255-60) and add / substract
>>> 60 when setting / getting the brightness.
>>>
>>> E.g. do something like this:
>>>
>>> #define SCREENPAD_MIN_BRIGHTNESS 60
>>> #define SCREENPAD_MAX_BRIGHTNESS 255
>>>
>>> props.max_brightness = SCREENPAD_MAX_BRIGHTNESS -
>>> SCREENPAD_MIN_BRIGHTNESS;
>>>
>>> And in update_screenpad_bl_status() do:
>>>
>>> ctrl_param = bd->props.brightness +
>>> SCREENPAD_MIN_BRIGHTNESS;
>>>
>>> And when reading the brightness substract SCREENPAD_MIN_BRIGHTNESS,
>>> clamping to a minimum value of 0.
>>>
>>> This avoids a dead-zone in the brightness range between 0-60 .
>>
>> Hi Hans, I think this is the wrong thing to do.
>>
>> The initial point of that first `brightness = 60;` is only to set
>> it to
>> an acceptable brightness on boot. We don't want to prevent the user
>> from going below that brightness at all - this is just to ensure the
>> screen is visible on boot if the brightness was under that value,
>> and
>> it is usually only under that value if it was set to off before
>> shutdown/reboot.
>>
>> It's not to try and put the range between 60-255, it's just to make
>> the
>> screen visible on boot if it was off previously. The folks who have
>> tested this have found that this is the desired behaviour they
>> expect.
>
> I see.
>
> So if I understand things correctly then 60 is a good default,
> but the screen can go darker and still be usable.
>
> But 1 leads to an unusable screen, so we still need
> a SCREENPAD_MIN_BRIGHTNESS to avoid the screen being able
> to go so dark that it is no longer usable and then still
> move the range a bit, but just not by 60, but by some
> other number (something in the 10-30 range I guess?)
>
> Combined with adding a:
>
> #define SCREENPAD_DEFAULT_BRIGHTNESS 60
>
> And at boot when the read back brightness <
> SCREENPAD_MIN_BRIGHTNESS set it to SCREENPAD_DEFAULT_BRIGHTNESS.
>
> We really want to avoid users to be able to set an unusable
> low brightness level. As mentioned in the new panel brightness
> API proposal:
>
> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@redhat.com/
>
> "3. The meaning of 0 is not clearly defined, it can be either off,
> or minimum brightness at which the display is still readable
> (in a low light environment)"
>
> and the plan going forward is to:
>
> "Unlike the /sys/class/backlight/foo/brightness this brightness
> property
> has a clear definition for the value 0. The kernel must ensure that 0
> means minimum brightness (so 0 should _never_ turn the backlight off).
> If necessary the kernel must enforce a minimum value by adding
> an offset to the value seen in the property to ensure this behavior."
>
> So I really want to see this new backlight driver implement the
> new planned behavior for 0 from the start, with 0 meaning minimum
> *usable* brightness.
Hi Hans, yeah okay that makes sense. I'll get on to it.
Cheers,
Luke.
Powered by blists - more mailing lists