From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michal Kazior <michal.kazior@tieto.com>
Date: Mon, 29 Apr 2013 09:27:44 +0200
Subject: [ath9k-devel] [PATCH 20/20] ath10k: Remove unneeded locks
 during HTT RX attach
In-Reply-To: <20862.7299.137818.979432@gargle.gargle.HOWL>
References: <1367163098-22787-1-git-send-email-sujith@msujith.org>
	<1367163098-22787-20-git-send-email-sujith@msujith.org>
	<517E0E2F.7000408@tieto.com>	<20862.5637.708673.839832@gargle.gargle.HOWL>
	<20862.6048.74676.291282@gargle.gargle.HOWL>
	<20862.7299.137818.979432@gargle.gargle.HOWL>
Message-ID: <517E20F0.9020200@tieto.com>
List-Id: <ath9k-devel.lists.ath9k.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: ath9k-devel@lists.ath9k.org

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.