| 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: <aWUvvzDCtVCOaBwq@archlinux>
Date: Mon, 12 Jan 2026 23:33:52 +0530
From: Krishna Chomal <krishna.chomal108@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Hans de Goede <hansg@...nel.org>, linux@...ck-us.net,
platform-driver-x86@...r.kernel.org, linux-hwmon@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] platform/x86: hp-wmi: add manual fan control for
Victus S models
On Mon, Jan 12, 2026 at 05:13:05PM +0200, Ilpo Järvinen wrote:
>On Tue, 30 Dec 2025, Krishna Chomal wrote:
>
[snip]
>> #include <linux/string.h>
>> #include <linux/dmi.h>
>> +#include <linux/fixp-arith.h>
>> +#include <linux/limits.h>
>> +#include <linux/minmax.h>
>> +#include <linux/compiler_attributes.h>
>>
[snip]
>> +
>> +struct victus_s_fan_table_header {
>> + u8 unknown;
>> + u8 num_entries;
>> +} __packed;
>
>Please add #include for __packed.
>
__packed is defined in compiler_attributes.h, which is included in this
patch. Please let me know if there are any other headers that should be
included.
>> +struct victus_s_fan_table_entry {
>> + u8 cpu_rpm;
>> + u8 gpu_rpm;
>> + u8 unknown;
>> +} __packed;
>> +
>> +struct victus_s_fan_table {
>> + struct victus_s_fan_table_header header;
>> + struct victus_s_fan_table_entry entries[];
>> +} __packed;
>> +
>> +static inline u8 rpm_to_pwm(u8 rpm, struct hp_wmi_hwmon_priv *priv)
>> +{
>> + return fixp_linear_interpolate(0, 0, priv->max_rpm, U8_MAX,
>> + clamp_val(rpm, 0, priv->max_rpm));
>
>Please align the correctly.
>
Apologies for the bad alignment of multi-line function calls and
if-statements in this patch. I have noted them and will fix them all in
v3.
[snip]
>> -static int hp_wmi_fan_speed_max_reset(void)
>> -{
>> - int ret;
>> + /*
>> + * GPU fan speed is always a little higher than CPU fan speed, we fetch
>> + * this delta value from the fan table during hwmon init.
>> + * Exception: Speed is set to HP_FAN_SPEED_AUTOMATIC, to revert to
>> + * automatic mode.
>> + */
>> + if (speed != HP_FAN_SPEED_AUTOMATIC)
>> + fan_speed[GPU_FAN] = clamp_val((unsigned int)speed +
>> + (unsigned int)priv->gpu_delta,
>> + 0, U8_MAX);
>
>Add braces is it's multiline if.
>
>If you use unsigned int, clamp to 0 makes no sense as those values have
>already underflowed.
>
>You also have an alignment problem here, but this seems a cleaner way
>which doesn't have underflow issues:
>
> if (...) {
> int new_speed = speed + priv->gpu_delta;
>
> fan_speed[GPU_FAN] = clamp_val(new_speed, 0, U8_MAX);
> }
>
Understood. I will introduce a new "gpu_speed" variable and follow this
style in v3.
[snip]
>> @@ -2147,16 +2244,21 @@ static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> *val = ret;
>> return 0;
>> case hwmon_pwm:
>> - switch (hp_wmi_fan_speed_max_get()) {
>> - case 0:
>> - /* 0 is automatic fan, which is 2 for hwmon */
>> - *val = 2;
>> + if (attr == hwmon_pwm_input) {
>> + if (!is_victus_s_thermal_profile())
>> + return -EOPNOTSUPP;
>
>Add an empty line here.
>
Will do.
>> + ret = hp_wmi_get_fan_speed_victus_s(channel);
>> + if (ret < 0)
>> + return ret;
>> + current_rpm = ret;
>
>I'm not sure if using ret here makes things better or not, I'd prefer just
>assigning directly to current_rpm without ret as intermediate var.
>
Understood. I will directly use current_rpm then.
[snip]
>> static int hp_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
>> u32 attr, int channel, long val)
>> {
>> + struct hp_wmi_hwmon_priv *priv;
>> + int current_rpm, ret;
>> +
>> + priv = dev_get_drvdata(dev);
>> switch (type) {
>> case hwmon_pwm:
>> + if (attr == hwmon_pwm_input) {
>> + if (!is_victus_s_thermal_profile())
>> + return -EOPNOTSUPP;
>> + /* pwm input is invalid when not in manual mode */
>
>PWM (capitalize textual/comment "pwm"s correctly please).
>
Corrected.
>> + if (priv->mode != PWM_MODE_MANUAL)
>> + return -EINVAL;
>
>ADd empty line here.
>
Added.
>> + /* ensure pwm input is within valid fan speeds */
>
>PWM
>
Fixed.
>> + priv->pwm = rpm_to_pwm(clamp_val(pwm_to_rpm(val, priv),
>> + priv->min_rpm,
>> + priv->max_rpm),
>
>These look misaligned.
>
>I suggest you split this to multiple lines though, it will likely be
>easier to read that way.
>
Agreed, this is too much nesting. I will split it into separate statements
for each function call.
>> + priv);
>> + return hp_wmi_apply_fan_settings(priv);
>> + }
>> switch (val) {
>> - case 0:
>> - if (is_victus_s_thermal_profile())
>> - hp_wmi_get_fan_count_userdefine_trigger();
>> - /* 0 is no fan speed control (max), which is 1 for us */
>> - return hp_wmi_fan_speed_max_set(1);
>> - case 2:
>> - /* 2 is automatic speed control, which is 0 for us */
>> - if (is_victus_s_thermal_profile()) {
>> - hp_wmi_get_fan_count_userdefine_trigger();
>> - return hp_wmi_fan_speed_max_reset();
>> - } else
>> - return hp_wmi_fan_speed_max_set(0);
>> + case PWM_MODE_MAX:
>> + priv->mode = PWM_MODE_MAX;
>> + return hp_wmi_apply_fan_settings(priv);
>> + case PWM_MODE_MANUAL:
>> + if (!is_victus_s_thermal_profile())
>> + return -EOPNOTSUPP;
>> + /*
>> + * When switching to manual mode, set fan speed to
>> + * current RPM values to ensure a smooth transition.
>> + */
>> + ret = hp_wmi_get_fan_speed_victus_s(channel);
>
>Assign directly to current_rpm ?
>
Yes, will do in v3.
>> + if (ret < 0)
>> + return ret;
>> + current_rpm = ret;
>> + priv->pwm = rpm_to_pwm(current_rpm / 100, priv);
>> + priv->mode = PWM_MODE_MANUAL;
>> + return hp_wmi_apply_fan_settings(priv);
>> + case PWM_MODE_AUTO:
>> + priv->mode = PWM_MODE_AUTO;
>> + return hp_wmi_apply_fan_settings(priv);
>> default:
>> - /* we don't support manual fan speed control */
>> return -EINVAL;
>> }
>> default:
>> @@ -2196,7 +2322,7 @@ static int hp_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
>>
>> static const struct hwmon_channel_info * const info[] = {
>> HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT),
>> - HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE),
>> + HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE | HWMON_PWM_INPUT),
>> NULL
>> };
>>
>> @@ -2211,12 +2337,57 @@ static const struct hwmon_chip_info chip_info = {
>> .info = info,
>> };
>>
>> +static int hp_wmi_setup_fan_settings(struct hp_wmi_hwmon_priv *priv)
>> +{
>> + u8 fan_data[128] = { 0 };
>> + struct victus_s_fan_table *fan_table;
>> + u8 min_rpm, max_rpm, gpu_delta;
>> + int ret;
>> +
>> + /* Default behaviour on hwmon init is automatic mode */
>> + priv->mode = PWM_MODE_AUTO;
>> +
>> + /* Bypass all non-Victus S devices */
>> + if (!is_victus_s_thermal_profile())
>> + return 0;
>> +
>> + ret = hp_wmi_perform_query(HPWMI_VICTUS_S_GET_FAN_TABLE_QUERY,
>> + HPWMI_GM, &fan_data, 4, sizeof(fan_data));
>> + if (ret)
>> + return ret;
>> +
>> + fan_table = (struct victus_s_fan_table *)fan_data;
>> + if (fan_table->header.num_entries == 0 ||
>> + sizeof(struct victus_s_fan_table_header) +
>> + sizeof(struct victus_s_fan_table_entry) * fan_table->header.num_entries >
>> + sizeof(fan_data))
>
>Badly misaligned.
>
>Splitting at > is somewhat misleading so I'd prefer to avoid it (you can
>use up to 100 chars per line if needed).
>
Ok, will fix the alignment and ensure to keep the > in a single line.
Powered by blists - more mailing lists