[PATCH] wifi: iwlwifi: acpi/dsm: cache error retcode for iwl_acpi_get_dsm

[PATCH] wifi: iwlwifi: acpi/dsm: cache error retcode for iwl_acpi_get_dsm

[David Wang]

On some HW, acpi _DSM query would failed for iwlwifi device
and everytime when network is reactiaved (boot,
suspend/resume, manually restart network, etc.),
bunch of kernel warning shows up together:
  ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
  ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
  ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
  ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
  ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
  ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
  ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
  ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
since iwlwifi would make 8 acpi/dsm queries for lari config.
But for iwlwifi, it is safe to cache the _DSM errors,
since it is not possible to correct it without upgrading BIOS.
With this patch, those kernel warnings would only show up once when
booting the system and unnecessary acpi/dsm queries are avoid.

Signed-off-by: David Wang <00107082@163.com>
---
 drivers/net/wireless/intel/iwlwifi/fw/acpi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/wireless/intel/iwlwifi/fw/acpi.c b/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
index 79774c8c7ff4..3f98f522daac 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
+++ b/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
@@ -30,6 +30,8 @@ static const size_t acpi_dsm_size[DSM_FUNC_NUM_FUNCS] = {
 	[DSM_FUNC_ENABLE_11BE] =		sizeof(u32),
 };
 
