| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <B3AA4333-03DC-47D6-9519-7FA9496220E5@lyndeno.ca>
Date: Sat, 11 May 2024 09:59:17 -0600
From: Lyndon Sanche <lsanche@...deno.ca>
To: "Limonciello, Mario" <mario.limonciello@....com>
CC: pali@...nel.org, W_Armin@....de, srinivas.pandruvada@...ux.intel.com,
ilpo.jarvinen@...ux.intel.com, lkp@...el.com, hdegoede@...hat.com,
Yijun.Shen@...l.com, Matthew Garrett <mjg59@...f.ucam.org>,
Heiner Kallweit <hkallweit1@...il.com>, Randy Dunlap <rdunlap@...radead.org>,
Jonathan Corbet <corbet@....net>, Vegard Nossum <vegard.nossum@...cle.com>,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
Dell.Client.Kernel@...l.com
Subject: Re: [PATCH v6 2/2] platform/x86: dell-laptop: Implement platform_profile
On May 11, 2024 9:16:56 a.m. MDT, "Limonciello, Mario" <mario.limonciello@....com> wrote:
>
>
>On 5/10/2024 9:36 PM, 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@...deno.ca>
>> ---
>> drivers/platform/x86/dell/Kconfig | 2 +
>> drivers/platform/x86/dell/dell-laptop.c | 242 +++++++++++++++++++
>> drivers/platform/x86/dell/dell-smbios-base.c | 1 +
>> drivers/platform/x86/dell/dell-smbios.h | 1 +
>> 4 files changed, 246 insertions(+)
>>
>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>> index bd9f445974cc..26679f22733c 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
>> @@ -57,6 +58,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..07f54f1cbac5 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,236 @@ 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 commands not supported, exit without error */
>> + if (!dell_laptop_check_supported_cmds(CLASS_INFO))
>> + return 0;
>> +
>> + /* 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 +2472,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 +2556,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 +2585,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 6ae09d7f76fb..387fa5618f7a 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},
>> };
>
>So when Alex checked on v5 that this doesn't load on workstations, it has made me realize that doing this will block the interface totally even on workstations.
>
>So I think there are a few ways to go to handle this:
>
>1) Rename dell-laptop to dell-client or dell-pc and let dell-laptop load on more form factors. This would require some internal handling in the module for which features make sense for different form factors.
>
>2) Add a new module just for the thermal handling and put all this code into it instead.
>
>I don't have a strong opinion, but I do think one of them should be done to ensure there aren't problems on workstations losing access to thermal control.
>
A dell-client/laptop separation makes more sense IMO. I don't think keyboard control would belong in a the dell-client module either. Separating just thermal control would be easier, but not as clean I think.
Thanks,
Lyndon
Powered by blists - more mailing lists