Mailing List Archive: [PATCH] platform/x86: dell-laptop: Implement platform

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

Apr 25, 2024, 10:27 AM

Post #1 of 15 (51 views)

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 <lsanche@lyndeno.ca>
---
drivers/platform/x86/dell/dell-laptop.c | 220 ++++++++++++++++++++++++
drivers/platform/x86/dell/dell-smbios.h | 1 +
2 files changed, 221 insertions(+)

diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index 42f7de2b4522..7f9c4e0e5ef5 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -27,6 +27,7 @@
#include <linux/i8042.h>
#include <linux/debugfs.h>
#include <linux/seq_file.h>
+#include <linux/platform_profile.h>
#include <acpi/video.h>
#include "dell-rbtn.h"
#include "dell-smbios.h"
@@ -95,10 +96,18 @@ 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;

+enum thermal_mode_bits {
+ DELL_BALANCED = 0,
+ DELL_COOL_BOTTOM = 1,
+ DELL_QUIET = 2,
+ DELL_PERFORMANCE = 3,
+};
+
module_param(force_rfkill, bool, 0444);
MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");

@@ -2199,6 +2208,211 @@ 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)
+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) & 1)
+ return DELL_BALANCED;
+ else if ((state >> DELL_COOL_BOTTOM) & 1)
+ return DELL_COOL_BOTTOM;
+ else if ((state >> DELL_QUIET) & 1)
+ return DELL_QUIET;
+ else if ((state >> DELL_PERFORMANCE) & 1)
+ return DELL_PERFORMANCE;
+ else
+ return 0;
+}
+
+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 = buffer.output[1] & 0xF;
+ 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 = ((buffer.output[3] >> 8) & 0xFF);
+ 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, (acc_mode << 8) | BIT(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)
+{
+ int ret;
+
+ switch (profile) {
+ case PLATFORM_PROFILE_BALANCED:
+ ret = thermal_set_mode(DELL_BALANCED);
+ break;
+ case PLATFORM_PROFILE_PERFORMANCE:
+ ret = thermal_set_mode(DELL_PERFORMANCE);
+ break;
+ case PLATFORM_PROFILE_QUIET:
+ ret = thermal_set_mode(DELL_QUIET);
+ break;
+ case PLATFORM_PROFILE_COOL:
+ ret = thermal_set_mode(DELL_COOL_BOTTOM);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return ret;
+}
+
+static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
+ enum platform_profile_option *profile)
+{
+ switch (thermal_get_mode()) {
+ 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;
+}
+
+int thermal_init(void)
+{
+ int ret;
+ int supported_modes;
+
+ ret = thermal_get_supported_modes(&supported_modes);
+
+ if (ret != 0 || supported_modes == 0)
+ return -ENXIO;
+
+ 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) & 1)
+ set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
+ if ((supported_modes >> DELL_COOL_BOTTOM) & 1)
+ set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
+ if ((supported_modes >> DELL_BALANCED) & 1)
+ set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
+ if ((supported_modes >> DELL_PERFORMANCE) & 1)
+ set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
+
+ platform_profile_register(thermal_handler);
+
+ return 0;
+}
+
+void thermal_cleanup(void)
+{
+ platform_profile_remove();
+ kfree(thermal_handler);
+}
+
static struct led_classdev mute_led_cdev = {
.name = "platform::mute",
.max_brightness = 1,
@@ -2266,6 +2480,11 @@ static int __init dell_init(void)
mute_led_registered = true;
}

+ // Do not fail module if thermal modes not supported,
+ // just skip
+ if (thermal_init() != 0)
+ pr_warn("Unable to setup platform_profile, skipping");
+
if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
return 0;

@@ -2344,6 +2563,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.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

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

mario.limonciello at amd

Apr 25, 2024, 1:07 PM

Post #2 of 15 (51 views)

Permalink

+ Srinivas

On 4/25/2024 12:27, Lyndon Sanche wrote:
> 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 <lsanche@lyndeno.ca>
> ---

When you developed this was it using a Dell Intel or Dell AMD system?

If it was an Intel system, did you test it with thermald installed and
active?

I'm wondering how all this stuff jives with the stuff that thermald
does. I don't know if they fight for any of the same "resources".

