diff mbox

wcn36xx: handle rx skb allocation failure to avoid system crash

Message ID 20151211140833.GA22332@localhost (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show

Commit Message

Bob Copeland Dec. 11, 2015, 2:08 p.m. UTC 
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:




...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.

Comments

Fengwei Yin Dec. 12, 2015, 1:12 a.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 	}