[PATCH] wcn36xx: handle rx skb allocation failure to avoid system crash
fengwei.yin
fengwei.yin at linaro.org
Fri Dec 11 17:12:57 PST 2015
On 2015/12/11 22:08, Bob Copeland wrote:
> On Fri, Dec 11, 2015 at 09:44:54PM +0800, fengwei.yin wrote:
>>> /* skip this frame if we can't alloc a new rx buffer */
>>> if (ret)
>>> goto drop;
>> This can't work because we need to initialize the DMA for the old skb again.
>> Which is done in following
>> switch (ch->ch_type) {
>> block.
>
> Hmm, good point. You could still move that out to a function like this:
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
> index f8dfa05..fd447bf 100644
> --- a/drivers/net/wireless/ath/wcn36xx/dxe.c
> +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
> @@ -467,6 +467,27 @@ out_err:
>
> }
>
> +/* or whatever name makes sense... */
> +static void wcn36xx_restart_dma(struct wcn36xx *wcn,
> + struct wcn36xx_dxe_ch *ch,
> + struct wcn36xx_dxe_desc *dxe)
> +{
> + switch (ch->ch_type) {
> + case WCN36XX_DXE_CH_RX_L:
> + dxe->ctrl = WCN36XX_DXE_CTRL_RX_L;
> + wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
> + WCN36XX_DXE_INT_CH1_MASK);
> + break;
> + case WCN36XX_DXE_CH_RX_H:
> + dxe->ctrl = WCN36XX_DXE_CTRL_RX_H;
> + wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
> + WCN36XX_DXE_INT_CH3_MASK);
> + break;
> + default:
> + wcn36xx_warn("Unknown channel\n");
> + }
> +}
> +
> static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
> struct wcn36xx_dxe_ch *ch)
> {
> @@ -478,26 +499,18 @@ static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
> while (!(dxe->ctrl & WCN36XX_DXE_CTRL_VALID_MASK)) {
> skb = ctl->skb;
> dma_addr = dxe->dst_addr_l;
> - wcn36xx_dxe_fill_skb(wcn->dev, ctl);
> + ret = wcn36xx_dxe_fill_skb(wcn->dev, ctl);
>
> - switch (ch->ch_type) {
> - case WCN36XX_DXE_CH_RX_L:
> - dxe->ctrl = WCN36XX_DXE_CTRL_RX_L;
> - wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
> - WCN36XX_DXE_INT_CH1_MASK);
> - break;
> - case WCN36XX_DXE_CH_RX_H:
> - dxe->ctrl = WCN36XX_DXE_CTRL_RX_H;
> - wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
> - WCN36XX_DXE_INT_CH3_MASK);
> - break;
> - default:
> - wcn36xx_warn("Unknown channel\n");
> - }
> + /* skip this frame in OOM condition */
> + if (ret)
> + goto drop;
>
> dma_unmap_single(wcn->dev, dma_addr, WCN36XX_PKT_SIZE,
> DMA_FROM_DEVICE);
> wcn36xx_rx_skb(wcn, skb);
> +
> +drop:
> + wcn36xx_restart_dma(wcn, ch, dxe);
> ctl = ctl->next;
> dxe = ctl->desc;
> }
>
>
>
> ...that said, not really sure it's worth it now that the 'goto' is only
> skipping two lines. So, I would be ok with the original patch too.
>
I don't want to introduce "goto". But I really like your choice to create
wcn36xx_restart_dma. I will keep some original patch to avoid "goto" and
adopt the function wcn36xx_restart_dma. Will send the patch out.
Regards
Yin, Fengwei
More information about the wcn36xx
mailing list