linux-kernel - Re: [PATCH] asus-wmi: fixup screenpad brightness

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <c9b74180-8e3b-2ed8-bb68-03ca9d8ed074@linux.intel.com>
Date: Mon, 9 Jun 2025 10:31:42 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Luke Jones <luke@...nes.dev>
cc: LKML <linux-kernel@...r.kernel.org>, Hans de Goede <hdegoede@...hat.com>, 
    platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] asus-wmi: fixup screenpad brightness

On Sun, 25 May 2025, Luke Jones wrote:

> Fix up some inconsistent behaviour involving the screenpad on some
> ASUS laptops. This fixes:
> - illogical screen off control (0/1 flipped depending on WMI state)
> - bad brightness depending on the last screenpad power state
> - incorrect brightness scaling

Why did you put them all into a single patch? I understand there's some 
overlap in lines they touch but if they can be separated, it would be 
much better and I'd likely have much higher confidence on each change.

> Signed-off-by: Luke Jones <luke@...nes.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 52 +++++++++++++--------------------
>  1 file changed, 21 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index f52884e90759..cec509171971 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -123,7 +123,6 @@ module_param(fnlock_default, bool, 0444);
>  #define NVIDIA_TEMP_MIN		75
>  #define NVIDIA_TEMP_MAX		87
>  
> -#define ASUS_SCREENPAD_BRIGHT_MIN 20
>  #define ASUS_SCREENPAD_BRIGHT_MAX 255
>  #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
>  
> @@ -4239,43 +4238,37 @@ static int read_screenpad_brightness(struct backlight_device *bd)
>  		return err;
>  	/* The device brightness can only be read if powered, so return stored */
>  	if (err == BACKLIGHT_POWER_OFF)
> -		return asus->driver->screenpad_brightness - ASUS_SCREENPAD_BRIGHT_MIN;
> +		return bd->props.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) - ASUS_SCREENPAD_BRIGHT_MIN;
> +	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;
> +	int err = 0;
>  	u32 ctrl_param;
>  
> -	power = read_screenpad_backlight_power(asus);
> -	if (power < 0)
> -		return power;
> -
> -	if (bd->props.power != power) {
> -		if (power != BACKLIGHT_POWER_ON) {
> -			/* Only brightness > 0 can power it back on */
> -			ctrl_param = asus->driver->screenpad_brightness - ASUS_SCREENPAD_BRIGHT_MIN;
> -			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 == BACKLIGHT_POWER_ON) {
> -		/* Only set brightness if powered on or we get invalid/unsync state */
> -		ctrl_param = bd->props.brightness + ASUS_SCREENPAD_BRIGHT_MIN;
> +	ctrl_param = bd->props.brightness;
> +	if (ctrl_param >= 0 && bd->props.power) {
> +		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 1,
> +					    NULL);
> +		if (err < 0)
> +			return err;
> +		ctrl_param = bd->props.brightness;
>  		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_LIGHT, ctrl_param, NULL);
> +		if (err < 0)
> +			return err;
>  	}
>  
> -	/* Ensure brightness is stored to turn back on with */
> -	if (err == 0)
> -		asus->driver->screenpad_brightness = bd->props.brightness + ASUS_SCREENPAD_BRIGHT_MIN;
> +	if (!bd->props.power) {
> +		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_SCREENPAD_POWER, 0, NULL);
> +		if (err < 0)
> +			return err;
> +	}
>  
>  	return err;
>  }
> @@ -4293,22 +4286,19 @@ static int asus_screenpad_init(struct asus_wmi *asus)
>  	int err, power;
>  	int brightness = 0;
>  
> -	power = read_screenpad_backlight_power(asus);
> +	power = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);
>  	if (power < 0)
>  		return power;
>  
> -	if (power != BACKLIGHT_POWER_OFF) {
> +	if (power) {
>  		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 < ASUS_SCREENPAD_BRIGHT_MIN)
> -		brightness = ASUS_SCREENPAD_BRIGHT_DEFAULT;
>  
>  	memset(&props, 0, sizeof(struct backlight_properties));
>  	props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked */
> -	props.max_brightness = ASUS_SCREENPAD_BRIGHT_MAX - ASUS_SCREENPAD_BRIGHT_MIN;
> +	props.max_brightness = ASUS_SCREENPAD_BRIGHT_MAX;
>  	bd = backlight_device_register("asus_screenpad",
>  				       &asus->platform_device->dev, asus,
>  				       &asus_screenpad_bl_ops, &props);
> @@ -4319,7 +4309,7 @@ static int asus_screenpad_init(struct asus_wmi *asus)
>  
>  	asus->screenpad_backlight_device = bd;
>  	asus->driver->screenpad_brightness = brightness;
> -	bd->props.brightness = brightness - ASUS_SCREENPAD_BRIGHT_MIN;
> +	bd->props.brightness = brightness;
>  	bd->props.power = power;
>  	backlight_update_status(bd);
>  
> 

-- 
 i.