| Message ID | 20231204081323.5582-1-quic_bqiang@quicinc.com |
|---|---|
| Headers | show |
| Series | wifi: ath11k: add support for 6 GHz station for various modes : LPI, SP and VLP | expand |
On 12/4/2023 11:53 PM, Aditya Kumar Singh wrote: > On 12/4/23 13:43, Baochen Qiang wrote: >> From: Wen Gong <quic_wgong@quicinc.com> >> >> When station is connected to a 6 GHz AP, it has 2 ways to configure >> the power limit to firmware. The first way is to send 2 WMI commands >> WMI_PDEV_PARAM_TXPOWER_LIMIT2G/WMI_PDEV_PARAM_TXPOWER_LIMIT5G to >> firmware, the second way is to send WMI_VDEV_SET_TPC_POWER_CMDID to >> firmware which include more parameters for power control. >> >> When firmware support SERVICE_EXT_TPC_REG, it means firmware support >> the second way for WMI_VDEV_SET_TPC_POWER_CMDID, then ath11k discard >> BSS_CHANGED_TXPOWER flag from mac80211 which is used to the first way >> for 6 GHz band and select the second way. >> >> Tested-on: WCN6855 hw2.0 PCI >> WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23 >> >> Signed-off-by: Wen Gong <quic_wgong@quicinc.com> >> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> >> --- > ...snip... >> @@ -3596,9 +3608,13 @@ static void >> ath11k_mac_op_bss_info_changed(struct ieee80211_hw *hw, >> if (changed & BSS_CHANGED_TXPOWER) { >> ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "vdev_id %i txpower %d\n", >> arvif->vdev_id, info->txpower); >> - >> - arvif->txpower = info->txpower; >> - ath11k_mac_txpower_recalc(ar); >> + if (ath11k_mac_supports_station_tpc(ar, arvif, >> &info->chandef)) { >> + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, >> + "discard tx power, change to set TPC power\n"); >> + } else { >> + arvif->txpower = info->txpower; >> + ath11k_mac_txpower_recalc(ar); >> + } > > Could you check v6 once? I remember Wen told he would drop this check > and let FW take the min value. If we do like this, then user could not > set his own desired value even if that is well inside the reg limits. I did notice this comment in V6, but came out of a different opinion: it is OK to discard the TX power here, because that will be sent to firmware using WMI_VDEV_SET_TPC_POWER_CMDID command in another patch. Please correct me if wrong. >
On 12/4/23 13:43, Baochen Qiang wrote: > --- a/drivers/net/wireless/ath/ath11k/mac.h > +++ b/drivers/net/wireless/ath/ath11k/mac.h > @@ -159,7 +159,6 @@ struct ath11k_vif *ath11k_mac_get_vif_up(struct ath11k_base *ab); > > struct ath11k *ath11k_mac_get_ar_by_vdev_id(struct ath11k_base *ab, u32 vdev_id); > struct ath11k *ath11k_mac_get_ar_by_pdev_id(struct ath11k_base *ab, u32 pdev_id); > - Irrelevant change w.r.t commit message? > void ath11k_mac_drain_tx(struct ath11k *ar); > void ath11k_mac_peer_cleanup_all(struct ath11k *ar); > int ath11k_mac_tx_mgmt_pending_free(int buf_id, void *skb, void *ctx); ... > @@ -4749,6 +4749,11 @@ static int ath11k_wmi_tlv_ext_soc_hal_reg_caps_parse(struct ath11k_base *soc, > soc->pdevs[0].pdev_id = 0; > } > > + if (!soc->reg_info_store) > + soc->reg_info_store = kcalloc(soc->num_radios, > + sizeof(*soc->reg_info_store), > + GFP_ATOMIC); What if this memory allocation request fails? Any negative case check should be present? > + > return 0; > } > > @@ -7071,33 +7076,54 @@ static bool ath11k_reg_is_world_alpha(char *alpha) > return false; > } > > -static int ath11k_reg_chan_list_event(struct ath11k_base *ab, > - struct sk_buff *skb, > - enum wmi_reg_chan_list_cmd_type id) > +void ath11k_reg_reset_info(struct cur_regulatory_info *reg_info) > { > - struct cur_regulatory_info *reg_info = NULL; > - struct ieee80211_regdomain *regd = NULL; > - bool intersect = false; > - int ret = 0, pdev_idx, i, j; > - struct ath11k *ar; > + int i, j; > > - reg_info = kzalloc(sizeof(*reg_info), GFP_ATOMIC); > - if (!reg_info) { > - ret = -ENOMEM; > - goto fallback; > - } > + if (reg_info) { > + kfree(reg_info->reg_rules_2ghz_ptr); > > - if (id == WMI_REG_CHAN_LIST_CC_ID) > - ret = ath11k_pull_reg_chan_list_update_ev(ab, skb, reg_info); > - else > - ret = ath11k_pull_reg_chan_list_ext_update_ev(ab, skb, reg_info); > + kfree(reg_info->reg_rules_5ghz_ptr); > > - if (ret) { > - ath11k_warn(ab, "failed to extract regulatory info from received event\n"); > - goto fallback; > + for (i = 0; i < WMI_REG_CURRENT_MAX_AP_TYPE; i++) { > + kfree(reg_info->reg_rules_6ghz_ap_ptr[i]); > + for (j = 0; j < WMI_REG_MAX_CLIENT_TYPE; j++) > + kfree(reg_info->reg_rules_6ghz_client_ptr[i][j]); > + } > + > + memset(reg_info, 0, sizeof(*reg_info)); > } > +} > + > +static > +enum wmi_vdev_type ath11k_reg_get_ar_vdev_type(struct ath11k *ar) > +{ > + struct ath11k_vif *arvif; > + > + /* Currently each struct ath11k maps to one struct ieee80211_hw/wiphy > + * and one struct ieee80211_regdomain, so it could only store one group > + * reg rules. It means muti-interface concurrency in the same ath11k is > + * not support for the regdomain. So get the vdev type of the first entry > + * now. After concurrency support for the regdomain, this should change. > + */ > + arvif = list_first_entry_or_null(&ar->arvifs, struct ath11k_vif, list); > + if (arvif) > + return arvif->vdev_type; > + > + return WMI_VDEV_TYPE_UNSPEC; > +} > + > +int ath11k_reg_handle_chan_list(struct ath11k_base *ab, > + struct cur_regulatory_info *reg_info, > + enum ieee80211_ap_reg_power power_type) > +{ > + struct ieee80211_regdomain *regd; > + bool intersect = false; > + int pdev_idx; > + struct ath11k *ar; > + enum wmi_vdev_type vdev_type; > > - ath11k_dbg(ab, ATH11K_DBG_WMI, "event reg chan list id %d", id); > + ath11k_dbg(ab, ATH11K_DBG_WMI, "event reg handle chan list"); I believe this debug was helpful in the sense it showed which type of event came. Can't we still print this somehow? Or may be somewhere else?
On 12/4/23 13:43, Baochen Qiang wrote: > --- a/drivers/net/wireless/ath/ath11k/mac.h > +++ b/drivers/net/wireless/ath/ath11k/mac.h > @@ -156,7 +156,6 @@ struct ath11k_vif *ath11k_mac_get_arvif_by_vdev_id(struct ath11k_base *ab, > u8 ath11k_mac_get_target_pdev_id(struct ath11k *ar); > u8 ath11k_mac_get_target_pdev_id_from_vif(struct ath11k_vif *arvif); > struct ath11k_vif *ath11k_mac_get_vif_up(struct ath11k_base *ab); > - Irrelevant change? > struct ath11k *ath11k_mac_get_ar_by_vdev_id(struct ath11k_base *ab, u32 vdev_id); > struct ath11k *ath11k_mac_get_ar_by_pdev_id(struct ath11k_base *ab, u32 pdev_id); > void ath11k_mac_drain_tx(struct ath11k *ar); > diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c > index 6f0a35fcc9c1..9a0568676a74 100644 > --- a/drivers/net/wireless/ath/ath11k/wmi.c > +++ b/drivers/net/wireless/ath/ath11k/wmi.c > @@ -9858,3 +9858,9 @@ int ath11k_wmi_sta_keepalive(struct ath11k *ar, > > return ath11k_wmi_cmd_send(wmi, skb, WMI_STA_KEEPALIVE_CMDID); > } > + > +bool ath11k_wmi_supports_6ghz_cc_ext(struct ath11k *ar) > +{ > + return (test_bit(WMI_TLV_SERVICE_REG_CC_EXT_EVENT_SUPPORT, > + ar->ab->wmi_ab.svc_map)) && ar->supports_6ghz; What is the use of first parenthesis? I don't see a closing one after ar->supports_6ghz so its just guarding test_bit() which is not required. I believe intention here was to guard whole expression?
On 12/6/23 11:04, Baochen Qiang wrote: > > > On 12/4/2023 11:53 PM, Aditya Kumar Singh wrote: >> On 12/4/23 13:43, Baochen Qiang wrote: >>> From: Wen Gong <quic_wgong@quicinc.com> >>> >>> When station is connected to a 6 GHz AP, it has 2 ways to configure >>> the power limit to firmware. The first way is to send 2 WMI commands >>> WMI_PDEV_PARAM_TXPOWER_LIMIT2G/WMI_PDEV_PARAM_TXPOWER_LIMIT5G to >>> firmware, the second way is to send WMI_VDEV_SET_TPC_POWER_CMDID to >>> firmware which include more parameters for power control. >>> >>> When firmware support SERVICE_EXT_TPC_REG, it means firmware support >>> the second way for WMI_VDEV_SET_TPC_POWER_CMDID, then ath11k discard >>> BSS_CHANGED_TXPOWER flag from mac80211 which is used to the first way >>> for 6 GHz band and select the second way. >>> >>> Tested-on: WCN6855 hw2.0 PCI >>> WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23 >>> >>> Signed-off-by: Wen Gong <quic_wgong@quicinc.com> >>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> >>> --- >> ...snip... >>> @@ -3596,9 +3608,13 @@ static void >>> ath11k_mac_op_bss_info_changed(struct ieee80211_hw *hw, >>> if (changed & BSS_CHANGED_TXPOWER) { >>> ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "vdev_id %i txpower %d\n", >>> arvif->vdev_id, info->txpower); >>> - >>> - arvif->txpower = info->txpower; >>> - ath11k_mac_txpower_recalc(ar); >>> + if (ath11k_mac_supports_station_tpc(ar, arvif, >>> &info->chandef)) { >>> + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, >>> + "discard tx power, change to set TPC power\n"); >>> + } else { >>> + arvif->txpower = info->txpower; >>> + ath11k_mac_txpower_recalc(ar); >>> + } >> >> Could you check v6 once? I remember Wen told he would drop this check >> and let FW take the min value. If we do like this, then user could not >> set his own desired value even if that is well inside the reg limits. > I did notice this comment in V6, but came out of a different opinion: it > is OK to discard the TX power here, because that will be sent to > firmware using WMI_VDEV_SET_TPC_POWER_CMDID command in another patch. > Please correct me if wrong. Yeah that is correct but applies only during initial bring up. What if after client gets connected and user still wants to lower power level by giving command "iw wlanX set txpower fixed 1000" something like this? This time again it will be ignored but it won't be sent to FW.
On 12/7/2023 11:31 AM, Aditya Kumar Singh wrote: > On 12/6/23 11:04, Baochen Qiang wrote: >> >> >> On 12/4/2023 11:53 PM, Aditya Kumar Singh wrote: >>> On 12/4/23 13:43, Baochen Qiang wrote: >>>> From: Wen Gong <quic_wgong@quicinc.com> >>>> >>>> When station is connected to a 6 GHz AP, it has 2 ways to configure >>>> the power limit to firmware. The first way is to send 2 WMI commands >>>> WMI_PDEV_PARAM_TXPOWER_LIMIT2G/WMI_PDEV_PARAM_TXPOWER_LIMIT5G to >>>> firmware, the second way is to send WMI_VDEV_SET_TPC_POWER_CMDID to >>>> firmware which include more parameters for power control. >>>> >>>> When firmware support SERVICE_EXT_TPC_REG, it means firmware support >>>> the second way for WMI_VDEV_SET_TPC_POWER_CMDID, then ath11k discard >>>> BSS_CHANGED_TXPOWER flag from mac80211 which is used to the first way >>>> for 6 GHz band and select the second way. >>>> >>>> Tested-on: WCN6855 hw2.0 PCI >>>> WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.23 >>>> >>>> Signed-off-by: Wen Gong <quic_wgong@quicinc.com> >>>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> >>>> --- >>> ...snip... >>>> @@ -3596,9 +3608,13 @@ static void >>>> ath11k_mac_op_bss_info_changed(struct ieee80211_hw *hw, >>>> if (changed & BSS_CHANGED_TXPOWER) { >>>> ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "vdev_id %i txpower %d\n", >>>> arvif->vdev_id, info->txpower); >>>> - >>>> - arvif->txpower = info->txpower; >>>> - ath11k_mac_txpower_recalc(ar); >>>> + if (ath11k_mac_supports_station_tpc(ar, arvif, >>>> &info->chandef)) { >>>> + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, >>>> + "discard tx power, change to set TPC power\n"); >>>> + } else { >>>> + arvif->txpower = info->txpower; >>>> + ath11k_mac_txpower_recalc(ar); >>>> + } >>> >>> Could you check v6 once? I remember Wen told he would drop this check >>> and let FW take the min value. If we do like this, then user could >>> not set his own desired value even if that is well inside the reg >>> limits. >> I did notice this comment in V6, but came out of a different opinion: >> it is OK to discard the TX power here, because that will be sent to >> firmware using WMI_VDEV_SET_TPC_POWER_CMDID command in another patch. >> Please correct me if wrong. > Yeah that is correct but applies only during initial bring up. What if > after client gets connected and user still wants to lower power level by > giving command "iw wlanX set txpower fixed 1000" something like this? > This time again it will be ignored but it won't be sent to FW. Exactly, will drop this patch in V9. >
On 12/7/2023 11:15 AM, Aditya Kumar Singh wrote: > On 12/4/23 13:43, Baochen Qiang wrote: > >> --- a/drivers/net/wireless/ath/ath11k/mac.h >> +++ b/drivers/net/wireless/ath/ath11k/mac.h >> @@ -156,7 +156,6 @@ struct ath11k_vif >> *ath11k_mac_get_arvif_by_vdev_id(struct ath11k_base *ab, >> u8 ath11k_mac_get_target_pdev_id(struct ath11k *ar); >> u8 ath11k_mac_get_target_pdev_id_from_vif(struct ath11k_vif *arvif); >> struct ath11k_vif *ath11k_mac_get_vif_up(struct ath11k_base *ab); >> - > > Irrelevant change? > >> struct ath11k *ath11k_mac_get_ar_by_vdev_id(struct ath11k_base *ab, >> u32 vdev_id); >> struct ath11k *ath11k_mac_get_ar_by_pdev_id(struct ath11k_base *ab, >> u32 pdev_id); >> void ath11k_mac_drain_tx(struct ath11k *ar); >> diff --git a/drivers/net/wireless/ath/ath11k/wmi.c >> b/drivers/net/wireless/ath/ath11k/wmi.c >> index 6f0a35fcc9c1..9a0568676a74 100644 >> --- a/drivers/net/wireless/ath/ath11k/wmi.c >> +++ b/drivers/net/wireless/ath/ath11k/wmi.c >> @@ -9858,3 +9858,9 @@ int ath11k_wmi_sta_keepalive(struct ath11k *ar, >> return ath11k_wmi_cmd_send(wmi, skb, WMI_STA_KEEPALIVE_CMDID); >> } >> + >> +bool ath11k_wmi_supports_6ghz_cc_ext(struct ath11k *ar) >> +{ >> + return (test_bit(WMI_TLV_SERVICE_REG_CC_EXT_EVENT_SUPPORT, >> + ar->ab->wmi_ab.svc_map)) && ar->supports_6ghz; > > What is the use of first parenthesis? I don't see a closing one after > ar->supports_6ghz so its just guarding test_bit() which is not required. > I believe intention here was to guard whole expression? I don't see an need to guard the whole expression or the test_bit(), so will drop extra parenthesis in V9.
On 12/7/2023 11:15 AM, Aditya Kumar Singh wrote: > On 12/4/23 13:43, Baochen Qiang wrote: >> --- a/drivers/net/wireless/ath/ath11k/mac.h >> +++ b/drivers/net/wireless/ath/ath11k/mac.h >> @@ -159,7 +159,6 @@ struct ath11k_vif *ath11k_mac_get_vif_up(struct >> ath11k_base *ab); >> struct ath11k *ath11k_mac_get_ar_by_vdev_id(struct ath11k_base *ab, >> u32 vdev_id); >> struct ath11k *ath11k_mac_get_ar_by_pdev_id(struct ath11k_base *ab, >> u32 pdev_id); >> - > > Irrelevant change w.r.t commit message? > > >> void ath11k_mac_drain_tx(struct ath11k *ar); >> void ath11k_mac_peer_cleanup_all(struct ath11k *ar); >> int ath11k_mac_tx_mgmt_pending_free(int buf_id, void *skb, void *ctx); > ... >> @@ -4749,6 +4749,11 @@ static int >> ath11k_wmi_tlv_ext_soc_hal_reg_caps_parse(struct ath11k_base *soc, >> soc->pdevs[0].pdev_id = 0; >> } >> + if (!soc->reg_info_store) >> + soc->reg_info_store = kcalloc(soc->num_radios, >> + sizeof(*soc->reg_info_store), >> + GFP_ATOMIC); > What if this memory allocation request fails? Any negative case check > should be present? > > >> + >> return 0; >> } >> @@ -7071,33 +7076,54 @@ static bool ath11k_reg_is_world_alpha(char >> *alpha) >> return false; >> } >> -static int ath11k_reg_chan_list_event(struct ath11k_base *ab, >> - struct sk_buff *skb, >> - enum wmi_reg_chan_list_cmd_type id) >> +void ath11k_reg_reset_info(struct cur_regulatory_info *reg_info) >> { >> - struct cur_regulatory_info *reg_info = NULL; >> - struct ieee80211_regdomain *regd = NULL; >> - bool intersect = false; >> - int ret = 0, pdev_idx, i, j; >> - struct ath11k *ar; >> + int i, j; >> - reg_info = kzalloc(sizeof(*reg_info), GFP_ATOMIC); >> - if (!reg_info) { >> - ret = -ENOMEM; >> - goto fallback; >> - } >> + if (reg_info) { >> + kfree(reg_info->reg_rules_2ghz_ptr); >> - if (id == WMI_REG_CHAN_LIST_CC_ID) >> - ret = ath11k_pull_reg_chan_list_update_ev(ab, skb, reg_info); >> - else >> - ret = ath11k_pull_reg_chan_list_ext_update_ev(ab, skb, >> reg_info); >> + kfree(reg_info->reg_rules_5ghz_ptr); >> - if (ret) { >> - ath11k_warn(ab, "failed to extract regulatory info from >> received event\n"); >> - goto fallback; >> + for (i = 0; i < WMI_REG_CURRENT_MAX_AP_TYPE; i++) { >> + kfree(reg_info->reg_rules_6ghz_ap_ptr[i]); >> + for (j = 0; j < WMI_REG_MAX_CLIENT_TYPE; j++) >> + kfree(reg_info->reg_rules_6ghz_client_ptr[i][j]); >> + } >> + >> + memset(reg_info, 0, sizeof(*reg_info)); >> } >> +} >> + >> +static >> +enum wmi_vdev_type ath11k_reg_get_ar_vdev_type(struct ath11k *ar) >> +{ >> + struct ath11k_vif *arvif; >> + >> + /* Currently each struct ath11k maps to one struct >> ieee80211_hw/wiphy >> + * and one struct ieee80211_regdomain, so it could only store one >> group >> + * reg rules. It means muti-interface concurrency in the same >> ath11k is >> + * not support for the regdomain. So get the vdev type of the >> first entry >> + * now. After concurrency support for the regdomain, this should >> change. >> + */ >> + arvif = list_first_entry_or_null(&ar->arvifs, struct ath11k_vif, >> list); >> + if (arvif) >> + return arvif->vdev_type; >> + >> + return WMI_VDEV_TYPE_UNSPEC; >> +} >> + >> +int ath11k_reg_handle_chan_list(struct ath11k_base *ab, >> + struct cur_regulatory_info *reg_info, >> + enum ieee80211_ap_reg_power power_type) >> +{ >> + struct ieee80211_regdomain *regd; >> + bool intersect = false; >> + int pdev_idx; >> + struct ath11k *ar; >> + enum wmi_vdev_type vdev_type; >> - ath11k_dbg(ab, ATH11K_DBG_WMI, "event reg chan list id %d", id); >> + ath11k_dbg(ab, ATH11K_DBG_WMI, "event reg handle chan list"); > > I believe this debug was helpful in the sense it showed which type of > event came. Can't we still print this somehow? Or may be somewhere else?You can check the event type from logs of ath11k_pull_reg_chan_list_update_ev() and ath11k_pull_reg_chan_list_ext_update_ev().
Baochen Qiang <quic_bqiang@quicinc.com> writes: > On 12/7/2023 11:15 AM, Aditya Kumar Singh wrote: >> On 12/4/23 13:43, Baochen Qiang wrote: >>> --- a/drivers/net/wireless/ath/ath11k/mac.h >>> +++ b/drivers/net/wireless/ath/ath11k/mac.h >>> @@ -159,7 +159,6 @@ struct ath11k_vif *ath11k_mac_get_vif_up(struct >>> ath11k_base *ab); >>> struct ath11k *ath11k_mac_get_ar_by_vdev_id(struct ath11k_base >>> *ab, u32 vdev_id); >>> struct ath11k *ath11k_mac_get_ar_by_pdev_id(struct ath11k_base >>> *ab, u32 pdev_id); >>> - >> Irrelevant change w.r.t commit message? >> >>> void ath11k_mac_drain_tx(struct ath11k *ar); >>> void ath11k_mac_peer_cleanup_all(struct ath11k *ar); >>> int ath11k_mac_tx_mgmt_pending_free(int buf_id, void *skb, void *ctx); >> ... >>> @@ -4749,6 +4749,11 @@ static int >>> ath11k_wmi_tlv_ext_soc_hal_reg_caps_parse(struct ath11k_base *soc, >>> soc->pdevs[0].pdev_id = 0; >>> } >>> + if (!soc->reg_info_store) >>> + soc->reg_info_store = kcalloc(soc->num_radios, >>> + sizeof(*soc->reg_info_store), >>> + GFP_ATOMIC); >> What if this memory allocation request fails? Any negative case >> check should be present? >> >>> + >>> return 0; >>> } >>> @@ -7071,33 +7076,54 @@ static bool ath11k_reg_is_world_alpha(char >>> *alpha) >>> return false; >>> } >>> -static int ath11k_reg_chan_list_event(struct ath11k_base *ab, >>> - struct sk_buff *skb, >>> - enum wmi_reg_chan_list_cmd_type id) >>> +void ath11k_reg_reset_info(struct cur_regulatory_info *reg_info) >>> { >>> - struct cur_regulatory_info *reg_info = NULL; >>> - struct ieee80211_regdomain *regd = NULL; >>> - bool intersect = false; >>> - int ret = 0, pdev_idx, i, j; >>> - struct ath11k *ar; >>> + int i, j; >>> - reg_info = kzalloc(sizeof(*reg_info), GFP_ATOMIC); >>> - if (!reg_info) { >>> - ret = -ENOMEM; >>> - goto fallback; >>> - } >>> + if (reg_info) { >>> + kfree(reg_info->reg_rules_2ghz_ptr); >>> - if (id == WMI_REG_CHAN_LIST_CC_ID) >>> - ret = ath11k_pull_reg_chan_list_update_ev(ab, skb, reg_info); >>> - else >>> - ret = ath11k_pull_reg_chan_list_ext_update_ev(ab, skb, >>> reg_info); >>> + kfree(reg_info->reg_rules_5ghz_ptr); >>> - if (ret) { >>> - ath11k_warn(ab, "failed to extract regulatory info from >>> received event\n"); >>> - goto fallback; >>> + for (i = 0; i < WMI_REG_CURRENT_MAX_AP_TYPE; i++) { >>> + kfree(reg_info->reg_rules_6ghz_ap_ptr[i]); >>> + for (j = 0; j < WMI_REG_MAX_CLIENT_TYPE; j++) >>> + kfree(reg_info->reg_rules_6ghz_client_ptr[i][j]); >>> + } >>> + >>> + memset(reg_info, 0, sizeof(*reg_info)); >>> } >>> +} >>> + >>> +static >>> +enum wmi_vdev_type ath11k_reg_get_ar_vdev_type(struct ath11k *ar) >>> +{ >>> + struct ath11k_vif *arvif; >>> + >>> + /* Currently each struct ath11k maps to one struct >>> ieee80211_hw/wiphy >>> + * and one struct ieee80211_regdomain, so it could only store >>> one group >>> + * reg rules. It means muti-interface concurrency in the same >>> ath11k is >>> + * not support for the regdomain. So get the vdev type of the >>> first entry >>> + * now. After concurrency support for the regdomain, this >>> should change. >>> + */ >>> + arvif = list_first_entry_or_null(&ar->arvifs, struct >>> ath11k_vif, list); >>> + if (arvif) >>> + return arvif->vdev_type; >>> + >>> + return WMI_VDEV_TYPE_UNSPEC; >>> +} >>> + >>> +int ath11k_reg_handle_chan_list(struct ath11k_base *ab, >>> + struct cur_regulatory_info *reg_info, >>> + enum ieee80211_ap_reg_power power_type) >>> +{ >>> + struct ieee80211_regdomain *regd; >>> + bool intersect = false; >>> + int pdev_idx; >>> + struct ath11k *ar; >>> + enum wmi_vdev_type vdev_type; >>> - ath11k_dbg(ab, ATH11K_DBG_WMI, "event reg chan list id %d", id); >>> + ath11k_dbg(ab, ATH11K_DBG_WMI, "event reg handle chan list"); >> I believe this debug was helpful in the sense it showed which type >> of event came. Can't we still print this somehow? Or may be >> somewhere else?You can check the event type from logs of > ath11k_pull_reg_chan_list_update_ev() and > ath11k_pull_reg_chan_list_ext_update_ev(). Baochen, I didn't see any comments from you. Did you send an empty mail by accident?
On 12/11/2023 11:18 PM, Kalle Valo wrote: > Baochen Qiang <quic_bqiang@quicinc.com> writes: > >> On 12/7/2023 11:15 AM, Aditya Kumar Singh wrote: >>> On 12/4/23 13:43, Baochen Qiang wrote: >>>> --- a/drivers/net/wireless/ath/ath11k/mac.h >>>> +++ b/drivers/net/wireless/ath/ath11k/mac.h >>>> @@ -159,7 +159,6 @@ struct ath11k_vif *ath11k_mac_get_vif_up(struct >>>> ath11k_base *ab); >>>> struct ath11k *ath11k_mac_get_ar_by_vdev_id(struct ath11k_base >>>> *ab, u32 vdev_id); >>>> struct ath11k *ath11k_mac_get_ar_by_pdev_id(struct ath11k_base >>>> *ab, u32 pdev_id); >>>> - >>> Irrelevant change w.r.t commit message? >>> >>>> void ath11k_mac_drain_tx(struct ath11k *ar); >>>> void ath11k_mac_peer_cleanup_all(struct ath11k *ar); >>>> int ath11k_mac_tx_mgmt_pending_free(int buf_id, void *skb, void *ctx); >>> ... >>>> @@ -4749,6 +4749,11 @@ static int >>>> ath11k_wmi_tlv_ext_soc_hal_reg_caps_parse(struct ath11k_base *soc, >>>> soc->pdevs[0].pdev_id = 0; >>>> } >>>> + if (!soc->reg_info_store) >>>> + soc->reg_info_store = kcalloc(soc->num_radios, >>>> + sizeof(*soc->reg_info_store), >>>> + GFP_ATOMIC); >>> What if this memory allocation request fails? Any negative case >>> check should be present? >>> >>>> + >>>> return 0; >>>> } >>>> @@ -7071,33 +7076,54 @@ static bool ath11k_reg_is_world_alpha(char >>>> *alpha) >>>> return false; >>>> } >>>> -static int ath11k_reg_chan_list_event(struct ath11k_base *ab, >>>> - struct sk_buff *skb, >>>> - enum wmi_reg_chan_list_cmd_type id) >>>> +void ath11k_reg_reset_info(struct cur_regulatory_info *reg_info) >>>> { >>>> - struct cur_regulatory_info *reg_info = NULL; >>>> - struct ieee80211_regdomain *regd = NULL; >>>> - bool intersect = false; >>>> - int ret = 0, pdev_idx, i, j; >>>> - struct ath11k *ar; >>>> + int i, j; >>>> - reg_info = kzalloc(sizeof(*reg_info), GFP_ATOMIC); >>>> - if (!reg_info) { >>>> - ret = -ENOMEM; >>>> - goto fallback; >>>> - } >>>> + if (reg_info) { >>>> + kfree(reg_info->reg_rules_2ghz_ptr); >>>> - if (id == WMI_REG_CHAN_LIST_CC_ID) >>>> - ret = ath11k_pull_reg_chan_list_update_ev(ab, skb, reg_info); >>>> - else >>>> - ret = ath11k_pull_reg_chan_list_ext_update_ev(ab, skb, >>>> reg_info); >>>> + kfree(reg_info->reg_rules_5ghz_ptr); >>>> - if (ret) { >>>> - ath11k_warn(ab, "failed to extract regulatory info from >>>> received event\n"); >>>> - goto fallback; >>>> + for (i = 0; i < WMI_REG_CURRENT_MAX_AP_TYPE; i++) { >>>> + kfree(reg_info->reg_rules_6ghz_ap_ptr[i]); >>>> + for (j = 0; j < WMI_REG_MAX_CLIENT_TYPE; j++) >>>> + kfree(reg_info->reg_rules_6ghz_client_ptr[i][j]); >>>> + } >>>> + >>>> + memset(reg_info, 0, sizeof(*reg_info)); >>>> } >>>> +} >>>> + >>>> +static >>>> +enum wmi_vdev_type ath11k_reg_get_ar_vdev_type(struct ath11k *ar) >>>> +{ >>>> + struct ath11k_vif *arvif; >>>> + >>>> + /* Currently each struct ath11k maps to one struct >>>> ieee80211_hw/wiphy >>>> + * and one struct ieee80211_regdomain, so it could only store >>>> one group >>>> + * reg rules. It means muti-interface concurrency in the same >>>> ath11k is >>>> + * not support for the regdomain. So get the vdev type of the >>>> first entry >>>> + * now. After concurrency support for the regdomain, this >>>> should change. >>>> + */ >>>> + arvif = list_first_entry_or_null(&ar->arvifs, struct >>>> ath11k_vif, list); >>>> + if (arvif) >>>> + return arvif->vdev_type; >>>> + >>>> + return WMI_VDEV_TYPE_UNSPEC; >>>> +} >>>> + >>>> +int ath11k_reg_handle_chan_list(struct ath11k_base *ab, >>>> + struct cur_regulatory_info *reg_info, >>>> + enum ieee80211_ap_reg_power power_type) >>>> +{ >>>> + struct ieee80211_regdomain *regd; >>>> + bool intersect = false; >>>> + int pdev_idx; >>>> + struct ath11k *ar; >>>> + enum wmi_vdev_type vdev_type; >>>> - ath11k_dbg(ab, ATH11K_DBG_WMI, "event reg chan list id %d", id); >>>> + ath11k_dbg(ab, ATH11K_DBG_WMI, "event reg handle chan list"); >>> I believe this debug was helpful in the sense it showed which type >>> of event came. Can't we still print this somehow? Or may be >>> somewhere else?You can check the event type from logs of >> ath11k_pull_reg_chan_list_update_ev() and >> ath11k_pull_reg_chan_list_ext_update_ev(). > > Baochen, I didn't see any comments from you. Did you send an empty mail > by accident? I did have comments, but some how, I don't know why, it gets merged in Aditya's comments. Will repost it here: You can check the event type from logs of ath11k_pull_reg_chan_list_update_ev() and ath11k_pull_reg_chan_list_ext_update_ev(). >
This introduced some new concept: power type of AP(STANDARD_POWER_AP, INDOOR_AP, VERY_LOW_POWER_AP) power type of STATION(DEFAULT_CLIENT, SUBORDINATE_CLIENT) power spectral density(psd) This patchset is to implement the new rules for 6 GHz band in ath11k. ath11k parsed the reg rules from new WMI event WMI_REG_CHAN_LIST_CC_EXT_EVENTID and parse the transmit power envelope element in beacon of AP and then set new WMI command WMI_VDEV_SET_TPC_POWER_CMDID to firmware when connect to 6G AP, also support backward compatibility with firmware which not support new wmi cmd WMI_VDEV_SET_TPC_POWER_CMDID. v8: add my own s-o-b tag to each patch if not present. Also rebased to ToT. v7: address review comments per Kalle, Jeff and Aditya. Also rebased to ToT. v6: (NOT depends to any patch now) 1. The dependent patch "wifi: cfg80211: save power spectral density(psd) of regulatory rule" has upstream to wireless-next https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?id=ddd7f45c899f7524bdbe6a32fe4906cde8b07b9b The prerequisite-patch is cherry-pick from wireless-next So add back the other patches in v3 since no dependency to cfg80211 public patch above now. [v3,08/15] wifi: ath11k: save power spectral density(psd) of regulatory rule [v3,09/15] wifi: ath11k: add parse of transmit power envelope element [v3,10/15] wifi: ath11k: save max tx power in vdev start response event from firmware [v3,11/15] wifi: ath11k: fill parameters for vdev_set_tpc_power wmi command [v3,12/15] wifi: ath11k: add WMI_TLV_SERVICE_EXT_TPC_REG_SUPPORT service bit [v3,13/15] wifi: ath11k: discard BSS_CHANGED_TXPOWER when EXT_TPC_REG_SUPPORT for 6 GHz [v3,14/15] wifi: ath11k: add handler for WMI_VDEV_SET_TPC_POWER_CMDID [v3,15/15] wifi: ath11k: send TPC power to firmware for 6 GHz station 2. rename some "6g" to "6ghz" 3. remove "static" for ath11k_reg_ap_pwr_convert() 4. add 20 Mhz check in ath11k_mac_get_eirp_power() 5. remove min_t() in ath11k_mac_fill_reg_tpc_info() for not is_tpe_present 6. rebased to ath-202309051328 link of v5: [PATCH v5 0/5] fix wrong TX power and frequency in regdomain by dynamic switch 6 GHz reg rules of LPI/SP/VLP for station mode https://lore.kernel.org/linux-wireless/20230803071701.15084-1-quic_wgong@quicinc.com/ v5: change per Kalle and rebased to ath.git ath-202306211808 1. ath11k_ieee80211_ap_pwr_type_convert() to ath11k_reg_ap_pwr_convert() 2. used list_first_entry_or_null() and add comments 3. ath11k_dbg() to ath11k_warn() 4. ath11k_hw_supports_6g_cc_ext() to ath11k_mac_supports_6g_cc_ext() 5. add mesh in commit log v4: (NOT depends to any patch now). 1. removed patches which depends on wifi: cfg80211: save Power Spectral Density (PSD) of the regulatory rule https://lore.kernel.org/linux-wireless/20230315132904.31779-3-quic_adisi@quicinc.com/ removed: [v3,08/15] wifi: ath11k: save power spectral density(psd) of regulatory rule [v3,09/15] wifi: ath11k: add parse of transmit power envelope element [v3,10/15] wifi: ath11k: save max tx power in vdev start response event from firmware [v3,11/15] wifi: ath11k: fill parameters for vdev_set_tpc_power wmi command [v3,12/15] wifi: ath11k: add WMI_TLV_SERVICE_EXT_TPC_REG_SUPPORT service bit [v3,13/15] wifi: ath11k: discard BSS_CHANGED_TXPOWER when EXT_TPC_REG_SUPPORT for 6 GHz [v3,14/15] wifi: ath11k: add handler for WMI_VDEV_SET_TPC_POWER_CMDID [v3,15/15] wifi: ath11k: send TPC power to firmware for 6 GHz station 2. rebased to ath.git ath-202304281700 3. deleted "wifi: ath11k: Add support to parse new wmi event for 6 GHz regulatory" which is alreay upstream. link of v3: [v3,00/15] wifi: ath11k: add support for 6 GHz station for various modes : LPI, SP and VLP https://patchwork.kernel.org/project/linux-wireless/cover/20220913051518.23051-1-quic_wgong@quicinc.com/ v3: 1. added "ath11k: fix a possible dead lock caused by ab->base_lock". 3. deleted "ath11k: add support for extended wmi service bit" which is alreay upstream. v2: 1. change some minor comments by Kalle. 2. rebased to ath.git ath-202112220603 Baochen Qiang (1): wifi: ath11k: fix a possible dead lock caused by ab->base_lock Wen Gong (11): wifi: ath11k: add support to select 6 GHz regulatory type wifi: ath11k: store cur_regulatory_info for each radio wifi: ath11k: update regulatory rules when interface added wifi: ath11k: update regulatory rules when connect to AP on 6 GHz band for station wifi: ath11k: save power spectral density(PSD) of regulatory rule wifi: ath11k: add parse of transmit power envelope element wifi: ath11k: save max tx power in vdev start response event from firmware wifi: ath11k: fill parameters for vdev set tpc power WMI command wifi: ath11k: add WMI_TLV_SERVICE_EXT_TPC_REG_SUPPORT service bit wifi: ath11k: add handler for WMI_VDEV_SET_TPC_POWER_CMDID wifi: ath11k: discard BSS_CHANGED_TXPOWER when EXT_TPC_REG_SUPPORT for 6 GHz drivers/net/wireless/ath/ath11k/core.h | 40 ++ drivers/net/wireless/ath/ath11k/mac.c | 516 ++++++++++++++++++++++++- drivers/net/wireless/ath/ath11k/mac.h | 5 +- drivers/net/wireless/ath/ath11k/reg.c | 89 ++++- drivers/net/wireless/ath/ath11k/reg.h | 6 +- drivers/net/wireless/ath/ath11k/wmi.c | 231 ++++++++--- drivers/net/wireless/ath/ath11k/wmi.h | 69 ++++ 7 files changed, 882 insertions(+), 74 deletions(-) base-commit: b648266cb860d3ea51f0a2b36d2ce4a9cec1ed16