Re: [PATCH v8 02/12] wifi: ath11k: store cur_regulatory_info for each radio

[Baochen Qiang <quic_bqiang@xxxxxxxxxxx>]

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().