> drivers/platform/x86/dell/dell-laptop.c | 220 ++++++++++++++++++++++++
> drivers/platform/x86/dell/dell-smbios.h | 1 +
> 2 files changed, 221 insertions(+)
>
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 42f7de2b4522..7f9c4e0e5ef5 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -27,6 +27,7 @@
> #include <linux/i8042.h>
> #include <linux/debugfs.h>
> #include <linux/seq_file.h>
> +#include <linux/platform_profile.h>
> #include <acpi/video.h>
> #include "dell-rbtn.h"
> #include "dell-smbios.h"
> @@ -95,10 +96,18 @@ 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;
>
> +enum thermal_mode_bits {
> + DELL_BALANCED = 0,
> + DELL_COOL_BOTTOM = 1,
> + DELL_QUIET = 2,
> + DELL_PERFORMANCE = 3,
> +};
> +
> module_param(force_rfkill, bool, 0444);
> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
>
> @@ -2199,6 +2208,211 @@ 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)
> +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) & 1)
> + return DELL_BALANCED;
> + else if ((state >> DELL_COOL_BOTTOM) & 1)
> + return DELL_COOL_BOTTOM;
> + else if ((state >> DELL_QUIET) & 1)
> + return DELL_QUIET;
> + else if ((state >> DELL_PERFORMANCE) & 1)
> + return DELL_PERFORMANCE;
> + else
> + return 0;
> +}
> +
> +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 = buffer.output[1] & 0xF;
> + 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 = ((buffer.output[3] >> 8) & 0xFF);
> + 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, (acc_mode << 8) | BIT(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)
> +{
> + int ret;
> +
> + switch (profile) {
> + case PLATFORM_PROFILE_BALANCED:
> + ret = thermal_set_mode(DELL_BALANCED);
> + break;
> + case PLATFORM_PROFILE_PERFORMANCE:
> + ret = thermal_set_mode(DELL_PERFORMANCE);
> + break;
> + case PLATFORM_PROFILE_QUIET:
> + ret = thermal_set_mode(DELL_QUIET);
> + break;
> + case PLATFORM_PROFILE_COOL:
> + ret = thermal_set_mode(DELL_COOL_BOTTOM);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
> + enum platform_profile_option *profile)
> +{
> + switch (thermal_get_mode()) {
> + 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;
> +}
> +
> +int thermal_init(void)
> +{
> + int ret;
> + int supported_modes;
> +
> + ret = thermal_get_supported_modes(&supported_modes);
> +
> + if (ret != 0 || supported_modes == 0)
> + return -ENXIO;
> +
> + 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) & 1)
> + set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
> + if ((supported_modes >> DELL_COOL_BOTTOM) & 1)
> + set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
> + if ((supported_modes >> DELL_BALANCED) & 1)
> + set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
> + if ((supported_modes >> DELL_PERFORMANCE) & 1)
> + set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
> +
> + platform_profile_register(thermal_handler);
> +
> + return 0;
> +}
> +
> +void thermal_cleanup(void)
> +{
> + platform_profile_remove();
> + kfree(thermal_handler);
> +}
> +
> static struct led_classdev mute_led_cdev = {
> .name = "platform::mute",
> .max_brightness = 1,
> @@ -2266,6 +2480,11 @@ static int __init dell_init(void)
> mute_led_registered = true;
> }
>
> + // Do not fail module if thermal modes not supported,
> + // just skip
> + if (thermal_init() != 0)
> + pr_warn("Unable to setup platform_profile, skipping");
> +
> if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> return 0;
>
> @@ -2344,6 +2563,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.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

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

pali at kernel

Apr 25, 2024, 1:12 PM

Post #3 of 15 (51 views)

Permalink

On Thursday 25 April 2024 11:27:57 Lyndon Sanche wrote:
> +int thermal_init(void)
> +{
> + int ret;
> + int supported_modes;
> +
> + ret = thermal_get_supported_modes(&supported_modes);
> +
> + if (ret != 0 || supported_modes == 0)
> + return -ENXIO;
> +
> + 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) & 1)
> + set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
> + if ((supported_modes >> DELL_COOL_BOTTOM) & 1)
> + set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
> + if ((supported_modes >> DELL_BALANCED) & 1)
> + set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
> + if ((supported_modes >> DELL_PERFORMANCE) & 1)
> + set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
> +
> + platform_profile_register(thermal_handler);
> +
> + return 0;
> +}
> +
> +void thermal_cleanup(void)
> +{
> + platform_profile_remove();
> + kfree(thermal_handler);
> +}
> +
> static struct led_classdev mute_led_cdev = {
> .name = "platform::mute",
> .max_brightness = 1,
> @@ -2266,6 +2480,11 @@ static int __init dell_init(void)
> mute_led_registered = true;
> }
>
> + // Do not fail module if thermal modes not supported,
> + // just skip
> + if (thermal_init() != 0)
> + pr_warn("Unable to setup platform_profile, skipping");

I think that -ENOMEM error should be failure of the loading the driver.
It does not make sense to continue of memory allocation failed.

