Messages in this thread | | | Date | Sun, 29 Aug 2021 19:10:32 +1200 | From | Luke Jones <> | Subject | Re: [PATCH v5] asus-wmi: Add support for custom fan curves |
| |
Thanks heaps Barnabás, I think I've gotten a very good improvement with your help. Let's see how V6 fairs.
On Sat, Aug 28 2021 at 14:39:40 +0000, Barnabás Pőcze <pobrn@protonmail.com> wrote: > Hi > > > 2021. augusztus 28., szombat 8:56 keltezéssel, Luke Jones írta: >> [...] >> >> +/* >> >> + * The expected input is of the format >> >> + * "30:1,49:2,59:3,69:4,79:31,89:49,99:56,109:58" >> >> + * where a pair is 30:1, with 30 = temperature, and 1 = >> percentage >> >> +*/ >> >> +static int fan_curve_write(struct asus_wmi *asus, u32 dev, char >> >> *curve) >> >> +{ >> >> + char * buf, *set, *pair_tmp, *pair, *set_end, *pair_end; >> >> + int err, ret; >> >> + >> >> + char *set_delimiter = ","; >> >> + char *pair_delimiter = ":"; >> >> + bool half_complete = false; >> >> + bool pair_start = true; >> >> + u32 prev_percent = 0; >> >> + u32 prev_temp = 0; >> >> + u32 percent = 0; >> >> + u32 shift = 0; >> >> + u32 temp = 0; >> >> + u32 arg1 = 0; >> >> + u32 arg2 = 0; >> >> + u32 arg3 = 0; >> >> + u32 arg4 = 0; >> >> + >> >> + buf = set_end = pair_end = kstrdup(curve, GFP_KERNEL); >> >> + >> >> + while( (set = strsep(&set_end, set_delimiter)) != NULL ) { >> >> + pair_tmp = kstrdup(set, GFP_KERNEL); >> >> + pair_start = true; >> >> + while( (pair = strsep(&pair_tmp, pair_delimiter)) != NULL ) { >> >> + err = kstrtouint(pair, 10, &ret); >> >> + if (err) { >> >> + kfree(pair_tmp); >> >> + kfree(buf); >> >> + return err; >> >> + } >> >> + >> >> + if (pair_start) { >> >> + temp = ret; >> >> + pair_start = false; >> >> + } else { >> >> + percent = ret; >> >> + } >> >> + } >> >> + kfree(pair_tmp); >> >> + >> >> + if (temp < prev_temp || percent < prev_percent || percent > >> 100) >> >> { >> >> + pr_info("Fan curve invalid"); >> >> + pr_info("A value is sequentially lower or percentage is > >> 100"); >> >> + kfree(buf); >> >> + return -EINVAL; >> >> + } >> >> + >> >> + prev_temp = temp; >> >> + prev_percent = percent; >> >> + >> >> + if (!half_complete) { >> >> + arg1 += temp << shift; >> >> + arg3 += percent << shift; >> >> + } else { >> >> + arg2 += temp << shift; >> >> + arg4 += percent << shift; >> >> + } >> > >> > As far as I see using 64-bit integers would avoid the need for >> > `half_complete`, et al. >> >> Reworked all that as part of the u8-array stuff. Look forward to >> seeing >> what you think. >> >> > >> > >> >> + shift += 8; >> >> + >> >> + if (shift == 32) { >> >> + shift = 0; >> >> + half_complete = true; >> >> + } >> >> + } >> >> + kfree(buf); >> >> + >> > >> > If you don't insist on using commas, I think it is much simpler to >> > parse it using `sscanf()`, e.g.: >> > >> > unsigned int temp, prct; >> > int at = 0, len; >> > >> > while (sscanf(&buf[at], "%u:%u %n", &temp, &prct, &len) == 2) { >> > /* process `temp` and `prct` */ >> > >> > at += len; >> > } >> > >> > if (buf[at] != '\0') >> > /* error */; >> > >> > This also has the advantage that you don't need dynamic memory >> > allocation. >> >> Half the reason I did it in the format of 10:20,30:40,.. is to keep >> close to a format that many people using some external tools for fan >> curves (using acpi_call modue!) are using. I'm open to improvements >> ofc. >> > > If you don't insist on *requiring* commas, then I think the following > works: > > while (sscanf(&buf[at], "%u:%u %n", &temp, &prct, &len) == 2) { > /* process `temp` and `prct` */ > > at += len; > at += strspn(&buf[at], ","); > } > > But please, whatever parser you end up submitting, make sure it is > thoroughly tested. > > >> [...] >> >> +static ssize_t gpu_fan_curve_quiet_show(struct device *dev, >> >> + struct device_attribute *attr, char *buf) >> >> +{ >> >> + struct asus_wmi *asus = dev_get_drvdata(dev); >> >> + return scnprintf(buf, PAGE_SIZE, "%s", >> asus->gpu_fan_curve.quiet); >> >> +} >> >> + >> >> +static ssize_t gpu_fan_curve_quiet_store(struct device *dev, >> >> + struct device_attribute *attr, >> >> + const char *buf, size_t count) >> >> +{ >> >> + struct asus_wmi *asus = dev_get_drvdata(dev); >> >> + return fan_curve_store(asus, buf, count, >> >> ASUS_WMI_DEVID_GPU_FAN_CURVE, >> >> + &asus->gpu_fan_curve.quiet, >> >> + asus->gpu_fan_curve.quiet_default); >> >> +} >> >> + >> >> +static DEVICE_ATTR_RW(gpu_fan_curve_quiet); >> > >> > Even though it is a hwmon thing, I think `SENSOR_ATTR_2()` (from >> > linux/hwmon-sysfs.h) >> > would be very useful here as you'd avoid creating n+1 functions, >> e.g: >> > >> > static ssize_t fan_curve_show(struct device *dev, struct >> > device_attribute *attr, char *buf) >> > { >> > struct sensor_device_attribute_2 *sattr = >> > to_sensor_dev_attr_2(attr); >> > struct asus_wmi *asus = dev_get_drvdata(dev); >> > >> > /* >> > * if you stored fan curves in an array, you could then access >> > the fan >> > * curve in `asus->fans[sattr->index].curves[sattr->nr]` >> > * / >> > } >> > >> > static SENSOR_DEVICE_ATTR_2(some_name1, 0644, fan_curve_show, >> > fan_curve_store, >> > FAN_CPU /* index in the "fans" >> array */, >> > ASUS_THROTTLE_THERMAL_POLICY_SILENT >> /* >> > index in the "curves" array */); >> > >> >> I'm sorry I don't really understand how this works. Is there a good >> doc >> for it anywhere? Being unfamiliar with C makes it look a little more >> intimidating than what I've managed to do so far. >> > > I am not sure, you can find some uses among hwmon drivers. > > If you look into linux/hwmon-sysfs.h, then you can see that > `SENSOR_DEVICE_ATTR_2()` > defines and initializes a `struct sensor_device_attribute_2` object: > > struct sensor_device_attribute_2 { > struct device_attribute dev_attr; > u8 index; > u8 nr; > }; > > So it has a normal device attribute inside it, and two extra pieces > of data. > One difference is that when you create the `struct attribute` array > (`platform_attributes`), then you will need to use > `&some_name1.dev_attr.attr`. > > And the idea here is that the show/store callbacks receive a pointer > to the > device attribute that is being read/written, and we know for a fact, > that this > device attribute is inside a `sensor_device_attribute_2` struct. And > thus we can > use the `to_sensor_dev_attr_2()` macro to get a pointer to the "outer" > `sensor_device_attribute_2` struct that contains the > `device_attribute` struct > that we have a pointer to. > > So now the `index` and `nr` members of that struct can be accessed. > You could > store the index of the fan (e.g. 0 for CPU, 1 for GPU) in `index`, > and the profile > in `nr`. The `ASUS_THROTTLE_THERMAL_POLICY_*` macros go from 0 to 2, > so I think > those would be perfect candidates for the curve index. That's why I > used > `ASUS_THROTTLE_THERMAL_POLICY_SILENT` in the example. > > The fan curve associated with the attribute can now be > accessed in `asus->fans[sattr->index].curves[sattr->nr]`. > > `to_sensor_dev_attr_2()` is just a wrapper around `container_of()`, > so if you're > familiar with the idea behind that, this shouldn't be too hard to > wrap your > head around. > > #define to_sensor_dev_attr_2(_dev_attr) \ > container_of(_dev_attr, struct sensor_device_attribute_2, > dev_attr) > > What it does, is that if you give it a pointer to the `dev_attr` > member of a > `struct sensor_device_attribute_2`, then it'll give you back a pointer > to the `struct sensor_device_attribute_2`. `container_of()` basically > does a > "conversion" from pointer-to-member-of-struct-X to > pointer-to-struct-X. > > In some sense, you might think of `struct device_attribute` as the > "base class", > and the `struct sensor_device_attribute_2` as the "derived class" > here. And what > `to_sensor_dev_attr_2()` is a down-cast from the base class to the > derived, > e.g. something like this in C++: > > struct device_attribute { ... }; > struct sensor_device_attribute_2 : device_attribute { > u8 index; > u8 nr; > }; > > /* `device_attr` is of type `struct device_attribute *` */ > static_cast<sensor_device_attribute_2 *>(device_attr); > /* there's also dynamic_cast which can do the same down-cast, > but it does runtime type checking as well */ > /* both of the mentioned C++ casts check if the pointer is nullptr, > normal container_of() does not that, but there is > container_of_safe() */ > > It may be too detailed, I'm not sure; please let me know if you have > other questions. > > >> [...] > > > Best regards, > Barnabás Pőcze
| |