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

["fengwei.yin" <fengwei.yin@linaro.org>]

On 2015/12/15 6:47, Julian Calaby wrote:
> 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.
OK. I will remove the ugly macros. Thanks a lot for reviewing it.

Regards
Yin, Fengwei

>
> Thanks,
>