On the other hand when the thermal modes are not support (e.g. old
Latitude models) then there should not be a warning message. It is
expected that on systems without thermal modes the setup fails.

> +
> if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> return 0;

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

lsanche at lyndeno

Apr 25, 2024, 1:24 PM

Post #4 of 15 (51 views)

Permalink

On Thu, Apr 25, 2024, at 2:07 PM, Mario Limonciello wrote:
> + Srinivas
>
> On 4/25/2024 12:27, Lyndon Sanche wrote:
>> 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 <lsanche@lyndeno.ca>
>> ---
>
> When you developed this was it using a Dell Intel or Dell AMD system?
>
> If it was an Intel system, did you test it with thermald installed and
> active?
>
> I'm wondering how all this stuff jives with the stuff that thermald
> does. I don't know if they fight for any of the same "resources".

Thank you for your response.

I did my development and testing on a Dell Intel system. Specifically the XPS 15 9560 with i7-7700HQ.

I do have thermald running, though I admit I am not really aware of what exactly it does, besides being related to thermals in some way.

I normally set the thermal mode with Dell's smbios-thermal-ctl program. I am not too sure all the values that the bios configures on it's own depending on the provided mode, so I am not sure if thermald conflicts. But my understanding is that would be out of scope of this driver, since we are only telling the bios what we want at a high level.

Lyndon

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

lsanche at lyndeno

Apr 25, 2024, 1:27 PM

Post #5 of 15 (51 views)

Permalink

On Thu, Apr 25, 2024, at 2:12 PM, Pali Rohár wrote:
> On Thursday 25 April 2024 11:27:57 Lyndon Sanche wrote:
>> +int thermal_init(void)
>> +{
>> + int ret;
>> + int supported_modes;
>> +
>> + ret = thermal_get_supported_modes(&supported_modes);
>> +
>> + if (ret != 0 || supported_modes == 0)
>> + return -ENXIO;
>> +
>> + 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) & 1)
>> + set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
>> + if ((supported_modes >> DELL_COOL_BOTTOM) & 1)
>> + set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
>> + if ((supported_modes >> DELL_BALANCED) & 1)
>> + set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
>> + if ((supported_modes >> DELL_PERFORMANCE) & 1)
>> + set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
>> +
>> + platform_profile_register(thermal_handler);
>> +
>> + return 0;
>> +}
>> +
>> +void thermal_cleanup(void)
>> +{
>> + platform_profile_remove();
>> + kfree(thermal_handler);
>> +}
>> +
>> static struct led_classdev mute_led_cdev = {
>> .name = "platform::mute",
>> .max_brightness = 1,
>> @@ -2266,6 +2480,11 @@ static int __init dell_init(void)
>> mute_led_registered = true;
>> }
>>
>> + // Do not fail module if thermal modes not supported,
>> + // just skip
>> + if (thermal_init() != 0)
>> + pr_warn("Unable to setup platform_profile, skipping");
>
> I think that -ENOMEM error should be failure of the loading the driver.
> It does not make sense to continue of memory allocation failed.
>
> On the other hand when the thermal modes are not support (e.g. old
> Latitude models) then there should not be a warning message. It is
> expected that on systems without thermal modes the setup fails.

Thank you for your feedback.

I agree with your suggestion. -ENOMEM would indicate something bigger is amiss. I can add a check:

If -ENOMEM, fail driver.
If anything other error, skip, but do not show a message.

Lyndon

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

mario.limonciello at amd

Apr 25, 2024, 1:28 PM

Post #6 of 15 (51 views)

Permalink

On 4/25/2024 15:24, Lyndon Sanche wrote:
> On Thu, Apr 25, 2024, at 2:07 PM, Mario Limonciello wrote:
>> + Srinivas
>>
>> On 4/25/2024 12:27, Lyndon Sanche wrote:
>>> 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 <lsanche@lyndeno.ca>
>>> ---
>>
>> When you developed this was it using a Dell Intel or Dell AMD system?
>>
>> If it was an Intel system, did you test it with thermald installed and
>> active?
>>
>> I'm wondering how all this stuff jives with the stuff that thermald
>> does. I don't know if they fight for any of the same "resources".
>
> Thank you for your response.
>
> I did my development and testing on a Dell Intel system. Specifically the XPS 15 9560 with i7-7700HQ.
>
> I do have thermald running, though I admit I am not really aware of what exactly it does, besides being related to thermals in some way.
>
> I normally set the thermal mode with Dell's smbios-thermal-ctl program. I am not too sure all the values that the bios configures on it's own depending on the provided mode, so I am not sure if thermald conflicts. But my understanding is that would be out of scope of this driver, since we are only telling the bios what we want at a high level.
>
> Lyndon

