lkml.org 
[lkml]   [2021]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2021-08-29 09:12    [W:0.075 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site