Received-SPF: pass (google.com: domain of linux-wireless+bounces-8312-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ceba870f-34d3-4545-884e-f04f966646ab@ti.com>
Date: Thu, 30 May 2024 15:25:59 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH wireless-next 6/8] wifi: wlcore: add pn16 support
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
CC: Kalle Valo <kvalo@kernel.org>, Johannes Berg <johannes.berg@intel.com>,
        <linux-kernel@vger.kernel.org>, <linux-wireless@vger.kernel.org>
References: <ZlWhH4HleGILuUtN@shell.armlinux.org.uk>
 <E1sBsy7-00E8vu-Nc@rmk-PC.armlinux.org.uk>
 <e6ae6dfa-6554-4e88-abb0-31dbbd8df03f@ti.com>
 <Zlg2zGb7s7zu6jb+@shell.armlinux.org.uk>
Content-Language: en-US
From: "Nemanov, Michael" <michael.nemanov@ti.com>
In-Reply-To: <Zlg2zGb7s7zu6jb+@shell.armlinux.org.uk>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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.