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
