Re: [PATCH] pch_gbe: don't come up if we can't allocate buffers
flat view
From: Eric Dumazet <hidden>
Date: 2012-10-25 08:41:02
On Thu, 2012-10-25 at 09:10 +0200, Veaceslav Falico wrote:
Currently, even if pch_gbe_alloc_tx/rx_buffers (even partially) fail, we bring the interface up. It mihgt confuse the user (if he requested specific amount of buffers) and can lead to different bugs. Add error handling to these functions and clean after them in case of failure. It also resolves a possible NULL pointer dereference in pch_gbe_alloc_tx_buffers(), in case of netdev_alloc_skb() failure. It's ok to (paritally) fail for pch_gbe_alloc_rx_buffers() in some situation, so don't clean inside and rather handle skb freeing outside of the function. This patch also adds tx_alloc_buff_failed ethtool counter.
This should not be needed :
quoted hunk ↗ jump to hunk
-static void pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter, +static int pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter, struct pch_gbe_tx_ring *tx_ring) { struct pch_gbe_buffer *buffer_info;@@ -1506,6 +1521,7 @@ static void pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter, unsigned int i; unsigned int bufsz; struct pch_gbe_tx_desc *tx_desc; + int err; bufsz = adapter->hw.mac.max_frame_size + PCH_GBE_DMA_ALIGN + NET_IP_ALIGN;@@ -1513,12 +1529,26 @@ static void pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter, for (i = 0; i < tx_ring->count; i++) { buffer_info = &tx_ring->buffer_info[i]; skb = netdev_alloc_skb(adapter->netdev, bufsz); + if (unlikely(!skb)) { + adapter->stats.tx_alloc_buff_failed++; + err = -ENOMEM; + goto freeall; + }
Hmmm These skbs should be allocated using GFP_KERNEL, to lower risk of OOM, using a mere alloc_skb(bufsz, GFP_KERNEL) Only skbs for rx 'need' netdev_alloc_skb() to add a NET_SKB_PAD headroom. skb = alloc_skb(bufsz, GFP_KERNEL); if (!skb) goto fail; skb_reserve(skb, NET_IP_ALIGN); And BTW, we dont really need skbs here, only a bounce buffer so that we can copy frames on it... (kmalloc() instead of alloc_skb())
skb_reserve(skb, PCH_GBE_DMA_ALIGN);
PCH_GBE_DMA_ALIGN is zero ...
quoted hunk ↗ jump to hunk
buffer_info->skb = skb; tx_desc = PCH_GBE_TX_DESC(*tx_ring, i); tx_desc->gbec_status = (DSC_INIT16); } - return; + return 0; + +freeall: + for (i--; i >= 0; i--) { + dev_kfree_skb_any(buffer_info->skb); + buffer_info->skb = NULL; + tx_desc = PCH_GBE_TX_DESC(*tx_ring, i); + tx_desc->gbec_status = 0; + } + return err; }
Also I would call pch_gbe_alloc_tx_buffers() _after_ pch_gbe_alloc_rx_buffers() (Since rx allocations probably are using GFP_ATOMIC allocations)