Re: [PATCH v7] asus-wmi: Add support for custom fan curves

[Luke Jones]

On Wed, Sep 1 2021 at 15:24:40 +0000, Barnabás Pőcze 
<pobrn@xxxxxxxxxxxxxx> wrote:
> 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;

Got it. Sometimes I forget switch/case exists. I'll adjust the v8 patch 
I sent out earlier.

> >  >
> >  >
> >  >>  +
> >  >>  +	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...

We can add the fact that a variation exists with a more typical setup 
also. The G713Q has no throttle_thermal and in the dsdt dump looks like 
it possible can read back the curve that is set by the user. This works 
in our favour though.

> >  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).

There is. I develop asusctl in conjunction with these patches. I'd 
certainly like to find the best way to fit all of this together.

> This would simplify the kernel code, add no new ABI, and
> potentially provide greater control over policy for the
> user space.

I agree.

> >  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)?

It looks like I need to adjust how pwm[n]_enable works anyway:

`pwm[1-*]_enable`
Fan speed control method:
- 0: no fan speed control (i.e. fan at full speed)
- 1: manual fan speed control enabled (using `pwm[1-*]`)
- 2+: automatic fan speed control enabled

So maybe on "manual" I can make it so the get method does what is in 
fan_curve_check_present() (and change that part also) and fetches the 
"defaults" on enable. This might even give us the fan curve that was 
set (and looks like it will on the machine that has no thermal throttle 
profile, v8 patch) - I'll try this anyway and see what it looks like. 
This seems to be the best approach given how the G713Q works anyway.

The issue I have with the above is that it overwrites any curve set. 
But given that it'll most likely be managed in userspace that's maybe 
*not* an issue. Otherwise would it be sensible to add something like 
`pwm1_reset`? I don't see anything like that in the related docs 
though. `pwm1_reset` would be to re-read the defaults from the acpi 
method.

Central to the above is that we can still read 0, 1, 2 curves from acpi 
- I was thinking to use the throttle_thermal mode to choose which one 
and that would be the only use of it. And won't store them as 
per-profile, which becomes a moot point when userspace is managing it 
anyway.

Many thanks,
Luke.


> >  [...]
>
> [0]: https://gitlab.com/asus-linux/asusctl
>
>
> Best regards,
> Barnabás Pőcze