Re: [PATCH v4 3/5] platform/x86: acer-wmi: Improve error handling when reading gaming system information

Armin Wolf
2024-12-10T04:08:11Z

Am 10.12.24 um 03:06 schrieb Kurt Borja:

> On Tue, Dec 10, 2024 at 01:16:55AM +0100, Armin Wolf wrote:
>> If a call to ACER_WMID_GET_GAMING_SYS_INFO_METHODID fails, the lower
>> 8 bits will be non-zero. Introduce a helper function to check this and
>> use it when reading gaming system information.
>>
>> Tested-by: Rayan Margham <rayanmargham4@gmail.com>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> drivers/platform/x86/acer-wmi.c | 55 +++++++++++++++++++++------------
>> 1 file changed, 36 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
>> index dd57787466b9..ac4500f33b8c 100644
>> --- a/drivers/platform/x86/acer-wmi.c
>> +++ b/drivers/platform/x86/acer-wmi.c
>> @@ -70,6 +70,7 @@ MODULE_LICENSE("GPL");
>>
>> #define ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET 0x54
>>
>> +#define ACER_PREDATOR_V4_RETURN_STATUS_BIT_MASK GENMASK_ULL(7, 0)
>> #define ACER_PREDATOR_V4_FAN_SPEED_READ_BIT_MASK GENMASK(20, 8)
>>
>> /*
>> @@ -1513,6 +1514,24 @@ static acpi_status WMID_gaming_get_u64(u64 *value, u32 cap)
>> return status;
>> }
>>
>> +static int WMID_gaming_get_sys_info(u32 command, u64 *out)
>> +{
>> + acpi_status status;
>> + u64 result;
>> +
>> + status = WMI_gaming_execute_u64(ACER_WMID_GET_GAMING_SYS_INFO_METHODID, command, &result);
>> + if (ACPI_FAILURE(status))
>> + return -EIO;
>> +
>> + /* The return status must be zero for the operation to have succeeded */
>> + if (FIELD_GET(ACER_PREDATOR_V4_RETURN_STATUS_BIT_MASK, result))
>> + return -EIO;
>> +
>> + *out = result;
>> +
>> + return 0;
>> +}
>> +
>> static void WMID_gaming_set_fan_mode(u8 fan_mode)
>> {
>> /* fan_mode = 1 is used for auto, fan_mode = 2 used for turbo*/
>> @@ -1762,22 +1781,23 @@ static int acer_gsensor_event(void)
>>
>> static int acer_get_fan_speed(int fan)
>> {
>> - if (quirks->predator_v4) {
>> - acpi_status status;
>> - u64 fanspeed;
>> + u64 fanspeed;
>> + u32 command;
>> + int ret;
>>
>> - status = WMI_gaming_execute_u64(
>> - ACER_WMID_GET_GAMING_SYS_INFO_METHODID,
>> - fan == 0 ? ACER_WMID_CMD_GET_PREDATOR_V4_CPU_FAN_SPEED :
>> - ACER_WMID_CMD_GET_PREDATOR_V4_GPU_FAN_SPEED,
>> - &fanspeed);
>> + if (!quirks->predator_v4)
>> + return -EOPNOTSUPP;
>>
>> - if (ACPI_FAILURE(status))
>> - return -EIO;
>> + if (fan == 0)
>> + command = ACER_WMID_CMD_GET_PREDATOR_V4_CPU_FAN_SPEED;
>> + else
>> + command = ACER_WMID_CMD_GET_PREDATOR_V4_GPU_FAN_SPEED;
>>
>> - return FIELD_GET(ACER_PREDATOR_V4_FAN_SPEED_READ_BIT_MASK, fanspeed);
>> - }
>> - return -EOPNOTSUPP;
>> + ret = WMID_gaming_get_sys_info(command, &fanspeed);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return FIELD_GET(ACER_PREDATOR_V4_FAN_SPEED_READ_BIT_MASK, fanspeed);
>> }
> I wonder if it's necessary to refactor this function if you are going to
> drop it on Patch 4.

I did it so each patch has a single well-defined scope. And refactoring this function was necessary for
the patch being complete.

Thanks,
Armin Wolf

>> /*
>> @@ -1942,12 +1962,9 @@ static int acer_thermal_profile_change(void)
>> return err;
>>
>> /* Check power source */
>> - status = WMI_gaming_execute_u64(
>> - ACER_WMID_GET_GAMING_SYS_INFO_METHODID,
>> - ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS, &on_AC);
>> -
>> - if (ACPI_FAILURE(status))
>> - return -EIO;
>> + err = WMID_gaming_get_sys_info(ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS, &on_AC);
>> + if (err < 0)
>> + return err;
>>
>> switch (current_tp) {
>> case ACER_PREDATOR_V4_THERMAL_PROFILE_TURBO:
>> --
>> 2.39.5
>>