| 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: Fri, 07 Jul 2023 10:23:20 +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, 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.
Cheers,
Luke.
>
> > +
> > + memset(&props, 0, sizeof(struct backlight_properties));
> > + props.type = BACKLIGHT_RAW; /* ensure this bd is last to be
> > picked */
> > + props.max_brightness = 255;
> > + bd = backlight_device_register("asus_screenpad",
> > + &asus->platform_device->dev,
> > asus,
> > + &asus_screenpad_bl_ops,
> > &props);
> > + if (IS_ERR(bd)) {
> > + pr_err("Could not register backlight device\n");
> > + return PTR_ERR(bd);
> > + }
> > +
> > + asus->screenpad_backlight_device = bd;
> > + asus->driver->screenpad_brightness = brightness;
> > + bd->props.brightness = brightness;
> > + bd->props.power = power;
> > + backlight_update_status(bd);
> > +
> > + return 0;
> > +}
> > +
> > +static void asus_screenpad_exit(struct asus_wmi *asus)
> > +{
> > + backlight_device_unregister(asus-
> > >screenpad_backlight_device);
> > +
> > + asus->screenpad_backlight_device = NULL;
> > +}
> > +
> > /* Fn-lock
> > *******************************************************************
> > */
> >
> > static bool asus_wmi_has_fnlock_key(struct asus_wmi *asus)
> > @@ -4504,6 +4623,12 @@ static int asus_wmi_add(struct
> > platform_device *pdev)
> > } else if (asus->driver->quirks-
> > >wmi_backlight_set_devstate)
> > err =
> > asus_wmi_set_devstate(ASUS_WMI_DEVID_BACKLIGHT, 2, NULL);
> >
> > + if (asus_wmi_dev_is_present(asus,
> > ASUS_WMI_DEVID_SCREENPAD_LIGHT)) {
> > + err = asus_screenpad_init(asus);
> > + if (err && err != -ENODEV)
> > + goto fail_screenpad;
> > + }
> > +
> > if (asus_wmi_has_fnlock_key(asus)) {
> > asus->fnlock_locked = fnlock_default;
> > asus_wmi_fnlock_update(asus);
> > @@ -4527,6 +4652,8 @@ static int asus_wmi_add(struct
> > platform_device *pdev)
> > asus_wmi_backlight_exit(asus);
> > fail_backlight:
> > asus_wmi_rfkill_exit(asus);
> > +fail_screenpad:
> > + asus_screenpad_exit(asus);
> > fail_rfkill:
> > asus_wmi_led_exit(asus);
> > fail_leds:
> > @@ -4553,6 +4680,7 @@ static int asus_wmi_remove(struct
> > platform_device *device)
> > asus = platform_get_drvdata(device);
> > wmi_remove_notify_handler(asus->driver->event_guid);
> > asus_wmi_backlight_exit(asus);
> > + asus_screenpad_exit(asus);
> > asus_wmi_input_exit(asus);
> > asus_wmi_led_exit(asus);
> > asus_wmi_rfkill_exit(asus);
> > diff --git a/drivers/platform/x86/asus-wmi.h
> > b/drivers/platform/x86/asus-wmi.h
> > index a478ebfd34df..5fbdd0eafa02 100644
> > --- a/drivers/platform/x86/asus-wmi.h
> > +++ b/drivers/platform/x86/asus-wmi.h
> > @@ -57,6 +57,7 @@ struct quirk_entry {
> > struct asus_wmi_driver {
> > int brightness;
> > int panel_power;
> > + int screenpad_brightness;
> > int wlan_ctrl_by_user;
> >
> > const char *name;
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h
> > b/include/linux/platform_data/x86/asus-wmi.h
> > index d17ae2eb0f8d..61ba70b32846 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -58,6 +58,10 @@
> > #define ASUS_WMI_DEVID_KBD_BACKLIGHT 0x00050021
> > #define ASUS_WMI_DEVID_LIGHT_SENSOR 0x00050022 /* ?? */
> > #define ASUS_WMI_DEVID_LIGHTBAR 0x00050025
> > +/* This can only be used to disable the screen, not re-enable */
> > +#define ASUS_WMI_DEVID_SCREENPAD_POWER 0x00050031
> > +/* Writing a brightness re-enables the screen if disabled */
> > +#define ASUS_WMI_DEVID_SCREENPAD_LIGHT 0x00050032
> > #define ASUS_WMI_DEVID_FAN_BOOST_MODE 0x00110018
> > #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
> >
>
Powered by blists - more mailing lists