Yeah it's not say it's a "new" conflict, it would just become a lot more
prevalent since software like GNOME and KDE use power-profiles-daemon to
manipulate the new power profile you're exporting from the driver.

If there really is no conflict, then great!
If there is a conflict then I was just wondering if there needs to be an
easy way to turn on/off the profile support when thermald is in use.

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

pali at kernel

Apr 25, 2024, 1:31 PM

Post #7 of 15 (51 views)

Permalink

On Thursday 25 April 2024 14:27:32 Lyndon Sanche wrote:
> On Thu, Apr 25, 2024, at 2:12 PM, Pali Rohár wrote:
> > On Thursday 25 April 2024 11:27:57 Lyndon Sanche wrote:
> >> +int thermal_init(void)
> >> +{
> >> + int ret;
> >> + int supported_modes;
> >> +
> >> + ret = thermal_get_supported_modes(&supported_modes);
> >> +
> >> + if (ret != 0 || supported_modes == 0)
> >> + return -ENXIO;
> >> +
> >> + 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) & 1)
> >> + set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
> >> + if ((supported_modes >> DELL_COOL_BOTTOM) & 1)
> >> + set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
> >> + if ((supported_modes >> DELL_BALANCED) & 1)
> >> + set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
> >> + if ((supported_modes >> DELL_PERFORMANCE) & 1)
> >> + set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
> >> +
> >> + platform_profile_register(thermal_handler);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +void thermal_cleanup(void)
> >> +{
> >> + platform_profile_remove();
> >> + kfree(thermal_handler);
> >> +}
> >> +
> >> static struct led_classdev mute_led_cdev = {
> >> .name = "platform::mute",
> >> .max_brightness = 1,
> >> @@ -2266,6 +2480,11 @@ static int __init dell_init(void)
> >> mute_led_registered = true;
> >> }
> >>
> >> + // Do not fail module if thermal modes not supported,
> >> + // just skip
> >> + if (thermal_init() != 0)
> >> + pr_warn("Unable to setup platform_profile, skipping");
> >
> > I think that -ENOMEM error should be failure of the loading the driver.
> > It does not make sense to continue of memory allocation failed.
> >
> > On the other hand when the thermal modes are not support (e.g. old
> > Latitude models) then there should not be a warning message. It is
> > expected that on systems without thermal modes the setup fails.
>
> Thank you for your feedback.
>
> I agree with your suggestion. -ENOMEM would indicate something bigger is amiss. I can add a check:
>
> If -ENOMEM, fail driver.
> If anything other error, skip, but do not show a message.
>
> Lyndon

Maybe you can simplify it more. thermal_init() could return 0 also for
the case when thermal modes are not supported. And dell_init() then can
unconditionally fail when thermal_init() returns non-zero value. It has
benefit that in case thermal_init() is extended in future for some other
fatal error, it would not be required to update also caller to handle
another error (and not just ENOMEM).

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

W_Armin at gmx

Apr 25, 2024, 2:07 PM

Post #8 of 15 (51 views)

Permalink

Am 25.04.24 um 19:27 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 <lsanche@lyndeno.ca>
> ---
> drivers/platform/x86/dell/dell-laptop.c | 220 ++++++++++++++++++++++++
> drivers/platform/x86/dell/dell-smbios.h | 1 +
> 2 files changed, 221 insertions(+)
>
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 42f7de2b4522..7f9c4e0e5ef5 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -27,6 +27,7 @@
> #include <linux/i8042.h>
> #include <linux/debugfs.h>
> #include <linux/seq_file.h>
> +#include <linux/platform_profile.h>
> #include <acpi/video.h>
> #include "dell-rbtn.h"
> #include "dell-smbios.h"
> @@ -95,10 +96,18 @@ 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;
>
> +enum thermal_mode_bits {
> + DELL_BALANCED = 0,
> + DELL_COOL_BOTTOM = 1,
> + DELL_QUIET = 2,
> + DELL_PERFORMANCE = 3,
> +};
> +
> module_param(force_rfkill, bool, 0444);
> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
>
> @@ -2199,6 +2208,211 @@ 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),

Hi,

checkpatch reports: WARNING: line length of 101 exceeds 100 columns

> +// Bit 0 AAC (Balanced)
> +// Bit 1 AAC (Cool Bottom
> +// Bit 2 AAC (Quiet)
> +// Bit 3 AAC (Performance)
> +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) & 1)
> + return DELL_BALANCED;
> + else if ((state >> DELL_COOL_BOTTOM) & 1)
> + return DELL_COOL_BOTTOM;
> + else if ((state >> DELL_QUIET) & 1)
> + return DELL_QUIET;
> + else if ((state >> DELL_PERFORMANCE) & 1)
> + return DELL_PERFORMANCE;
> + else
> + return 0;

