Hi Fengwei,
On Mon, Dec 14, 2015 at 9:06 PM, Fengwei Yin <fengwei.yin@xxxxxxxxxx> 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 <lking@xxxxxxxxxxxxxxxx>
> Signed-off-by: Fengwei Yin <fengwei.yin@xxxxxxxxxx>
> ---
> Changes from v1:
> * Move switch block out of while loop.
> * Remove the warning of unknown channel because we didn't deal with it.
>
> drivers/net/wireless/ath/wcn36xx/dxe.c | 50 ++++++++++++++++++++--------------
> 1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
> index f8dfa05..6b61874 100644
> --- a/drivers/net/wireless/ath/wcn36xx/dxe.c
> +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
> @@ -467,6 +467,18 @@ out_err:
>
> }
>
> +#define GET_CH_CTRL_VALUE(x) \
> + ({ u32 __v = WCN36XX_DXE_CTRL_RX_H; \
> + if ((x) == WCN36XX_DXE_CH_RX_L) \
> + __v = WCN36XX_DXE_CTRL_RX_L; \
> + __v; })
> +
> +#define GET_CH_INT_MASK(x) \
> + ({ u32 __v = WCN36XX_DXE_INT_CH3_MASK; \
> + if ((x) == WCN36XX_DXE_CH_RX_L) \
> + __v = WCN36XX_DXE_INT_CH1_MASK; \
> + __v; })
> +
Why add these ugly macros if you're only calling them once?
> static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
> struct wcn36xx_dxe_ch *ch)
> {
> @@ -474,36 +486,34 @@ static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
> struct wcn36xx_dxe_desc *dxe = ctl->desc;
> dma_addr_t dma_addr;
> struct sk_buff *skb;
> + int ret = 0, int_mask;
> + u32 value;
> +
Surely something like:
if (ch->ch_type == WCN36XX_DXE_CH_RX_L) {
value = WCN36XX_DXE_CTRL_RX_L;
int_mask = WCN36XX_DXE_INT_CH1_MASK;
} else {
value = WCN36XX_DXE_CTRL_RX_H;
int_mask = WCN36XX_DXE_INT_CH3_MASK;
}
would be much cleaner.
Thanks,
--
Julian Calaby
Email: julian.calaby@xxxxxxxxx
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
