Message-Id: <D7U6T8P3M0Z8.2RBEJ9LYOYPU9@gmail.com>
Date: Sun, 16 Feb 2025 16:27:55 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "Armin Wolf" <W_Armin@....de>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>
Cc: <platform-driver-x86@...r.kernel.org>, "Hans de Goede"
 <hdegoede@...hat.com>, <Dell.Client.Kernel@...l.com>,
 <linux-kernel@...r.kernel.org>, "Guenter Roeck" <linux@...ck-us.net>
Subject: Re: [PATCH 07/10] platform/x86: alienware-wmi-wmax: Add HWMON
 support

On Sun Feb 16, 2025 at 1:06 AM -05, Armin Wolf wrote:
> Am 08.02.25 um 06:16 schrieb Kurt Borja:
>
>> All models with the "AWCC" WMAX device support monitoring fan speed and
>> temperature sensors. Expose this feature through the HWMON interface.
>>
>> Sensor readings are cached for 1 second before refreshing them to
>> mitigate the performance cost of calling WMI methods.
>
> Since each fan can be accessed independently, i do not think that the caching is actually
> needed here.
>
> The dell-wmi-ddv driver only uses caching because it can only read all fan sensors
> at the same time, which means each call is quite expensive. The AWCC interface however
> seems to allow access to each individual fan, so each call should be quite cheap (for
> a WMI method call).
>
> Do you know of a device which has such slow WMI calls? If not then i suggest you remove
> the caching.

I was under the impression that all WMI calls are a bit "expensive" in
the performance sense. Also the AWCC OEM windows app slows down your
computer a LOT, actually unusable while gaming and I figured this had
something to do with it.

I'm going to do some tests. If the performance cost is not noticeable I
will drop it.

-- 
 ~ Kurt