+static int acpi_dsm_func_retcode[DSM_FUNC_NUM_FUNCS] = {0};
+
 static int iwl_acpi_get_handle(struct device *dev, acpi_string method,
 			       acpi_handle *ret_handle)
 {
@@ -169,6 +171,10 @@ int iwl_acpi_get_dsm(struct iwl_fw_runtime *fwrt,
 	if (WARN_ON(func >= ARRAY_SIZE(acpi_dsm_size)))
 		return -EINVAL;
 
+	/* If HW return an error once, do not bother try again. */
+	if (acpi_dsm_func_retcode[func])
+		return acpi_dsm_func_retcode[func];
+
 	expected_size = acpi_dsm_size[func];
 
 	/* Currently all ACPI DSMs are either 8-bit or 32-bit */
@@ -177,6 +183,7 @@ int iwl_acpi_get_dsm(struct iwl_fw_runtime *fwrt,
 
 	ret = iwl_acpi_get_dsm_integer(fwrt->dev, ACPI_DSM_REV, func,
 				       &iwl_guid, &tmp, expected_size);
+	acpi_dsm_func_retcode[func] = ret;
 	if (ret)
 		return ret;
 
-- 
2.39.2

Re: [PATCH] wifi: iwlwifi: acpi/dsm: cache error retcode for iwl_acpi_get_dsm

[Kalle Valo]

David Wang <00107082@163.com> writes:

> On some HW, acpi _DSM query would failed for iwlwifi device
> and everytime when network is reactiaved (boot,
> suspend/resume, manually restart network, etc.),
> bunch of kernel warning shows up together:
>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
> since iwlwifi would make 8 acpi/dsm queries for lari config.
> But for iwlwifi, it is safe to cache the _DSM errors,
> since it is not possible to correct it without upgrading BIOS.
> With this patch, those kernel warnings would only show up once when
> booting the system and unnecessary acpi/dsm queries are avoid.
>
> Signed-off-by: David Wang <00107082@163.com>
> ---
>  drivers/net/wireless/intel/iwlwifi/fw/acpi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/fw/acpi.c b/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
> index 79774c8c7ff4..3f98f522daac 100644
> --- a/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
> +++ b/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
> @@ -30,6 +30,8 @@ static const size_t acpi_dsm_size[DSM_FUNC_NUM_FUNCS] = {
>  	[DSM_FUNC_ENABLE_11BE] =		sizeof(u32),
>  };
>  
> +static int acpi_dsm_func_retcode[DSM_FUNC_NUM_FUNCS] = {0};
> +
>  static int iwl_acpi_get_handle(struct device *dev, acpi_string method,
>  			       acpi_handle *ret_handle)
>  {
> @@ -169,6 +171,10 @@ int iwl_acpi_get_dsm(struct iwl_fw_runtime *fwrt,
>  	if (WARN_ON(func >= ARRAY_SIZE(acpi_dsm_size)))
>  		return -EINVAL;
>  
> +	/* If HW return an error once, do not bother try again. */
> +	if (acpi_dsm_func_retcode[func])
> +		return acpi_dsm_func_retcode[func];

Static variables are usually avoided because they are problematic if
there are multiple iwlwifi devices on the same host. Should the error
message be just removed entirely?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: iwlwifi: acpi/dsm: cache error retcode for iwl_acpi_get_dsm

[David Wang]

At 2024-08-27 14:26:42, "Kalle Valo" <kvalo@kernel.org> wrote:
>David Wang <00107082@163.com> writes:
>
>> On some HW, acpi _DSM query would failed for iwlwifi device
>> and everytime when network is reactiaved (boot,
>> suspend/resume, manually restart network, etc.),
>> bunch of kernel warning shows up together:
>>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>   ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>> since iwlwifi would make 8 acpi/dsm queries for lari config.
>> But for iwlwifi, it is safe to cache the _DSM errors,
>> since it is not possible to correct it without upgrading BIOS.
>> With this patch, those kernel warnings would only show up once when
>> booting the system and unnecessary acpi/dsm queries are avoid.
>>
>> Signed-off-by: David Wang <00107082@163.com>
>> ---
>>  drivers/net/wireless/intel/iwlwifi/fw/acpi.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/wireless/intel/iwlwifi/fw/acpi.c b/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
>> index 79774c8c7ff4..3f98f522daac 100644
>> --- a/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
>> +++ b/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
>> @@ -30,6 +30,8 @@ static const size_t acpi_dsm_size[DSM_FUNC_NUM_FUNCS] = {
>>  	[DSM_FUNC_ENABLE_11BE] =		sizeof(u32),
>>  };
>>  
>> +static int acpi_dsm_func_retcode[DSM_FUNC_NUM_FUNCS] = {0};
>> +
>>  static int iwl_acpi_get_handle(struct device *dev, acpi_string method,
>>  			       acpi_handle *ret_handle)
>>  {
>> @@ -169,6 +171,10 @@ int iwl_acpi_get_dsm(struct iwl_fw_runtime *fwrt,
>>  	if (WARN_ON(func >= ARRAY_SIZE(acpi_dsm_size)))
>>  		return -EINVAL;
>>  
>> +	/* If HW return an error once, do not bother try again. */
>> +	if (acpi_dsm_func_retcode[func])
>> +		return acpi_dsm_func_retcode[func];
>
>Static variables are usually avoided because they are problematic if
>there are multiple iwlwifi devices on the same host. Should the error
>message be just removed entirely?

Thanks for the review~
Yep,  you're quite right! I did not consider multiple iwlwifi devices.

The repeated error messages really bother  me,  but they are from acpi driver, I don't think it can be removed  entirely....

Would moving the cache into device structure acceptable?


Thanks
David 

>
>-- 
>https://patchwork.kernel.org/project/linux-wireless/list/
>
>https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: iwlwifi: acpi/dsm: cache error retcode for iwl_acpi_get_dsm

[Ben Greear]

On 8/26/24 23:26, Kalle Valo wrote:
> David Wang <00107082@163.com> writes:
> 
>> On some HW, acpi _DSM query would failed for iwlwifi device
>> and everytime when network is reactiaved (boot,
>> suspend/resume, manually restart network, etc.),
>> bunch of kernel warning shows up together:
>>    ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>    ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>    ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>    ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>    ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>    ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>    ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>>    ACPI: \: failed to evaluate _DSM bf0212f2-788f-c64d-a5b3-1f738e285ade (0x1001)
>> since iwlwifi would make 8 acpi/dsm queries for lari config.
>> But for iwlwifi, it is safe to cache the _DSM errors,
>> since it is not possible to correct it without upgrading BIOS.
>> With this patch, those kernel warnings would only show up once when
>> booting the system and unnecessary acpi/dsm queries are avoid.
>>
>> Signed-off-by: David Wang <00107082@163.com>
>> ---
>>   drivers/net/wireless/intel/iwlwifi/fw/acpi.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/wireless/intel/iwlwifi/fw/acpi.c b/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
>> index 79774c8c7ff4..3f98f522daac 100644
>> --- a/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
>> +++ b/drivers/net/wireless/intel/iwlwifi/fw/acpi.c
>> @@ -30,6 +30,8 @@ static const size_t acpi_dsm_size[DSM_FUNC_NUM_FUNCS] = {
>>   	[DSM_FUNC_ENABLE_11BE] =		sizeof(u32),
>>   };
>>   
>> +static int acpi_dsm_func_retcode[DSM_FUNC_NUM_FUNCS] = {0};
>> +
>>   static int iwl_acpi_get_handle(struct device *dev, acpi_string method,
>>   			       acpi_handle *ret_handle)
>>   {
>> @@ -169,6 +171,10 @@ int iwl_acpi_get_dsm(struct iwl_fw_runtime *fwrt,
>>   	if (WARN_ON(func >= ARRAY_SIZE(acpi_dsm_size)))
>>   		return -EINVAL;
>>   
>> +	/* If HW return an error once, do not bother try again. */
>> +	if (acpi_dsm_func_retcode[func])
>> +		return acpi_dsm_func_retcode[func];
> 
> Static variables are usually avoided because they are problematic if
> there are multiple iwlwifi devices on the same host. Should the error
> message be just removed entirely?

In this particular case, probably static would be best since it would not
be helpful to see the duplicated errors for each individual interface anyway?

But also, I'm fine with just making the warning go away entirely.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com