[v5] platform/x86: dell-laptop: Implement platform_profile

Commit Message

Lyndon Sanche May 1, 2024, 9:58 p.m. UTC

Some Dell laptops support configuration of preset fan modes through
smbios tables.

If the platform supports these fan modes, set up platform_profile to
change these modes. If not supported, skip enabling platform_profile.

Signed-off-by: Lyndon Sanche <[e-mail]>
---
v5:
 - Fix indent in smbios-thermal-ctl comment
 - Remove linux/wmi.h include
 - Add 'select ACPI_PLATFORM_PROFILE' to Dell KConfig
v4:
 - Make thermal_init and thermal_cleanup static
 - Rearrange order of added includes, did not edit current includes
 - Include bits.h
 - Switch comment style
 - Return error if platform_profile registering failed
 - Add thermal calls to call_blacklist
 - Align defines with tabs
 - Correct separation of function and error handling
 - Propagate error codes up
v3:
 - Convert smbios-thermal-ctl docs to multiline comment and wrap
 - Change thermal_mode_bits enum to directly be BIT() values
	- Convert related code to use this
 - Use FIELD_GET/PREP and GENNMASK for getting/setting thermal modes
	- Correct offset for getting current ACC mode, setting offset
		unchanged
 - Check if thermal_handler is allocated before freeing and
	 unregistering platform_profile
v2:
 - Wrap smbios-thermal-ctl comment
 - Return proper error code when invalid state returned
 - Simplify platform_profile_get returns
 - Propogate ENOMEM error
---
 drivers/platform/x86/dell/Kconfig            |   1 +
 drivers/platform/x86/dell/dell-laptop.c      | 238 +++++++++++++++++++
 drivers/platform/x86/dell/dell-smbios-base.c |   1 +
 drivers/platform/x86/dell/dell-smbios.h      |   1 +
 4 files changed, 241 insertions(+)

Comments

kernel test robot May 3, 2024, 10:19 a.m. UTC | #1