This would return DELL_BALANCED if no option is set. Please return an appropriate error code.

> +}
> +
> +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 = buffer.output[1] & 0xF;
> + 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 = ((buffer.output[3] >> 8) & 0xFF);
> + 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, (acc_mode << 8) | BIT(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)
> +{
> + int ret;
> +
> + switch (profile) {
> + case PLATFORM_PROFILE_BALANCED:
> + ret = thermal_set_mode(DELL_BALANCED);
> + break;

Maybe using "return thermal_set_mode()" would be better in this cases.

> + case PLATFORM_PROFILE_PERFORMANCE:
> + ret = thermal_set_mode(DELL_PERFORMANCE);
> + break;
> + case PLATFORM_PROFILE_QUIET:
> + ret = thermal_set_mode(DELL_QUIET);
> + break;
> + case PLATFORM_PROFILE_COOL:
> + ret = thermal_set_mode(DELL_COOL_BOTTOM);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
> + enum platform_profile_option *profile)
> +{
> + switch (thermal_get_mode()) {

Please check if thermal_get_mode() returned an error code and return it in this case.

> + 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;
> +}
> +
> +int thermal_init(void)
> +{
> + int ret;
> + int supported_modes;
> +
> + ret = thermal_get_supported_modes(&supported_modes);
> +
> + if (ret != 0 || supported_modes == 0)
> + return -ENXIO;
> +
> + 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) & 1)
> + set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
> + if ((supported_modes >> DELL_COOL_BOTTOM) & 1)
> + set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
> + if ((supported_modes >> DELL_BALANCED) & 1)
> + set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
> + if ((supported_modes >> DELL_PERFORMANCE) & 1)
> + set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
> +
> + platform_profile_register(thermal_handler);
> +
> + return 0;

Please check the return value of platform_profile_register() and return the error if this function fails,
see commit fe0e04cf66a1 ("platform/surface: platform_profile: Propagate error if profile registration fails")
for an explanation.

> +}
> +
> +void thermal_cleanup(void)
> +{
> + platform_profile_remove();
> + kfree(thermal_handler);
> +}
> +
> static struct led_classdev mute_led_cdev = {
> .name = "platform::mute",
> .max_brightness = 1,
> @@ -2266,6 +2480,11 @@ static int __init dell_init(void)
> mute_led_registered = true;
> }
>
> + // Do not fail module if thermal modes not supported,
> + // just skip
> + if (thermal_init() != 0)
> + pr_warn("Unable to setup platform_profile, skipping");
> +
> if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> return 0;
>
> @@ -2344,6 +2563,7 @@ static void __exit dell_exit(void)
> platform_device_unregister(platform_device);
> platform_driver_unregister(&platform_driver);
> }
> + thermal_cleanup();

Should only be called when thermal_init() was successful.

Thanks,
Armin Wolf

