Johannes Berg <johannes@xxxxxxxxxxxxxxxx> writes:
> On Wed, 2019-10-23 at 11:59 +0200, Toke Høiland-Jørgensen wrote:
>>
>> + if (info->tx_time_est) {
>> + struct sta_info *sta = NULL, *s;
>> + struct rhlist_head *tmp;
>> +
>> + rcu_read_lock();
>> +
>> + for_each_sta_info(local, hdr->addr1, s, tmp) {
>> + /* skip wrong virtual interface */
>> + if (!ether_addr_equal(hdr->addr2, s->sdata->vif.addr))
>> + continue;
>> +
>> + sta = s;
>> + break;
>> + }
>
> I guess that is better than looking up the sdata and then using
> sta_info_get(), but I think I'd like to see this wrapped into a function
> (even if it's an inline) in sta_info.{c,h}.
OK, can do.
>> + airtime = ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>> + skb->len);
>> + if (airtime) {
>> + /* We only have 10 bits in tx_time_est, so store airtime
>> + * in increments of 4us and clamp the maximum to 2**12-1
>> + */
>> + airtime = min_t(u32, airtime, 4095) & ~3U;
>> + info->tx_time_est = airtime >> 2;
>> + ieee80211_sta_update_pending_airtime(local, tx.sta,
>> + txq->ac, airtime,
>> + false);
>
> I wonder if it'd be better to pass the shifted value to
> ieee80211_sta_update_pending_airtime() to avoid all the shifting in all
> places?
>
> You could even store the shifted value in "aql_tx_pending" and
> "aql_total_pending_airtime" etc., it's completely equivalent, and only
> shift it out for people looking at debugfs.
My reasoning for doing it this way was that we have another set of APIs
dealing with airtime which doesn't do any shifting; so keeping the APIs
in the same unit (straight airtime) seemed less confusing.
We could add (inline) setter and getter functions for the tx_time_est
field instead to avoid sprinkling shifts all over the place? :)
-Toke
