On 5/30/2024 11:20 AM, Russell King (Oracle) wrote: [...]
> The original code was > tx_lnk_free_pkts = status->counters.tx_lnk_free_pkts[i]; > diff = (tx_lnk_free_pkts - lnk->prev_freed_pkts) & 0xff; > > if (diff == 0) > continue; > > I wonder if comparing tx_lnk_free_pkts to 0 was added intentionally? This is > monotonously incremented counter so 0 is not significant, unlike the diff. > Have I missed something? You are... While you're correct about the original code, your quote is somewhat incomplete. + if ( (isSta == true) && (i == wlvifSta->sta.hlid) && (test_bit(WLVIF_FLAG_STA_AUTHORIZED, &wlvifSta->flags)) && (status->counters.tx_lnk_free_pkts[i] > 0) ) ... + } + if ( (isAp == true) && (test_bit(i, &wlvifAp->ap.sta_hlid_map[0])) && (test_bit(WLVIF_FLAG_AP_STARTED, &wlvifAp->flags)) && (wlvifAp->inconn_count == 0) && (status->counters.tx_lnk_free_pkts[i] > 0) ) ... + } }
Sorry, considered only the diff with base branch, not the original patch.
Note that both of these if() conditions can only be executed if the final condition in each is true. Both check for the same thing, which is: status->counters.tx_lnk_free_pkts[i] > 0 In my patch, tx_lnk_free_pkts is status->counters.tx_lnk_free_pkts. Therefore, there is no point in evaluating either of these excessively long if() conditions in the original code when tx_lnk_free_pkts is less than zero or zero - and thus the logic between TI's original patch and my change is preserved. Whether that condition in the original patch is correct or not is the subject of that FIXME comment - I believe TI's code is incorrect, since it is possible that tx_lnk_free_pkts, which is a u8 that is incremented by the number of free packets, will hit zero at some point just as a matter of one extra packet being freed when the counter was 255. Moving it out of those two if() statements makes the issue very obvious. It would be nice to get a view from TI on whether the original patch is actually correct in this regard. I believe TI's original patch is buggy.
After consulting with the author I do not believe it is buggy. It was the most painless way to prevent issues with the recovery flow. Indeed there will be case where tx_lnk_free_pkts is 0 again in which case the internal counters will not be updated. This was considered OK as this is usually a transient state and the counter will advance eventually. For the unlikely case where FW crashed just after update was skipped, well, there will be a small rollback in the PN after recovery which means a few frames will get lost. This as considered acceptable.
Michael.
