[ath9k-devel] [PATCH 20/20] ath10k: Remove unneeded locks during HTT RX attach

[Michal Kazior <michal.kazior@tieto.com>]

On 29/04/13 09:08, Sujith Manoharan wrote:
> Sujith Manoharan wrote:
>> ...and it doesn't seem correct to proceed with init after htt_rx_attach() has
>> failed due to memory allocation, since subsequent allocations would probably fail
>> too.
>
> How about something like this ?
>
> [PATCH] ath10k: Remove unneeded locks during HTT RX attach
>
> If buffer allocation fails during attach(), bail out properly
> so the replenish timer doesn't get fired.
>
> Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
> ---
 > (...)
> @@ -641,23 +645,29 @@ int ath10k_htt_rx_attach(struct ath10k_htt *htt)
>   	setup_timer(timer, ath10k_htt_rx_ring_refill_retry, (unsigned long)htt);
>
>   	spin_lock_init(&htt->rx_ring.lock);
> -	spin_lock_bh(&htt->rx_ring.lock);
> +
>   	htt->rx_ring.fill_cnt = 0;
> -	ath10k_htt_rx_ring_fill_n(htt, htt->rx_ring.fill_level);
> -	spin_unlock_bh(&htt->rx_ring.lock);
> +	if (ath10k_htt_rx_ring_fill_n(htt, htt->rx_ring.fill_level))
> +		goto err_fill_ring;

Won't this still fail the lockdep assertion?

We're also leaking memory upon failure. We need to free up the skbuffs 
we successfully allocated & mapped.

If you want to check if the ath10k_htt_rx_ring_fill_n() failed you could 
simply compare fill_level and fill_cnt while (still) holding the spinlock:

	spin_lock_bh(&htt->rx_ring.lock);
	htt->rx_ring.fill_cnt = 0;
	ath10k_htt_rx_ring_fill_n(htt, htt->rx_ring.fill_level);
	if (htt->rx_ring.fill_cnt != htt->rx_ring.fill_level) {
		// handle failure
	}
	spin_unlock_bh(&htt->rx_ring.lock);

I'm not even sure if we should abort right away. We replenish htt rx 
buffers using GFP_ATOMIC allocation. This can fail under memory pressure 
more often (then, say, GFP_KERNEL). I wouldn't really consider this as 
fatal error. Although we could probably have a ath10k_warn/info if the 
initial allocation doesn't fill everything up. Perhaps we could fail 
only if the fill_cnt is still 0, or below a defined threshold?

We could also try and split the replenishing functions and use 
GFP_KERNEL allocation during htt rx attach.


-- Pozdrawiam / Best regards, Michal Kazior.