Re: [EXTERNAL] [PATCH wireless-next 6/8] wifi: wlcore: add pn16 support

[Nemanov, Michael]

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.