Received-SPF: pass (google.com: domain of linux-wireless+bounces-8305-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <e6ae6dfa-6554-4e88-abb0-31dbbd8df03f@ti.com>
Date: Thu, 30 May 2024 11:06:40 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [EXTERNAL] [PATCH wireless-next 6/8] wifi: wlcore: add pn16
 support
To: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>,
        Kalle Valo
	<kvalo@kernel.org>
CC: 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>
Content-Language: en-US
From: "Nemanov, Michael" <michael.nemanov@ti.com>
In-Reply-To: <E1sBsy7-00E8vu-Nc@rmk-PC.armlinux.org.uk>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


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?

Michael.