Hi Lyndon,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.9-rc6 next-20240503]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lyndon-Sanche/platform-x86-dell-laptop-Implement-platform_profile/20240502-060146
base:   linus/master
patch link:    https://lore.kernel.org/r/20240501215829.4991-2-lsanche%40lyndeno.ca
patch subject: [PATCH v5] platform/x86: dell-laptop: Implement platform_profile
config: i386-kismet-CONFIG_ACPI_PLATFORM_PROFILE-CONFIG_DELL_LAPTOP-0-0 (https://download.01.org/0day-ci/archive/20240503/[e-mail]/config)
reproduce: (https://download.01.org/0day-ci/archive/20240503/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for ACPI_PLATFORM_PROFILE when selected by DELL_LAPTOP
   WARNING: unmet direct dependencies detected for ACPI_PLATFORM_PROFILE
     Depends on [n]: ACPI [=n]
     Selected by [y]:
     - DELL_LAPTOP [=y] && X86_PLATFORM_DEVICES [=y] && X86_PLATFORM_DRIVERS_DELL [=y] && DMI [=y] && BACKLIGHT_CLASS_DEVICE [=y] && (ACPI_VIDEO [=n] || ACPI_VIDEO [=n]=n) && (RFKILL [=n] || RFKILL [=n]=n) && (DELL_WMI [=n] || DELL_WMI [=n]=n) && SERIO_I8042 [=y] && DELL_SMBIOS [=y]

Armin Wolf May 3, 2024, 9:19 p.m. UTC | #2

Am 01.05.24 um 23:58 schrieb Lyndon Sanche:

> Some Dell laptops support configuration of preset fan modes through
> smbios tables.
>
> If the platform supports these fan modes, set up platform_profile to
> change these modes. If not supported, skip enabling platform_profile.
>
> Signed-off-by: Lyndon Sanche <[e-mail]>
> ---
> v5:
>   - Fix indent in smbios-thermal-ctl comment
>   - Remove linux/wmi.h include
>   - Add 'select ACPI_PLATFORM_PROFILE' to Dell KConfig
> v4:
>   - Make thermal_init and thermal_cleanup static
>   - Rearrange order of added includes, did not edit current includes
>   - Include bits.h
>   - Switch comment style
>   - Return error if platform_profile registering failed
>   - Add thermal calls to call_blacklist
>   - Align defines with tabs
>   - Correct separation of function and error handling
>   - Propagate error codes up
> v3:
>   - Convert smbios-thermal-ctl docs to multiline comment and wrap
>   - Change thermal_mode_bits enum to directly be BIT() values
> 	- Convert related code to use this
>   - Use FIELD_GET/PREP and GENNMASK for getting/setting thermal modes
> 	- Correct offset for getting current ACC mode, setting offset
> 		unchanged
>   - Check if thermal_handler is allocated before freeing and
> 	 unregistering platform_profile
> v2:
>   - Wrap smbios-thermal-ctl comment
>   - Return proper error code when invalid state returned
>   - Simplify platform_profile_get returns
>   - Propogate ENOMEM error
> ---
>   drivers/platform/x86/dell/Kconfig            |   1 +
>   drivers/platform/x86/dell/dell-laptop.c      | 238 +++++++++++++++++++
>   drivers/platform/x86/dell/dell-smbios-base.c |   1 +
>   drivers/platform/x86/dell/dell-smbios.h      |   1 +
>   4 files changed, 241 insertions(+)
>
> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index bd9f445974cc..5195ad59b44d 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -57,6 +57,7 @@ config DELL_LAPTOP
>   	select POWER_SUPPLY
>   	select LEDS_CLASS
>   	select NEW_LEDS
> +	select ACPI_PLATFORM_PROFILE
>   	help
>   	This driver adds support for rfkill and backlight control to Dell
>   	laptops (except for some models covered by the Compal driver).
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 42f7de2b4522..dc530a4f5047 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -27,6 +27,9 @@
>   #include <linux/i8042.h>
>   #include <linux/debugfs.h>
>   #include <linux/seq_file.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/platform_profile.h>
>   #include <acpi/video.h>
>   #include "dell-rbtn.h"
>   #include "dell-smbios.h"
> @@ -95,6 +98,7 @@ static struct backlight_device *dell_backlight_device;
>   static struct rfkill *wifi_rfkill;
>   static struct rfkill *bluetooth_rfkill;
>   static struct rfkill *wwan_rfkill;
> +static struct platform_profile_handler *thermal_handler;
>   static bool force_rfkill;
>   static bool micmute_led_registered;
>   static bool mute_led_registered;
> @@ -2199,6 +2203,232 @@ static int mute_led_set(struct led_classdev *led_cdev,
>   	return 0;
>   }
>
> +/* Derived from smbios-thermal-ctl
> + *
> + * cbClass 17
> + * cbSelect 19
> + * User Selectable Thermal Tables(USTT)
> + * cbArg1 determines the function to be performed
> + * cbArg1 0x0 = Get Thermal Information
> + *  cbRES1         Standard return codes (0, -1, -2)
> + *  cbRES2, byte 0  Bitmap of supported thermal modes. A mode is supported if
> + *                  its bit is set to 1
> + *     Bit 0 Balanced
> + *     Bit 1 Cool Bottom
> + *     Bit 2 Quiet
> + *     Bit 3 Performance
> + *  cbRES2, byte 1 Bitmap of supported Active Acoustic Controller (AAC) modes.
> + *                 Each mode corresponds to the supported thermal modes in
> + *                  byte 0. A mode is supported if its bit is set to 1.
> + *     Bit 0 AAC (Balanced)
> + *     Bit 1 AAC (Cool Bottom
> + *     Bit 2 AAC (Quiet)
> + *     Bit 3 AAC (Performance)
> + *  cbRes3, byte 0 Current Thermal Mode
> + *     Bit 0 Balanced
> + *     Bit 1 Cool Bottom
> + *     Bit 2 Quiet
> + *     Bit 3 Performanc
> + *  cbRes3, byte 1  AAC Configuration type
> + *          0       Global (AAC enable/disable applies to all supported USTT modes)
> + *          1       USTT mode specific
> + *  cbRes3, byte 2  Current Active Acoustic Controller (AAC) Mode
> + *     If AAC Configuration Type is Global,
> + *          0       AAC mode disabled
> + *          1       AAC mode enabled
> + *     If AAC Configuration Type is USTT mode specific (multiple bits may be set),
> + *          Bit 0 AAC (Balanced)
> + *          Bit 1 AAC (Cool Bottom
> + *          Bit 2 AAC (Quiet)
> + *          Bit 3 AAC (Performance)
> + *  cbRes3, byte 3  Current Fan Failure Mode
> + *     Bit 0 Minimal Fan Failure (at least one fan has failed, one fan working)
> + *     Bit 1 Catastrophic Fan Failure (all fans have failed)
> + *
> + * cbArg1 0x1   (Set Thermal Information), both desired thermal mode and
> + *               desired AAC mode shall be applied
> + * cbArg2, byte 0  Desired Thermal Mode to set
> + *                  (only one bit may be set for this parameter)
> + *     Bit 0 Balanced
> + *     Bit 1 Cool Bottom
> + *     Bit 2 Quiet
> + *     Bit 3 Performance
> + * cbArg2, byte 1  Desired Active Acoustic Controller (AAC) Mode to set
> + *     If AAC Configuration Type is Global,
> + *         0  AAC mode disabled
> + *         1  AAC mode enabled
> + *     If AAC Configuration Type is USTT mode specific
> + *     (multiple bits may be set for this parameter),
> + *         Bit 0 AAC (Balanced)
> + *         Bit 1 AAC (Cool Bottom
> + *         Bit 2 AAC (Quiet)
> + *         Bit 3 AAC (Performance)
> + */
> +
> +#define DELL_ACC_GET_FIELD		GENMASK(19, 16)
> +#define DELL_ACC_SET_FIELD		GENMASK(11, 8)
> +#define DELL_THERMAL_SUPPORTED	GENMASK(3, 0)
> +
> +enum thermal_mode_bits {
> +	DELL_BALANCED = BIT(0),
> +	DELL_COOL_BOTTOM = BIT(1),
> +	DELL_QUIET = BIT(2),
> +	DELL_PERFORMANCE = BIT(3),
> +};
> +
> +static int thermal_get_mode(void)
> +{
> +	struct calling_interface_buffer buffer;
> +	int state;
> +	int ret;
> +
> +	dell_fill_request(&buffer, 0x0, 0, 0, 0);
> +	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
> +	if (ret)
> +		return ret;
> +	state = buffer.output[2];
> +	if (state & DELL_BALANCED)
> +		return DELL_BALANCED;
> +	else if (state & DELL_COOL_BOTTOM)
> +		return DELL_COOL_BOTTOM;
> +	else if (state & DELL_QUIET)
> +		return DELL_QUIET;
> +	else if (state & DELL_PERFORMANCE)
> +		return DELL_PERFORMANCE;
> +	else
> +		return -ENXIO;
> +}
> +
> +static int thermal_get_supported_modes(int *supported_bits)
> +{
> +	struct calling_interface_buffer buffer;
> +	int ret;
> +
> +	dell_fill_request(&buffer, 0x0, 0, 0, 0);
> +	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
> +	if (ret)
> +		return ret;
> +	*supported_bits = FIELD_GET(DELL_THERMAL_SUPPORTED, buffer.output[1]);
> +	return 0;
> +}
> +
> +static int thermal_get_acc_mode(int *acc_mode)
> +{
> +	struct calling_interface_buffer buffer;
> +	int ret;
> +
> +	dell_fill_request(&buffer, 0x0, 0, 0, 0);
> +	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
> +	if (ret)
> +		return ret;
> +	*acc_mode = FIELD_GET(DELL_ACC_GET_FIELD, buffer.output[3]);
> +	return 0;
> +}
> +
> +static int thermal_set_mode(enum thermal_mode_bits state)
> +{
> +	struct calling_interface_buffer buffer;
> +	int ret;
> +	int acc_mode;
> +
> +	ret = thermal_get_acc_mode(&acc_mode);
> +	if (ret)
> +		return ret;
> +
> +	dell_fill_request(&buffer, 0x1, FIELD_PREP(DELL_ACC_SET_FIELD, acc_mode) | state, 0, 0);
> +	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
> +	return ret;
> +}
> +
> +static int thermal_platform_profile_set(struct platform_profile_handler *pprof,
> +					enum platform_profile_option profile)
> +{
> +	switch (profile) {
> +	case PLATFORM_PROFILE_BALANCED:
> +		return thermal_set_mode(DELL_BALANCED);
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		return thermal_set_mode(DELL_PERFORMANCE);
> +	case PLATFORM_PROFILE_QUIET:
> +		return thermal_set_mode(DELL_QUIET);
> +	case PLATFORM_PROFILE_COOL:
> +		return thermal_set_mode(DELL_COOL_BOTTOM);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
> +					enum platform_profile_option *profile)
> +{
> +	int ret;
> +
> +	ret = thermal_get_mode();
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret) {
> +	case DELL_BALANCED:
> +		*profile = PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case DELL_PERFORMANCE:
> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	case DELL_COOL_BOTTOM:
> +		*profile = PLATFORM_PROFILE_COOL;
> +		break;
> +	case DELL_QUIET:
> +		*profile = PLATFORM_PROFILE_QUIET;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int thermal_init(void)
> +{
> +	int ret;
> +	int supported_modes;
> +
> +	/* If thermal modes not supported, exit without error */
> +	ret = thermal_get_supported_modes(&supported_modes);
> +	if (ret < 0)
> +		return ret;

Hi,

some older models might not support the USTT commands, which would prevent dell-laptop
from loading on such machines.
Since dell-smbios-base already knows which commands are supported (stored in da_supported_commands),
maybe you can add a function for checking if a certain class of commands is supported and
skip thermal_init() if the USTT commands are not supported.

Thanks,
Armin Wolf

> +	if (!supported_modes)
> +		return 0;
> +
> +	thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
> +	if (!thermal_handler)
> +		return -ENOMEM;
> +	thermal_handler->profile_get = thermal_platform_profile_get;
> +	thermal_handler->profile_set = thermal_platform_profile_set;
> +
> +	if (supported_modes & DELL_QUIET)
> +		set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
> +	if (supported_modes & DELL_COOL_BOTTOM)
> +		set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
> +	if (supported_modes & DELL_BALANCED)
> +		set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
> +	if (supported_modes & DELL_PERFORMANCE)
> +		set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
> +
> +	/* Clean up if failed */
> +	ret = platform_profile_register(thermal_handler);
> +	if (ret)
> +		kfree(thermal_handler);
> +
> +	return ret;
> +}
> +
> +static void thermal_cleanup(void)
> +{
> +	if (thermal_handler) {
> +		platform_profile_remove();
> +		kfree(thermal_handler);
> +	}
> +}
> +
>   static struct led_classdev mute_led_cdev = {
>   	.name = "platform::mute",
>   	.max_brightness = 1,
> @@ -2238,6 +2468,11 @@ static int __init dell_init(void)
>   		goto fail_rfkill;
>   	}
>
> +	/* Do not fail module if thermal modes not supported, just skip */
> +	ret = thermal_init();
> +	if (ret)
> +		goto fail_thermal;
> +
>   	if (quirks && quirks->touchpad_led)
>   		touchpad_led_init(&platform_device->dev);
>
> @@ -2317,6 +2552,8 @@ static int __init dell_init(void)
>   		led_classdev_unregister(&mute_led_cdev);
>   fail_led:
>   	dell_cleanup_rfkill();
> +fail_thermal:
> +	thermal_cleanup();
>   fail_rfkill:
>   	platform_device_del(platform_device);
>   fail_platform_device2:
> @@ -2344,6 +2581,7 @@ static void __exit dell_exit(void)
>   		platform_device_unregister(platform_device);
>   		platform_driver_unregister(&platform_driver);
>   	}
> +	thermal_cleanup();
>   }
>
>   /* dell-rbtn.c driver export functions which will not work correctly (and could
> diff --git a/drivers/platform/x86/dell/dell-smbios-base.c b/drivers/platform/x86/dell/dell-smbios-base.c
> index e61bfaf8b5c4..5bc2e394dd1c 100644
> --- a/drivers/platform/x86/dell/dell-smbios-base.c
> +++ b/drivers/platform/x86/dell/dell-smbios-base.c
> @@ -71,6 +71,7 @@ static struct smbios_call call_blacklist[] = {
>   	/* handled by kernel: dell-laptop */
>   	{0x0000, CLASS_INFO, SELECT_RFKILL},
>   	{0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT},
> +	{0x0000, CLASS_INFO, SELECT_THERMAL_MANAGEMENT},
>   };
>
>   struct token_range {
> diff --git a/drivers/platform/x86/dell/dell-smbios.h b/drivers/platform/x86/dell/dell-smbios.h
> index eb341bf000c6..585d042f1779 100644
> --- a/drivers/platform/x86/dell/dell-smbios.h
> +++ b/drivers/platform/x86/dell/dell-smbios.h
> @@ -19,6 +19,7 @@
>   /* Classes and selects used only in kernel drivers */
>   #define CLASS_KBD_BACKLIGHT 4
>   #define SELECT_KBD_BACKLIGHT 11
> +#define SELECT_THERMAL_MANAGEMENT 19
>
>   /* Tokens used in kernel drivers, any of these
>    * should be filtered from userspace access

Lyndon Sanche May 4, 2024, 12:59 a.m. UTC | #3

On Fri, May 3 2024 at 11:19:07 PM +02:00:00, Armin Wolf 
<[e-mail]> wrote:
> Am 01.05.24 um 23:58 schrieb Lyndon Sanche:
>> +static int thermal_init(void)
>> +{
>> +	int ret;
>> +	int supported_modes;
>> +
>> +	/* If thermal modes not supported, exit without error */
>> +	ret = thermal_get_supported_modes(&supported_modes);
>> +	if (ret < 0)
>> +		return ret;
> 
> Hi,
> 
> some older models might not support the USTT commands, which would 
> prevent dell-laptop
> from loading on such machines.
> Since dell-smbios-base already knows which commands are supported 
> (stored in da_supported_commands),
> maybe you can add a function for checking if a certain class of 
> commands is supported and
> skip thermal_init() if the USTT commands are not supported.
> 
> Thanks,
> Armin Wolf

This is a good idea, I will have a look at dell-smbios-base to see 
exactly how I can check for support. If support is not available, 
thermal_init will skip or return 0 (depending on where I put the check).

Thanks,

Lyndon

Lyndon Sanche May 4, 2024, 1:03 a.m. UTC | #4

On Fri, May 3 2024 at 06:19:18 PM +08:00:00, kernel test robot 
<[e-mail]> wrote:
> Hi Lyndon,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on linus/master]
> [also build test WARNING on v6.9-rc6 next-20240503]
> [If your patch is applied to the wrong git tree, kindly drop us a 
> note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    
> https://github.com/intel-lab-lkp/linux/commits/Lyndon-Sanche/platform-x86-dell-laptop-Implement-platform_profile/20240502-060146
> base:   linus/master
> patch link:    
> https://lore.kernel.org/r/20240501215829.4991-2-lsanche%40lyndeno.ca
> patch subject: [PATCH v5] platform/x86: dell-laptop: Implement 
> platform_profile
> config: 
> i386-kismet-CONFIG_ACPI_PLATFORM_PROFILE-CONFIG_DELL_LAPTOP-0-0 
> (https://download.01.org/0day-ci/archive/20240503/[e-mail]/config)
> reproduce: 
> (https://download.01.org/0day-ci/archive/20240503/[e-mail]/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new 
> version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[e-mail]>
> | Closes: 
> https://lore.kernel.org/oe-kbuild-all/[e-mail]/
> 
> kismet warnings: (new ones prefixed by >>)
>>>  kismet: WARNING: unmet direct dependencies detected for 
>>> ACPI_PLATFORM_PROFILE when selected by DELL_LAPTOP
>    WARNING: unmet direct dependencies detected for 
> ACPI_PLATFORM_PROFILE
>      Depends on [n]: ACPI [=n]
>      Selected by [y]:
>      - DELL_LAPTOP [=y] && X86_PLATFORM_DEVICES [=y] && 
> X86_PLATFORM_DRIVERS_DELL [=y] && DMI [=y] && BACKLIGHT_CLASS_DEVICE 
> [=y] && (ACPI_VIDEO [=n] || ACPI_VIDEO [=n]=n) && (RFKILL [=n] || 
> RFKILL [=n]=n) && (DELL_WMI [=n] || DELL_WMI [=n]=n) && SERIO_I8042 
> [=y] && DELL_SMBIOS [=y]
> 
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

I will try reproducing this test on my machine, to avoid spamming the 
mailing list with the same error over and over.

Hans de Goede May 6, 2024, 10:18 a.m. UTC | #5

Hi Lyndon,

Thank you for your patch!

On 5/4/24 3:03 AM, Lyndon Sanche wrote:
> 
> 
> On Fri, May 3 2024 at 06:19:18 PM +08:00:00, kernel test robot <[e-mail]> wrote:
>> Hi Lyndon,
>>
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on linus/master]
>> [also build test WARNING on v6.9-rc6 next-20240503]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Lyndon-Sanche/platform-x86-dell-laptop-Implement-platform_profile/20240502-060146
>> base:   linus/master
>> patch link:    https://lore.kernel.org/r/20240501215829.4991-2-lsanche%40lyndeno.ca
>> patch subject: [PATCH v5] platform/x86: dell-laptop: Implement platform_profile
>> config: i386-kismet-CONFIG_ACPI_PLATFORM_PROFILE-CONFIG_DELL_LAPTOP-0-0 (https://download.01.org/0day-ci/archive/20240503/[e-mail]/config)
>> reproduce: (https://download.01.org/0day-ci/archive/20240503/[e-mail]/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <[e-mail]>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
>>
>> kismet warnings: (new ones prefixed by >>)
>>>>  kismet: WARNING: unmet direct dependencies detected for ACPI_PLATFORM_PROFILE when selected by DELL_LAPTOP
>>    WARNING: unmet direct dependencies detected for ACPI_PLATFORM_PROFILE
>>      Depends on [n]: ACPI [=n]
>>      Selected by [y]:
>>      - DELL_LAPTOP [=y] && X86_PLATFORM_DEVICES [=y] && X86_PLATFORM_DRIVERS_DELL [=y] && DMI [=y] && BACKLIGHT_CLASS_DEVICE [=y] && (ACPI_VIDEO [=n] || ACPI_VIDEO [=n]=n) && (RFKILL [=n] || RFKILL [=n]=n) && (DELL_WMI [=n] || DELL_WMI [=n]=n) && SERIO_I8042 [=y] && DELL_SMBIOS [=y]
>>
>> -- 
>> 0-DAY CI Kernel Test Service
>> https://github.com/intel/lkp-tests/wiki
> 
> I will try reproducing this test on my machine, to avoid spamming the mailing list with the same error over and over.

No need to reproduce this. When you select something in Kconfig you must ensure
that the item doing the selecting depends on all the dependencies of what you
are selecting.

IOW if you add this change to your next version then that should fix this:

diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index bd9f445974cc..d18fbc6a5fbf 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -47,6 +47,7 @@ config DCDBAS
 config DELL_LAPTOP
 	tristate "Dell Laptop Extras"
 	default m
+	depends on ACPI
 	depends on DMI
 	depends on BACKLIGHT_CLASS_DEVICE
 	depends on ACPI_VIDEO || ACPI_VIDEO = n

And please also address Armin's remark about making sure that failure
to initialize platform_profile support should not cause the entire driver
to fail to probe.

I see that Armin suggests to check da_supported_commands for this,
this is a good idea but atm this is private to dell-smbios-base. So
you will first need to do a small preparation patch adding a small:

bool dell_laptop_check_supported_cmds(struct calling_interface_buffer *buffer)
{
	return da_supported_commands & (1 << buffer->cmd_class);
}
EXPORT_SYMBOL_GPL(dell_laptop_check_supported_cmds):

helper for this.

If this check fails (returns false) make the code not register
the platform_profile() while allowing probe() to continue / succeed,
please do not log anything in this case (or use dev_dbg())

If this check succeeds but subsequent dell_smbios_call()'s
fail during probe, then it is ok to log an error but please
still let probe() continue / succeed (without registering
a platform_profile handler).

Regards,

Hans

Lyndon Sanche May 7, 2024, 4 p.m. UTC | #6

On Mon, May 6 2024 at 12:18:05 PM +02:00:00, Hans de Goede 
<[e-mail]> wrote:
> Hi Lyndon,
> 
> Thank you for your patch!
> 
> On 5/4/24 3:03 AM, Lyndon Sanche wrote:
>> 
>> 
>>  On Fri, May 3 2024 at 06:19:18 PM +08:00:00, kernel test robot 
>> <[e-mail]> wrote:
>>>  Hi Lyndon,
>>> 
>>>  kernel test robot noticed the following build warnings:
>>> 
>>>  [auto build test WARNING on linus/master]
>>>  [also build test WARNING on v6.9-rc6 next-20240503]
>>>  [If your patch is applied to the wrong git tree, kindly drop us a 
>>> note.
>>>  And when submitting patch, we suggest to use '--base' as 
>>> documented in
>>>  https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>> 
>>>  url:    
>>> https://github.com/intel-lab-lkp/linux/commits/Lyndon-Sanche/platform-x86-dell-laptop-Implement-platform_profile/20240502-060146
>>>  base:   linus/master
>>>  patch link:    
>>> https://lore.kernel.org/r/20240501215829.4991-2-lsanche%40lyndeno.ca
>>>  patch subject: [PATCH v5] platform/x86: dell-laptop: Implement 
>>> platform_profile
>>>  config: 
>>> i386-kismet-CONFIG_ACPI_PLATFORM_PROFILE-CONFIG_DELL_LAPTOP-0-0 
>>> (https://download.01.org/0day-ci/archive/20240503/[e-mail]/config)
>>>  reproduce: 
>>> (https://download.01.org/0day-ci/archive/20240503/[e-mail]/reproduce)
>>> 
>>>  If you fix the issue in a separate patch/commit (i.e. not just a 
>>> new version of
>>>  the same patch/commit), kindly add following tags
>>>  | Reported-by: kernel test robot <[e-mail]>
>>>  | Closes: 
>>> https://lore.kernel.org/oe-kbuild-all/[e-mail]/
>>> 
>>>  kismet warnings: (new ones prefixed by >>)
>>>>>   kismet: WARNING: unmet direct dependencies detected for 
>>>>> ACPI_PLATFORM_PROFILE when selected by DELL_LAPTOP
>>>     WARNING: unmet direct dependencies detected for 
>>> ACPI_PLATFORM_PROFILE
>>>       Depends on [n]: ACPI [=n]
>>>       Selected by [y]:
>>>       - DELL_LAPTOP [=y] && X86_PLATFORM_DEVICES [=y] && 
>>> X86_PLATFORM_DRIVERS_DELL [=y] && DMI [=y] && 
>>> BACKLIGHT_CLASS_DEVICE [=y] && (ACPI_VIDEO [=n] || ACPI_VIDEO 
>>> [=n]=n) && (RFKILL [=n] || RFKILL [=n]=n) && (DELL_WMI [=n] || 
>>> DELL_WMI [=n]=n) && SERIO_I8042 [=y] && DELL_SMBIOS [=y]
>>> 
>>>  --
>>>  0-DAY CI Kernel Test Service
>>>  https://github.com/intel/lkp-tests/wiki
>> 
>>  I will try reproducing this test on my machine, to avoid spamming 
>> the mailing list with the same error over and over.
> 
> No need to reproduce this. When you select something in Kconfig you 
> must ensure
> that the item doing the selecting depends on all the dependencies of 
> what you
> are selecting.
> 
> IOW if you add this change to your next version then that should fix 
> this:
> 
> diff --git a/drivers/platform/x86/dell/Kconfig 
> b/drivers/platform/x86/dell/Kconfig
> index bd9f445974cc..d18fbc6a5fbf 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -47,6 +47,7 @@ config DCDBAS
>  config DELL_LAPTOP
>  	tristate "Dell Laptop Extras"
>  	default m
> +	depends on ACPI
>  	depends on DMI
>  	depends on BACKLIGHT_CLASS_DEVICE
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
> 
> And please also address Armin's remark about making sure that failure
> to initialize platform_profile support should not cause the entire 
> driver
> to fail to probe.
> 
> I see that Armin suggests to check da_supported_commands for this,
> this is a good idea but atm this is private to dell-smbios-base. So
> you will first need to do a small preparation patch adding a small:
> 
> bool dell_laptop_check_supported_cmds(struct calling_interface_buffer 
> *buffer)
> {
> 	return da_supported_commands & (1 << buffer->cmd_class);
> }
> EXPORT_SYMBOL_GPL(dell_laptop_check_supported_cmds):
> 
> helper for this.
> 
> If this check fails (returns false) make the code not register
> the platform_profile() while allowing probe() to continue / succeed,
> please do not log anything in this case (or use dev_dbg())
> 
> If this check succeeds but subsequent dell_smbios_call()'s
> fail during probe, then it is ok to log an error but please
> still let probe() continue / succeed (without registering
> a platform_profile handler).
> 
> Regards,
> 
> Hans
> 
> 
Hello Hans:

Thank you very much for your feedback and suggestions! I have been busy 
the past few days, but will be able to tackle this this week. These are 
good ideas which I plan to implement.

Thank you,

Lyndon

Shen, Yijun May 8, 2024, 2:24 p.m. UTC | #7

Hi Lyndon,

 Thanks for working on this patch.


Internal Use - Confidential
> -----Original Message-----
> From: Lyndon Sanche <[e-mail]>
> Sent: Thursday, May 2, 2024 5:58 AM
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Hans de Goede <[e-mail]>; Matthew Garrett
> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner Kallweit
> <[e-mail]>; Vegard Nossum <[e-mail]>;
> [e-mail]; [e-mail]; Dell Client
> Kernel <[e-mail]>
> Subject: [PATCH v5] platform/x86: dell-laptop: Implement platform_profile
>
>
> [EXTERNAL EMAIL]
>
> Some Dell laptops support configuration of preset fan modes through smbios
> tables.
>
> If the platform supports these fan modes, set up platform_profile to change
> these modes. If not supported, skip enabling platform_profile.
>
> Signed-off-by: Lyndon Sanche <[e-mail]>
> ---
> v5:
>  - Fix indent in smbios-thermal-ctl comment
>  - Remove linux/wmi.h include
>  - Add 'select ACPI_PLATFORM_PROFILE' to Dell KConfig
> v4:
>  - Make thermal_init and thermal_cleanup static
>  - Rearrange order of added includes, did not edit current includes
>  - Include bits.h
>  - Switch comment style
>  - Return error if platform_profile registering failed
>  - Add thermal calls to call_blacklist
>  - Align defines with tabs
>  - Correct separation of function and error handling
>  - Propagate error codes up
> v3:
>  - Convert smbios-thermal-ctl docs to multiline comment and wrap
>  - Change thermal_mode_bits enum to directly be BIT() values
>       - Convert related code to use this
>  - Use FIELD_GET/PREP and GENNMASK for getting/setting thermal modes
>       - Correct offset for getting current ACC mode, setting offset
>               unchanged
>  - Check if thermal_handler is allocated before freeing and
>        unregistering platform_profile
> v2:
>  - Wrap smbios-thermal-ctl comment
>  - Return proper error code when invalid state returned
>  - Simplify platform_profile_get returns
>  - Propogate ENOMEM error
> ---

 Dell side has an initial testing with this patch on some laptops, it looks good. While changing the platform profile:
1. The corresponding USTT option in BIOS will be changed.
2. thermald will not be impacted. The related PSVT and ITMT will be loaded.
 Some Dell DTs does not have the USTT, Dell'll have a check if nothing is broken.

  Additional, with this patch, follow behavior is found:
 1. For example, the platform profile is quiet.
 2. Reboot the system and change the USTT to performance.
 3. Boot to desktop, the platform profile is "quiet", the USTT will be changed back to "quiet".
 This looks like not a proper user experience. The platform profile should honor the BIOS setting, aka, the platform profile should be switched to "performance".

>  drivers/platform/x86/dell/Kconfig            |   1 +
>  drivers/platform/x86/dell/dell-laptop.c      | 238 +++++++++++++++++++
>  drivers/platform/x86/dell/dell-smbios-base.c |   1 +
>  drivers/platform/x86/dell/dell-smbios.h      |   1 +
>  4 files changed, 241 insertions(+)
>
> diff --git a/drivers/platform/x86/dell/Kconfig
> b/drivers/platform/x86/dell/Kconfig
> index bd9f445974cc..5195ad59b44d 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -57,6 +57,7 @@ config DELL_LAPTOP
>       select POWER_SUPPLY
>       select LEDS_CLASS
>       select NEW_LEDS
> +     select ACPI_PLATFORM_PROFILE
>       help
>       This driver adds support for rfkill and backlight control to Dell
>       laptops (except for some models covered by the Compal driver).
> diff --git a/drivers/platform/x86/dell/dell-laptop.c
> b/drivers/platform/x86/dell/dell-laptop.c
> index 42f7de2b4522..dc530a4f5047 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -27,6 +27,9 @@
>  #include <linux/i8042.h>
>  #include <linux/debugfs.h>
>  #include <linux/seq_file.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/platform_profile.h>
>  #include <acpi/video.h>
>  #include "dell-rbtn.h"
>  #include "dell-smbios.h"
> @@ -95,6 +98,7 @@ static struct backlight_device *dell_backlight_device;
> static struct rfkill *wifi_rfkill;  static struct rfkill *bluetooth_rfkill;  static struct
> rfkill *wwan_rfkill;
> +static struct platform_profile_handler *thermal_handler;
>  static bool force_rfkill;
>  static bool micmute_led_registered;
>  static bool mute_led_registered;
> @@ -2199,6 +2203,232 @@ static int mute_led_set(struct led_classdev
> *led_cdev,
>       return 0;
>  }
>
> +/* Derived from smbios-thermal-ctl
> + *
> + * cbClass 17
> + * cbSelect 19
> + * User Selectable Thermal Tables(USTT)
> + * cbArg1 determines the function to be performed
> + * cbArg1 0x0 = Get Thermal Information
> + *  cbRES1         Standard return codes (0, -1, -2)
> + *  cbRES2, byte 0  Bitmap of supported thermal modes. A mode is
> supported if
> + *                  its bit is set to 1
> + *     Bit 0 Balanced
> + *     Bit 1 Cool Bottom
> + *     Bit 2 Quiet
> + *     Bit 3 Performance
> + *  cbRES2, byte 1 Bitmap of supported Active Acoustic Controller (AAC)
> modes.
> + *                 Each mode corresponds to the supported thermal modes in
> + *                  byte 0. A mode is supported if its bit is set to 1.
> + *     Bit 0 AAC (Balanced)
> + *     Bit 1 AAC (Cool Bottom
> + *     Bit 2 AAC (Quiet)
> + *     Bit 3 AAC (Performance)
> + *  cbRes3, byte 0 Current Thermal Mode
> + *     Bit 0 Balanced
> + *     Bit 1 Cool Bottom
> + *     Bit 2 Quiet
> + *     Bit 3 Performanc
> + *  cbRes3, byte 1  AAC Configuration type
> + *          0       Global (AAC enable/disable applies to all supported USTT
> modes)
> + *          1       USTT mode specific
> + *  cbRes3, byte 2  Current Active Acoustic Controller (AAC) Mode
> + *     If AAC Configuration Type is Global,
> + *          0       AAC mode disabled
> + *          1       AAC mode enabled
> + *     If AAC Configuration Type is USTT mode specific (multiple bits may be
> set),
> + *          Bit 0 AAC (Balanced)
> + *          Bit 1 AAC (Cool Bottom
> + *          Bit 2 AAC (Quiet)
> + *          Bit 3 AAC (Performance)
> + *  cbRes3, byte 3  Current Fan Failure Mode
> + *     Bit 0 Minimal Fan Failure (at least one fan has failed, one fan working)
> + *     Bit 1 Catastrophic Fan Failure (all fans have failed)
> + *
> + * cbArg1 0x1   (Set Thermal Information), both desired thermal mode and
> + *               desired AAC mode shall be applied
> + * cbArg2, byte 0  Desired Thermal Mode to set
> + *                  (only one bit may be set for this parameter)
> + *     Bit 0 Balanced
> + *     Bit 1 Cool Bottom
> + *     Bit 2 Quiet
> + *     Bit 3 Performance
> + * cbArg2, byte 1  Desired Active Acoustic Controller (AAC) Mode to set
> + *     If AAC Configuration Type is Global,
> + *         0  AAC mode disabled
> + *         1  AAC mode enabled
> + *     If AAC Configuration Type is USTT mode specific
> + *     (multiple bits may be set for this parameter),
> + *         Bit 0 AAC (Balanced)
> + *         Bit 1 AAC (Cool Bottom
> + *         Bit 2 AAC (Quiet)
> + *         Bit 3 AAC (Performance)
> + */
> +
> +#define DELL_ACC_GET_FIELD           GENMASK(19, 16)
> +#define DELL_ACC_SET_FIELD           GENMASK(11, 8)
> +#define DELL_THERMAL_SUPPORTED       GENMASK(3, 0)
> +
> +enum thermal_mode_bits {
> +     DELL_BALANCED = BIT(0),
> +     DELL_COOL_BOTTOM = BIT(1),
> +     DELL_QUIET = BIT(2),
> +     DELL_PERFORMANCE = BIT(3),
> +};
> +
> +static int thermal_get_mode(void)
> +{
> +     struct calling_interface_buffer buffer;
> +     int state;
> +     int ret;
> +
> +     dell_fill_request(&buffer, 0x0, 0, 0, 0);
> +     ret = dell_send_request(&buffer, CLASS_INFO,
> SELECT_THERMAL_MANAGEMENT);
> +     if (ret)
> +             return ret;
> +     state = buffer.output[2];
> +     if (state & DELL_BALANCED)
> +             return DELL_BALANCED;
> +     else if (state & DELL_COOL_BOTTOM)
> +             return DELL_COOL_BOTTOM;
> +     else if (state & DELL_QUIET)
> +             return DELL_QUIET;
> +     else if (state & DELL_PERFORMANCE)
> +             return DELL_PERFORMANCE;
> +     else
> +             return -ENXIO;
> +}
> +
> +static int thermal_get_supported_modes(int *supported_bits) {
> +     struct calling_interface_buffer buffer;
> +     int ret;
> +
> +     dell_fill_request(&buffer, 0x0, 0, 0, 0);
> +     ret = dell_send_request(&buffer, CLASS_INFO,
> SELECT_THERMAL_MANAGEMENT);
> +     if (ret)
> +             return ret;
> +     *supported_bits = FIELD_GET(DELL_THERMAL_SUPPORTED,
> buffer.output[1]);
> +     return 0;
> +}
> +
> +static int thermal_get_acc_mode(int *acc_mode) {
> +     struct calling_interface_buffer buffer;
> +     int ret;
> +
> +     dell_fill_request(&buffer, 0x0, 0, 0, 0);
> +     ret = dell_send_request(&buffer, CLASS_INFO,
> SELECT_THERMAL_MANAGEMENT);
> +     if (ret)
> +             return ret;
> +     *acc_mode = FIELD_GET(DELL_ACC_GET_FIELD, buffer.output[3]);
> +     return 0;
> +}
> +
> +static int thermal_set_mode(enum thermal_mode_bits state) {
> +     struct calling_interface_buffer buffer;
> +     int ret;
> +     int acc_mode;
> +
> +     ret = thermal_get_acc_mode(&acc_mode);
> +     if (ret)
> +             return ret;
> +
> +     dell_fill_request(&buffer, 0x1, FIELD_PREP(DELL_ACC_SET_FIELD,
> acc_mode) | state, 0, 0);
> +     ret = dell_send_request(&buffer, CLASS_INFO,
> SELECT_THERMAL_MANAGEMENT);
> +     return ret;
> +}
> +
> +static int thermal_platform_profile_set(struct platform_profile_handler
> *pprof,
> +                                     enum platform_profile_option
> profile) {
> +     switch (profile) {
> +     case PLATFORM_PROFILE_BALANCED:
> +             return thermal_set_mode(DELL_BALANCED);
> +     case PLATFORM_PROFILE_PERFORMANCE:
> +             return thermal_set_mode(DELL_PERFORMANCE);
> +     case PLATFORM_PROFILE_QUIET:
> +             return thermal_set_mode(DELL_QUIET);
> +     case PLATFORM_PROFILE_COOL:
> +             return thermal_set_mode(DELL_COOL_BOTTOM);
> +     default:
> +             return -EOPNOTSUPP;
> +     }
> +}
> +
> +static int thermal_platform_profile_get(struct platform_profile_handler
> *pprof,
> +                                     enum platform_profile_option
> *profile) {
> +     int ret;
> +
> +     ret = thermal_get_mode();
> +     if (ret < 0)
> +             return ret;
> +
> +     switch (ret) {
> +     case DELL_BALANCED:
> +             *profile = PLATFORM_PROFILE_BALANCED;
> +             break;
> +     case DELL_PERFORMANCE:
> +             *profile = PLATFORM_PROFILE_PERFORMANCE;
> +             break;
> +     case DELL_COOL_BOTTOM:
> +             *profile = PLATFORM_PROFILE_COOL;
> +             break;
> +     case DELL_QUIET:
> +             *profile = PLATFORM_PROFILE_QUIET;
> +             break;
> +     default:
> +             return -EINVAL;
> +     }
> +
> +     return 0;
> +}
> +
> +static int thermal_init(void)
> +{
> +     int ret;
> +     int supported_modes;
> +
> +     /* If thermal modes not supported, exit without error */
> +     ret = thermal_get_supported_modes(&supported_modes);
> +     if (ret < 0)
> +             return ret;
> +     if (!supported_modes)
> +             return 0;
> +
> +     thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
> +     if (!thermal_handler)
> +             return -ENOMEM;
> +     thermal_handler->profile_get = thermal_platform_profile_get;
> +     thermal_handler->profile_set = thermal_platform_profile_set;
> +
> +     if (supported_modes & DELL_QUIET)
> +             set_bit(PLATFORM_PROFILE_QUIET, thermal_handler-
> >choices);
> +     if (supported_modes & DELL_COOL_BOTTOM)
> +             set_bit(PLATFORM_PROFILE_COOL, thermal_handler-
> >choices);
> +     if (supported_modes & DELL_BALANCED)
> +             set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler-
> >choices);
> +     if (supported_modes & DELL_PERFORMANCE)
> +             set_bit(PLATFORM_PROFILE_PERFORMANCE,
> thermal_handler->choices);
> +
> +     /* Clean up if failed */
> +     ret = platform_profile_register(thermal_handler);
> +     if (ret)
> +             kfree(thermal_handler);
> +
> +     return ret;
> +}
> +
> +static void thermal_cleanup(void)
> +{
> +     if (thermal_handler) {
> +             platform_profile_remove();
> +             kfree(thermal_handler);
> +     }
> +}
> +
>  static struct led_classdev mute_led_cdev = {
>       .name = "platform::mute",
>       .max_brightness = 1,
> @@ -2238,6 +2468,11 @@ static int __init dell_init(void)
>               goto fail_rfkill;
>       }
>
> +     /* Do not fail module if thermal modes not supported, just skip */
> +     ret = thermal_init();
> +     if (ret)
> +             goto fail_thermal;
> +
>       if (quirks && quirks->touchpad_led)
>               touchpad_led_init(&platform_device->dev);
>
> @@ -2317,6 +2552,8 @@ static int __init dell_init(void)
>               led_classdev_unregister(&mute_led_cdev);
>  fail_led:
>       dell_cleanup_rfkill();
> +fail_thermal:
> +     thermal_cleanup();
>  fail_rfkill:
>       platform_device_del(platform_device);
>  fail_platform_device2:
> @@ -2344,6 +2581,7 @@ static void __exit dell_exit(void)
>               platform_device_unregister(platform_device);
>               platform_driver_unregister(&platform_driver);
>       }
> +     thermal_cleanup();
>  }
>
>  /* dell-rbtn.c driver export functions which will not work correctly (and could
> diff --git a/drivers/platform/x86/dell/dell-smbios-base.c
> b/drivers/platform/x86/dell/dell-smbios-base.c
> index e61bfaf8b5c4..5bc2e394dd1c 100644
> --- a/drivers/platform/x86/dell/dell-smbios-base.c
> +++ b/drivers/platform/x86/dell/dell-smbios-base.c
> @@ -71,6 +71,7 @@ static struct smbios_call call_blacklist[] = {
>       /* handled by kernel: dell-laptop */
>       {0x0000, CLASS_INFO, SELECT_RFKILL},
>       {0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT},
> +     {0x0000, CLASS_INFO, SELECT_THERMAL_MANAGEMENT},
>  };
>
>  struct token_range {
> diff --git a/drivers/platform/x86/dell/dell-smbios.h
> b/drivers/platform/x86/dell/dell-smbios.h
> index eb341bf000c6..585d042f1779 100644
> --- a/drivers/platform/x86/dell/dell-smbios.h
> +++ b/drivers/platform/x86/dell/dell-smbios.h
> @@ -19,6 +19,7 @@
>  /* Classes and selects used only in kernel drivers */  #define
> CLASS_KBD_BACKLIGHT 4  #define SELECT_KBD_BACKLIGHT 11
> +#define SELECT_THERMAL_MANAGEMENT 19
>
>  /* Tokens used in kernel drivers, any of these
>   * should be filtered from userspace access
> --
> 2.42.0

Limonciello, Mario May 8, 2024, 3:53 p.m. UTC | #8

On 5/8/2024 09:24, Shen, Yijun wrote:
> Hi Lyndon,
> 
>   Thanks for working on this patch.
> 
>>
>   Dell side has an initial testing with this patch on some laptops, it looks good. While changing the platform profile:
> 1. The corresponding USTT option in BIOS will be changed.
> 2. thermald will not be impacted. The related PSVT and ITMT will be loaded.
>   Some Dell DTs does not have the USTT, Dell'll have a check if nothing is broken.

Hi Alex!

Have you had a check both on both your AMD laptops and workstations too, 
or just the Intel ones?  I think it would be good to make sure it's 
getting the correct experience in both cases.

> 
>    Additional, with this patch, follow behavior is found:
>   1. For example, the platform profile is quiet.
>   2. Reboot the system and change the USTT to performance.
>   3. Boot to desktop, the platform profile is "quiet", the USTT will be changed back to "quiet".
>   This looks like not a proper user experience. The platform profile should honor the BIOS setting, aka, the platform profile should be switched to "performance".
> 

I agree, this sounds like the initial profile needs to be read from the 
BIOS settings too.

Furthermore I wanted to ask is there also a WMI setting that corresponds 
to this that dell-wmi-sysman offers?  I'm wondering if both should be 
probed in case the SMBIOS one goes away one day.  Or should that one 
just be used as preference?

It seems like maybe ThermalManagement corresponds.  There was some test 
data in fwupd for it:

https://github.com/fwupd/fwupd/tree/main/libfwupdplugin/tests/bios-attrs/dell-xps13-9310/dell-wmi-sysman/attributes/ThermalManagement

Lyndon Sanche May 9, 2024, 3:10 p.m. UTC | #9

On Wed, May 8, 2024, at 8:24 AM, Shen, Yijun wrote:
> Hi Lyndon,
>
>  Thanks for working on this patch.
>
>
> Internal Use - Confidential
>> -----Original Message-----
>> From: Lyndon Sanche <[e-mail]>
>> Sent: Thursday, May 2, 2024 5:58 AM
>> To: [e-mail]
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; Hans de Goede <[e-mail]>; Matthew Garrett
>> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner Kallweit
>> <[e-mail]>; Vegard Nossum <[e-mail]>;
>> [e-mail]; [e-mail]; Dell Client
>> Kernel <[e-mail]>
>> Subject: [PATCH v5] platform/x86: dell-laptop: Implement platform_profile
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Some Dell laptops support configuration of preset fan modes through smbios
>> tables.
>>
>> If the platform supports these fan modes, set up platform_profile to change
>> these modes. If not supported, skip enabling platform_profile.
>>
>> Signed-off-by: Lyndon Sanche <[e-mail]>
>> ---
>> v5:
>>  - Fix indent in smbios-thermal-ctl comment
>>  - Remove linux/wmi.h include
>>  - Add 'select ACPI_PLATFORM_PROFILE' to Dell KConfig
>> v4:
>>  - Make thermal_init and thermal_cleanup static
>>  - Rearrange order of added includes, did not edit current includes
>>  - Include bits.h
>>  - Switch comment style
>>  - Return error if platform_profile registering failed
>>  - Add thermal calls to call_blacklist
>>  - Align defines with tabs
>>  - Correct separation of function and error handling
>>  - Propagate error codes up
>> v3:
>>  - Convert smbios-thermal-ctl docs to multiline comment and wrap
>>  - Change thermal_mode_bits enum to directly be BIT() values
>>       - Convert related code to use this
>>  - Use FIELD_GET/PREP and GENNMASK for getting/setting thermal modes
>>       - Correct offset for getting current ACC mode, setting offset
>>               unchanged
>>  - Check if thermal_handler is allocated before freeing and
>>        unregistering platform_profile
>> v2:
>>  - Wrap smbios-thermal-ctl comment
>>  - Return proper error code when invalid state returned
>>  - Simplify platform_profile_get returns
>>  - Propogate ENOMEM error
>> ---
>
>  Dell side has an initial testing with this patch on some laptops, it 
> looks good. While changing the platform profile:
> 1. The corresponding USTT option in BIOS will be changed.
> 2. thermald will not be impacted. The related PSVT and ITMT will be 
> loaded.
>  Some Dell DTs does not have the USTT, Dell'll have a check if nothing 
> is broken.
>
>   Additional, with this patch, follow behavior is found:
>  1. For example, the platform profile is quiet.
>  2. Reboot the system and change the USTT to performance.
>  3. Boot to desktop, the platform profile is "quiet", the USTT will be 
> changed back to "quiet".
>  This looks like not a proper user experience. The platform profile 
> should honor the BIOS setting, aka, the platform profile should be 
> switched to "performance".

Hello:

Thank you for your email. This is definitely undesirable behaviour, I will have a look at the code to see why this is happening. Does it always revert to quiet on boot, or always the mode that you had switched to prior to reboot?

Do you happen to have power-profiles-daemon or something similar running? My understanding is it remembers profiles across reboots, this could potentially also revert the profile back to what it was. See this release for details:
https://gitlab.freedesktop.org/upower/power-profiles-daemon/-/releases/0.9.0

I will assume there is a bug in my code at this point. I will test with and without ppd running on my system to see if it changes across reboots.

Are USTT settings exposed in your BIOS configuration menu? On my laptop they are not and I have to use smbios-thermal-ctl.

Thank you,

Lyndon

Lyndon Sanche May 11, 2024, 1:49 a.m. UTC | #10

On Thu, May 9 2024 at 09:10:51 AM -06:00:00, Lyndon Sanche 
<[e-mail]> wrote:
> On Wed, May 8, 2024, at 8:24 AM, Shen, Yijun wrote:
>>  Hi Lyndon,
>> 
>>   Thanks for working on this patch.
>> 
>> 
>>   Dell side has an initial testing with this patch on some laptops, 
>> it
>>  looks good. While changing the platform profile:
>>  1. The corresponding USTT option in BIOS will be changed.
>>  2. thermald will not be impacted. The related PSVT and ITMT will be
>>  loaded.
>>   Some Dell DTs does not have the USTT, Dell'll have a check if 
>> nothing
>>  is broken.
>> 
>>    Additional, with this patch, follow behavior is found:
>>   1. For example, the platform profile is quiet.
>>   2. Reboot the system and change the USTT to performance.
>>   3. Boot to desktop, the platform profile is "quiet", the USTT will 
>> be
>>  changed back to "quiet".
>>   This looks like not a proper user experience. The platform profile
>>  should honor the BIOS setting, aka, the platform profile should be
>>  switched to "performance".
> 
> Hello:
> 
> Thank you for your email. This is definitely undesirable behaviour, I 
> will have a look at the code to see why this is happening. Does it 
> always revert to quiet on boot, or always the mode that you had 
> switched to prior to reboot?
> 
> Do you happen to have power-profiles-daemon or something similar 
> running? My understanding is it remembers profiles across reboots, 
> this could potentially also revert the profile back to what it was. 
> See this release for details:
> https://gitlab.freedesktop.org/upower/power-profiles-daemon/-/releases/0.9.0
> 
> I will assume there is a bug in my code at this point. I will test 
> with and without ppd running on my system to see if it changes across 
> reboots.
> 
> Are USTT settings exposed in your BIOS configuration menu? On my 
> laptop they are not and I have to use smbios-thermal-ctl.
> 
> Thank you,
> 
> Lyndon

Hi Yijun:

I tested this on my computer (XPS 9560). I do not have access to the 
USTT settings in the BIOS screen so to substitute that, I booted 
without the patch and set the USTT manually using smbios-thermal-ctl. 
Here are my findings:

Scenario #1: Without power-profiles-daemon (ppd) running

1. Boot with patch, set platform_profile to quiet
2. Boot without patch applied (no platform_profile)
 - smbios-thermal-ctl confirms USTT is set to quiet
 - use smbios-thermal-ctl to set USTT to performance
 - confirm set to performance
3. Boot with patch again
 - platform_profile is set to performance

Scenario #2: With ppd running
1. Boot with patch, set platform_profile to performance with ppd
 - Confirm platform_profile is performance
2. Boot without patch applied (no platform_profile)
 - smbios-thermal-ctl confirms USTT is set to performance
 - ppd reverts to balanced (only controlling intel_pstate in this case)
 - use smbios-thermal-ctl to set USTT to quiet
 - confirm set to quiet
3. Boot with patch again
 - platform_profile and ppd is set to performance

In my case, the setting in the smbios is honored if it was switched 
with another method. When using a userspace program that manipulates 
the platform_profile, the program seems to remember the previous state 
and switch to that.

So I do not think there is a bug in this patch related to this issue, 
at least in my case. Please let me know if you have any questions.

Thanks,

Lyndon

Shen, Yijun May 11, 2024, 3:05 p.m. UTC | #11

Internal Use - Confidential
> -----Original Message-----
> From: Mario Limonciello <[e-mail]>
> Sent: Wednesday, May 8, 2024 11:53 PM
> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
> <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Hans de Goede <[e-mail]>; Matthew Garrett
> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner Kallweit
> <[e-mail]>; Vegard Nossum <[e-mail]>;
> [e-mail]; [e-mail]; Dell Client
> Kernel <[e-mail]>
> Subject: Re: RE: [PATCH v5] platform/x86: dell-laptop: Implement
> platform_profile
>
>
> [EXTERNAL EMAIL]
>
> On 5/8/2024 09:24, Shen, Yijun wrote:
> > Hi Lyndon,
> >
> >   Thanks for working on this patch.
> >
> >>
> >   Dell side has an initial testing with this patch on some laptops, it looks
> good. While changing the platform profile:
> > 1. The corresponding USTT option in BIOS will be changed.
> > 2. thermald will not be impacted. The related PSVT and ITMT will be loaded.
> >   Some Dell DTs does not have the USTT, Dell'll have a check if nothing is
> broken.
>
> Hi Alex!
>
> Have you had a check both on both your AMD laptops and workstations too,
> or just the Intel ones?  I think it would be good to make sure it's getting the
> correct experience in both cases.
>
Hi Mario,

 I've a check for this, for both laptop and workstation, the dell_laptop module will not be loaded. So, AMD platform will not be impacted by this patch series.
Follow is one example output with workstation.
 #lsmod | grep dell
   dell_wmi               28672  0
   dell_smbios            32768  1 dell_wmi
   dcdbas                 20480  1 dell_smbios
   dell_wmi_descriptor    20480  2 dell_wmi,dell_smbios
   sparse_keymap          12288  1 dell_wmi
   ledtrig_audio          12288  3 snd_ctl_led,snd_hda_codec_generic,dell_wmi
   video                  73728  2 dell_wmi,nvidia_modeset
   wmi                    40960  5 video,dell_wmi,wmi_bmof,dell_smbios,dell_wmi_descriptor

> >
> >    Additional, with this patch, follow behavior is found:
> >   1. For example, the platform profile is quiet.
> >   2. Reboot the system and change the USTT to performance.
> >   3. Boot to desktop, the platform profile is "quiet", the USTT will be
> changed back to "quiet".
> >   This looks like not a proper user experience. The platform profile should
> honor the BIOS setting, aka, the platform profile should be switched to
> "performance".
> >
>
> I agree, this sounds like the initial profile needs to be read from the BIOS
> settings too.
>
> Furthermore I wanted to ask is there also a WMI setting that corresponds to
> this that dell-wmi-sysman offers?
 Yes, Mario, you're right. This thermal setting could also be toggled by dell-wmi-sysman.
But, for the Dell consumer AMD laptops, like Alienware, the BIOS is another variant which is different with the workstation one.
With this variant BIOS, there is no USTT and also no dell_wmi/dell-wmi-sysman.

> I'm wondering if both should be probed in case the SMBIOS one goes away one day.
 Yep, I think this is a good suggestion.

>
> It seems like maybe ThermalManagement corresponds.  There was some test
> data in fwupd for it:
>
> https://urldefense.com/v3/__https://github.com/fwupd/fwupd/tree/main/lib
> fwupdplugin/tests/bios-attrs/dell-xps13-9310/dell-wmi-
> sysman/attributes/ThermalManagement__;!!LpKI!iyfGSyfnGxLymc-
> cEg93dfcnBIOtTJbfmCckZlj46eGqvJ_pHJ7WqFZ7-
> zrklKWKkZifqNgJ13LFm6wuz2UlzYqMPXciVw$ [github[.]com]

Limonciello, Mario May 11, 2024, 3:12 p.m. UTC | #12

On 5/11/2024 10:05 AM, Shen, Yijun wrote:
> 
> Internal Use - Confidential
>> -----Original Message-----
>> From: Mario Limonciello <[e-mail]>
>> Sent: Wednesday, May 8, 2024 11:53 PM
>> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
>> <[e-mail]>
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; Hans de Goede <[e-mail]>; Matthew Garrett
>> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner Kallweit
>> <[e-mail]>; Vegard Nossum <[e-mail]>;
>> [e-mail]; [e-mail]; Dell Client
>> Kernel <[e-mail]>
>> Subject: Re: RE: [PATCH v5] platform/x86: dell-laptop: Implement
>> platform_profile
>>
>>
>> [EXTERNAL EMAIL]
>>
>> On 5/8/2024 09:24, Shen, Yijun wrote:
>>> Hi Lyndon,
>>>
>>>    Thanks for working on this patch.
>>>
>>>>
>>>    Dell side has an initial testing with this patch on some laptops, it looks
>> good. While changing the platform profile:
>>> 1. The corresponding USTT option in BIOS will be changed.
>>> 2. thermald will not be impacted. The related PSVT and ITMT will be loaded.
>>>    Some Dell DTs does not have the USTT, Dell'll have a check if nothing is
>> broken.
>>
>> Hi Alex!
>>
>> Have you had a check both on both your AMD laptops and workstations too,
>> or just the Intel ones?  I think it would be good to make sure it's getting the
>> correct experience in both cases.
>>
> Hi Mario,
> 
>   I've a check for this, for both laptop and workstation, the dell_laptop module will not be loaded. So, AMD platform will not be impacted by this patch series.
> Follow is one example output with workstation.
>   #lsmod | grep dell
>     dell_wmi               28672  0
>     dell_smbios            32768  1 dell_wmi
>     dcdbas                 20480  1 dell_smbios
>     dell_wmi_descriptor    20480  2 dell_wmi,dell_smbios
>     sparse_keymap          12288  1 dell_wmi
>     ledtrig_audio          12288  3 snd_ctl_led,snd_hda_codec_generic,dell_wmi
>     video                  73728  2 dell_wmi,nvidia_modeset
>     wmi                    40960  5 video,dell_wmi,wmi_bmof,dell_smbios,dell_wmi_descriptor
> 

Ah; right that makes sense.  In that case, is dell-laptop even the right 
place for this patch series?  I would think the same policies for the 
platform profile should be able to apply to desktop/workstation.

The v6 of this series would block smbios-thermal-ctl from working on a 
workstation too.

>>>
>>>     Additional, with this patch, follow behavior is found:
>>>    1. For example, the platform profile is quiet.
>>>    2. Reboot the system and change the USTT to performance.
>>>    3. Boot to desktop, the platform profile is "quiet", the USTT will be
>> changed back to "quiet".
>>>    This looks like not a proper user experience. The platform profile should
>> honor the BIOS setting, aka, the platform profile should be switched to
>> "performance".
>>>
>>
>> I agree, this sounds like the initial profile needs to be read from the BIOS
>> settings too.
>>
>> Furthermore I wanted to ask is there also a WMI setting that corresponds to
>> this that dell-wmi-sysman offers?
>   Yes, Mario, you're right. This thermal setting could also be toggled by dell-wmi-sysman.
> But, for the Dell consumer AMD laptops, like Alienware, the BIOS is another variant which is different with the workstation one.
> With this variant BIOS, there is no USTT and also no dell_wmi/dell-wmi-sysman.
> 
>> I'm wondering if both should be probed in case the SMBIOS one goes away one day.
>   Yep, I think this is a good suggestion.
> 

Great! Although something I wonder is if the policy when changed with 
dell-wmi-sysman is immediate or requires a reboot.  A lot of the 
settings in there aren't effective until after a reboot.

If this is one of them then it might not be a good idea to make it work 
for both.

Shen, Yijun May 11, 2024, 3:22 p.m. UTC | #13

Internal Use - Confidential
> -----Original Message-----
> From: Lyndon Sanche <[e-mail]>
> Sent: Saturday, May 11, 2024 9:49 AM
> To: Shen, Yijun <[e-mail]>
> Cc: Mario Limonciello <[e-mail]>; Pali Rohár
> <[e-mail]>; Armin Wolf <[e-mail]>;
> [e-mail]; Ilpo Järvinen
> <[e-mail]>; kernel test robot <[e-mail]>; Hans de
> Goede <[e-mail]>; Matthew Garrett <[e-mail]>;
> Jonathan Corbet <[e-mail]>; Heiner Kallweit
> <[e-mail]>; Vegard Nossum <[e-mail]>;
> [e-mail]; LKML <[e-mail]>;
> Dell Client Kernel <[e-mail]>
> Subject: Re: [PATCH v5] platform/x86: dell-laptop: Implement platform_profile
>
>
> [EXTERNAL EMAIL]
>
>
>
> On Thu, May 9 2024 at 09:10:51 AM -06:00:00, Lyndon Sanche
> <[e-mail]> wrote:
> > On Wed, May 8, 2024, at 8:24 AM, Shen, Yijun wrote:
> >>  Hi Lyndon,
> >>
> >>   Thanks for working on this patch.
> >>
> >>
> >>   Dell side has an initial testing with this patch on some laptops,
> >> it  looks good. While changing the platform profile:
> >>  1. The corresponding USTT option in BIOS will be changed.
> >>  2. thermald will not be impacted. The related PSVT and ITMT will be
> >> loaded.
> >>   Some Dell DTs does not have the USTT, Dell'll have a check if
> >> nothing  is broken.
> >>
> >>    Additional, with this patch, follow behavior is found:
> >>   1. For example, the platform profile is quiet.
> >>   2. Reboot the system and change the USTT to performance.
> >>   3. Boot to desktop, the platform profile is "quiet", the USTT will
> >> be  changed back to "quiet".
> >>   This looks like not a proper user experience. The platform profile
> >> should honor the BIOS setting, aka, the platform profile should be
> >> switched to "performance".
> >
> > Hello:
> >
> > Thank you for your email. This is definitely undesirable behaviour, I
> > will have a look at the code to see why this is happening. Does it
> > always revert to quiet on boot, or always the mode that you had
> > switched to prior to reboot?
> >
> > Do you happen to have power-profiles-daemon or something similar
> > running? My understanding is it remembers profiles across reboots,
> > this could potentially also revert the profile back to what it was.
> > See this release for details:
> > https://urldefense.com/v3/__https://gitlab.freedesktop.org/upower/powe
> > r-profiles-daemon/-/releases/0.9.0__;!!LpKI!jUAEHb-9foumkcmPlEKD6tnQrZ
> > sqjB1sXdPDsYvH2fJ-gPV6G35MUtDW4q3xhlJ4IeLcIgmVpb3ztXqaOg8$
> > [gitlab[.]freedesktop[.]org]
> >
> > I will assume there is a bug in my code at this point. I will test
> > with and without ppd running on my system to see if it changes across
> > reboots.
> >
> > Are USTT settings exposed in your BIOS configuration menu? On my
> > laptop they are not and I have to use smbios-thermal-ctl.
> >
> > Thank you,
> >
> > Lyndon
>
> Hi Yijun:
>
> I tested this on my computer (XPS 9560). I do not have access to the USTT
> settings in the BIOS screen so to substitute that, I booted without the patch
> and set the USTT manually using smbios-thermal-ctl.
> Here are my findings:
>
> Scenario #1: Without power-profiles-daemon (ppd) running
>
> 1. Boot with patch, set platform_profile to quiet 2. Boot without patch applied
> (no platform_profile)
>  - smbios-thermal-ctl confirms USTT is set to quiet
>  - use smbios-thermal-ctl to set USTT to performance
>  - confirm set to performance
> 3. Boot with patch again
>  - platform_profile is set to performance
>
> Scenario #2: With ppd running
> 1. Boot with patch, set platform_profile to performance with ppd
>  - Confirm platform_profile is performance 2. Boot without patch applied (no
> platform_profile)
>  - smbios-thermal-ctl confirms USTT is set to performance
>  - ppd reverts to balanced (only controlling intel_pstate in this case)
>  - use smbios-thermal-ctl to set USTT to quiet
>  - confirm set to quiet
> 3. Boot with patch again
>  - platform_profile and ppd is set to performance
>
> In my case, the setting in the smbios is honored if it was switched with
> another method. When using a userspace program that manipulates the
> platform_profile, the program seems to remember the previous state and
> switch to that.
>
> So I do not think there is a bug in this patch related to this issue, at least in my
> case. Please let me know if you have any questions.
>
> Thanks,
>
> Lyndon
>
>
>
Hi Lyndon,

 I've made a video recorder of the issue: https://dell.box.com/s/3f3znz1z8c6htbcll9juj6tyyu0zvvut
 My test environment is that I freshly installed the Fedora 40 and will not do any online updates. Then install the kernel with the v5 patch applied.

 XPS 9560 is a pretty old system which is RTS with 2017. No USTT setting in the BIOS is expected.
 I've a check that the Dell system, at least shipped from 2022, the USTT setting will be valid in the BIOS. The system used in above link, it is Latitude 7350 which is shipped by 2024 April.

 I think the key point to duplicate of this issue that, the USTT needs to be changed under BIOS but not under the Linux OS.

Thanks

Lyndon Sanche May 11, 2024, 3:54 p.m. UTC | #14

On May 11, 2024 9:22:23 a.m. MDT, "Shen, Yijun" <[e-mail]> wrote:
>
>
>
>Internal Use - Confidential
>> -----Original Message-----
>> From: Lyndon Sanche <[e-mail]>
>> Sent: Saturday, May 11, 2024 9:49 AM
>> To: Shen, Yijun <[e-mail]>
>> Cc: Mario Limonciello <[e-mail]>; Pali Rohár
>> <[e-mail]>; Armin Wolf <[e-mail]>;
>> [e-mail]; Ilpo Järvinen
>> <[e-mail]>; kernel test robot <[e-mail]>; Hans de
>> Goede <[e-mail]>; Matthew Garrett <[e-mail]>;
>> Jonathan Corbet <[e-mail]>; Heiner Kallweit
>> <[e-mail]>; Vegard Nossum <[e-mail]>;
>> [e-mail]; LKML <[e-mail]>;
>> Dell Client Kernel <[e-mail]>
>> Subject: Re: [PATCH v5] platform/x86: dell-laptop: Implement platform_profile
>>
>>
>> [EXTERNAL EMAIL]
>>
>>
>>
>> On Thu, May 9 2024 at 09:10:51 AM -06:00:00, Lyndon Sanche
>> <[e-mail]> wrote:
>> > On Wed, May 8, 2024, at 8:24 AM, Shen, Yijun wrote:
>> >>  Hi Lyndon,
>> >>
>> >>   Thanks for working on this patch.
>> >>
>> >>
>> >>   Dell side has an initial testing with this patch on some laptops,
>> >> it  looks good. While changing the platform profile:
>> >>  1. The corresponding USTT option in BIOS will be changed.
>> >>  2. thermald will not be impacted. The related PSVT and ITMT will be
>> >> loaded.
>> >>   Some Dell DTs does not have the USTT, Dell'll have a check if
>> >> nothing  is broken.
>> >>
>> >>    Additional, with this patch, follow behavior is found:
>> >>   1. For example, the platform profile is quiet.
>> >>   2. Reboot the system and change the USTT to performance.
>> >>   3. Boot to desktop, the platform profile is "quiet", the USTT will
>> >> be  changed back to "quiet".
>> >>   This looks like not a proper user experience. The platform profile
>> >> should honor the BIOS setting, aka, the platform profile should be
>> >> switched to "performance".
>> >
>> > Hello:
>> >
>> > Thank you for your email. This is definitely undesirable behaviour, I
>> > will have a look at the code to see why this is happening. Does it
>> > always revert to quiet on boot, or always the mode that you had
>> > switched to prior to reboot?
>> >
>> > Do you happen to have power-profiles-daemon or something similar
>> > running? My understanding is it remembers profiles across reboots,
>> > this could potentially also revert the profile back to what it was.
>> > See this release for details:
>> > https://urldefense.com/v3/__https://gitlab.freedesktop.org/upower/powe
>> > r-profiles-daemon/-/releases/0.9.0__;!!LpKI!jUAEHb-9foumkcmPlEKD6tnQrZ
>> > sqjB1sXdPDsYvH2fJ-gPV6G35MUtDW4q3xhlJ4IeLcIgmVpb3ztXqaOg8$
>> > [gitlab[.]freedesktop[.]org]
>> >
>> > I will assume there is a bug in my code at this point. I will test
>> > with and without ppd running on my system to see if it changes across
>> > reboots.
>> >
>> > Are USTT settings exposed in your BIOS configuration menu? On my
>> > laptop they are not and I have to use smbios-thermal-ctl.
>> >
>> > Thank you,
>> >
>> > Lyndon
>>
>> Hi Yijun:
>>
>> I tested this on my computer (XPS 9560). I do not have access to the USTT
>> settings in the BIOS screen so to substitute that, I booted without the patch
>> and set the USTT manually using smbios-thermal-ctl.
>> Here are my findings:
>>
>> Scenario #1: Without power-profiles-daemon (ppd) running
>>
>> 1. Boot with patch, set platform_profile to quiet 2. Boot without patch applied
>> (no platform_profile)
>>  - smbios-thermal-ctl confirms USTT is set to quiet
>>  - use smbios-thermal-ctl to set USTT to performance
>>  - confirm set to performance
>> 3. Boot with patch again
>>  - platform_profile is set to performance
>>
>> Scenario #2: With ppd running
>> 1. Boot with patch, set platform_profile to performance with ppd
>>  - Confirm platform_profile is performance 2. Boot without patch applied (no
>> platform_profile)
>>  - smbios-thermal-ctl confirms USTT is set to performance
>>  - ppd reverts to balanced (only controlling intel_pstate in this case)
>>  - use smbios-thermal-ctl to set USTT to quiet
>>  - confirm set to quiet
>> 3. Boot with patch again
>>  - platform_profile and ppd is set to performance
>>
>> In my case, the setting in the smbios is honored if it was switched with
>> another method. When using a userspace program that manipulates the
>> platform_profile, the program seems to remember the previous state and
>> switch to that.
>>
>> So I do not think there is a bug in this patch related to this issue, at least in my
>> case. Please let me know if you have any questions.
>>
>> Thanks,
>>
>> Lyndon
>>
>>
>>
>Hi Lyndon,
>
> I've made a video recorder of the issue: https://dell.box.com/s/3f3znz1z8c6htbcll9juj6tyyu0zvvut
> My test environment is that I freshly installed the Fedora 40 and will not do any online updates. Then install the kernel with the v5 patch applied.
>
> XPS 9560 is a pretty old system which is RTS with 2017. No USTT setting in the BIOS is expected.
> I've a check that the Dell system, at least shipped from 2022, the USTT setting will be valid in the BIOS. The system used in above link, it is Latitude 7350 which is shipped by 2024 April.
>
> I think the key point to duplicate of this issue that, the USTT needs to be changed under BIOS but not under the Linux OS.
>
>Thanks
>
>


Thanks for the video.

Fedora 40 has power-profiles-daemon enabled by default AFAIK. This would be changing the platform profile at load to match the last known state.

Are you able to rerun this test with PPD disabled? Just in case it is a difference between setting it in BIOS and smbios-thermal-ctl.

Thanks,

Lyndon

Shen, Yijun May 11, 2024, 3:56 p.m. UTC | #15

Internal Use - Confidential
> -----Original Message-----
> From: Limonciello, Mario <[e-mail]>
> Sent: Saturday, May 11, 2024 11:13 PM
> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
> <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Hans de Goede <[e-mail]>; Matthew Garrett
> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner Kallweit
> <[e-mail]>; Vegard Nossum <[e-mail]>;
> [e-mail]; [e-mail]; Dell Client
> Kernel <[e-mail]>
> Subject: Re: [PATCH v5] platform/x86: dell-laptop: Implement platform_profile
>
>
> [EXTERNAL EMAIL]
>
>
>
> On 5/11/2024 10:05 AM, Shen, Yijun wrote:
> >
> > Internal Use - Confidential
> >> -----Original Message-----
> >> From: Mario Limonciello <[e-mail]>
> >> Sent: Wednesday, May 8, 2024 11:53 PM
> >> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
> >> <[e-mail]>
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; Hans de Goede <[e-mail]>; Matthew
> Garrett
> >> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner
> >> Kallweit <[e-mail]>; Vegard Nossum
> >> <[e-mail]>; [e-mail];
> >> [e-mail]; Dell Client Kernel
> >> <[e-mail]>
> >> Subject: Re: RE: [PATCH v5] platform/x86: dell-laptop: Implement
> >> platform_profile
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> On 5/8/2024 09:24, Shen, Yijun wrote:
> >>> Hi Lyndon,
> >>>
> >>>    Thanks for working on this patch.
> >>>
> >>>>
> >>>    Dell side has an initial testing with this patch on some laptops,
> >>> it looks
> >> good. While changing the platform profile:
> >>> 1. The corresponding USTT option in BIOS will be changed.
> >>> 2. thermald will not be impacted. The related PSVT and ITMT will be
> loaded.
> >>>    Some Dell DTs does not have the USTT, Dell'll have a check if
> >>> nothing is
> >> broken.
> >>
> >> Hi Alex!
> >>
> >> Have you had a check both on both your AMD laptops and workstations
> >> too, or just the Intel ones?  I think it would be good to make sure
> >> it's getting the correct experience in both cases.
> >>
> > Hi Mario,
> >
> >   I've a check for this, for both laptop and workstation, the dell_laptop
> module will not be loaded. So, AMD platform will not be impacted by this
> patch series.
> > Follow is one example output with workstation.
> >   #lsmod | grep dell
> >     dell_wmi               28672  0
> >     dell_smbios            32768  1 dell_wmi
> >     dcdbas                 20480  1 dell_smbios
> >     dell_wmi_descriptor    20480  2 dell_wmi,dell_smbios
> >     sparse_keymap          12288  1 dell_wmi
> >     ledtrig_audio          12288  3 snd_ctl_led,snd_hda_codec_generic,dell_wmi
> >     video                  73728  2 dell_wmi,nvidia_modeset
> >     wmi                    40960  5
> video,dell_wmi,wmi_bmof,dell_smbios,dell_wmi_descriptor
> >
>
> Ah; right that makes sense.  In that case, is dell-laptop even the right place for
> this patch series?  I would think the same policies for the platform profile
> should be able to apply to desktop/workstation.
>
> The v6 of this series would block smbios-thermal-ctl from working on a
> workstation too.
>
> >>>
> >>>     Additional, with this patch, follow behavior is found:
> >>>    1. For example, the platform profile is quiet.
> >>>    2. Reboot the system and change the USTT to performance.
> >>>    3. Boot to desktop, the platform profile is "quiet", the USTT
> >>> will be
> >> changed back to "quiet".
> >>>    This looks like not a proper user experience. The platform
> >>> profile should
> >> honor the BIOS setting, aka, the platform profile should be switched
> >> to "performance".
> >>>
> >>
> >> I agree, this sounds like the initial profile needs to be read from
> >> the BIOS settings too.
> >>
> >> Furthermore I wanted to ask is there also a WMI setting that
> >> corresponds to this that dell-wmi-sysman offers?
> >   Yes, Mario, you're right. This thermal setting could also be toggled by dell-
> wmi-sysman.
> > But, for the Dell consumer AMD laptops, like Alienware, the BIOS is another
> variant which is different with the workstation one.
> > With this variant BIOS, there is no USTT and also no dell_wmi/dell-wmi-
> sysman.
> >
> >> I'm wondering if both should be probed in case the SMBIOS one goes
> away one day.
> >   Yep, I think this is a good suggestion.
> >
>
> Great! Although something I wonder is if the policy when changed with dell-
> wmi-sysman is immediate or requires a reboot.  A lot of the settings in there
> aren't effective until after a reboot.
>
> If this is one of them then it might not be a good idea to make it work for
> both.

Hi Mario,

 Just have a check, the check steps are:
1. stop the thermald
2. run the stress test
3. Toggle the thermal setting between UltraPerformance and Quiet via dell-wmi-sysman
4. Check the CPU FAN speed
 The system reboot is not needed, the CPU fan speed changes immediately.
 A screen recorder here: https://dell.box.com/s/p2bhd2b6cv8z5buk9eao3bosgrrp1lf9

Thanks

Lyndon Sanche May 11, 2024, 4:02 p.m. UTC | #16

On May 11, 2024 9:05:15 a.m. MDT, "Shen, Yijun" <[e-mail]> wrote:
>
>Internal Use - Confidential
>> -----Original Message-----
>> From: Mario Limonciello <[e-mail]>
>> Sent: Wednesday, May 8, 2024 11:53 PM
>> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
>> <[e-mail]>
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; Hans de Goede <[e-mail]>; Matthew Garrett
>> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner Kallweit
>> <[e-mail]>; Vegard Nossum <[e-mail]>;
>> [e-mail]; [e-mail]; Dell Client
>> Kernel <[e-mail]>
>> Subject: Re: RE: [PATCH v5] platform/x86: dell-laptop: Implement
>> platform_profile
>>
>>
>> [EXTERNAL EMAIL]
>>
>> On 5/8/2024 09:24, Shen, Yijun wrote:
>> > Hi Lyndon,
>> >
>> >   Thanks for working on this patch.
>> >
>> >>
>> >   Dell side has an initial testing with this patch on some laptops, it looks
>> good. While changing the platform profile:
>> > 1. The corresponding USTT option in BIOS will be changed.
>> > 2. thermald will not be impacted. The related PSVT and ITMT will be loaded.
>> >   Some Dell DTs does not have the USTT, Dell'll have a check if nothing is
>> broken.
>>
>> Hi Alex!
>>
>> Have you had a check both on both your AMD laptops and workstations too,
>> or just the Intel ones?  I think it would be good to make sure it's getting the
>> correct experience in both cases.
>>
>Hi Mario,
>
> I've a check for this, for both laptop and workstation, the dell_laptop module will not be loaded. So, AMD platform will not be impacted by this patch series.
>Follow is one example output with workstation.
> #lsmod | grep dell
>   dell_wmi               28672  0
>   dell_smbios            32768  1 dell_wmi
>   dcdbas                 20480  1 dell_smbios
>   dell_wmi_descriptor    20480  2 dell_wmi,dell_smbios
>   sparse_keymap          12288  1 dell_wmi
>   ledtrig_audio          12288  3 snd_ctl_led,snd_hda_codec_generic,dell_wmi
>   video                  73728  2 dell_wmi,nvidia_modeset
>   wmi                    40960  5 video,dell_wmi,wmi_bmof,dell_smbios,dell_wmi_descriptor
>

Should AMD platforms be affected? Do they support the USTT modes as well?

Shen, Yijun May 11, 2024, 4:12 p.m. UTC | #17

Internal Use - Confidential
> -----Original Message-----
> From: Lyndon Sanche <[e-mail]>
> Sent: Saturday, May 11, 2024 11:54 PM
> To: Shen, Yijun <[e-mail]>
> Cc: Mario Limonciello <[e-mail]>; Pali Rohár
> <[e-mail]>; Armin Wolf <[e-mail]>;
> [e-mail]; Ilpo Järvinen
> <[e-mail]>; kernel test robot <[e-mail]>; Hans de
> Goede <[e-mail]>; Matthew Garrett <[e-mail]>;
> Jonathan Corbet <[e-mail]>; Heiner Kallweit
> <[e-mail]>; Vegard Nossum <[e-mail]>;
> [e-mail]; LKML <[e-mail]>;
> Dell Client Kernel <[e-mail]>
> Subject: RE: [PATCH v5] platform/x86: dell-laptop: Implement platform_profile
>
>
> [EXTERNAL EMAIL]
>
>
>
> On May 11, 2024 9:22:23 a.m. MDT, "Shen, Yijun" <[e-mail]>
> wrote:
> >
> >
> >
> >Internal Use - Confidential
> >> -----Original Message-----
> >> From: Lyndon Sanche <[e-mail]>
> >> Sent: Saturday, May 11, 2024 9:49 AM
> >> To: Shen, Yijun <[e-mail]>
> >> Cc: Mario Limonciello <[e-mail]>; Pali Rohár
> >> <[e-mail]>; Armin Wolf <[e-mail]>;
> >> [e-mail]; Ilpo Järvinen
> >> <[e-mail]>; kernel test robot <[e-mail]>;
> >> Hans de Goede <[e-mail]>; Matthew Garrett
> >> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner
> >> Kallweit <[e-mail]>; Vegard Nossum
> >> <[e-mail]>; [e-mail];
> LKML
> >> <[e-mail]>; Dell Client Kernel
> >> <[e-mail]>
> >> Subject: Re: [PATCH v5] platform/x86: dell-laptop: Implement
> >> platform_profile
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >>
> >>
> >> On Thu, May 9 2024 at 09:10:51 AM -06:00:00, Lyndon Sanche
> >> <[e-mail]> wrote:
> >> > On Wed, May 8, 2024, at 8:24 AM, Shen, Yijun wrote:
> >> >>  Hi Lyndon,
> >> >>
> >> >>   Thanks for working on this patch.
> >> >>
> >> >>
> >> >>   Dell side has an initial testing with this patch on some
> >> >> laptops, it  looks good. While changing the platform profile:
> >> >>  1. The corresponding USTT option in BIOS will be changed.
> >> >>  2. thermald will not be impacted. The related PSVT and ITMT will
> >> >> be loaded.
> >> >>   Some Dell DTs does not have the USTT, Dell'll have a check if
> >> >> nothing  is broken.
> >> >>
> >> >>    Additional, with this patch, follow behavior is found:
> >> >>   1. For example, the platform profile is quiet.
> >> >>   2. Reboot the system and change the USTT to performance.
> >> >>   3. Boot to desktop, the platform profile is "quiet", the USTT
> >> >> will be  changed back to "quiet".
> >> >>   This looks like not a proper user experience. The platform
> >> >> profile should honor the BIOS setting, aka, the platform profile
> >> >> should be switched to "performance".
> >> >
> >> > Hello:
> >> >
> >> > Thank you for your email. This is definitely undesirable behaviour,
> >> > I will have a look at the code to see why this is happening. Does
> >> > it always revert to quiet on boot, or always the mode that you had
> >> > switched to prior to reboot?
> >> >
> >> > Do you happen to have power-profiles-daemon or something similar
> >> > running? My understanding is it remembers profiles across reboots,
> >> > this could potentially also revert the profile back to what it was.
> >> > See this release for details:
> >> > https://urldefense.com/v3/__https://gitlab.freedesktop.org/upower/p
> >> > owe
> >> > r-profiles-daemon/-/releases/0.9.0__;!!LpKI!jUAEHb-9foumkcmPlEKD6tn
> >> > QrZ sqjB1sXdPDsYvH2fJ-
> gPV6G35MUtDW4q3xhlJ4IeLcIgmVpb3ztXqaOg8$
> >> > [gitlab[.]freedesktop[.]org]
> >> >
> >> > I will assume there is a bug in my code at this point. I will test
> >> > with and without ppd running on my system to see if it changes
> >> > across reboots.
> >> >
> >> > Are USTT settings exposed in your BIOS configuration menu? On my
> >> > laptop they are not and I have to use smbios-thermal-ctl.
> >> >
> >> > Thank you,
> >> >
> >> > Lyndon
> >>
> >> Hi Yijun:
> >>
> >> I tested this on my computer (XPS 9560). I do not have access to the
> >> USTT settings in the BIOS screen so to substitute that, I booted
> >> without the patch and set the USTT manually using smbios-thermal-ctl.
> >> Here are my findings:
> >>
> >> Scenario #1: Without power-profiles-daemon (ppd) running
> >>
> >> 1. Boot with patch, set platform_profile to quiet 2. Boot without
> >> patch applied (no platform_profile)
> >>  - smbios-thermal-ctl confirms USTT is set to quiet
> >>  - use smbios-thermal-ctl to set USTT to performance
> >>  - confirm set to performance
> >> 3. Boot with patch again
> >>  - platform_profile is set to performance
> >>
> >> Scenario #2: With ppd running
> >> 1. Boot with patch, set platform_profile to performance with ppd
> >>  - Confirm platform_profile is performance 2. Boot without patch
> >> applied (no
> >> platform_profile)
> >>  - smbios-thermal-ctl confirms USTT is set to performance
> >>  - ppd reverts to balanced (only controlling intel_pstate in this
> >> case)
> >>  - use smbios-thermal-ctl to set USTT to quiet
> >>  - confirm set to quiet
> >> 3. Boot with patch again
> >>  - platform_profile and ppd is set to performance
> >>
> >> In my case, the setting in the smbios is honored if it was switched
> >> with another method. When using a userspace program that manipulates
> >> the platform_profile, the program seems to remember the previous
> >> state and switch to that.
> >>
> >> So I do not think there is a bug in this patch related to this issue,
> >> at least in my case. Please let me know if you have any questions.
> >>
> >> Thanks,
> >>
> >> Lyndon
> >>
> >>
> >>
> >Hi Lyndon,
> >
> > I've made a video recorder of the issue:
> >
> https://urldefense.com/v3/__https://dell.box.com/s/3f3znz1z8c6htbcll9juj6ty
> yu0zvvut__;!!LpKI!nHHxFbxzG-rJX_KZsebMC7ZcJU0WhNhkpXn-pk6nu-sUF38-
> RXz6x3YkALgzS4jnUP9TIWIu4mX_6cSDbwQ$ [dell[.]box[.]com] My test
> environment is that I freshly installed the Fedora 40 and will not do any online
> updates. Then install the kernel with the v5 patch applied.
> >
> > XPS 9560 is a pretty old system which is RTS with 2017. No USTT setting in
> the BIOS is expected.
> > I've a check that the Dell system, at least shipped from 2022, the USTT
> setting will be valid in the BIOS. The system used in above link, it is Latitude
> 7350 which is shipped by 2024 April.
> >
> > I think the key point to duplicate of this issue that, the USTT needs to be
> changed under BIOS but not under the Linux OS.
> >
> >Thanks
> >
> >
>
>
> Thanks for the video.
>
> Fedora 40 has power-profiles-daemon enabled by default AFAIK. This would
> be changing the platform profile at load to match the last known state.
>
> Are you able to rerun this test with PPD disabled? Just in case it is a difference
> between setting it in BIOS and smbios-thermal-ctl.
>
> Thanks,
>
> Lyndon

 If disable the PPD, the issue is gone.

Thanks

Armin Wolf May 12, 2024, 5:53 p.m. UTC | #18

Am 11.05.24 um 17:56 schrieb Shen, Yijun:

> Internal Use - Confidential
>> -----Original Message-----
>> From: Limonciello, Mario <[e-mail]>
>> Sent: Saturday, May 11, 2024 11:13 PM
>> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
>> <[e-mail]>
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; Hans de Goede <[e-mail]>; Matthew Garrett
>> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner Kallweit
>> <[e-mail]>; Vegard Nossum <[e-mail]>;
>> [e-mail]; [e-mail]; Dell Client
>> Kernel <[e-mail]>
>> Subject: Re: [PATCH v5] platform/x86: dell-laptop: Implement platform_profile
>>
>>
>> [EXTERNAL EMAIL]
>>
>>
>>
>> On 5/11/2024 10:05 AM, Shen, Yijun wrote:
>>> Internal Use - Confidential
>>>> -----Original Message-----
>>>> From: Mario Limonciello <[e-mail]>
>>>> Sent: Wednesday, May 8, 2024 11:53 PM
>>>> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
>>>> <[e-mail]>
>>>> Cc: [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; Hans de Goede <[e-mail]>; Matthew
>> Garrett
>>>> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner
>>>> Kallweit <[e-mail]>; Vegard Nossum
>>>> <[e-mail]>; [e-mail];
>>>> [e-mail]; Dell Client Kernel
>>>> <[e-mail]>
>>>> Subject: Re: RE: [PATCH v5] platform/x86: dell-laptop: Implement
>>>> platform_profile
>>>>
>>>>
>>>> [EXTERNAL EMAIL]
>>>>
>>>> On 5/8/2024 09:24, Shen, Yijun wrote:
>>>>> Hi Lyndon,
>>>>>
>>>>>     Thanks for working on this patch.
>>>>>
>>>>>     Dell side has an initial testing with this patch on some laptops,
>>>>> it looks
>>>> good. While changing the platform profile:
>>>>> 1. The corresponding USTT option in BIOS will be changed.
>>>>> 2. thermald will not be impacted. The related PSVT and ITMT will be
>> loaded.
>>>>>     Some Dell DTs does not have the USTT, Dell'll have a check if
>>>>> nothing is
>>>> broken.
>>>>
>>>> Hi Alex!
>>>>
>>>> Have you had a check both on both your AMD laptops and workstations
>>>> too, or just the Intel ones?  I think it would be good to make sure
>>>> it's getting the correct experience in both cases.
>>>>
>>> Hi Mario,
>>>
>>>    I've a check for this, for both laptop and workstation, the dell_laptop
>> module will not be loaded. So, AMD platform will not be impacted by this
>> patch series.
>>> Follow is one example output with workstation.
>>>    #lsmod | grep dell
>>>      dell_wmi               28672  0
>>>      dell_smbios            32768  1 dell_wmi
>>>      dcdbas                 20480  1 dell_smbios
>>>      dell_wmi_descriptor    20480  2 dell_wmi,dell_smbios
>>>      sparse_keymap          12288  1 dell_wmi
>>>      ledtrig_audio          12288  3 snd_ctl_led,snd_hda_codec_generic,dell_wmi
>>>      video                  73728  2 dell_wmi,nvidia_modeset
>>>      wmi                    40960  5
>> video,dell_wmi,wmi_bmof,dell_smbios,dell_wmi_descriptor
>> Ah; right that makes sense.  In that case, is dell-laptop even the right place for
>> this patch series?  I would think the same policies for the platform profile
>> should be able to apply to desktop/workstation.
>>
>> The v6 of this series would block smbios-thermal-ctl from working on a
>> workstation too.
>>
>>>>>      Additional, with this patch, follow behavior is found:
>>>>>     1. For example, the platform profile is quiet.
>>>>>     2. Reboot the system and change the USTT to performance.
>>>>>     3. Boot to desktop, the platform profile is "quiet", the USTT
>>>>> will be
>>>> changed back to "quiet".
>>>>>     This looks like not a proper user experience. The platform
>>>>> profile should
>>>> honor the BIOS setting, aka, the platform profile should be switched
>>>> to "performance".
>>>> I agree, this sounds like the initial profile needs to be read from
>>>> the BIOS settings too.
>>>>
>>>> Furthermore I wanted to ask is there also a WMI setting that
>>>> corresponds to this that dell-wmi-sysman offers?
>>>    Yes, Mario, you're right. This thermal setting could also be toggled by dell-
>> wmi-sysman.
>>> But, for the Dell consumer AMD laptops, like Alienware, the BIOS is another
>> variant which is different with the workstation one.
>>> With this variant BIOS, there is no USTT and also no dell_wmi/dell-wmi-
>> sysman.
>>>> I'm wondering if both should be probed in case the SMBIOS one goes
>> away one day.
>>>    Yep, I think this is a good suggestion.
>>>
>> Great! Although something I wonder is if the policy when changed with dell-
>> wmi-sysman is immediate or requires a reboot.  A lot of the settings in there
>> aren't effective until after a reboot.
>>
>> If this is one of them then it might not be a good idea to make it work for
>> both.
> Hi Mario,
>
>   Just have a check, the check steps are:
> 1. stop the thermald
> 2. run the stress test
> 3. Toggle the thermal setting between UltraPerformance and Quiet via dell-wmi-sysman
> 4. Check the CPU FAN speed
>   The system reboot is not needed, the CPU fan speed changes immediately.
>   A screen recorder here: https://dell.box.com/s/p2bhd2b6cv8z5buk9eao3bosgrrp1lf9
>
> Thanks
>
Hi,

i believe it should be the responsibility of the manufacturer (in this case Dell) that
the thermal state remains consistent across both interfaces.

I think that the official Windows utility only checks the thermal state reported by
the USTT interface, so we should match this behavior.

Thanks,
Armin Wolf

Limonciello, Mario May 12, 2024, 5:58 p.m. UTC | #19

On 5/12/2024 12:53 PM, Armin Wolf wrote:
> Am 11.05.24 um 17:56 schrieb Shen, Yijun:
> 
>> Internal Use - Confidential
>>> -----Original Message-----
>>> From: Limonciello, Mario <[e-mail]>
>>> Sent: Saturday, May 11, 2024 11:13 PM
>>> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
>>> <[e-mail]>
>>> Cc: [e-mail]; [e-mail];
>>> [e-mail]; [e-mail];
>>> [e-mail]; Hans de Goede <[e-mail]>; Matthew Garrett
>>> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner Kallweit
>>> <[e-mail]>; Vegard Nossum <[e-mail]>;
>>> [e-mail]; [e-mail]; 
>>> Dell Client
>>> Kernel <[e-mail]>
>>> Subject: Re: [PATCH v5] platform/x86: dell-laptop: Implement 
>>> platform_profile
>>>
>>>
>>> [EXTERNAL EMAIL]
>>>
>>>
>>>
>>> On 5/11/2024 10:05 AM, Shen, Yijun wrote:
>>>> Internal Use - Confidential
>>>>> -----Original Message-----
>>>>> From: Mario Limonciello <[e-mail]>
>>>>> Sent: Wednesday, May 8, 2024 11:53 PM
>>>>> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
>>>>> <[e-mail]>
>>>>> Cc: [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail];
>>>>> [e-mail]; Hans de Goede <[e-mail]>; Matthew
>>> Garrett
>>>>> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner
>>>>> Kallweit <[e-mail]>; Vegard Nossum
>>>>> <[e-mail]>; [e-mail];
>>>>> [e-mail]; Dell Client Kernel
>>>>> <[e-mail]>
>>>>> Subject: Re: RE: [PATCH v5] platform/x86: dell-laptop: Implement
>>>>> platform_profile
>>>>>
>>>>>
>>>>> [EXTERNAL EMAIL]
>>>>>
>>>>> On 5/8/2024 09:24, Shen, Yijun wrote:
>>>>>> Hi Lyndon,
>>>>>>
>>>>>>     Thanks for working on this patch.
>>>>>>
>>>>>>     Dell side has an initial testing with this patch on some laptops,
>>>>>> it looks
>>>>> good. While changing the platform profile:
>>>>>> 1. The corresponding USTT option in BIOS will be changed.
>>>>>> 2. thermald will not be impacted. The related PSVT and ITMT will be
>>> loaded.
>>>>>>     Some Dell DTs does not have the USTT, Dell'll have a check if
>>>>>> nothing is
>>>>> broken.
>>>>>
>>>>> Hi Alex!
>>>>>
>>>>> Have you had a check both on both your AMD laptops and workstations
>>>>> too, or just the Intel ones?  I think it would be good to make sure
>>>>> it's getting the correct experience in both cases.
>>>>>
>>>> Hi Mario,
>>>>
>>>>    I've a check for this, for both laptop and workstation, the 
>>>> dell_laptop
>>> module will not be loaded. So, AMD platform will not be impacted by this
>>> patch series.
>>>> Follow is one example output with workstation.
>>>>    #lsmod | grep dell
>>>>      dell_wmi               28672  0
>>>>      dell_smbios            32768  1 dell_wmi
>>>>      dcdbas                 20480  1 dell_smbios
>>>>      dell_wmi_descriptor    20480  2 dell_wmi,dell_smbios
>>>>      sparse_keymap          12288  1 dell_wmi
>>>>      ledtrig_audio          12288  3 
>>>> snd_ctl_led,snd_hda_codec_generic,dell_wmi
>>>>      video                  73728  2 dell_wmi,nvidia_modeset
>>>>      wmi                    40960  5
>>> video,dell_wmi,wmi_bmof,dell_smbios,dell_wmi_descriptor
>>> Ah; right that makes sense.  In that case, is dell-laptop even the 
>>> right place for
>>> this patch series?  I would think the same policies for the platform 
>>> profile
>>> should be able to apply to desktop/workstation.
>>>
>>> The v6 of this series would block smbios-thermal-ctl from working on a
>>> workstation too.
>>>
>>>>>>      Additional, with this patch, follow behavior is found:
>>>>>>     1. For example, the platform profile is quiet.
>>>>>>     2. Reboot the system and change the USTT to performance.
>>>>>>     3. Boot to desktop, the platform profile is "quiet", the USTT
>>>>>> will be
>>>>> changed back to "quiet".
>>>>>>     This looks like not a proper user experience. The platform
>>>>>> profile should
>>>>> honor the BIOS setting, aka, the platform profile should be switched
>>>>> to "performance".
>>>>> I agree, this sounds like the initial profile needs to be read from
>>>>> the BIOS settings too.
>>>>>
>>>>> Furthermore I wanted to ask is there also a WMI setting that
>>>>> corresponds to this that dell-wmi-sysman offers?
>>>>    Yes, Mario, you're right. This thermal setting could also be 
>>>> toggled by dell-
>>> wmi-sysman.
>>>> But, for the Dell consumer AMD laptops, like Alienware, the BIOS is 
>>>> another
>>> variant which is different with the workstation one.
>>>> With this variant BIOS, there is no USTT and also no dell_wmi/dell-wmi-
>>> sysman.
>>>>> I'm wondering if both should be probed in case the SMBIOS one goes
>>> away one day.
>>>>    Yep, I think this is a good suggestion.
>>>>
>>> Great! Although something I wonder is if the policy when changed with 
>>> dell-
>>> wmi-sysman is immediate or requires a reboot.  A lot of the settings 
>>> in there
>>> aren't effective until after a reboot.
>>>
>>> If this is one of them then it might not be a good idea to make it 
>>> work for
>>> both.
>> Hi Mario,
>>
>>   Just have a check, the check steps are:
>> 1. stop the thermald
>> 2. run the stress test
>> 3. Toggle the thermal setting between UltraPerformance and Quiet via 
>> dell-wmi-sysman
>> 4. Check the CPU FAN speed
>>   The system reboot is not needed, the CPU fan speed changes immediately.
>>   A screen recorder here: 
>> https://dell.box.com/s/p2bhd2b6cv8z5buk9eao3bosgrrp1lf9
>>
>> Thanks
>>
> Hi,
> 
> i believe it should be the responsibility of the manufacturer (in this 
> case Dell) that
> the thermal state remains consistent across both interfaces.
> 
> I think that the official Windows utility only checks the thermal state 
> reported by
> the USTT interface, so we should match this behavior.
> 
> Thanks,
> Armin Wolf
> 

Why?  Windows also does ACPI-WMI differently than Linux.  It's not as 
easy to check both from a Windows utility due to that.

Armin Wolf May 12, 2024, 6:47 p.m. UTC | #20

Am 12.05.24 um 19:58 schrieb Limonciello, Mario:

>
>
> On 5/12/2024 12:53 PM, Armin Wolf wrote:
>> Am 11.05.24 um 17:56 schrieb Shen, Yijun:
>>
>>> Internal Use - Confidential
>>>> -----Original Message-----
>>>> From: Limonciello, Mario <[e-mail]>
>>>> Sent: Saturday, May 11, 2024 11:13 PM
>>>> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
>>>> <[e-mail]>
>>>> Cc: [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; Hans de Goede <[e-mail]>; Matthew Garrett
>>>> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner
>>>> Kallweit
>>>> <[e-mail]>; Vegard Nossum <[e-mail]>;
>>>> [e-mail]; [e-mail];
>>>> Dell Client
>>>> Kernel <[e-mail]>
>>>> Subject: Re: [PATCH v5] platform/x86: dell-laptop: Implement
>>>> platform_profile
>>>>
>>>>
>>>> [EXTERNAL EMAIL]
>>>>
>>>>
>>>>
>>>> On 5/11/2024 10:05 AM, Shen, Yijun wrote:
>>>>> Internal Use - Confidential
>>>>>> -----Original Message-----
>>>>>> From: Mario Limonciello <[e-mail]>
>>>>>> Sent: Wednesday, May 8, 2024 11:53 PM
>>>>>> To: Shen, Yijun <[e-mail]>; Lyndon Sanche
>>>>>> <[e-mail]>
>>>>>> Cc: [e-mail]; [e-mail];
>>>>>> [e-mail]; [e-mail];
>>>>>> [e-mail]; Hans de Goede <[e-mail]>; Matthew
>>>> Garrett
>>>>>> <[e-mail]>; Jonathan Corbet <[e-mail]>; Heiner
>>>>>> Kallweit <[e-mail]>; Vegard Nossum
>>>>>> <[e-mail]>; [e-mail];
>>>>>> [e-mail]; Dell Client Kernel
>>>>>> <[e-mail]>
>>>>>> Subject: Re: RE: [PATCH v5] platform/x86: dell-laptop: Implement
>>>>>> platform_profile
>>>>>>
>>>>>>
>>>>>> [EXTERNAL EMAIL]
>>>>>>
>>>>>> On 5/8/2024 09:24, Shen, Yijun wrote:
>>>>>>> Hi Lyndon,
>>>>>>>
>>>>>>>     Thanks for working on this patch.
>>>>>>>
>>>>>>>     Dell side has an initial testing with this patch on some
>>>>>>> laptops,
>>>>>>> it looks
>>>>>> good. While changing the platform profile:
>>>>>>> 1. The corresponding USTT option in BIOS will be changed.
>>>>>>> 2. thermald will not be impacted. The related PSVT and ITMT will be
>>>> loaded.
>>>>>>>     Some Dell DTs does not have the USTT, Dell'll have a check if
>>>>>>> nothing is
>>>>>> broken.
>>>>>>
>>>>>> Hi Alex!
>>>>>>
>>>>>> Have you had a check both on both your AMD laptops and workstations
>>>>>> too, or just the Intel ones?  I think it would be good to make sure
>>>>>> it's getting the correct experience in both cases.
>>>>>>
>>>>> Hi Mario,
>>>>>
>>>>>    I've a check for this, for both laptop and workstation, the
>>>>> dell_laptop
>>>> module will not be loaded. So, AMD platform will not be impacted by
>>>> this
>>>> patch series.
>>>>> Follow is one example output with workstation.
>>>>>    #lsmod | grep dell
>>>>>      dell_wmi               28672  0
>>>>>      dell_smbios            32768  1 dell_wmi
>>>>>      dcdbas                 20480  1 dell_smbios
>>>>>      dell_wmi_descriptor    20480  2 dell_wmi,dell_smbios
>>>>>      sparse_keymap          12288  1 dell_wmi
>>>>>      ledtrig_audio          12288  3
>>>>> snd_ctl_led,snd_hda_codec_generic,dell_wmi
>>>>>      video                  73728  2 dell_wmi,nvidia_modeset
>>>>>      wmi                    40960  5
>>>> video,dell_wmi,wmi_bmof,dell_smbios,dell_wmi_descriptor
>>>> Ah; right that makes sense.  In that case, is dell-laptop even the
>>>> right place for
>>>> this patch series?  I would think the same policies for the
>>>> platform profile
>>>> should be able to apply to desktop/workstation.
>>>>
>>>> The v6 of this series would block smbios-thermal-ctl from working on a
>>>> workstation too.
>>>>
>>>>>>>      Additional, with this patch, follow behavior is found:
>>>>>>>     1. For example, the platform profile is quiet.
>>>>>>>     2. Reboot the system and change the USTT to performance.
>>>>>>>     3. Boot to desktop, the platform profile is "quiet", the USTT
>>>>>>> will be
>>>>>> changed back to "quiet".
>>>>>>>     This looks like not a proper user experience. The platform
>>>>>>> profile should
>>>>>> honor the BIOS setting, aka, the platform profile should be switched
>>>>>> to "performance".
>>>>>> I agree, this sounds like the initial profile needs to be read from
>>>>>> the BIOS settings too.
>>>>>>
>>>>>> Furthermore I wanted to ask is there also a WMI setting that
>>>>>> corresponds to this that dell-wmi-sysman offers?
>>>>>    Yes, Mario, you're right. This thermal setting could also be
>>>>> toggled by dell-
>>>> wmi-sysman.
>>>>> But, for the Dell consumer AMD laptops, like Alienware, the BIOS
>>>>> is another
>>>> variant which is different with the workstation one.
>>>>> With this variant BIOS, there is no USTT and also no
>>>>> dell_wmi/dell-wmi-
>>>> sysman.
>>>>>> I'm wondering if both should be probed in case the SMBIOS one goes
>>>> away one day.
>>>>>    Yep, I think this is a good suggestion.
>>>>>
>>>> Great! Although something I wonder is if the policy when changed
>>>> with dell-
>>>> wmi-sysman is immediate or requires a reboot.  A lot of the
>>>> settings in there
>>>> aren't effective until after a reboot.
>>>>
>>>> If this is one of them then it might not be a good idea to make it
>>>> work for
>>>> both.
>>> Hi Mario,
>>>
>>>   Just have a check, the check steps are:
>>> 1. stop the thermald
>>> 2. run the stress test
>>> 3. Toggle the thermal setting between UltraPerformance and Quiet via
>>> dell-wmi-sysman
>>> 4. Check the CPU FAN speed
>>>   The system reboot is not needed, the CPU fan speed changes
>>> immediately.
>>>   A screen recorder here:
>>> https://dell.box.com/s/p2bhd2b6cv8z5buk9eao3bosgrrp1lf9
>>>
>>> Thanks
>>>
>> Hi,
>>
>> i believe it should be the responsibility of the manufacturer (in
>> this case Dell) that
>> the thermal state remains consistent across both interfaces.
>>
>> I think that the official Windows utility only checks the thermal
>> state reported by
>> the USTT interface, so we should match this behavior.
>>
>> Thanks,
>> Armin Wolf
>>
>
> Why?  Windows also does ACPI-WMI differently than Linux.  It's not as
> easy to check both from a Windows utility due to that.

Actually, it is quite easy to check both interfaces from a Windows utility. Both ACPI-WMI objects can be accessed by
Windows applications, the utility just has to interact with an additional WMI object, but they decided to not do it.

Also the original smbios-thermal-ctl utility was created by Dell itself (i think?), so they likely would have implemented this
if it really was necessary.

As Dell likely only tests their machines with Windows (if at all), i propose that we try to match the Windows behavior.

Thanks,
Armin Wolf

Limonciello, Mario May 12, 2024, 10:14 p.m. UTC | #21

On 5/12/2024 1:47 PM, Armin Wolf wrote:
>> Why?  Windows also does ACPI-WMI differently than Linux.  It's not as
>> easy to check both from a Windows utility due to that.
> 
> Actually, it is quite easy to check both interfaces from a Windows 
> utility. Both ACPI-WMI objects can be accessed by
> Windows applications, the utility just has to interact with an 
> additional WMI object, but they decided to not do it.
> 

Ah I didn't realize that they're actually instanciable from the WMI 
repository in an application, but that makes perfect sense with how easy 
it is to do from PowerShell.

> Also the original smbios-thermal-ctl utility was created by Dell itself 
> (i think?), so they likely would have implemented this
> if it really was necessary.
> 

It was created at a time that the ACPI WMI BIOS attributes interface 
didn't exist.  I've understood that the general direction is to use the 
WMI BIOS attributes interface in the future.

That's why I was suggesting using both, if such a transition happens 
then this driver would be ready for it.

> As Dell likely only tests their machines with Windows (if at all), i 
> propose that we try to match the Windows behavior.

I don't have a strong horse in this game, but I see what you mean in 
terms of general compatibility.

Patch

diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index bd9f445974cc..5195ad59b44d 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -57,6 +57,7 @@  config DELL_LAPTOP
 	select POWER_SUPPLY
 	select LEDS_CLASS
 	select NEW_LEDS
+	select ACPI_PLATFORM_PROFILE
 	help
 	This driver adds support for rfkill and backlight control to Dell
 	laptops (except for some models covered by the Compal driver).
diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index 42f7de2b4522..dc530a4f5047 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -27,6 +27,9 @@ 
 #include <linux/i8042.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/platform_profile.h>
 #include <acpi/video.h>
 #include "dell-rbtn.h"
 #include "dell-smbios.h"
@@ -95,6 +98,7 @@  static struct backlight_device *dell_backlight_device;
 static struct rfkill *wifi_rfkill;
 static struct rfkill *bluetooth_rfkill;
 static struct rfkill *wwan_rfkill;
+static struct platform_profile_handler *thermal_handler;
 static bool force_rfkill;
 static bool micmute_led_registered;
 static bool mute_led_registered;
@@ -2199,6 +2203,232 @@  static int mute_led_set(struct led_classdev *led_cdev,
 	return 0;
 }
 
+/* Derived from smbios-thermal-ctl
+ *
+ * cbClass 17
+ * cbSelect 19
+ * User Selectable Thermal Tables(USTT)
+ * cbArg1 determines the function to be performed
+ * cbArg1 0x0 = Get Thermal Information
+ *  cbRES1         Standard return codes (0, -1, -2)
+ *  cbRES2, byte 0  Bitmap of supported thermal modes. A mode is supported if
+ *                  its bit is set to 1
+ *     Bit 0 Balanced
+ *     Bit 1 Cool Bottom
+ *     Bit 2 Quiet
+ *     Bit 3 Performance
+ *  cbRES2, byte 1 Bitmap of supported Active Acoustic Controller (AAC) modes.
+ *                 Each mode corresponds to the supported thermal modes in
+ *                  byte 0. A mode is supported if its bit is set to 1.
+ *     Bit 0 AAC (Balanced)
+ *     Bit 1 AAC (Cool Bottom
+ *     Bit 2 AAC (Quiet)
+ *     Bit 3 AAC (Performance)
+ *  cbRes3, byte 0 Current Thermal Mode
+ *     Bit 0 Balanced
+ *     Bit 1 Cool Bottom
+ *     Bit 2 Quiet
+ *     Bit 3 Performanc
+ *  cbRes3, byte 1  AAC Configuration type
+ *          0       Global (AAC enable/disable applies to all supported USTT modes)
+ *          1       USTT mode specific
+ *  cbRes3, byte 2  Current Active Acoustic Controller (AAC) Mode
+ *     If AAC Configuration Type is Global,
+ *          0       AAC mode disabled
+ *          1       AAC mode enabled
+ *     If AAC Configuration Type is USTT mode specific (multiple bits may be set),
+ *          Bit 0 AAC (Balanced)
+ *          Bit 1 AAC (Cool Bottom
+ *          Bit 2 AAC (Quiet)
+ *          Bit 3 AAC (Performance)
+ *  cbRes3, byte 3  Current Fan Failure Mode
+ *     Bit 0 Minimal Fan Failure (at least one fan has failed, one fan working)
+ *     Bit 1 Catastrophic Fan Failure (all fans have failed)
+ *
+ * cbArg1 0x1   (Set Thermal Information), both desired thermal mode and
+ *               desired AAC mode shall be applied
+ * cbArg2, byte 0  Desired Thermal Mode to set
+ *                  (only one bit may be set for this parameter)
+ *     Bit 0 Balanced
+ *     Bit 1 Cool Bottom
+ *     Bit 2 Quiet
+ *     Bit 3 Performance
+ * cbArg2, byte 1  Desired Active Acoustic Controller (AAC) Mode to set
+ *     If AAC Configuration Type is Global,
+ *         0  AAC mode disabled
+ *         1  AAC mode enabled
+ *     If AAC Configuration Type is USTT mode specific
+ *     (multiple bits may be set for this parameter),
+ *         Bit 0 AAC (Balanced)
+ *         Bit 1 AAC (Cool Bottom
+ *         Bit 2 AAC (Quiet)
+ *         Bit 3 AAC (Performance)
+ */
+
+#define DELL_ACC_GET_FIELD		GENMASK(19, 16)
+#define DELL_ACC_SET_FIELD		GENMASK(11, 8)
+#define DELL_THERMAL_SUPPORTED	GENMASK(3, 0)
+
+enum thermal_mode_bits {
+	DELL_BALANCED = BIT(0),
+	DELL_COOL_BOTTOM = BIT(1),
+	DELL_QUIET = BIT(2),
+	DELL_PERFORMANCE = BIT(3),
+};
+
+static int thermal_get_mode(void)
+{
+	struct calling_interface_buffer buffer;
+	int state;
+	int ret;
+
+	dell_fill_request(&buffer, 0x0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
+	if (ret)
+		return ret;
+	state = buffer.output[2];
+	if (state & DELL_BALANCED)
+		return DELL_BALANCED;
+	else if (state & DELL_COOL_BOTTOM)
+		return DELL_COOL_BOTTOM;
+	else if (state & DELL_QUIET)
+		return DELL_QUIET;
+	else if (state & DELL_PERFORMANCE)
+		return DELL_PERFORMANCE;
+	else
+		return -ENXIO;
+}
+
+static int thermal_get_supported_modes(int *supported_bits)
+{
+	struct calling_interface_buffer buffer;
+	int ret;
+
+	dell_fill_request(&buffer, 0x0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
+	if (ret)
+		return ret;
+	*supported_bits = FIELD_GET(DELL_THERMAL_SUPPORTED, buffer.output[1]);
+	return 0;
+}
+
+static int thermal_get_acc_mode(int *acc_mode)
+{
+	struct calling_interface_buffer buffer;
+	int ret;
+
+	dell_fill_request(&buffer, 0x0, 0, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
+	if (ret)
+		return ret;
+	*acc_mode = FIELD_GET(DELL_ACC_GET_FIELD, buffer.output[3]);
+	return 0;
+}
+
+static int thermal_set_mode(enum thermal_mode_bits state)
+{
+	struct calling_interface_buffer buffer;
+	int ret;
+	int acc_mode;
+
+	ret = thermal_get_acc_mode(&acc_mode);
+	if (ret)
+		return ret;
+
+	dell_fill_request(&buffer, 0x1, FIELD_PREP(DELL_ACC_SET_FIELD, acc_mode) | state, 0, 0);
+	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
+	return ret;
+}
+
+static int thermal_platform_profile_set(struct platform_profile_handler *pprof,
+					enum platform_profile_option profile)
+{
+	switch (profile) {
+	case PLATFORM_PROFILE_BALANCED:
+		return thermal_set_mode(DELL_BALANCED);
+	case PLATFORM_PROFILE_PERFORMANCE:
+		return thermal_set_mode(DELL_PERFORMANCE);
+	case PLATFORM_PROFILE_QUIET:
+		return thermal_set_mode(DELL_QUIET);
+	case PLATFORM_PROFILE_COOL:
+		return thermal_set_mode(DELL_COOL_BOTTOM);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
+					enum platform_profile_option *profile)
+{
+	int ret;
+
+	ret = thermal_get_mode();
+	if (ret < 0)
+		return ret;
+
+	switch (ret) {
+	case DELL_BALANCED:
+		*profile = PLATFORM_PROFILE_BALANCED;
+		break;
+	case DELL_PERFORMANCE:
+		*profile = PLATFORM_PROFILE_PERFORMANCE;
+		break;
+	case DELL_COOL_BOTTOM:
+		*profile = PLATFORM_PROFILE_COOL;
+		break;
+	case DELL_QUIET:
+		*profile = PLATFORM_PROFILE_QUIET;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int thermal_init(void)
+{
+	int ret;
+	int supported_modes;
+
+	/* If thermal modes not supported, exit without error */
+	ret = thermal_get_supported_modes(&supported_modes);
+	if (ret < 0)
+		return ret;
+	if (!supported_modes)
+		return 0;
+
+	thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
+	if (!thermal_handler)
+		return -ENOMEM;
+	thermal_handler->profile_get = thermal_platform_profile_get;
+	thermal_handler->profile_set = thermal_platform_profile_set;
+
+	if (supported_modes & DELL_QUIET)
+		set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
+	if (supported_modes & DELL_COOL_BOTTOM)
+		set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
+	if (supported_modes & DELL_BALANCED)
+		set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
+	if (supported_modes & DELL_PERFORMANCE)
+		set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
+
+	/* Clean up if failed */
+	ret = platform_profile_register(thermal_handler);
+	if (ret)
+		kfree(thermal_handler);
+
+	return ret;
+}
+
+static void thermal_cleanup(void)
+{
+	if (thermal_handler) {
+		platform_profile_remove();
+		kfree(thermal_handler);
+	}
+}
+
 static struct led_classdev mute_led_cdev = {
 	.name = "platform::mute",
 	.max_brightness = 1,
@@ -2238,6 +2468,11 @@  static int __init dell_init(void)
 		goto fail_rfkill;
 	}
 
+	/* Do not fail module if thermal modes not supported, just skip */
+	ret = thermal_init();
+	if (ret)
+		goto fail_thermal;
+
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_init(&platform_device->dev);
 
@@ -2317,6 +2552,8 @@  static int __init dell_init(void)
 		led_classdev_unregister(&mute_led_cdev);
 fail_led:
 	dell_cleanup_rfkill();
+fail_thermal:
+	thermal_cleanup();
 fail_rfkill:
 	platform_device_del(platform_device);
 fail_platform_device2:
@@ -2344,6 +2581,7 @@  static void __exit dell_exit(void)
 		platform_device_unregister(platform_device);
 		platform_driver_unregister(&platform_driver);
 	}
+	thermal_cleanup();
 }
 
 /* dell-rbtn.c driver export functions which will not work correctly (and could
diff --git a/drivers/platform/x86/dell/dell-smbios-base.c b/drivers/platform/x86/dell/dell-smbios-base.c
index e61bfaf8b5c4..5bc2e394dd1c 100644
--- a/drivers/platform/x86/dell/dell-smbios-base.c
+++ b/drivers/platform/x86/dell/dell-smbios-base.c
@@ -71,6 +71,7 @@  static struct smbios_call call_blacklist[] = {
 	/* handled by kernel: dell-laptop */
 	{0x0000, CLASS_INFO, SELECT_RFKILL},
 	{0x0000, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT},
+	{0x0000, CLASS_INFO, SELECT_THERMAL_MANAGEMENT},
 };
 
 struct token_range {
diff --git a/drivers/platform/x86/dell/dell-smbios.h b/drivers/platform/x86/dell/dell-smbios.h
index eb341bf000c6..585d042f1779 100644
--- a/drivers/platform/x86/dell/dell-smbios.h
+++ b/drivers/platform/x86/dell/dell-smbios.h
@@ -19,6 +19,7 @@ 
 /* Classes and selects used only in kernel drivers */
 #define CLASS_KBD_BACKLIGHT 4
 #define SELECT_KBD_BACKLIGHT 11
+#define SELECT_THERMAL_MANAGEMENT 19
 
 /* Tokens used in kernel drivers, any of these
  * should be filtered from userspace access