> }
>
> /* dell-rbtn.c driver export functions which will not work correctly (and could
> 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

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

srinivas.pandruvada at linux

Apr 25, 2024, 2:51 PM

Post #9 of 15 (49 views)

Permalink

On 4/25/24 13:28, Mario Limonciello wrote:
> On 4/25/2024 15:24, Lyndon Sanche wrote:
>> On Thu, Apr 25, 2024, at 2:07 PM, Mario Limonciello wrote:
>>> + Srinivas
>>>
>>> On 4/25/2024 12:27, Lyndon Sanche wrote:
>>>> 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 <lsanche@lyndeno.ca>
>>>> ---
>>>
>>> When you developed this was it using a Dell Intel or Dell AMD system?
>>>
>>> If it was an Intel system, did you test it with thermald installed and
>>> active?
>>>
>>> I'm wondering how all this stuff jives with the stuff that thermald
>>> does. I don't know if they fight for any of the same "resources".
>>
>> Thank you for your response.
>>
>> I did my development and testing on a Dell Intel system. Specifically
>> the XPS 15 9560 with i7-7700HQ.
>>
>> I do have thermald running, though I admit I am not really aware of
>> what exactly it does, besides being related to thermals in some way.
>>
>> I normally set the thermal mode with Dell's smbios-thermal-ctl
>> program. I am not too sure all the values that the bios configures on
>> it's own depending on the provided mode, so I am not sure if thermald
>> conflicts. But my understanding is that would be out of scope of this
>> driver, since we are only telling the bios what we want at a high level.
>>
>> Lyndon
>
> Yeah it's not say it's a "new" conflict, it would just become a lot
> more prevalent since software like GNOME and KDE use
> power-profiles-daemon to manipulate the new power profile you're
> exporting from the driver.
>
> If there really is no conflict, then great!
> If there is a conflict then I was just wondering if there needs to be
> an easy way to turn on/off the profile support when thermald is in use.

This shouldn't be in conflict as this should be directly changing some
settings in BIOS. BIOS should send some notification, if it wants some
changes in thermal tables used by thermald.

Thanks,

Srinivas

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

lsanche at lyndeno

Apr 25, 2024, 5:38 PM

Post #10 of 15 (48 views)

Permalink

On Thu, Apr 25 2024 at 02:51:42 PM -07:00:00, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On 4/25/24 13:28, Mario Limonciello wrote:
>> Yeah it's not say it's a "new" conflict, it would just become a lot
>> more prevalent since software like GNOME and KDE use
>> power-profiles-daemon to manipulate the new power profile you're
>> exporting from the driver.
>>
>> If there really is no conflict, then great!
>> If there is a conflict then I was just wondering if there needs to
>> be an easy way to turn on/off the profile support when thermald is
>> in use.
>
> This shouldn't be in conflict as this should be directly changing
> some settings in BIOS. BIOS should send some notification, if it
> wants some changes in thermal tables used by thermald.
>
>
> Thanks,
>
> Srinivas
>

If we do not think there is a conflict I can leave it without a toggle.

Lyndon

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

lsanche at lyndeno

Apr 25, 2024, 5:54 PM

Post #11 of 15 (48 views)

Permalink

On Thu, Apr 25 2024 at 11:07:22 PM +02:00:00, Armin Wolf
<W_Armin@gmx.de> wrote:
> Am 25.04.24 um 19:27 schrieb Lyndon Sanche:
>>
>> +// 1 AAC mode enabled
>> +//
>> +// If AAC Configuration Type is USTT mode specific (multiple
>> bits may be set for this parameter),
>
> Hi,
>
> checkpatch reports: WARNING: line length of 101 exceeds 100 columns

I can wrap this last line.

>
>> +// Bit 0 AAC (Balanced)
>> +// Bit 1 AAC (Cool Bottom
>> +// Bit 2 AAC (Quiet)
>> +// Bit 3 AAC (Performance)
>> +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) & 1)
>> + return DELL_BALANCED;
>> + else if ((state >> DELL_COOL_BOTTOM) & 1)
>> + return DELL_COOL_BOTTOM;
>> + else if ((state >> DELL_QUIET) & 1)
>> + return DELL_QUIET;
>> + else if ((state >> DELL_PERFORMANCE) & 1)
>> + return DELL_PERFORMANCE;
>> + else
>> + return 0;
>
> This would return DELL_BALANCED if no option is set. Please return an
> appropriate error code.

Thanks for catching this, I will return a proper error code.

>
>> +}
>> +
>> +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 = buffer.output[1] & 0xF;
>> + 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 = ((buffer.output[3] >> 8) & 0xFF);
>> + 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, (acc_mode << 8) | BIT(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)
>> +{
>> + int ret;
>> +
>> + switch (profile) {
>> + case PLATFORM_PROFILE_BALANCED:
>> + ret = thermal_set_mode(DELL_BALANCED);
>> + break;
>
> Maybe using "return thermal_set_mode()" would be better in this cases.

Good idea, I can clean up the code with this suggestion.

>
>> + case PLATFORM_PROFILE_PERFORMANCE:
>> + ret = thermal_set_mode(DELL_PERFORMANCE);
>> + break;
>> + case PLATFORM_PROFILE_QUIET:
>> + ret = thermal_set_mode(DELL_QUIET);
>> + break;
>> + case PLATFORM_PROFILE_COOL:
>> + ret = thermal_set_mode(DELL_COOL_BOTTOM);
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int thermal_platform_profile_get(struct
>> platform_profile_handler *pprof,
>> + enum platform_profile_option *profile)
>> +{
>> + switch (thermal_get_mode()) {
>
> Please check if thermal_get_mode() returned an error code and return
> it in this case.

Will add error checking.

>> + set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
>> +
>> + platform_profile_register(thermal_handler);
>> +
>> + return 0;
>
> Please check the return value of platform_profile_register() and
> return the error if this function fails,
> see commit fe0e04cf66a1 ("platform/surface: platform_profile:
> Propagate error if profile registration fails")
> for an explanation.

Thank you for catching this. I forgot to handle the return value.

>
>> +}
>> +
>> +void thermal_cleanup(void)
>> +{
>> + platform_profile_remove();
>> + kfree(thermal_handler);
>> +}
>> +
>> static struct led_classdev mute_led_cdev = {
>> .name = "platform::mute",
>> .max_brightness = 1,
>> @@ -2266,6 +2480,11 @@ static int __init dell_init(void)
>> mute_led_registered = true;
>> }
>>
>> + // Do not fail module if thermal modes not supported,
>> + // just skip
>> + if (thermal_init() != 0)
>> + pr_warn("Unable to setup platform_profile, skipping");
>> +
>> if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>> return 0;
>>
>> @@ -2344,6 +2563,7 @@ static void __exit dell_exit(void)
>> platform_device_unregister(platform_device);
>> platform_driver_unregister(&platform_driver);
>> }
>> + thermal_cleanup();
>
> Should only be called when thermal_init() was successful.

I do not believe it is incorrect to skip checking for this.

platform_profile_remove() does not return anything and does not panic
when a profile is not currently registered. My understanding from
reading the source is it handles the case of no profile gracefully.
Please let me know if my understanding is incorrect however.

kfree does not care of the thermal handler is allocated or not. Please
let me know if calling kfree on NULL pointers is poor form for this
application.

Thank you for your feedback, I do appreciate it.

Lyndon

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

ilpo.jarvinen at linux

Apr 25, 2024, 11:57 PM

Post #12 of 15 (46 views)

Permalink

On Thu, 25 Apr 2024, Lyndon Sanche wrote:

> 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.

These are too short lines, wrap at 72 (or 75) characters.

> Signed-off-by: Lyndon Sanche <lsanche@lyndeno.ca>
> ---
> drivers/platform/x86/dell/dell-laptop.c | 220 ++++++++++++++++++++++++
> drivers/platform/x86/dell/dell-smbios.h | 1 +
> 2 files changed, 221 insertions(+)
>
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 42f7de2b4522..7f9c4e0e5ef5 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -27,6 +27,7 @@
> #include <linux/i8042.h>
> #include <linux/debugfs.h>
> #include <linux/seq_file.h>
> +#include <linux/platform_profile.h>
> #include <acpi/video.h>
> #include "dell-rbtn.h"
> #include "dell-smbios.h"
> @@ -95,10 +96,18 @@ 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;
>
> +enum thermal_mode_bits {
> + DELL_BALANCED = 0,
> + DELL_COOL_BOTTOM = 1,
> + DELL_QUIET = 2,
> + DELL_PERFORMANCE = 3,
> +};

It would seem more more natural to define these with BIT(x) as that's how
they're used in the code below?

> module_param(force_rfkill, bool, 0444);
> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
>
> @@ -2199,6 +2208,211 @@ 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)

Please use

/*
*
*/

