[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ceba870f-34d3-4545-884e-f04f966646ab@ti.com>
Date: Thu, 30 May 2024 15:25:59 +0300
From: "Nemanov, Michael" <michael.nemanov@...com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
CC: Kalle Valo <kvalo@...nel.org>, Johannes Berg <johannes.berg@...el.com>,
<linux-kernel@...r.kernel.org>, <linux-wireless@...r.kernel.org>
Subject: Re: [PATCH wireless-next 6/8] wifi: wlcore: add pn16 support
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.
Powered by blists - more mailing lists