>
>> Cc: Guenter Roeck <linux@...ck-us.net>
>> Signed-off-by: Kurt Borja <kuurtb@...il.com>
>> ---
>>   drivers/platform/x86/dell/Kconfig             |   1 +
>>   .../platform/x86/dell/alienware-wmi-wmax.c    | 341 ++++++++++++++++++
>>   2 files changed, 342 insertions(+)
>>
>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>> index f8a0dffcaab7..85a57c01aaad 100644
>> --- a/drivers/platform/x86/dell/Kconfig
>> +++ b/drivers/platform/x86/dell/Kconfig
>> @@ -43,6 +43,7 @@ config ALIENWARE_WMI_WMAX
>>   	bool "Alienware WMAX WMI device driver"
>>   	default y
>>   	depends on ALIENWARE_WMI
>> +	depends on HWMON
>>   	select ACPI_PLATFORM_PROFILE
>>   	help
>>   	 Alienware WMI driver with AlienFX LED, HDMI, amplifier, deep sleep and
>> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> index 0d31156f43bb..5f02da7ff25f 100644
>> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> @@ -11,9 +11,13 @@
>>   #include <linux/bitfield.h>
>>   #include <linux/bits.h>
>>   #include <linux/dmi.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/jiffies.h>
>>   #include <linux/moduleparam.h>
>> +#include <linux/mutex.h>
>>   #include <linux/overflow.h>
>>   #include <linux/platform_profile.h>
>> +#include <linux/units.h>
>>   #include <linux/wmi.h>
>>   #include "alienware-wmi.h"
>>
>> @@ -26,6 +30,7 @@
>>   #define WMAX_METHOD_BRIGHTNESS			0x3
>>   #define WMAX_METHOD_ZONE_CONTROL		0x4
>>
>> +#define AWCC_METHOD_GET_FAN_SENSORS		0x13
>>   #define AWCC_METHOD_THERMAL_INFORMATION		0x14
>>   #define AWCC_METHOD_THERMAL_CONTROL		0x15
>>   #define AWCC_METHOD_GAME_SHIFT_STATUS		0x25
>> @@ -36,6 +41,10 @@
>>   #define AWCC_THERMAL_MODE_MASK			GENMASK(3, 0)
>>   #define AWCC_RESOURCE_ID_MASK			GENMASK(7, 0)
>>
>> +static bool force_hwmon;
>> +module_param_unsafe(force_hwmon, bool, 0);
>> +MODULE_PARM_DESC(force_hwmon, "Force probing for HWMON support without checking if the WMI backend is available");
>> +
>>   static bool force_platform_profile;
>>   module_param_unsafe(force_platform_profile, bool, 0);
>>   MODULE_PARM_DESC(force_platform_profile, "Forces auto-detecting thermal profiles without checking if WMI thermal backend is available");
>> @@ -45,16 +54,19 @@ module_param_unsafe(force_gmode, bool, 0);
>>   MODULE_PARM_DESC(force_gmode, "Forces G-Mode when performance profile is selected");
>>
>>   struct awcc_quirks {
>> +	bool hwmon;
>>   	bool pprof;
>>   	bool gmode;
>>   };
>>
>>   static struct awcc_quirks g_series_quirks = {
>> +	.hwmon = true,
>>   	.pprof = true,
>>   	.gmode = true,
>>   };
>>
>>   static struct awcc_quirks generic_quirks = {
>> +	.hwmon = true,
>>   	.pprof = true,
>>   	.gmode = false,
>>   };
>> @@ -152,9 +164,17 @@ static const struct dmi_system_id awcc_dmi_table[] __initconst = {
>>   	},
>>   };
>>
>> +enum AWCC_GET_FAN_SENSORS_OPERATIONS {
>> +	AWCC_OP_GET_TEMP_SENSOR_ID		= 0x02,
>> +};
>> +
>>   enum AWCC_THERMAL_INFORMATION_OPERATIONS {
>>   	AWCC_OP_GET_SYSTEM_DESCRIPTION		= 0x02,
>>   	AWCC_OP_GET_RESOURCE_ID			= 0x03,
>> +	AWCC_OP_GET_TEMPERATURE			= 0x04,
>> +	AWCC_OP_GET_CURRENT_RPM			= 0x05,
>> +	AWCC_OP_GET_MIN_RPM			= 0x08,
>> +	AWCC_OP_GET_MAX_RPM			= 0x09,
>>   	AWCC_OP_GET_CURRENT_PROFILE		= 0x0B,
>>   };
>>
>> @@ -177,6 +197,11 @@ enum AWCC_SPECIAL_THERMAL_CODES {
>>   	AWCC_SPECIAL_PROFILE_GMODE		= 0xAB,
>>   };
>>
>> +enum AWCC_TEMP_SENSOR_TYPES {
>> +	AWCC_TEMP_SENSOR_CPU			= 0x01,
>> +	AWCC_TEMP_SENSOR_GPU			= 0x06,
>> +};
>> +
>>   enum awcc_thermal_profile {
>>   	AWCC_PROFILE_USTT_BALANCED,
>>   	AWCC_PROFILE_USTT_BALANCED_PERFORMANCE,
>> @@ -213,6 +238,23 @@ struct wmax_u32_args {
>>   	u8 arg3;
>>   } __packed;
>>
>> +struct awcc_fan_channel_data {
>> +	u8 id;
>> +	u32 state;
>> +	u32 min_rpm;
>> +	u32 max_rpm;
>> +	u8 temp_sensor;
>> +	u64 timestamp;
>> +	struct mutex lock; /* protects state and timestamp */
>> +};
>> +
>> +struct awcc_temp_channel_data {
>> +	u8 id;
>> +	u32 state;
>> +	u64 timestamp;
>> +	struct mutex lock; /* protects state and timestamp */
>> +};
>> +
>>   struct awcc_priv {
>>   	struct wmi_device *wdev;
>>   	union {
>> @@ -228,6 +270,10 @@ struct awcc_priv {
>>
>>   	struct device *ppdev;
>>   	u8 supported_profiles[PLATFORM_PROFILE_LAST];
>> +
>> +	struct device *hwdev;
>> +	struct awcc_temp_channel_data *temp_data;
>> +	struct awcc_fan_channel_data *fan_data;
>>   };
>>
>>   static const enum platform_profile_option awcc_mode_to_platform_profile[AWCC_PROFILE_LAST] = {
>> @@ -492,6 +538,18 @@ static int __awcc_wmi_command(struct wmi_device *wdev, u32 method_id,
>>   	return 0;
>>   }
>>
>> +static inline int awcc_get_fan_sensors(struct wmi_device *wdev, u8 fan_id, u32 *out)
>> +{
>> +	struct wmax_u32_args args = {
>> +		.operation = AWCC_OP_GET_TEMP_SENSOR_ID,
>> +		.arg1 = fan_id,
>> +		.arg2 = 0,
>> +		.arg3 = 0,
>> +	};
>> +
>> +	return __awcc_wmi_command(wdev, AWCC_METHOD_GET_FAN_SENSORS, &args, out);
>> +}
>> +
>>   static inline int awcc_thermal_information(struct wmi_device *wdev, u8 operation,
>>   					   u8 arg, u32 *out)
>>   {
>> @@ -562,6 +620,276 @@ static inline int awcc_op_get_resource_id(struct wmi_device *wdev, u8 index, u32
>>   	return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out);
>>   }
>>
>> +/*
>> + * HWMON
>> + *  - Provides temperature and fan speed monitoring as well as manual fan
>> + *    control
>> + */
>> +static int awcc_hwmon_update_temp(struct wmi_device *wdev,
>> +				  struct awcc_temp_channel_data *data)
>> +{
>> +	u32 temp;
>> +	int ret;
>> +
>> +	lockdep_assert_held(data->lock);
>> +
>> +	if (time_is_after_jiffies64(data->timestamp + secs_to_jiffies(1)))
>> +		return 0;
>> +
>> +	ret = awcc_thermal_information(wdev, AWCC_OP_GET_TEMPERATURE, data->id,
>> +				       &temp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	data->state = temp * MILLIDEGREE_PER_DEGREE;
>> +	data->timestamp = get_jiffies_64();
>> +
>> +	return 0;
>> +}
>> +
>> +static int awcc_hwmon_update_fan(struct wmi_device *wdev,
>> +				 struct awcc_fan_channel_data *data)
>> +{
>> +	u32 rpm;
>> +	int ret;
>> +
>> +	lockdep_assert_held(data->lock);
>> +
>> +	if (time_is_after_jiffies64(data->timestamp + secs_to_jiffies(1)))
>> +		return 0;
>> +
>> +	ret = awcc_thermal_information(wdev, AWCC_OP_GET_CURRENT_RPM, data->id,
>> +				       &rpm);
>> +	if (ret)
>> +		return ret;
>> +
>> +	data->state = rpm;
>> +	data->timestamp = get_jiffies_64();
>> +
>> +	return 0;
>> +}
>> +
>> +static umode_t awcc_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>> +				     u32 attr, int channel)
>> +{
>> +	const struct awcc_priv *priv = drvdata;
>> +
>> +	switch (type) {
>> +	case hwmon_temp:
>> +		if (channel < priv->temp_count)
>> +			return 0444;
>> +
>> +		break;
>> +	case hwmon_fan:
>> +		if (channel < priv->fan_count)
>> +			return 0444;
>> +
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int awcc_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> +			   u32 attr, int channel, long *val)
>> +{
>> +	struct awcc_priv *priv = dev_get_drvdata(dev);
>> +	struct awcc_temp_channel_data *temp;
>> +	struct awcc_fan_channel_data *fan;
>> +	int ret;
>> +
>> +	switch (type) {
>> +	case hwmon_temp:
>> +		temp = &priv->temp_data[channel];
>> +
>> +		switch (attr) {
>> +		case hwmon_temp_input:
>> +			mutex_lock(&temp->lock);
>> +			ret = awcc_hwmon_update_temp(priv->wdev, temp);
>> +			mutex_unlock(&temp->lock);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = temp->state;
>> +			break;
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +
>> +		break;
>> +	case hwmon_fan:
>> +		fan = &priv->fan_data[channel];
>> +
>> +		switch (attr) {
>> +		case hwmon_fan_input:
>> +			mutex_lock(&fan->lock);
>> +			ret = awcc_hwmon_update_fan(priv->wdev, fan);
>> +			mutex_unlock(&fan->lock);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = fan->state;
>> +			break;
>> +		case hwmon_fan_min:
>> +			*val = fan->min_rpm;
>> +			break;
>> +		case hwmon_fan_max:
>> +			*val = fan->max_rpm;
>> +			break;
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int awcc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
>> +				  u32 attr, int channel, const char **str)
>> +{
>> +	struct awcc_priv *priv = dev_get_drvdata(dev);
>> +	u8 temp_id;
>> +
>> +	switch (type) {
>> +	case hwmon_temp:
>> +		temp_id = priv->temp_data[channel].id;
>> +
>> +		switch (temp_id) {
>> +		case AWCC_TEMP_SENSOR_CPU:
>> +			*str = "CPU";
>> +			break;
>> +		case AWCC_TEMP_SENSOR_GPU:
>> +			*str = "GPU";
>> +			break;
>> +		default:
>> +			*str = "Unknown";
>> +			break;
>> +		}
>> +
>> +		break;
>> +	case hwmon_fan:
>> +		temp_id = priv->fan_data[channel].temp_sensor;
>> +
>> +		switch (temp_id) {
>> +		case AWCC_TEMP_SENSOR_CPU:
>> +			*str = "Processor Fan";
>> +			break;
>> +		case AWCC_TEMP_SENSOR_GPU:
>> +			*str = "Video Fan";
>> +			break;
>> +		default:
>> +			*str = "Unknown Fan";
>> +			break;
>> +		}
>> +
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct hwmon_ops awcc_hwmon_ops = {
>> +	.is_visible = awcc_hwmon_is_visible,
>> +	.read = awcc_hwmon_read,
>> +	.read_string = awcc_hwmon_read_string,
>> +};
>> +
>> +static const struct hwmon_channel_info * const awcc_hwmon_info[] = {
>> +	HWMON_CHANNEL_INFO(temp,
>> +			   HWMON_T_LABEL | HWMON_T_INPUT,
>> +			   HWMON_T_LABEL | HWMON_T_INPUT,
>> +			   HWMON_T_LABEL | HWMON_T_INPUT
>> +			   ),
>> +	HWMON_CHANNEL_INFO(fan,
>> +			   HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX,
>> +			   HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX,
>> +			   HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX,
>> +			   HWMON_F_LABEL | HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX
>> +			   ),
>> +	NULL
>> +};
>> +
>> +static const struct hwmon_chip_info awcc_hwmon_chip_info = {
>> +	.ops = &awcc_hwmon_ops,
>> +	.info = awcc_hwmon_info,
>> +};
>> +
>> +static int awcc_hwmon_init(struct wmi_device *wdev)
>> +{
>> +	struct awcc_priv *priv = dev_get_drvdata(&wdev->dev);
>> +	u32 id, temp_sensor, min_rpm, max_rpm;
>> +	int ret;
>> +
>> +	priv->fan_data = devm_kcalloc(&wdev->dev, priv->fan_count,
>> +				      sizeof(*priv->fan_data), GFP_KERNEL);
>> +	if (!priv->fan_data)
>> +		return -ENOMEM;
>> +
>> +	priv->temp_data = devm_kcalloc(&wdev->dev, priv->temp_count,
>> +				       sizeof(*priv->temp_data), GFP_KERNEL);
>> +	if (!priv->temp_data)
>> +		return -ENOMEM;
>> +
>> +	for (u32 i = 0; i < priv->fan_count; i++) {
>> +		/*
>> +		 * Fan IDs are listed first at offset 0
>> +		 */
>> +		ret = awcc_op_get_resource_id(wdev, i, &id);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = awcc_thermal_information(wdev, AWCC_OP_GET_MIN_RPM, id,
>> +					       &min_rpm);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = awcc_thermal_information(wdev, AWCC_OP_GET_MAX_RPM, id,
>> +					       &max_rpm);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = awcc_get_fan_sensors(wdev, id, &temp_sensor);
>> +		if (ret)
>> +			return ret;
>> +
>> +		priv->fan_data[i].id = FIELD_GET(AWCC_RESOURCE_ID_MASK, id);
>> +		priv->fan_data[i].min_rpm = min_rpm;
>> +		priv->fan_data[i].max_rpm = max_rpm;
>> +		priv->fan_data[i].temp_sensor = temp_sensor;
>> +		ret = devm_mutex_init(&wdev->dev, &priv->fan_data[i].lock);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	for (u32 i = 0; i < priv->temp_count; i++) {
>> +		/*
>> +		 * Temperature sensors IDs are listed after the fan IDs at
>> +		 * offset `fan_count`
>> +		 */
>> +		ret = awcc_op_get_resource_id(wdev, i + priv->fan_count, &id);
>> +		if (ret)
>> +			return ret;
>> +
>> +		priv->temp_data[i].id = FIELD_GET(AWCC_RESOURCE_ID_MASK, id);
>> +		ret = devm_mutex_init(&wdev->dev, &priv->temp_data[i].lock);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	priv->hwdev = devm_hwmon_device_register_with_info(
>> +		&wdev->dev, "alienware_wmi", priv, &awcc_hwmon_chip_info, NULL);
>
> checkpatch says "CHECK: Lines should not end with a '('". Please fix this.
>
> Other than that the code looks promising.
>
> Thanks,
> Armin Wolf
>
>> +
>> +	return PTR_ERR_OR_ZERO(priv->hwdev);
>> +}
>> +
>>   /*
>>    * Thermal Profile control
>>    *  - Provides thermal profile control through the Platform Profile API
>> @@ -734,6 +1062,12 @@ static int alienware_awcc_setup(struct wmi_device *wdev)
>>   	priv->wdev = wdev;
>>   	dev_set_drvdata(&wdev->dev, priv);
>>
>> +	if (awcc->hwmon) {
>> +		ret = awcc_hwmon_init(wdev);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	if (awcc->pprof) {
>>   		ret = awcc_platform_profile_init(wdev);
>>   		if (ret)
>> @@ -814,6 +1148,13 @@ int __init alienware_wmax_wmi_init(void)
>>   	if (id)
>>   		awcc = id->driver_data;
>>
>> +	if (force_hwmon) {
>> +		if (!awcc)
>> +			awcc = &empty_quirks;
>> +
>> +		awcc->hwmon = true;
>> +	}
>> +
>>   	if (force_platform_profile) {
>>   		if (!awcc)
>>   			awcc = &empty_quirks;

Powered by blists - more mailing lists