> -----Original Message----- > From: Stanislaw Gruszka [mailto:sgruszka@xxxxxxxxxx] > Sent: Thursday, October 04, 2018 9:19 PM > To: Tony Chuang > Cc: kvalo@xxxxxxxxxxxxxx; Larry.Finger@xxxxxxxxxxxx; Pkshih; Andy Huang; > linux-wireless@xxxxxxxxxxxxxxx > Subject: Re: [RFC v2 04/12] rtw88: trx files > > On Wed, Oct 03, 2018 at 04:02:20PM +0800, yhchuang@xxxxxxxxxxx wrote: > > +static void rtw_rx_rssi_add(struct rtw_dev *rtwdev, > > + struct rtw_rx_pkt_stat *pkt_stat, > > + struct ieee80211_hdr *hdr) > > +{ > > + struct ieee80211_vif *vif; > > + struct rtw_vif *rtwvif; > > + struct rtw_sta_info *si; > > + __le16 fc = hdr->frame_control; > > + u8 *bssid; > > + u8 macid = RTW_BC_MC_MACID; > > + bool match_bssid = false; > > + bool is_packet_match_bssid; > > + bool if_addr_match; > > + bool hw_err; > > + bool ctl; > > + > > + rcu_read_lock(); > > + > > + bssid = get_hdr_bssid(hdr); > > + rtwvif = get_hdr_vif(rtwdev, hdr); > > + vif = rtwvif ? rtwvif->vif : NULL; > > + pkt_stat->vif = vif; > > + if (unlikely(is_broadcast_ether_addr(hdr->addr1) || > > + is_multicast_ether_addr(hdr->addr1))) > > + match_bssid = get_hdr_match_bssid(rtwdev, hdr, bssid); > > + else if (vif) > > + match_bssid = ether_addr_equal(vif->bss_conf.bssid, bssid); > > + si = get_hdr_sta(rtwdev, vif, hdr); > > + macid = si ? si->mac_id : RTW_BC_MC_MACID; > > + pkt_stat->mac_id = macid; > > + pkt_stat->si = si; > > + > > + if_addr_match = !!vif; > > + hw_err = pkt_stat->crc_err || pkt_stat->icv_err; > > + ctl = ieee80211_is_ctl(fc); > > + is_packet_match_bssid = !hw_err && !ctl && match_bssid; > > + > > + if (((match_bssid && if_addr_match) || ieee80211_is_beacon(fc)) && > > + (!hw_err && !ctl) && (pkt_stat->phy_status && pkt_stat->si)) > > + ewma_rssi_add(&pkt_stat->si->avg_rssi, pkt_stat->rssi); > > + > > + rcu_read_unlock(); > > What for rcu_read_lock/unlock is here ? Maybe is needed, > but perhaps not to protect entire function ? > I thought that the entire function uses pointer si and vif, and hence should be protected by rcu read lock, am I using the lock in a wrong way? > > +static u8 get_tx_ampdu_factor(struct ieee80211_sta *sta) > > +{ > > + u8 exp = sta->ht_cap.ampdu_factor; > > + > > + /* the least ampdu factor is 8K, and the value in the tx desc is the > > + * max aggregation num, which represents val * 2 packets can be > > + * aggregated in an AMPDU, so here we should use 8/2=4 as the base > > + */ > > + return (BIT(2) << exp) - 1; > Using 4 whould be much more readable. > OK > > +static void rtw_tx_data_pkt_info_update(struct rtw_dev *rtwdev, > > + struct rtw_tx_pkt_info *pkt_info, > > + struct ieee80211_tx_control *control, > > + struct sk_buff *skb) > > +{ > <snip> > > + if (sta->vht_cap.vht_supported) > > + rate = get_highest_vht_tx_rate(rtwdev, sta); > > + else if (sta->ht_cap.ht_supported) > > + rate = get_highest_ht_tx_rate(rtwdev, sta); > > + else if (sta->supp_rates[0] <= 0xf) > > + rate = DESC_RATE11M; > > + else > > + rate = DESC_RATE54M; > No rate control, just use highest possible rate for each standard ? > The major rate control system is in firmware, this is just a hint for it. > > + > > + pkt_info->bmc = bmc; > > + pkt_info->sec_type = sec_type; > > + pkt_info->tx_pkt_size = skb->len; > > + pkt_info->offset = chip->tx_pkt_desc_sz; > > + pkt_info->qsel = skb->priority; > > Shouldn't be qsel somehow mapped from skb->priority ? Firmware handles it. > > Thanks > Stanislaw Thanks Yan-Hsuan Chuang
