| Message ID | 7595c4f0-3dbb-2fe5-4daf-4b9a266f67d7@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | asus-wmi: Support of ASUS TUF Gaming series laptops | expand |
>-----Original Message----- >From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver- >x86-owner@vger.kernel.org] On Behalf Of Yurii Pavlovskyi >Sent: Friday, April 19, 2019 3:43 PM >Cc: Corentin Chary <corentin.chary@gmail.com>; Darren Hart ><dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; Daniel >Drake <drake@endlessm.com>; acpi4asus-user@lists.sourceforge.net; >platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org >Subject: [PATCH v3 08/11] platform/x86: asus-wmi: Enhance detection of >thermal data > >The obviously wrong value 1 for temperature device ID in this driver is >returned by at least some devices, including TUF Gaming series laptops, >instead of 0 as expected previously. Observable effect is that a temp1_input >in hwmon reads temperature near absolute zero. > >* Consider 0.1 K an erroneous value in addition to 0 K. >* Refactor detection of thermal input availability to a separate function. > >Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com> >--- > drivers/platform/x86/asus-wmi.c | 45 ++++++++++++++++++++++++++++----- > 1 file changed, 38 insertions(+), 7 deletions(-) > >diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus- >wmi.c index e69e55635afb..1b8272374660 100644 >--- a/drivers/platform/x86/asus-wmi.c >+++ b/drivers/platform/x86/asus-wmi.c >@@ -178,6 +178,7 @@ struct asus_wmi { > struct asus_rfkill gps; > struct asus_rfkill uwb; > >+ bool asus_hwmon_thermal_available; > bool asus_hwmon_fan_manual_mode; > int asus_hwmon_num_fans; > int asus_hwmon_pwm; >@@ -1375,6 +1376,32 @@ static struct attribute *hwmon_attributes[] = { > NULL > }; > >+static int asus_hwmon_check_thermal_available(struct asus_wmi *asus) { >+ u32 value = ASUS_WMI_UNSUPPORTED_METHOD; >+ int err; >+ >+ asus->asus_hwmon_thermal_available = false; >+ err = asus_wmi_get_devstate(asus, >ASUS_WMI_DEVID_THERMAL_CTRL, >+&value); >+ >+ if (err < 0) { >+ if (err == -ENODEV) >+ return 0; >+ >+ return err; >+ } >+ >+ /* >+ * If the temperature value in deci-Kelvin is near the absolute >+ * zero temperature, something is clearly wrong. >+ */ >+ if (!value || value == 1) >+ return 0; Do you still need to return 0 in case of wrong/failure case ? Shouldn't you return error here ? >+ >+ asus->asus_hwmon_thermal_available = true; >+ return 0; >+} >+ > static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj, > struct attribute *attr, int idx) { @@ - >1388,8 +1415,6 @@ static umode_t asus_hwmon_sysfs_is_visible(struct >kobject *kobj, > > if (attr == &dev_attr_pwm1.attr) > dev_id = ASUS_WMI_DEVID_FAN_CTRL; >- else if (attr == &dev_attr_temp1_input.attr) >- dev_id = ASUS_WMI_DEVID_THERMAL_CTRL; > > if (attr == &dev_attr_fan1_input.attr > || attr == &dev_attr_fan1_label.attr @@ -1414,15 +1439,13 @@ >static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj, > * - reverved bits are non-zero > * - sfun and presence bit are not set > */ >- if (value == ASUS_WMI_UNSUPPORTED_METHOD || value & >0xFFF80000 >+ if (value == ASUS_WMI_UNSUPPORTED_METHOD || (value & >0xFFF80000) > || (!asus->sfun && !(value & >ASUS_WMI_DSTS_PRESENCE_BIT))) > ok = false; > else > ok = fan_attr <= asus->asus_hwmon_num_fans; >- } else if (dev_id == ASUS_WMI_DEVID_THERMAL_CTRL) { >- /* If value is zero, something is clearly wrong */ >- if (!value) >- ok = false; >+ } else if (attr == &dev_attr_temp1_input.attr) { >+ ok = asus->asus_hwmon_thermal_available; > } else if (fan_attr <= asus->asus_hwmon_num_fans && fan_attr != -1) >{ > ok = true; > } else { >@@ -1469,6 +1492,14 @@ static int asus_wmi_fan_init(struct asus_wmi >*asus) > } > > pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans); >+ >+ status = asus_hwmon_check_thermal_available(asus); >+ if (status) { >+ pr_warn("Could not check if thermal available: %d\n", status); >+ return -ENXIO; >+ } >+ >+ pr_info("Thermal available: %d\n", >+asus->asus_hwmon_thermal_available); > return 0; > } > >-- >2.17.1
On 24.04.19 20:25, Pawnikar, Sumeet R wrote: >> + /* >> + * If the temperature value in deci-Kelvin is near the absolute >> + * zero temperature, something is clearly wrong. >> + */ >> + if (!value || value == 1) >> + return 0; > Do you still need to return 0 in case of wrong/failure case ? > Shouldn't you return error here ? Yes, I think 0 is right there, it's not an error condition in the check itself (such as IO or memory error). The function returns two values: check result in asus->asus_hwmon_thermal_available and error code. This is still a successful negative result. The 0 or 1 (as added in this patch) there merely indicates that the temperature measurement is not available this way. It is safe to assume that no device that does provide temperature will return 0.1 K. The temperature measurement is not critical for driver operation so the driver proceeds without it. One could return ENODEV when the check result is negative, but that would just move the actual check and assignment further to asus_wmi_fan_init. This is how it might look like in DSDT in this case: Method (TMPR, 0, NotSerialized) { Return (One) } .. If ((IIA0 == 0x00110011)) { Return ((TMPR () & 0xFFFF)) } This is indeed the right method ID but it's nothing there. Regards, Yurii
On Fri, Apr 19, 2019 at 1:12 PM Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com> wrote: > > The obviously wrong value 1 for temperature device ID in this driver is > returned by at least some devices, including TUF Gaming series laptops, > instead of 0 as expected previously. Observable effect is that a > temp1_input in hwmon reads temperature near absolute zero. > > * Consider 0.1 K an erroneous value in addition to 0 K. > * Refactor detection of thermal input availability to a separate function. > +static int asus_hwmon_check_thermal_available(struct asus_wmi *asus) > +{ > + u32 value = ASUS_WMI_UNSUPPORTED_METHOD; > + int err; > + > + asus->asus_hwmon_thermal_available = false; > + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_THERMAL_CTRL, &value); > + The blank line probably should go before a call, but definitely not here. > + if (err < 0) { This needs comment why -ENODEV considered as non-fatal. > + if (err == -ENODEV) > + return 0; > + > + return err; > + } > + > + /* > + * If the temperature value in deci-Kelvin is near the absolute > + * zero temperature, something is clearly wrong. > + */ > + if (!value || value == 1) > + return 0; > + > + asus->asus_hwmon_thermal_available = true; > + return 0; > +} > if (attr == &dev_attr_pwm1.attr) > dev_id = ASUS_WMI_DEVID_FAN_CTRL; > - else if (attr == &dev_attr_temp1_input.attr) > - dev_id = ASUS_WMI_DEVID_THERMAL_CTRL; I don't see how this change affects the user output or driver behaviour. Why is it done? > - if (value == ASUS_WMI_UNSUPPORTED_METHOD || value & 0xFFF80000 > + if (value == ASUS_WMI_UNSUPPORTED_METHOD || (value & 0xFFF80000) Seems like a bug fix and thus should be a separate commit predecessing the series. > - } else if (dev_id == ASUS_WMI_DEVID_THERMAL_CTRL) { > + } else if (attr == &dev_attr_temp1_input.attr) { So, I don't see why you change this line. > + pr_warn("Could not check if thermal available: %d\n", status); > + pr_info("Thermal available: %d\n", asus->asus_hwmon_thermal_available); dev_*() ?
On 08.05.19 15:58, Andy Shevchenko wrote: > On Fri, Apr 19, 2019 at 1:12 PM Yurii Pavlovskyi > <yurii.pavlovskyi@gmail.com> wrote: > >> - if (value == ASUS_WMI_UNSUPPORTED_METHOD || value & 0xFFF80000 >> + if (value == ASUS_WMI_UNSUPPORTED_METHOD || (value & 0xFFF80000) > > Seems like a bug fix and thus should be a separate commit predecessing > the series. The previous one should theoretically work as well, just thought that would help readability, will revert this. >> - else if (attr == &dev_attr_temp1_input.attr) >> - dev_id = ASUS_WMI_DEVID_THERMAL_CTRL; > > I don't see how this change affects the user output or driver > behaviour. Why is it done? > >> - } else if (dev_id == ASUS_WMI_DEVID_THERMAL_CTRL) { > >> + } else if (attr == &dev_attr_temp1_input.attr) { > > So, I don't see why you change this line. > Yes, looking at this patch now I'd guess the refactoring there is really misguided as it adds a lot more code than it removes, will drop it completely and just add a new condition to the current check instead in next version: - /* If value is zero, something is clearly wrong */ - if (!value) + if (!value || value == 1) Thanks, Yurii
On Thu, May 9, 2019 at 8:49 PM Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com> wrote: > On 08.05.19 15:58, Andy Shevchenko wrote: > Yes, looking at this patch now I'd guess the refactoring there is really > misguided as it adds a lot more code than it removes, will drop it > completely and just add a new condition to the current check instead in > next version: > - /* If value is zero, something is clearly wrong */ > - if (!value) > + if (!value || value == 1) Perhaps here makes sense to explicitly show value == 0.
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index e69e55635afb..1b8272374660 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -178,6 +178,7 @@ struct asus_wmi { struct asus_rfkill gps; struct asus_rfkill uwb; + bool asus_hwmon_thermal_available; bool asus_hwmon_fan_manual_mode; int asus_hwmon_num_fans; int asus_hwmon_pwm; @@ -1375,6 +1376,32 @@ static struct attribute *hwmon_attributes[] = { NULL }; +static int asus_hwmon_check_thermal_available(struct asus_wmi *asus) +{ + u32 value = ASUS_WMI_UNSUPPORTED_METHOD; + int err; + + asus->asus_hwmon_thermal_available = false; + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_THERMAL_CTRL, &value); + + if (err < 0) { + if (err == -ENODEV) + return 0; + + return err; + } + + /* + * If the temperature value in deci-Kelvin is near the absolute + * zero temperature, something is clearly wrong. + */ + if (!value || value == 1) + return 0; + + asus->asus_hwmon_thermal_available = true; + return 0; +} + static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj, struct attribute *attr, int idx) { @@ -1388,8 +1415,6 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj, if (attr == &dev_attr_pwm1.attr) dev_id = ASUS_WMI_DEVID_FAN_CTRL; - else if (attr == &dev_attr_temp1_input.attr) - dev_id = ASUS_WMI_DEVID_THERMAL_CTRL; if (attr == &dev_attr_fan1_input.attr || attr == &dev_attr_fan1_label.attr @@ -1414,15 +1439,13 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj, * - reverved bits are non-zero * - sfun and presence bit are not set */ - if (value == ASUS_WMI_UNSUPPORTED_METHOD || value & 0xFFF80000 + if (value == ASUS_WMI_UNSUPPORTED_METHOD || (value & 0xFFF80000) || (!asus->sfun && !(value & ASUS_WMI_DSTS_PRESENCE_BIT))) ok = false; else ok = fan_attr <= asus->asus_hwmon_num_fans; - } else if (dev_id == ASUS_WMI_DEVID_THERMAL_CTRL) { - /* If value is zero, something is clearly wrong */ - if (!value) - ok = false; + } else if (attr == &dev_attr_temp1_input.attr) { + ok = asus->asus_hwmon_thermal_available; } else if (fan_attr <= asus->asus_hwmon_num_fans && fan_attr != -1) { ok = true; } else { @@ -1469,6 +1492,14 @@ static int asus_wmi_fan_init(struct asus_wmi *asus) } pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans); + + status = asus_hwmon_check_thermal_available(asus); + if (status) { + pr_warn("Could not check if thermal available: %d\n", status); + return -ENXIO; + } + + pr_info("Thermal available: %d\n", asus->asus_hwmon_thermal_available); return 0; }
The obviously wrong value 1 for temperature device ID in this driver is returned by at least some devices, including TUF Gaming series laptops, instead of 0 as expected previously. Observable effect is that a temp1_input in hwmon reads temperature near absolute zero. * Consider 0.1 K an erroneous value in addition to 0 K. * Refactor detection of thermal input availability to a separate function. Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com> --- drivers/platform/x86/asus-wmi.c | 45 ++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 7 deletions(-)