diff mbox series

[ath-next,v2] wifi: ath12k: Fix incorrect rates sent to firmware

Message ID 20250319153547.771843-1-quic_rdevanat@quicinc.com (mailing list archive)
State New
Delegated to: Jeff Johnson
Headers show
Series [ath-next,v2] wifi: ath12k: Fix incorrect rates sent to firmware | expand

Checks

Context Check Description
wifibot/fixes_present success Fixes tag not required for -next series
wifibot/series_format success Single patches do not need cover letters
wifibot/tree_selection success Clearly marked for ath-next
wifibot/ynl success Generated files up to date; no warnings/errors; no diff in generated;
wifibot/build_32bit success Errors and warnings before: 0 this patch: 0
wifibot/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
wifibot/build_clang success Errors and warnings before: 0 this patch: 0
wifibot/build_clang_rust success No Rust files in patch. Skipping build
wifibot/build_tools success No tools touched, skip
wifibot/check_selftest success No net selftest shell script
wifibot/checkpatch success total: 0 errors, 0 warnings, 0 checks, 26 lines checked
wifibot/deprecated_api success None detected
wifibot/header_inline success No static functions without inline keyword in header files
wifibot/kdoc success Errors and warnings before: 0 this patch: 0
wifibot/source_inline success Was 0 now: 0
wifibot/verify_fixes success No Fixes tag
wifibot/verify_signedoff success Signed-off-by tag matches author and committer

Commit Message

Roopni Devanathan March 19, 2025, 3:35 p.m. UTC 
From: Pradeep Kumar Chitrapu <quic_pradeepc@quicinc.com>

Before firmware assert, if there is a station interface in the device
which is not associated with an AP, the basic rates are set to zero.
Following this, during firmware recovery, when basic rates are zero,
ath12k driver is sending invalid rate codes, which are negative values,
to firmware. This results in firmware assert.

Fix this by checking if rate codes are valid, before sending them
to the firmware.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.4.1-00199-QCAHKSWPL_SILICONZ-1

Signed-off-by: Pradeep Kumar Chitrapu <quic_pradeepc@quicinc.com>
Signed-off-by: Roopni Devanathan <quic_rdevanat@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)


base-commit: b6f473c96421b8b451a8df8ccb620bcd71d4b3f4

Comments

Ping-Ke Shih March 20, 2025, 12:24 a.m. UTC | #1 
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 9fda97667d4e..661167acaa69 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -3450,7 +3450,9 @@ static void ath12k_recalculate_mgmt_rate(struct ath12k *ar,
>         }
> 
>         sband = hw->wiphy->bands[def->chan->band];
> -       basic_rate_idx = ffs(bss_conf->basic_rates) - 1;
> +       basic_rate_idx = __ffs(bss_conf->basic_rates);
> +       if (basic_rate_idx)
> +               basic_rate_idx -= 1;

It looks like you misunderstood what I meant. 

The difference of ffs() and __ffs():
ffs(0x0) = 0, ffs(0x1) = 1
__ffs(0x0) = undefined, __ffs(0x1) = 0

So you need to ensure argument isn't zero before calling __ffs(), and no
need to minus 1 after the call.

