Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Convert kernel hwmon interface to use PWM (0-255) intsead of RPM #261

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Convert kernel hwmon interface to use PWM (0-255) intsead of RPM #261

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
241c023
fix: kernel module provides pwm hwmon interface (instead of rpm)
Sep 22, 2024
b44ee8f
chore: Use PWM kernel interface in Ptyhon but keep RPM for user anf file
Sep 22, 2023
4ef6609
kernel: fix legion wmi fan curves
antheas Mar 21, 2024
fb7db66
fix: Output 0 if point id not in curve
Oct 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 133 additions & 58 deletions kernel_module/legion-laptop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1455,7 +1455,7 @@ static int acpi_process_buffer_to_ints(const char *id_name, int id_nr,
//}

static int wmi_exec_ints(const char *guid, u8 instance, u32 method_id,
const struct acpi_buffer *params, u8 *res,
const struct acpi_buffer *params, void *res,
size_t ressize)
{
acpi_status status;
Expand Down Expand Up @@ -1518,7 +1518,7 @@ static int wmi_exec_noarg_int(const char *guid, u8 instance, u32 method_id,
}

static int wmi_exec_noarg_ints(const char *guid, u8 instance, u32 method_id,
u8 *res, size_t ressize)
void *res, size_t ressize)
{
struct acpi_buffer params;

Expand Down Expand Up @@ -1712,6 +1712,12 @@ enum OtherMethodFeature {
OtherMethodFeature_C_U1 = 0x05010000,
OtherMethodFeature_TEMP_CPU = 0x05040000,
OtherMethodFeature_TEMP_GPU = 0x05050000,
OtherMethodFeature_FAN_FULLSPEED = 0x04020000,
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the source for that? Is there already a function that uses it that is not in this commit? Maybe you already have info how to use it. It is currently not used in this file.

};

struct other_method_val {
u32 param;
u32 val;
};

static ssize_t wmi_other_method_get_value(enum OtherMethodFeature feature_id,
Expand All @@ -1731,6 +1737,20 @@ static ssize_t wmi_other_method_get_value(enum OtherMethodFeature feature_id,
return error;
}

static ssize_t wmi_other_method_set_value(enum OtherMethodFeature feature_id,
int value)
{
int error;
struct other_method_val param = {
.param=feature_id,
.val=value
};

error = wmi_exec_arg(LEGION_WMI_LENOVO_OTHER_METHOD_GUID, 0,
WMI_METHOD_ID_SET_FEATURE_VALUE, &param, sizeof(param));
return error;
}

/* =================================== */
/* EC RAM Access with memory mapped IO */
/* =================================== */
Expand Down Expand Up @@ -2828,10 +2848,23 @@ struct WMIFanTable {
u16 FSS7;
u16 FSS8;
u16 FSS9;
u8 FSSNULL;
u32 FTLE;
u16 FST0;
u16 FST1;
u16 FST2;
u16 FST3;
u16 FST4;
u16 FST5;
u16 FST6;
u16 FST7;
u16 FST8;
u16 FST9;
u8 FSTNULL;
} __packed;

struct WMIFanTableRead {
u32 FSFL;
u32 FSTL;
u32 FSS0;
u32 FSS1;
u32 FSS2;
Expand All @@ -2842,84 +2875,93 @@ struct WMIFanTableRead {
u32 FSS7;
u32 FSS8;
u32 FSS9;
u32 FSSA;
// Ignored for now
u32 FTLE;
u32 FST0;
u32 FST1;
u32 FST2;
u32 FST3;
u32 FST4;
u32 FST5;
u32 FST6;
u32 FST7;
u32 FST8;
u32 FST9;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is FTLE? Length?
FSTL: is this field renamed in Legion Go?
FST* = temp;
FSS* = speed; is it 0-100 (percent), pwn (0-255), or in rpm?

} __packed;

static ssize_t wmi_read_fancurve_custom(const struct model_config *model,
struct fancurve *fancurve)
{
u8 buffer[88];
struct WMIFanTableRead fantable;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. By adding this new fields we get exactly 88 Bytes. These fields where not defined in the ACPI code of other models.

int err;

// The output buffer from the ACPI call is 88 bytes and larger
// than the returned object
pr_info("Size of object: %lu\n", sizeof(struct WMIFanTableRead));
err = wmi_exec_noarg_ints(WMI_GUID_LENOVO_FAN_METHOD, 0,
WMI_METHOD_ID_FAN_GET_TABLE, buffer,
sizeof(buffer));
struct acpi_buffer params;
u32 fanid = 0;
params.length = sizeof(fanid);
params.pointer = &fanid;

err = wmi_exec_ints(WMI_GUID_LENOVO_FAN_METHOD, 0,
WMI_METHOD_ID_FAN_GET_TABLE, &params, &fantable,
sizeof(fantable));
print_hex_dump(KERN_INFO, "legion_laptop fan table wmi buffer",
DUMP_PREFIX_ADDRESS, 16, 1, buffer, sizeof(buffer),
true);
DUMP_PREFIX_ADDRESS, 16, 1, &fantable,
sizeof(fantable), true);
if (!err) {
struct WMIFanTableRead *fantable =
(struct WMIFanTableRead *)&buffer[0];
fancurve->current_point_i = 0;
fancurve->size = 10;
fancurve->size = fantable.FSTL;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This does not work for other models because FSTL is not set to 10 but just not set.

fancurve->fan_speed_unit = FAN_SPEED_UNIT_PERCENT;
fancurve->points[0].speed1 = fantable->FSS0;
fancurve->points[1].speed1 = fantable->FSS1;
fancurve->points[2].speed1 = fantable->FSS2;
fancurve->points[3].speed1 = fantable->FSS3;
fancurve->points[4].speed1 = fantable->FSS4;
fancurve->points[5].speed1 = fantable->FSS5;
fancurve->points[6].speed1 = fantable->FSS6;
fancurve->points[7].speed1 = fantable->FSS7;
fancurve->points[8].speed1 = fantable->FSS8;
fancurve->points[9].speed1 = fantable->FSS9;
//fancurve->points[10].speed1 = fantable->FSSA;
fancurve->points[0].speed1 = fantable.FSS0;
fancurve->points[1].speed1 = fantable.FSS1;
fancurve->points[2].speed1 = fantable.FSS2;
fancurve->points[3].speed1 = fantable.FSS3;
fancurve->points[4].speed1 = fantable.FSS4;
fancurve->points[5].speed1 = fantable.FSS5;
fancurve->points[6].speed1 = fantable.FSS6;
fancurve->points[7].speed1 = fantable.FSS7;
fancurve->points[8].speed1 = fantable.FSS8;
fancurve->points[9].speed1 = fantable.FSS9;
//fancurve->points[10].speed1 = fantable.FSSA;
}
return err;
}

static ssize_t wmi_write_fancurve_custom(const struct model_config *model,
const struct fancurve *fancurve)
{
u8 buffer[0x20];
struct WMIFanTable fan;
fan.FSTL = 10;
fan.FSS0 = fancurve->points[0].speed1;
fan.FSS1 = fancurve->points[1].speed1;
fan.FSS2 = fancurve->points[2].speed1;
fan.FSS3 = fancurve->points[3].speed1;
fan.FSS4 = fancurve->points[4].speed1;
fan.FSS5 = fancurve->points[5].speed1;
fan.FSS6 = fancurve->points[6].speed1;
fan.FSS7 = fancurve->points[7].speed1;
fan.FSS8 = fancurve->points[8].speed1;
fan.FSS9 = fancurve->points[9].speed1;
fan.FSSNULL = 0;
// TODO: These should be variable and may be set on later models
// on Legion Go, they are hardcoded
fan.FTLE = 10;
fan.FST0 = 10;
fan.FST1 = 20;
fan.FST2 = 30;
fan.FST3 = 40;
fan.FST4 = 50;
fan.FST5 = 60;
fan.FST6 = 70;
fan.FST7 = 80;
fan.FST8 = 90;
fan.FST9 = 100;
fan.FSTNULL = 0;
int err;

// The buffer is read like this in ACPI firmware
//
// CreateByteField (Arg2, Zero, FSTM)
// CreateByteField (Arg2, One, FSID)
// CreateDWordField (Arg2, 0x02, FSTL)
// CreateByteField (Arg2, 0x06, FSS0)
// CreateByteField (Arg2, 0x08, FSS1)
// CreateByteField (Arg2, 0x0A, FSS2)
// CreateByteField (Arg2, 0x0C, FSS3)
// CreateByteField (Arg2, 0x0E, FSS4)
// CreateByteField (Arg2, 0x10, FSS5)
// CreateByteField (Arg2, 0x12, FSS6)
// CreateByteField (Arg2, 0x14, FSS7)
// CreateByteField (Arg2, 0x16, FSS8)
// CreateByteField (Arg2, 0x18, FSS9)

memset(buffer, 0, sizeof(buffer));
buffer[0x06] = fancurve->points[0].speed1;
buffer[0x08] = fancurve->points[1].speed1;
buffer[0x0A] = fancurve->points[2].speed1;
buffer[0x0C] = fancurve->points[3].speed1;
buffer[0x0E] = fancurve->points[4].speed1;
buffer[0x10] = fancurve->points[5].speed1;
buffer[0x12] = fancurve->points[6].speed1;
buffer[0x14] = fancurve->points[7].speed1;
buffer[0x16] = fancurve->points[8].speed1;
buffer[0x18] = fancurve->points[9].speed1;

print_hex_dump(KERN_INFO, "legion_laptop fan table wmi write buffer",
DUMP_PREFIX_ADDRESS, 16, 1, buffer, sizeof(buffer),
DUMP_PREFIX_ADDRESS, 16, 1, &fan, sizeof(fan),
true);
err = wmi_exec_arg(WMI_GUID_LENOVO_FAN_METHOD, 0,
WMI_METHOD_ID_FAN_SET_TABLE, buffer, sizeof(buffer));
WMI_METHOD_ID_FAN_SET_TABLE, &fan, sizeof(fan));
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the old code we write a buffer of size 0x20. Now we write an object of size 0x34. Will this break stuff in old models?

return err;
}

Expand Down Expand Up @@ -5694,6 +5736,34 @@ static struct attribute *fancurve_hwmon_attributes[] = {
&sensor_dev_attr_pwm1_mode.dev_attr.attr, NULL
};

static struct attribute *fancurve_hwmon_attributes_legion_go[] = {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why create new hwmon entries? Do we need new ones with other properties?

&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
&sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr,
&sensor_dev_attr_pwm1_auto_point4_pwm.dev_attr.attr,
&sensor_dev_attr_pwm1_auto_point5_pwm.dev_attr.attr,
&sensor_dev_attr_pwm1_auto_point6_pwm.dev_attr.attr,
&sensor_dev_attr_pwm1_auto_point7_pwm.dev_attr.attr,
&sensor_dev_attr_pwm1_auto_point8_pwm.dev_attr.attr,
&sensor_dev_attr_pwm1_auto_point9_pwm.dev_attr.attr,
&sensor_dev_attr_pwm1_auto_point10_pwm.dev_attr.attr,
// These should be read only if the driver is properly implemented
// and either fail to set or noop
// &sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, we can make set read/write properties depending on model or just error out/ignore it.

// &sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
// &sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr,
// &sensor_dev_attr_pwm1_auto_point4_temp.dev_attr.attr,
// &sensor_dev_attr_pwm1_auto_point5_temp.dev_attr.attr,
// &sensor_dev_attr_pwm1_auto_point6_temp.dev_attr.attr,
// &sensor_dev_attr_pwm1_auto_point7_temp.dev_attr.attr,
// &sensor_dev_attr_pwm1_auto_point8_temp.dev_attr.attr,
// &sensor_dev_attr_pwm1_auto_point9_temp.dev_attr.attr,
// &sensor_dev_attr_pwm1_auto_point10_temp.dev_attr.attr,
//
&sensor_dev_attr_auto_points_size.dev_attr.attr,
&sensor_dev_attr_pwm1_mode.dev_attr.attr, NULL
};

static umode_t legion_hwmon_is_visible(struct kobject *kobj,
struct attribute *attr, int idx)
{
Expand All @@ -5720,6 +5790,11 @@ static const struct attribute_group legion_hwmon_fancurve_group = {
.is_visible = legion_hwmon_is_visible,
};

static const struct attribute_group legion_go_hwmon_fancurve_group = {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. I guess, this might be removed.

.attrs = fancurve_hwmon_attributes_legion_go,
.is_visible = legion_hwmon_is_visible,
};

static const struct attribute_group *legion_hwmon_groups[] = {
&legion_hwmon_sensor_group, &legion_hwmon_fancurve_group, NULL
};
Expand Down
Loading