diff mbox series

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

Message ID 20241210001657.3362-4-W_Armin@gmx.de (mailing list archive)
State Accepted, archived
Headers show
Series acer-wmi: Various improvements | expand

Commit Message

Armin Wolf Dec. 10, 2024, 12:16 a.m. UTC 
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(-)

--
2.39.5

Comments

Kurt Borja Dec. 10, 2024, 2:06 a.m. UTC | #1 
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.

> 
>  /*
> @@ -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
>
Armin Wolf Dec. 10, 2024, 4:08 a.m. UTC | #2 
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
>>
Kurt Borja Dec. 10, 2024, 4:18 a.m. UTC | #3 
On Tue, Dec 10, 2024 at 05:08:01AM +0100, Armin Wolf wrote:
> 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.

I'll keep that in mind when submitting.

In that case:

Reviewed-by: Kurt Borja <kuurtb@gmail.com>

I apologize If this was out of place, as I'm still learning!

~ Kurt

> 
> 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
> > >
diff mbox series

Patch

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);
 }

 /*
@@ -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: