Re: [PATCH v2] wcn36xx: handle rx skb allocation failure to avoid system crash

[Julian Calaby <julian.calaby@gmail.com>]

Hi Fengwei,

On Mon, Dec 14, 2015 at 9:06 PM, Fengwei Yin <fengwei.yin@linaro.org> 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@qti.qualcomm.com>
> Signed-off-by: Fengwei Yin <fengwei.yin@linaro.org>
> ---
> 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@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/