format for multiline comments.

Don't exceed 80 characters with comments.

> +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) & 1)
> + return DELL_BALANCED;
> + else if ((state >> DELL_COOL_BOTTOM) & 1)
> + return DELL_COOL_BOTTOM;
> + else if ((state >> DELL_QUIET) & 1)
> + return DELL_QUIET;
> + else if ((state >> DELL_PERFORMANCE) & 1)
> + return DELL_PERFORMANCE;

When you convert defines to use BIT(), these become simpler.

> + else
> + return 0;
> +}
> +
> +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 = buffer.output[1] & 0xF;

Add named define + use FIELD_GET() + add #include <linux/bitfield.h> if
not there already.

> + 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 = ((buffer.output[3] >> 8) & 0xFF);

Use named define + FIELD_GET()

> + 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, (acc_mode << 8) | BIT(state), 0, 0);

Named define + FIELD_PREP(XX, acc_mode)

After converting the enum values to use BIT(), you can remove BIT() from
here.

> + 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)
> +{
> + int ret;
> +
> + switch (profile) {
> + case PLATFORM_PROFILE_BALANCED:
> + ret = thermal_set_mode(DELL_BALANCED);
> + break;
> + case PLATFORM_PROFILE_PERFORMANCE:
> + ret = thermal_set_mode(DELL_PERFORMANCE);
> + break;
> + case PLATFORM_PROFILE_QUIET:
> + ret = thermal_set_mode(DELL_QUIET);
> + break;
> + case PLATFORM_PROFILE_COOL:
> + ret = thermal_set_mode(DELL_COOL_BOTTOM);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return ret;
> +}
> +
> +static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
> + enum platform_profile_option *profile)
> +{
> + switch (thermal_get_mode()) {
> + 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;
> +}
> +
> +int thermal_init(void)
> +{
> + int ret;
> + int supported_modes;
> +
> + ret = thermal_get_supported_modes(&supported_modes);
> +
> + if (ret != 0 || supported_modes == 0)

Don't leave empty lines between call and its error handling.

> + return -ENXIO;
> +
> + 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) & 1)
> + set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
> + if ((supported_modes >> DELL_COOL_BOTTOM) & 1)
> + set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
> + if ((supported_modes >> DELL_BALANCED) & 1)
> + set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
> + if ((supported_modes >> DELL_PERFORMANCE) & 1)

