Received-SPF: pass (google.com: domain of linux-wireless+bounces-8307-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 30 May 2024 09:20:28 +0100
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: "Nemanov, Michael" <michael.nemanov@ti.com>
Cc: Kalle Valo <kvalo@kernel.org>, Johannes Berg <johannes.berg@intel.com>,
	linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org
Subject: Re: [EXTERNAL] [PATCH wireless-next 6/8] wifi: wlcore: add pn16
 support
Message-ID: <Zlg2zGb7s7zu6jb+@shell.armlinux.org.uk>
References: <ZlWhH4HleGILuUtN@shell.armlinux.org.uk>
 <E1sBsy7-00E8vu-Nc@rmk-PC.armlinux.org.uk>
 <e6ae6dfa-6554-4e88-abb0-31dbbd8df03f@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e6ae6dfa-6554-4e88-abb0-31dbbd8df03f@ti.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Thu, May 30, 2024 at 11:06:40AM +0300, Nemanov, Michael wrote:
> 
> On 5/28/2024 12:18 PM, Russell King (Oracle) wrote:
> 
> [...]
> 
> >    static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
> >    {
> > +	struct wl12xx_vif *wlvifsta;
> > +	struct wl12xx_vif *wlvifap;
> >    	struct wl12xx_vif *wlvif;
> >    	u32 old_tx_blk_count = wl->tx_blocks_available;
> >    	int avail, freed_blocks;
> > @@ -410,23 +412,100 @@ static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
> >    		wl->tx_pkts_freed[i] = status->counters.tx_released_pkts[i];
> >    	}
> [...]
> >    	for_each_set_bit(i, wl->links_map, wl->num_links) {
> > +		u16 diff16, sec_pn16;
> >    		u8 diff, tx_lnk_free_pkts;
> > +
> >    		lnk = &wl->links[i];
> >    		/* prevent wrap-around in freed-packets counter */
> >    		tx_lnk_free_pkts = status->counters.tx_lnk_free_pkts[i];
> >    		diff = (tx_lnk_free_pkts - lnk->prev_freed_pkts) & 0xff;
> > -		if (diff == 0)
> > +		if (diff) {
> > +			lnk->allocated_pkts -= diff;
> > +			lnk->prev_freed_pkts = tx_lnk_free_pkts;
> > +		}
> > +
> > +		/* Get the current sec_pn16 value if present */
> > +		if (status->counters.tx_lnk_sec_pn16)
> > +			sec_pn16 = __le16_to_cpu(status->counters.tx_lnk_sec_pn16[i]);
> > +		else
> > +			sec_pn16 = 0;
> > +		/* prevent wrap-around in pn16 counter */
> > +		diff16 = (sec_pn16 - lnk->prev_sec_pn16) & 0xffff;
> > +
> > +		/* FIXME: since free_pkts is a 8-bit counter of packets that
> > +		 * rolls over, it can become zero. If it is zero, then we
> > +		 * omit processing below. Is that really correct?
> > +		 */
> > +		if (tx_lnk_free_pkts <= 0)
> >    			continue;
> 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) )
...
+		}
 	}

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!