Messages in this thread | | | Date | Mon, 29 Apr 2024 12:51:31 -0500 | Subject | Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile | From | Mario Limonciello <> |
| |
On 4/29/2024 12:45, Mario Limonciello wrote: > On 4/29/2024 11:48, 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> >> --- >> 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/dell-laptop.c | 232 ++++++++++++++++++++++++ >> drivers/platform/x86/dell/dell-smbios.h | 1 + >> 2 files changed, 233 insertions(+) >> >> diff --git a/drivers/platform/x86/dell/dell-laptop.c >> b/drivers/platform/x86/dell/dell-laptop.c >> index 42f7de2b4522..fa58e7751d06 100644 >> --- a/drivers/platform/x86/dell/dell-laptop.c >> +++ b/drivers/platform/x86/dell/dell-laptop.c >> @@ -27,6 +27,8 @@ >> #include <linux/i8042.h> >> #include <linux/debugfs.h> >> #include <linux/seq_file.h> >> +#include <linux/platform_profile.h> >> +#include <linux/bitfield.h> > > These should be inserted in alphabetical order. > >> #include <acpi/video.h> >> #include "dell-rbtn.h" >> #include "dell-smbios.h" >> @@ -95,6 +97,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 +2202,227 @@ 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 = 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; >> +} >> + >> +int thermal_init(void) >> +{ >> + int ret; >> + int supported_modes; >> + >> + ret = thermal_get_supported_modes(&supported_modes); >> + if (ret || !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 but do not fail > > Switch comment style to /* */ > >> + if (platform_profile_register(thermal_handler)) >> + kfree(thermal_handler); > > Don't you also want to return an error in this case? Because this means > that the platform supports thermal modes but it failed to setup due to > other issues. > > It's different than the case of no supported modes in which case > returning 0 makes sense. > > Maybe like this: > > > ret = platform_profile_register(thermal_handler); > if (ret) > kfree(thermal_handler); > > return ret; > > >> + >> + return 0; >> +} >> + >> +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 +2462,11 @@ static int __init dell_init(void) >> goto fail_rfkill; >> } >> + // Do not fail module if thermal modes not supported, just skip > > Switch comment style to /* */ > >> + ret = thermal_init(); >> + if (ret) >> + goto fail_thermal; >> + >> if (quirks && quirks->touchpad_led) >> touchpad_led_init(&platform_device->dev); >> @@ -2317,6 +2546,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 +2575,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
I think you should insert this into dell-smbios-base.c under call_blacklist. This will prevent userspace from fighting with the kernel on the same data when this code goes in.
>> /* Tokens used in kernel drivers, any of these >> * should be filtered from userspace access >
| |