Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] platform/x86: hp-wmi: add support for thermal policy
To:     Elia Devito <eliadevito@gmail.com>
Cc:     Darren Hart <dvhart@infradead.org>,
        Andy Shevchenko <andy@infradead.org>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20200914170252.41125-1-eliadevito@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <af8ccfbf-8a6f-8667-aee6-d196e8e7c845@redhat.com>
Date:   Thu, 17 Sep 2020 13:03:22 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200914170252.41125-1-eliadevito@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 9/14/20 7:02 PM, Elia Devito wrote:
> HP Spectre notebooks (and probabily other model as well)
> support at least 3 thermal policy:
>   - Default
>   - Performance
>   - Cool
> 
> the correct thermal policy configuration make the firmware to correctly
> set OEM variables for Intel DPTF and optimize fan management to reach
> the best performance.

You mention DPTF, have you tested this patch together with Matthew Garret's
modified thermald which supports dynamic DPTF profiles ? :

https://mjg59.dreamwidth.org/54923.html

And if you have, have you alsoe tested it without this ?


> 
> Signed-off-by: Elia Devito <eliadevito@gmail.com>
> ---
>   drivers/platform/x86/hp-wmi.c | 80 +++++++++++++++++++++++++++++++++++
>   1 file changed, 80 insertions(+)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 1762f335bac9..14ee176f5588 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -81,6 +81,7 @@ enum hp_wmi_commandtype {
>   	HPWMI_FEATURE2_QUERY		= 0x0d,
>   	HPWMI_WIRELESS2_QUERY		= 0x1b,
>   	HPWMI_POSTCODEERROR_QUERY	= 0x2a,
> +	HPWMI_THERMAL_POLICY_QUERY	= 0x4c
>   };
>   
>   enum hp_wmi_command {
> @@ -114,6 +115,12 @@ enum hp_wireless2_bits {
>   	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
>   };
>   
> +enum hp_thermal_policy {
> +	HP_THERMAL_POLICY_PERFORMANCE	= 0x00,
> +	HP_THERMAL_POLICY_DEFAULT		= 0x01,
> +	HP_THERMAL_POLICY_COOL			= 0x02
> +};
> +
>   #define IS_HWBLOCKED(x) ((x & HPWMI_POWER_FW_OR_HW) != HPWMI_POWER_FW_OR_HW)
>   #define IS_SWBLOCKED(x) !(x & HPWMI_POWER_SOFT)
>   
> @@ -458,6 +465,28 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
>   	return sprintf(buf, "0x%x\n", value);
>   }
>   
> +static ssize_t thermal_policy_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	int value;
> +
> +	/* Get the current thermal policy */
> +	value = hp_wmi_read_int(HPWMI_THERMAL_POLICY_QUERY);
> +	if (value < 0)
> +		return value;
> +
> +	switch (value) {
> +	case HP_THERMAL_POLICY_PERFORMANCE:
> +		return sprintf(buf, "Performance (%x)\n", value);
> +	case HP_THERMAL_POLICY_DEFAULT:
> +		return sprintf(buf, "Default (%x)\n", value);
> +	case HP_THERMAL_POLICY_COOL:
> +		return sprintf(buf, "Cool (%x)\n", value);
> +	default:
> +		return sprintf(buf, "Unknown (%x)\n", value);
> +	}
> +}
> +

So your showing it as a string here.


>   static ssize_t als_store(struct device *dev, struct device_attribute *attr,
>   			 const char *buf, size_t count)
>   {
> @@ -499,12 +528,35 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>   	return count;
>   }
>   
> +static ssize_t thermal_policy_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	u32 tmp;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 10, &tmp);
> +	if (ret)
> +		return ret;


But then taking an integer value here. That is not really a good userspace interface IMHO.

What you can do is put the strings in an array of strings and then loop
over the array in show, adding [] around the selected option, so that
showing it will e.g. output:

Performance [Default] Cool

or:

[Performance] Default Cool

(and in the unknown case none of the 3 would have [] around it)

And then in the store callback also loop over the array,
comparing the user provided string with the 3 strings and
then selecting the value based on that; or return -EINVAL
if non of the strings match. Note I'm open to other
suggestions, but this is more or less how we usually deal with
exporting enums in sysfs now a days.

Note please use sysfs_match_string for the store function,
this will do things like ignoring the '\n' which echo
adds for you without needing to code all this out.


> +
> +	if (tmp < HP_THERMAL_POLICY_PERFORMANCE || tmp > HP_THERMAL_POLICY_COOL)
> +		return -EINVAL;
> +
> +	/* Set thermal policy */
> +	ret = hp_wmi_perform_query(HPWMI_THERMAL_POLICY_QUERY, HPWMI_WRITE, &tmp,
> +				       sizeof(tmp), sizeof(tmp));
> +	if (ret)
> +		return ret < 0 ? ret : -EINVAL;
> +
> +	return count;
> +}
> +
>   static DEVICE_ATTR_RO(display);
>   static DEVICE_ATTR_RO(hddtemp);
>   static DEVICE_ATTR_RW(als);
>   static DEVICE_ATTR_RO(dock);
>   static DEVICE_ATTR_RO(tablet);
>   static DEVICE_ATTR_RW(postcode);
> +static DEVICE_ATTR_RW(thermal_policy);
>   
>   static struct attribute *hp_wmi_attrs[] = {
>   	&dev_attr_display.attr,
> @@ -861,6 +913,30 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
>   	return err;
>   }
>   
> +static int thermal_policy_setup(struct platform_device *device)
> +{
> +	int err, tp;
> +
> +	tp = hp_wmi_read_int(HPWMI_THERMAL_POLICY_QUERY);
> +	if (tp < 0)
> +		return tp;
> +
> +	/*
> +	 * set thermal policy to ensure that the firmware correctly
> +	 * sets the OEM variables for the DPTF
> +	 */
> +	err = hp_wmi_perform_query(HPWMI_THERMAL_POLICY_QUERY, HPWMI_WRITE, &tp,
> +							   sizeof(tp), 0);
> +	if (err)
> +		return err;
> +
> +	err = device_create_file(&device->dev, &dev_attr_thermal_policy);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
>   static int __init hp_wmi_bios_setup(struct platform_device *device)
>   {
>   	/* clear detected rfkill devices */
> @@ -872,6 +948,8 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>   	if (hp_wmi_rfkill_setup(device))
>   		hp_wmi_rfkill2_setup(device);
>   
> +	thermal_policy_setup(device);
> +
>   	return 0;
>   }
>   
> @@ -879,6 +957,8 @@ static int __exit hp_wmi_bios_remove(struct platform_device *device)
>   {
>   	int i;
>   
> +	device_remove_file(&device->dev, &dev_attr_thermal_policy);
> +
>   	for (i = 0; i < rfkill2_count; i++) {
>   		rfkill_unregister(rfkill2[i].rfkill);
>   		rfkill_destroy(rfkill2[i].rfkill);
> 

Otherwise this looks good to me.

Regsrds,

Hans