These too will get simpler when the values are using BIT().

> + set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
> +
> + platform_profile_register(thermal_handler);
> +
> + return 0;
> +}
> +
> +void thermal_cleanup(void)
> +{
> + platform_profile_remove();

This should be called if thermal_init() failed, sysfs_remove_group() will
be called for a group that was never created and I don't think that's
okay.

> + kfree(thermal_handler);
> +}
> +
> static struct led_classdev mute_led_cdev = {
> .name = "platform::mute",
> .max_brightness = 1,
> @@ -2266,6 +2480,11 @@ static int __init dell_init(void)
> mute_led_registered = true;
> }
>
> + // Do not fail module if thermal modes not supported,
> + // just skip

Fits to one line.

> + if (thermal_init() != 0)
> + pr_warn("Unable to setup platform_profile, skipping");
> +
> if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> return 0;
>
> @@ -2344,6 +2563,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.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
>

--
i.

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

srinivas.pandruvada at linux

Apr 26, 2024, 9:14 AM

Post #13 of 15 (46 views)

Permalink

On Thu, 2024-04-25 at 14:24 -0600, Lyndon Sanche wrote:
> On Thu, Apr 25, 2024, at 2:07 PM, Mario Limonciello wrote:
> > + Srinivas
> >
> > On 4/25/2024 12:27, Lyndon Sanche wrote:
> > > 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 <lsanche@lyndeno.ca>
> > > ---
> >
> > When you developed this was it using a Dell Intel or Dell AMD
> > system?
> >
> > If it was an Intel system, did you test it with thermald installed
> > and
> > active?
> >
> > I'm wondering how all this stuff jives with the stuff that thermald
> > does. I don't know if they fight for any of the same "resources".
>
> Thank you for your response.
>
> I did my development and testing on a Dell Intel system. Specifically
> the XPS 15 9560 with i7-7700HQ.
>
> I do have thermald running, though I admit I am not really aware of
> what exactly it does, besides being related to thermals in some way.
>
> I normally set the thermal mode with Dell's smbios-thermal-ctl
> program. I am not too sure all the values that the bios configures on
> it's own depending on the provided mode, so I am not sure if thermald
> conflicts. But my understanding is that would be out of scope of this
> driver, since we are only telling the bios what we want at a high
> level.
>
> Lyndon
Can you share output of acpidump tool to me? I want to make sure if
there is some way the platform will bypass thermal table if you changed
to some profile.

Thanks,
Srinivas

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

lsanche at lyndeno

Apr 26, 2024, 11:23 AM

Post #14 of 15 (46 views)

Permalink

On Fri, Apr 26 2024 at 09:14:07 AM -07:00:00, srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>> Can you share output of acpidump tool to me? I want to make sure if
> there is some way the platform will bypass thermal table if you
> changed
> to some profile.
>
> Thanks,
> Srinivas

Hello Srinivas:

I used acpidump. For sake of completeness I ran acpidump with each of
the thermal modes enabled. The files are too large to provide here so I
uploaded them to a public gist:
https://gist.github.com/Lyndeno/65ade5a15f1f2cd07175256dc021f551

Please let me know if there is a more appropriate medium for sharing
files like this, and I will remember for next time.

Thank you,

Lyndon

Re: [PATCH] platform/x86: dell-laptop: Implement platform_profile [ In reply to ]

srinivas.pandruvada at linux

Apr 26, 2024, 11:24 AM

Post #15 of 15 (46 views)

Permalink

On Fri, 2024-04-26 at 12:23 -0600, Lyndon Sanche wrote:
>
>
> On Fri, Apr 26 2024 at 09:14:07 AM -07:00:00, srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > > Can you share output of acpidump tool to me? I want to make sure
> > > if
> > there is some way the platform will bypass thermal table if you
> > changed
> > to some profile.
> >
> > Thanks,
> > Srinivas
>
> Hello Srinivas:
>
> I used acpidump. For sake of completeness I ran acpidump with each of
> the thermal modes enabled. The files are too large to provide here so
> I
> uploaded them to a public gist:
> https://gist.github.com/Lyndeno/65ade5a15f1f2cd07175256dc021f551
>
> Please let me know if there is a more appropriate medium for sharing
> files like this, and I will remember for next time.

This is fine. Thanks for uploading.

-Srinivas

>
> Thank you,
>
> Lyndon
>
>