Message-ID: <Z3uTWHyeRPzaHU0iSW56m1ltGsYr5DOfRoJLyGlfnObU0ph-mVf9M6KCbSV66AeY_voEARTrP6bOtqXS1ZubuSj4Cu25VSRu0VMBIf3whow=@protonmail.com>
Date:   Wed, 01 Sep 2021 15:24:40 +0000
From:   Barnabás Pőcze <pobrn@...tonmail.com>
To:     Luke Jones <luke@...nes.dev>
Cc:     linux-kernel@...r.kernel.org, hdegoede@...hat.com,
        linux@...ck-us.net, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v7] asus-wmi: Add support for custom fan curves

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

Powered by blists - more mailing lists