Return-path: Received: from mail-qg0-f47.google.com ([209.85.192.47]:33393 "EHLO mail-qg0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750960AbbLKNhm (ORCPT ); Fri, 11 Dec 2015 08:37:42 -0500 Received: by qgef78 with SMTP id f78so7129014qge.0 for ; Fri, 11 Dec 2015 05:37:42 -0800 (PST) Date: Fri, 11 Dec 2015 08:37:30 -0500 From: Bob Copeland To: "fengwei.yin" Cc: linux-wireless@vger.kernel.org, wcn36xx@lists.infradead.org, k.eugene.e@gmail.com, bjorn.andersson@sonymobile.com, lking@qti.qualcomm.com Subject: Re: [PATCH] wcn36xx: handle rx skb allocation failure to avoid system crash Message-ID: <20151211133730.GA8835@localhost> (sfid-20151211_143748_152898_F1DFE668) References: <1449034051-12536-1-git-send-email-fengwei.yin@linaro.org> <566ACC1C.1070304@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <566ACC1C.1070304@linaro.org> Sender: linux-wireless-owner@vger.kernel.org List-ID: On Fri, Dec 11, 2015 at 09:14:04PM +0800, fengwei.yin wrote: > > On 2015/12/2 13:27, Fengwei Yin wrote: > >Lawrence reported that git clone could make system crash on a > >Qualcomm ARM soc based device (DragonBoard, 1G memory without > >swap) running 64bit Debian. > > > >It's turned out the crash is related with rx skb allocation > >failure. git could consume more than 600MB anonymous memory. > >And system is in extremely memory shortage case. > > > >But driver didn't handle the rx allocation failure case. This patch > >doesn't submit skb to upper layer if rx skb allocation fails. > >Instead, it reuse the old skb for rx DMA again. It's more like > >drop the packets if system is in memory shortage case. > > > >With this change, git clone is OOMed instead of system crash. > > > >Reported-by: King, Lawrence > >Signed-off-by: Fengwei Yin Concept makes sense to me, but: > > dma_addr = dxe->dst_addr_l; > >- wcn36xx_dxe_fill_skb(wcn->dev, ctl); > >+ ret = wcn36xx_dxe_fill_skb(wcn->dev, ctl); > >+ if (0 == ret) { I find this "success handling" to be unclear and traditionally this kind of thing is a source of bugs; how about instead: > >+ /* new skb allocation ok. Use the new one and queue > >+ * the old one to network system. > >+ */ > >+ dma_unmap_single(wcn->dev, dma_addr, WCN36XX_PKT_SIZE, > >+ DMA_FROM_DEVICE); > >+ wcn36xx_rx_skb(wcn, skb); > >+ } ret = wcn36xx_dxe_fill_skb(wcn->dev, ctl); /* skip this frame if we can't alloc a new rx buffer */ if (ret) goto drop; > > switch (ch->ch_type) { > > case WCN36XX_DXE_CH_RX_L: > >@@ -495,9 +504,6 @@ static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn, > > wcn36xx_warn("Unknown channel\n"); > > } > > > >- dma_unmap_single(wcn->dev, dma_addr, WCN36XX_PKT_SIZE, > >- DMA_FROM_DEVICE); > >- wcn36xx_rx_skb(wcn, skb); drop: > > ctl = ctl->next; > > dxe = ctl->desc; > > } -- Bob Copeland %% http://bobcopeland.com/