1. Patchew
  2. linux
  3. View series

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

Lyndon Sanche posted 1 patch 1 year, 7 months ago
There is a newer version of this series
drivers/platform/x86/dell/dell-laptop.c | 232 ++++++++++++++++++++++++
drivers/platform/x86/dell/dell-smbios.h |   1 +
2 files changed, 233 insertions(+)
[PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by Lyndon Sanche 1 year, 7 months ago 
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>
 #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
+	if (platform_profile_register(thermal_handler))
+		kfree(thermal_handler);
+
+	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
+	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
 
 /* Tokens used in kernel drivers, any of these
  * should be filtered from userspace access
-- 
2.42.0
Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by kernel test robot 1 year, 7 months ago 
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-20240501]
[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/20240430-135932
base:   linus/master
patch link:    https://lore.kernel.org/r/20240429164844.7544-2-lsanche%40lyndeno.ca
patch subject: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
config: x86_64-randconfig-r122-20240501 (https://download.01.org/0day-ci/archive/20240502/202405020009.iVV1DXVS-lkp@intel.com/config)
compiler: clang version 18.1.4 (https://github.com/llvm/llvm-project e6c3289804a67ea0bb6a86fadbe454dd93b8d855)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240502/202405020009.iVV1DXVS-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405020009.iVV1DXVS-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/platform/x86/dell/dell-laptop.c:2418:6: sparse: sparse: symbol 'thermal_cleanup' was not declared. Should it be static?

vim +/thermal_cleanup +2418 drivers/platform/x86/dell/dell-laptop.c

  2386	
> 2387	int thermal_init(void)
  2388	{
  2389		int ret;
  2390		int supported_modes;
  2391	
  2392		ret = thermal_get_supported_modes(&supported_modes);
  2393		if (ret || !supported_modes)
  2394			return 0;
  2395	
  2396		thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
  2397		if (!thermal_handler)
  2398			return -ENOMEM;
  2399		thermal_handler->profile_get = thermal_platform_profile_get;
  2400		thermal_handler->profile_set = thermal_platform_profile_set;
  2401	
  2402		if (supported_modes & DELL_QUIET)
  2403			set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
  2404		if (supported_modes & DELL_COOL_BOTTOM)
  2405			set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
  2406		if (supported_modes & DELL_BALANCED)
  2407			set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
  2408		if (supported_modes & DELL_PERFORMANCE)
  2409			set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
  2410	
  2411		// Clean up but do not fail
  2412		if (platform_profile_register(thermal_handler))
  2413			kfree(thermal_handler);
  2414	
  2415		return 0;
  2416	}
  2417	
> 2418	void thermal_cleanup(void)
  2419	{
  2420		if (thermal_handler) {
  2421			platform_profile_remove();
  2422			kfree(thermal_handler);
  2423		}
  2424	}
  2425	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by kernel test robot 1 year, 7 months ago 
Hi Lyndon,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.9-rc6 next-20240501]
[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/20240430-135932
base:   linus/master
patch link:    https://lore.kernel.org/r/20240429164844.7544-2-lsanche%40lyndeno.ca
patch subject: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
config: x86_64-buildonly-randconfig-001-20240501 (https://download.01.org/0day-ci/archive/20240502/202405020004.y49VOvGS-lkp@intel.com/config)
compiler: gcc-9 (Ubuntu 9.5.0-4ubuntu2) 9.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240502/202405020004.y49VOvGS-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405020004.y49VOvGS-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/tests/drm_probe_helper_test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/tests/drm_rect_test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/gud/gud.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/udl/udl.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-kunit.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-ram.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-raw-ram.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-spmi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mfd/pcf50633-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dax/dax.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/firewire/firewire-uapi-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/misc/isight_firmware.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/mon/usbmon.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/chipidea/ci_hdrc_msm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/libcomposite.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_acm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_ss_lb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/u_serial.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_serial.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/u_ether.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_ncm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_ecm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_mass_storage.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_printer.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_tcm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/legacy/g_zero.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/input/touchscreen/cyttsp_i2c_common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/input/tests/input_test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/rtc/lib_test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/rtc/rtc-goldfish.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/i2c/busses/i2c-ali1563.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/corsair-cpro.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/watchdog/menz69_wdt.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/mmc_core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/sdio_uart.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-a4tech.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-apple.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-aureal.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-betopff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-bigbenff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-cherry.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-chicony.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-dr.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-emsff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-elecom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-elo.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-holtek-kbd.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-holtek-mouse.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-ite.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-keytouch.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-lcpower.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-letsketch.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-logitech.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-lg-g15.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-magicmouse.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-maltron.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-mf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-megaworld.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-microsoft.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-ntrig.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-ortek.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-pl.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-razer.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-redragon.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-retrode.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-saitek.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-semitek.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-sony.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-speedlink.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-steam.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-steelseries.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-sunplus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-gaff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-tmff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-tivo.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-twinhan.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-uclogic.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-zydacron.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-waltop.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/greybus/gb-es2.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-bootrom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-hid.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-loopback.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-power-supply.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-gbphy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-usb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/platform/x86/intel/speed_select_if/isst_if_common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/platform/goldfish/goldfish_pipe.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_powersave.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwtracing/intel_th/intel_th_msu_sink.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/siox/siox-bus-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_cif.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_netx.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dca/dca.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/configfs/configfs_sample.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kprobes/kprobe_example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kprobes/kretprobe_example.o
>> ERROR: modpost: "platform_profile_register" [drivers/platform/x86/dell/dell-laptop.ko] undefined!
>> ERROR: modpost: "platform_profile_remove" [drivers/platform/x86/dell/dell-laptop.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by kernel test robot 1 year, 7 months ago 
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-20240430]
[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/20240430-135932
base:   linus/master
patch link:    https://lore.kernel.org/r/20240429164844.7544-2-lsanche%40lyndeno.ca
patch subject: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
config: i386-buildonly-randconfig-001-20240501 (https://download.01.org/0day-ci/archive/20240501/202405011636.CDJVLh7l-lkp@intel.com/config)
compiler: clang version 18.1.4 (https://github.com/llvm/llvm-project e6c3289804a67ea0bb6a86fadbe454dd93b8d855)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240501/202405011636.CDJVLh7l-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405011636.CDJVLh7l-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/dell/dell-laptop.c:2387:5: warning: no previous prototype for function 'thermal_init' [-Wmissing-prototypes]
    2387 | int thermal_init(void)
         |     ^
   drivers/platform/x86/dell/dell-laptop.c:2387:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
    2387 | int thermal_init(void)
         | ^
         | static 
>> drivers/platform/x86/dell/dell-laptop.c:2418:6: warning: no previous prototype for function 'thermal_cleanup' [-Wmissing-prototypes]
    2418 | void thermal_cleanup(void)
         |      ^
   drivers/platform/x86/dell/dell-laptop.c:2418:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
    2418 | void thermal_cleanup(void)
         | ^
         | static 
   2 warnings generated.


vim +/thermal_init +2387 drivers/platform/x86/dell/dell-laptop.c

  2386	
> 2387	int thermal_init(void)
  2388	{
  2389		int ret;
  2390		int supported_modes;
  2391	
  2392		ret = thermal_get_supported_modes(&supported_modes);
  2393		if (ret || !supported_modes)
  2394			return 0;
  2395	
  2396		thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
  2397		if (!thermal_handler)
  2398			return -ENOMEM;
  2399		thermal_handler->profile_get = thermal_platform_profile_get;
  2400		thermal_handler->profile_set = thermal_platform_profile_set;
  2401	
  2402		if (supported_modes & DELL_QUIET)
  2403			set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
  2404		if (supported_modes & DELL_COOL_BOTTOM)
  2405			set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
  2406		if (supported_modes & DELL_BALANCED)
  2407			set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
  2408		if (supported_modes & DELL_PERFORMANCE)
  2409			set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
  2410	
  2411		// Clean up but do not fail
  2412		if (platform_profile_register(thermal_handler))
  2413			kfree(thermal_handler);
  2414	
  2415		return 0;
  2416	}
  2417	
> 2418	void thermal_cleanup(void)
  2419	{
  2420		if (thermal_handler) {
  2421			platform_profile_remove();
  2422			kfree(thermal_handler);
  2423		}
  2424	}
  2425	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by kernel test robot 1 year, 7 months ago 
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-20240430]
[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/20240430-135932
base:   linus/master
patch link:    https://lore.kernel.org/r/20240429164844.7544-2-lsanche%40lyndeno.ca
patch subject: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20240430/202404302351.31IWk45T-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240430/202404302351.31IWk45T-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404302351.31IWk45T-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/dell/dell-laptop.c:2387:5: warning: no previous prototype for 'thermal_init' [-Wmissing-prototypes]
    2387 | int thermal_init(void)
         |     ^~~~~~~~~~~~
>> drivers/platform/x86/dell/dell-laptop.c:2418:6: warning: no previous prototype for 'thermal_cleanup' [-Wmissing-prototypes]
    2418 | void thermal_cleanup(void)
         |      ^~~~~~~~~~~~~~~


vim +/thermal_init +2387 drivers/platform/x86/dell/dell-laptop.c

  2386	
> 2387	int thermal_init(void)
  2388	{
  2389		int ret;
  2390		int supported_modes;
  2391	
  2392		ret = thermal_get_supported_modes(&supported_modes);
  2393		if (ret || !supported_modes)
  2394			return 0;
  2395	
  2396		thermal_handler = kzalloc(sizeof(*thermal_handler), GFP_KERNEL);
  2397		if (!thermal_handler)
  2398			return -ENOMEM;
  2399		thermal_handler->profile_get = thermal_platform_profile_get;
  2400		thermal_handler->profile_set = thermal_platform_profile_set;
  2401	
  2402		if (supported_modes & DELL_QUIET)
  2403			set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
  2404		if (supported_modes & DELL_COOL_BOTTOM)
  2405			set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
  2406		if (supported_modes & DELL_BALANCED)
  2407			set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
  2408		if (supported_modes & DELL_PERFORMANCE)
  2409			set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
  2410	
  2411		// Clean up but do not fail
  2412		if (platform_profile_register(thermal_handler))
  2413			kfree(thermal_handler);
  2414	
  2415		return 0;
  2416	}
  2417	
> 2418	void thermal_cleanup(void)
  2419	{
  2420		if (thermal_handler) {
  2421			platform_profile_remove();
  2422			kfree(thermal_handler);
  2423		}
  2424	}
  2425	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by Ilpo Järvinen 1 year, 7 months ago 
On Mon, 29 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.
> 
> 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>
>  #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)

Please align these with tabs.

> +enum thermal_mode_bits {
> +	DELL_BALANCED = BIT(0),
> +	DELL_COOL_BOTTOM = BIT(1),
> +	DELL_QUIET = BIT(2),
> +	DELL_PERFORMANCE = BIT(3),

You need #include <linux/bits.h> for BIT().

> +};
> +
> +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)

I think I already mentioned about this, change to:

	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;
> +}
> +
> +int thermal_init(void)
> +{
> +	int ret;
> +	int supported_modes;
> +
> +	ret = thermal_get_supported_modes(&supported_modes);
> +	if (ret || !supported_modes)
> +		return 0;

I think you should propagate the error code differently from nothing 
supported:

	if (ret < 0)
		return ret;
	if (!supported_modes)
		return 0;

-- 
 i.

> +
> +	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
> +	if (platform_profile_register(thermal_handler))
> +		kfree(thermal_handler);
> +
> +	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
> +	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
>  
>  /* Tokens used in kernel drivers, any of these
>   * should be filtered from userspace access
>
Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by Lyndon Sanche 1 year, 7 months ago 
On Tue, Apr 30, 2024, at 4:31 AM, Ilpo Järvinen wrote:
> On Mon, 29 Apr 2024, Lyndon Sanche wrote:
>> + */
>> +
>> +#define DELL_ACC_GET_FIELD GENMASK(19, 16)
>> +#define DELL_ACC_SET_FIELD GENMASK(11, 8)
>> +#define DELL_THERMAL_SUPPORTED GENMASK(3, 0)
>
> Please align these with tabs.
>

Agreed.

>> +enum thermal_mode_bits {
>> +	DELL_BALANCED = BIT(0),
>> +	DELL_COOL_BOTTOM = BIT(1),
>> +	DELL_QUIET = BIT(2),
>> +	DELL_PERFORMANCE = BIT(3),
>
> You need #include <linux/bits.h> for BIT().
>

Agreed.

>> +	}
>> +}
>> +
>> +static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
>> +					enum platform_profile_option *profile)
>> +{
>> +	int ret = thermal_get_mode();
>> +
>> +	if (ret < 0)
>
> I think I already mentioned about this, change to:
>
> 	int ret;
>
> 	ret = thermal_get_mode();
> 	if (ret < 0)
>

I missed this.

>> +		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;
>
> I think you should propagate the error code differently from nothing 
> supported:
>
> 	if (ret < 0)
> 		return ret;
> 	if (!supported_modes)
> 		return 0;
>

Agreed.

Thank you for your feedback.

Lyndon
Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by Mario Limonciello 1 year, 7 months ago 
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
>   
>   /* Tokens used in kernel drivers, any of these
>    * should be filtered from userspace access
Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by Lyndon Sanche 1 year, 7 months ago 

On Mon, Apr 29 2024 at 12:45:19 PM -05:00:00, Mario Limonciello 
<mario.limonciello@amd.com> wrote:
> On 4/29/2024 11:48, Lyndon Sanche wrote:
>>   #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.

Agree

>> +
>> +	// Clean up but do not fail
> 
> Switch comment style to /* */

Agree

> 
>> +	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;

Good idea, I will propogate this error. Failure of module will then 
occur on allocation or platform_profile error.

> 
> 
>> 
>>   		goto fail_rfkill;
>>   	}
>>   +	// Do not fail module if thermal modes not supported, just skip
> 
> Switch comment style to /* */

Agree.

Thank you for this feedback.

Lyndon
>
Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by Mario Limonciello 1 year, 7 months ago 
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
>
Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
Posted by Lyndon Sanche 1 year, 7 months ago 

On Mon, Apr 29 2024 at 12:51:31 PM -05:00:00, Mario Limonciello 
<mario.limonciello@amd.com> wrote:
> On 4/29/2024 12:45, Mario Limonciello wrote:
>> On 4/29/2024 11:48, Lyndon Sanche wrote:
>>>   #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.

Good idea, I have been using smbios-thermal-ctl for checking the state 
when testing. I will include this in the patch, then smbios-thermal-ctl 
cannot interfere.

Thanks,

Lyndon

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.