| 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
| ||
|
Message-Id: <QB98UR.2KNZGZXO8NF12@ljones.dev>
Date: Sat, 06 May 2023 20:09:26 +1200
From: Luke Jones <luke@...nes.dev>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
acpi4asus-user@...ts.sourceforge.net, hdegoede@...hat.com,
corentin.chary@...il.com, markgross@...nel.org, jdelvare@...e.com
Subject: Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS
screenpad
On Fri, May 5 2023 at 21:43:44 -0700, Guenter Roeck
<linux@...ck-us.net> wrote:
> On 5/5/23 20:48, Luke Jones wrote:
>>
>>
>> On Fri, May 5 2023 at 18:30:36 -0700, Guenter Roeck
>> <linux@...ck-us.net> wrote:
>>> On 5/5/23 16:43, Luke Jones wrote:
>>>>
>>>>
>>>> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen
>>>> <ilpo.jarvinen@...ux.intel.com> wrote:
>>>>> On Fri, 5 May 2023, 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>
>>>>>> ---
>>>>>> .../ABI/testing/sysfs-platform-asus-wmi | 2 +-
>>>>>> drivers/platform/x86/asus-wmi.c | 132
>>>>>> ++++++++++++++++++
>>>>>> drivers/platform/x86/asus-wmi.h | 1 +
>>>>>> include/linux/platform_data/x86/asus-wmi.h | 4 +
>>>>>> 4 files changed, 138 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>> b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>> index a77a004a1baa..df9817c6233a 100644
>>>>>> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>>>>>> @@ -97,4 +97,4 @@ Contact: "Luke Jones" <luke@...nes.dev>
>>>>>> Description:
>>>>>> Enable an LCD response-time boost to reduce or remove
>>>>>> ghosting:
>>>>>> * 0 - Disable,
>>>>>> - * 1 - Enable
>>>>>> + * 1 - Enable
>>>>>> \ No newline at end of file
>>>>>
>>>>> Spurious change?
>>>>
>>>> Indeed it is. Not sure how that occurred.
>>>>
>>>>>
>>>>>> diff --git a/drivers/platform/x86/asus-wmi.c
>>>>>> b/drivers/platform/x86/asus-wmi.c
>>>>>> index 1038dfdcdd32..0528eef02ef7 100644
>>>>>> --- a/drivers/platform/x86/asus-wmi.c
>>>>>> +++ b/drivers/platform/x86/asus-wmi.c
>>>>>> @@ -200,6 +200,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;
>>>>>> @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +/* Screenpad backlight */
>>>>>> +
>>>>>> +static int read_screenpad_backlight_power(struct asus_wmi
>>>>>> *asus)
>>>>>> +{
>>>>>> + int ret = asus_wmi_get_devstate_simple(asus,
>>>>>> ASUS_WMI_DEVID_SCREENPAD_POWER);
>>>>>
>>>>> Please move this to own line because now you have the extra
>>>>> newline
>>>>> in between the call and error handling.
>>>>
>>>> I don't understand what you mean sorry. Remove the new line or:
>>>> 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 == -ENODEV)
>>>>>> + return err;
>>>>>
>>>>> Just return 0. Or is there perhaps something wrong/missing here?
>>>>
>>>> I thought the correct thing was to return any possible error state
>>>> (here, anything less than 0 would be an error, right?)
>>>>
>>>
>>> Well, yes, but you are not returning an error. You are returning
>>> 'err'
>>> which is 0 at this point. So, at the very least, this code is (very)
>>> misleading since it suggests that it would return some error
>>> (as saved in the 'err' variable) when it doesn't.
>>>
>>> Guenter
>>>
>>
>> Oh! Right I see it now, I'm sorry, I just kept skipping over it
>> somehow.
>>
>> So I should change to:
>> power = read_screenpad_backlight_power(asus);
>> if (power < 0)
>> return power;
>>
>> Is that acceptable?
>>
>
> That depends on what the code is supposed to do. Right now it is
> "If read_screenpad_backlight_power() returns -ENODEV, stop and return
> no error (let the rest of the code continue). If it returns another
> error,
> return it".
> Changing the code as suggested above changes the semantics to
> "If read_screenpad_backlight_power() returns an error, always return
> it".
>
> Looking at the patch, I don't have a definite answer.
> asus_screenpad_init() happily registers the backlight if
> read_screenpad_backlight_power() returns -ENODEV. One could argue that
> the other functions should thus handle that situation gracefully as
> well,
> but read_screenpad_brightness() does return -ENODEV in that situation.
> I think you should decide how you want to handle that case and handle
> it consistently across all functions.
>
> Either case, there is another problem in asus_screenpad_init():
> If read_screenpad_brightness() fails, the function returns an error,
> but does not unregister the backlight device.
>
> Guenter
>
Thanks mate. I was working on this between my workload and getting a
few users to test. My first priority was to get the thing working for
them and as such I didn't put much thought in to errors and consistency.
I think since `asus_screenpad_init()` is not called unless the
brightness WMI method exists, then it and the other functions should
return all errors if the power WMI method fails at all since the device
functionality is compromised. If there is a hardware change in this
aspect we could revisit it then.
> Either case, there is another problem in asus_screenpad_init():
> If read_screenpad_brightness() fails, the function returns an error,
> but does not unregister the backlight device.
I wasn't entirely sure how to handle this. Mostly I followed what the
existing backlight code was doing, I've now added a `goto
fail_screenpad;` with:
fail_screenpad:
if (asus->screenpad_backlight_device)
asus_screenpad_exit(asus);
really I'm relying on others to guide me with this - I know I've got a
fair few patches in these days but they've been mostly simple things
except for TUF RGB, and I've had to learn a bunch of stuff as I go.
I'll await your response before I send in a V3.
Cheers,
Luke.
Powered by blists - more mailing lists