Messages in this thread Patch in this message |  | | Date | Sat, 17 Jul 2021 11:09:19 +1200 | From | Luke Jones <> | Subject | Re: [PATCH 2/3] asus-wmi: Add dgpu disable method |
| |
Thank you for the insightful feedback.
On Mon, Jul 5 2021 at 00:47:31 +0000, Barnabás Pőcze <pobrn@protonmail.com> wrote: > Hi > > I have added a couple comments inline. > > > 2021. július 5., hétfő 0:21 keltezéssel, Luke D. Jones írta: > >> In Windows the ASUS Armory Crate progrm can enable or disable the > ^ > "program" > My "a" key is a little hard to press sometimes :(
> >> dGPU via a WMI call. This functions much the same as various Linux >> methods in software where the dGPU is removed from the device tree. >> >> However the WMI call saves the state of dGPU enabled or not and this > > I think "[...] the WMI call saves whether the dGPU is enabled or not, > and [...]" > might be better. > Or "[...] the WMI call saves the state of the dGPU (enabled or not) > and [...]". > I've used the second option, thanks for pointing this out.
> >> then changes the dGPU visibility in Linux with no way for Linux >> users to re-enable it. We expose the WMI method so users can see >> and change the dGPU ACPI state. >> >> Signed-off-by: Luke D. Jones <luke@ljones.dev> >> --- >> drivers/platform/x86/asus-wmi.c | 98 >> ++++++++++++++++++++++ >> include/linux/platform_data/x86/asus-wmi.h | 3 + >> 2 files changed, 101 insertions(+) >> >> diff --git a/drivers/platform/x86/asus-wmi.c >> b/drivers/platform/x86/asus-wmi.c >> index 2468076d6cd8..8dc3f7ed021f 100644 >> --- a/drivers/platform/x86/asus-wmi.c >> +++ b/drivers/platform/x86/asus-wmi.c >> @@ -210,6 +210,9 @@ struct asus_wmi { >> u8 fan_boost_mode_mask; >> u8 fan_boost_mode; >> >> + bool dgpu_disable_available; >> + u8 dgpu_disable_mode; >> + >> bool throttle_thermal_policy_available; >> u8 throttle_thermal_policy_mode; >> >> @@ -427,6 +430,93 @@ static void >> lid_flip_tablet_mode_get_state(struct asus_wmi *asus) >> } >> } >> >> +/* dGPU >> ********************************************************************/ >> +static int dgpu_disable_check_present(struct asus_wmi *asus) >> +{ >> + u32 result; >> + int err; >> + >> + asus->dgpu_disable_available = false; >> + >> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_DGPU, &result); >> + if (err) { >> + if (err == -ENODEV) >> + return 0; >> + return err; >> + } >> + >> + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) >> + asus->dgpu_disable_available = true; >> + asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT; >> + > > Aren't braces missing here? > Yep. Fixed.
> >> + return 0; >> +} >> + >> +static int dgpu_disable_write(struct asus_wmi *asus) >> +{ >> + int err; >> + u8 value; >> + u32 retval; >> + >> + value = asus->dgpu_disable_mode; >> + >> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval); >> + >> + sysfs_notify(&asus->platform_device->dev.kobj, NULL, >> + "dgpu_disable"); > > A similar line with the exact same length in patch 3/3 is not broken > in two. > And shouldn't the notification be sent if the operation succeeded? > Fixed. I moved the sysfs_notify down below the error returns. > >> + >> + if (err) { >> + pr_warn("Failed to set dgpu disable: %d\n", err); >> + return err; >> + } >> + >> + if (retval > 1 || retval < 0) { >> + pr_warn("Failed to set dgpu disable (retval): 0x%x\n", >> + retval); >> + return -EIO; >> + } >> + >> + return 0; >> +} >> + >> +static ssize_t dgpu_disable_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct asus_wmi *asus = dev_get_drvdata(dev); >> + u8 mode = asus->dgpu_disable_mode; >> + >> + return scnprintf(buf, PAGE_SIZE, "%d\n", mode); > > You could use `sysfs_emit()`. > Thanks. Many things like this I'm actually unaware of. Most of these patches are done by reading existing code.
> >> +} >> + >> +static ssize_t dgpu_disable_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + int result; >> + u8 disable; >> + struct asus_wmi *asus = dev_get_drvdata(dev); >> + >> + result = kstrtou8(buf, 10, &disable); > > You could use `kstrtobool()`. I think that would be better since it > accepts > 'y', 'n', etc. in addition to 0 and 1. > Thanks! Wasn't aware of that. And since the setting for all 3 patches can only ever be 0/1 I've changed to use a bool instead of u8
> >> + if (result < 0) >> + return result; >> + >> + if (disable > 1 || disable < 0) >> + return -EINVAL; >> + >> + asus->dgpu_disable_mode = disable; >> + /* >> + * The ACPI call used does not save the mode unless the call is >> run twice. >> + * Once to disable, then once to check status and save - this is >> two code >> + * paths in the method in the ACPI dumps. >> + */ >> + dgpu_disable_write(asus); >> + dgpu_disable_write(asus); > > Is there any reason the potential error codes are not returned? > No I've fixed now. Guess I missed it.
> >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR_RW(dgpu_disable); >> + >> /* Battery >> ********************************************************************/ >> >> /* The battery maximum charging percentage */ >> @@ -2412,6 +2502,7 @@ static struct attribute >> *platform_attributes[] = { >> &dev_attr_camera.attr, >> &dev_attr_cardr.attr, >> &dev_attr_touchpad.attr, >> + &dev_attr_dgpu_disable.attr, >> &dev_attr_lid_resume.attr, >> &dev_attr_als_enable.attr, >> &dev_attr_fan_boost_mode.attr, >> @@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct >> kobject *kobj, >> devid = ASUS_WMI_DEVID_LID_RESUME; >> else if (attr == &dev_attr_als_enable.attr) >> devid = ASUS_WMI_DEVID_ALS_ENABLE; >> + else if (attr == &dev_attr_dgpu_disable.attr) >> + ok = asus->dgpu_disable_available; >> else if (attr == &dev_attr_fan_boost_mode.attr) >> ok = asus->fan_boost_mode_available; >> else if (attr == &dev_attr_throttle_thermal_policy.attr) >> @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct >> platform_device *pdev) >> if (err) >> goto fail_platform; >> >> + err = dgpu_disable_check_present(asus); >> + if (err) >> + goto fail_dgpu_disable; >> + > > Should this really be considered a "fatal" error? > I was modelling this on fail_fan_boost_mode and fail_throttle_thermal_policy since a laptop can't have both of those which indicates that a failed one is fine, it seemed appropriate to follow the same behaviour here
> >> err = fan_boost_mode_check_present(asus); >> if (err) >> goto fail_fan_boost_mode; >> @@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct >> platform_device *pdev) >> fail_sysfs: >> fail_throttle_thermal_policy: >> fail_fan_boost_mode: >> +fail_dgpu_disable: >> fail_platform: >> fail_panel_od: >> kfree(asus); >> diff --git a/include/linux/platform_data/x86/asus-wmi.h >> b/include/linux/platform_data/x86/asus-wmi.h >> index 428aea701c7b..a528f9d0e4b7 100644 >> --- a/include/linux/platform_data/x86/asus-wmi.h >> +++ b/include/linux/platform_data/x86/asus-wmi.h >> @@ -90,6 +90,9 @@ >> /* Keyboard dock */ >> #define ASUS_WMI_DEVID_KBD_DOCK 0x00120063 >> >> +/* dgpu on/off */ >> +#define ASUS_WMI_DEVID_DGPU 0x00090020 >> + >> /* DSTS masks */ >> #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 >> #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002 >> -- >> 2.31.1 > > > Regards, > Barnabás Pőcze
Many thanks for your feedback again. Luke.
|  |