Hi
> [...]
> >> --- a/drivers/platform/x86/asus-wmi.c
> >> +++ b/drivers/platform/x86/asus-wmi.c
> >> [...]
> >> +/*
> >> + * Returns as an error if the method output is not a buffer.
> >> Typically this
> >
> > It seems to me it will simply leave the output buffer uninitialized
> > if something
> > other than ACPI_TYPE_BUFFER and ACPI_TYPE_INTEGER is encountered and
> > return 0.
>
> Oops, see below inline reply:
>
> >
> >
> >> + * means that the method called is unsupported.
> >> + */
> >> +static int asus_wmi_evaluate_method_buf(u32 method_id,
> >> + u32 arg0, u32 arg1, u8 *ret_buffer)
> >> +{
> >> + struct bios_args args = {
> >> + .arg0 = arg0,
> >> + .arg1 = arg1,
> >> + .arg2 = 0,
> >> + };
> >> + struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
> >> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> >> + acpi_status status;
> >> + union acpi_object *obj;
> >> + u32 int_tmp = 0;
> >> +
> >> + status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID, 0, method_id,
> >> + &input, &output);
> >> +
> >> + if (ACPI_FAILURE(status))
> >> + return -EIO;
> >> +
> >> + obj = (union acpi_object *)output.pointer;
> >> +
> >> + if (obj && obj->type == ACPI_TYPE_INTEGER) {
> >> + int_tmp = (u32) obj->integer.value;
> >> + if (int_tmp == ASUS_WMI_UNSUPPORTED_METHOD)
> >> + return -ENODEV;
> >> + return int_tmp;
> >
> > Is anything known about the possible values? You are later
> > using it as if it was an errno (e.g. in `custom_fan_check_present()`).
> >
> > And `obj` is leaked in both of the previous two returns.
>
> The return for the method we're calling in this patch returns 0 if the
> input arg has no match.
>
> >
> >
> >> + }
> >> +
> >> + if (obj && obj->type == ACPI_TYPE_BUFFER)
> >> + memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
> >
> > I would suggest you add a "size_t size" argument to this function, and
> > return -ENOSPC/-ENODATA depending on whether the returned buffer is
> > too
> > big/small. Maybe return -ENODATA if `obj` is NULL, too.
>
> Got it. So something like this would be suitable?
>
> if (obj && obj->type == ACPI_TYPE_BUFFER)
> if (obj->buffer.length < size)
> err = -ENOSPC;
> if (!obj->buffer.length)
> err = -ENODATA;
> if (err) {
> kfree(obj);
> return err;
> }
> memcpy(ret_buffer, obj->buffer.pointer, obj->buffer.length);
> }
>
> if (obj && obj->type == ACPI_TYPE_INTEGER)
> int_tmp = (u32) obj->integer.value;
>
> kfree(obj);
>
> if (int_tmp == ASUS_WMI_UNSUPPORTED_METHOD)
> return -ENODEV;
>
> /* There is at least one method that returns a 0 with no buffer */
> if (obj == NULL || int_tmp == 0)
> return -ENODATA;
>
> return 0;
>
I had something like the following in mind:
int err = 0;
/* ... */
obj = output.pointer;
if (!obj)
return -ENODATA;
switch (obj->type) {
case ACPI_TYPE_BUFFER:
if (obj->buffer.length < size)
err = -ENODATA;
else if (obj->buffer.length > size)
err = -ENOSPC;
else
memcpy(ret_buffer, obj->buffer.pointer, size);
break;
case ACPI_TYPE_INTEGER:
switch (obj->integer.value) {
case ASUS_WMI_UNSUPPORTED_METHOD:
err = -EOPNOTSUPP;
break;
default:
err = -ENODATA;
break;
}
break;
default:
err = -ENODATA;
break;
}
kfree(obj);
return err;
> >
> >
> >> +
> >> + kfree(obj);
> >> +
> >> + return 0;
> >> +}
> [...]
> >> +/*
> >> + * Called only by throttle_thermal_policy_write()
> >> + */
> >
> > Am I correct in thinking that the firmware does not actually
> > support specifying fan curves for each mode, only a single one,
> > and the fan curve switching is done by this driver when
> > the performance mode is changed?
>
> I'm not 100% certain on this. The WMI method 0x00110024 takes an arg
> 0,1,2 which then returns some factory stored fan profiles, these fit
> the profiles of ASUS_THROTTLE_THERMAL_POLICY_*, but with 1 and 2
> swapped.
>
> Looking at the SET part, it seems to write to a different location than
> where the GET is fetching information.
>
The, unfortunately, that is not as simple as I initially thought...
> Because of the fact there are three sets of curves to get, I thought it
> would be good for users to be able to set per profile. I don't think
> the set is retained in acpi if the profile is switched.
>
> Do you think it would be best to not have the ability to store per
> profile in kernel?
If there was a method to set a fan curve, and one to retrieve it,
I would suggest just exposing that via the pwmN_auto_pointM_{pwm,temp}
attributes on a hwmon device, and that the profile-dependent switching
be implemented somewhere else. As far as I see, there is already
existing infrastructure for integrating such a feature [0]
(but please correct me if I'm wrong).
This would simplify the kernel code, add no new ABI, and
potentially provide greater control over policy for the
user space.
> How would I choose which profile get to populate the
> initial data with if so?
I assume there isn't a method that can query
the current fan curve (or it is unknown)?
> [...]
[0]: https://gitlab.com/asus-linux/asusctl
Best regards,
Barnabás Pőcze