>         bitrate = sband->bitrates[basic_rate_idx].bitrate;
> 
>         hw_rate_code = ath12k_mac_get_rate_hw_value(bitrate);
> @@ -3983,10 +3985,13 @@ static void ath12k_mac_bss_info_changed(struct ath12k *ar,
>                 band = def.chan->band;
>                 mcast_rate = info->mcast_rate[band];
> 
> -               if (mcast_rate > 0)
> +               if (mcast_rate > 0) {
>                         rateidx = mcast_rate - 1;
> -               else
> -                       rateidx = ffs(info->basic_rates) - 1;
> +               } else {
> +                       rateidx = __ffs(info->basic_rates);
> +                       if (rateidx)
> +                               rateidx -= 1;

Here should be:

if (info->basic_rates)
    rateidx = __ffs(info->basic_rates);
else
    rateidx = 0;

> +               }
> 
>                 if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP)
>                         rateidx += ATH12K_MAC_FIRST_OFDM_RATE_IDX;
> 
> base-commit: b6f473c96421b8b451a8df8ccb620bcd71d4b3f4
> --
> 2.34.1
>
Roopni Devanathan March 20, 2025, 6:54 a.m. UTC | #2 
On 3/20/2025 5:54 AM, Ping-Ke Shih wrote:
>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> index 9fda97667d4e..661167acaa69 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -3450,7 +3450,9 @@ static void ath12k_recalculate_mgmt_rate(struct ath12k *ar,
>>         }
>>
>>         sband = hw->wiphy->bands[def->chan->band];
>> -       basic_rate_idx = ffs(bss_conf->basic_rates) - 1;
>> +       basic_rate_idx = __ffs(bss_conf->basic_rates);
>> +       if (basic_rate_idx)
>> +               basic_rate_idx -= 1;
> 
> It looks like you misunderstood what I meant. 
> 
> The difference of ffs() and __ffs():
> ffs(0x0) = 0, ffs(0x1) = 1
> __ffs(0x0) = undefined, __ffs(0x1) = 0
> 
> So you need to ensure argument isn't zero before calling __ffs(), and no
> need to minus 1 after the call.
> 
Noted the difference, thanks for explaining. I'll do something like:
if (bss_conf->basic_rates)
	basic_rate_idx = __ffs(bss_conf->basic_rates);
else
	basic_rate_idx = 0;

>>         bitrate = sband->bitrates[basic_rate_idx].bitrate;
>>
>>         hw_rate_code = ath12k_mac_get_rate_hw_value(bitrate);
>> @@ -3983,10 +3985,13 @@ static void ath12k_mac_bss_info_changed(struct ath12k *ar,
>>                 band = def.chan->band;
>>                 mcast_rate = info->mcast_rate[band];
>>
>> -               if (mcast_rate > 0)
>> +               if (mcast_rate > 0) {
>>                         rateidx = mcast_rate - 1;
>> -               else
>> -                       rateidx = ffs(info->basic_rates) - 1;
>> +               } else {
>> +                       rateidx = __ffs(info->basic_rates);
>> +                       if (rateidx)
>> +                               rateidx -= 1;
> 
> Here should be:
> 
> if (info->basic_rates)
>     rateidx = __ffs(info->basic_rates);
> else
>     rateidx = 0;
> 
Sure, will change this in next version.

>> +               }
>>
>>                 if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP)
>>                         rateidx += ATH12K_MAC_FIRST_OFDM_RATE_IDX;
>>
>> base-commit: b6f473c96421b8b451a8df8ccb620bcd71d4b3f4
>> --
>> 2.34.1
>>
>
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 9fda97667d4e..661167acaa69 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -3450,7 +3450,9 @@  static void ath12k_recalculate_mgmt_rate(struct ath12k *ar,
 	}
 
 	sband = hw->wiphy->bands[def->chan->band];
-	basic_rate_idx = ffs(bss_conf->basic_rates) - 1;
+	basic_rate_idx = __ffs(bss_conf->basic_rates);
+	if (basic_rate_idx)
+		basic_rate_idx -= 1;
 	bitrate = sband->bitrates[basic_rate_idx].bitrate;
 
 	hw_rate_code = ath12k_mac_get_rate_hw_value(bitrate);
@@ -3983,10 +3985,13 @@  static void ath12k_mac_bss_info_changed(struct ath12k *ar,
 		band = def.chan->band;
 		mcast_rate = info->mcast_rate[band];
 
-		if (mcast_rate > 0)
+		if (mcast_rate > 0) {
 			rateidx = mcast_rate - 1;
-		else
-			rateidx = ffs(info->basic_rates) - 1;
+		} else {
+			rateidx = __ffs(info->basic_rates);
+			if (rateidx)
+				rateidx -= 1;
+		}
 
 		if (ar->pdev->cap.supported_bands & WMI_HOST_WLAN_5G_CAP)
 			rateidx += ATH12K_MAC_FIRST_OFDM_RATE_IDX;