pcnet32: fix reallocation error
Subsystems (3)
Don Fry
maintainerPATCH 1/2 pcnet32: fix reallocation error
pcnet32_realloc_rx_ring() only worked on the first log2 number of entries in the receive ring instead of the all the entries. Replaced "1 << size" with more descriptive variable.
This is my original bug from 2006. Found while testing another problem. Tested on 79C972 and 79C976.
--- drivers/net/ethernet/amd/pcnet32.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)
drivers/net/ethernet/amd/pcnet32.c +9 -9
@@ -549,35 +549,36 @@ static void pcnet32_realloc_rx_ring(struct net_device *dev,struct pcnet32_rx_head *new_rx_ring;struct sk_buff **new_skb_list;int new, overlap; +unsigned int entries =1 << size; new_rx_ring =pci_alloc_consistent (lp->pci_dev,sizeof (struct pcnet32_rx_head ) * - (1 << size), + entries, &new_ring_dma_addr);if (new_rx_ring ==NULL ) {netif_err (lp, drv, dev,"Consistent memory allocation failed\n" );return ; } -memset (new_rx_ring,0 ,sizeof (struct pcnet32_rx_head ) * (1 << size)); +memset (new_rx_ring,0 ,sizeof (struct pcnet32_rx_head ) * entries); - new_dma_addr_list =kcalloc (1 << size,sizeof (dma_addr_t),GFP_ATOMIC ); + new_dma_addr_list =kcalloc (entries,sizeof (dma_addr_t),GFP_ATOMIC );if (!new_dma_addr_list) goto free_new_rx_ring; - new_skb_list =kcalloc (1 << size,sizeof (struct sk_buff *), + new_skb_list =kcalloc (entries,sizeof (struct sk_buff *),GFP_ATOMIC );if (!new_skb_list) goto free_new_lists;/* first copy the current receive buffers */ - overlap =min (size, lp->rx_ring_size); + overlap =min (entries, lp->rx_ring_size);for (new =0 ; new < overlap; new++) { new_rx_ring[new] = lp->rx_ring[new]; new_dma_addr_list[new] = lp->rx_dma_addr[new]; new_skb_list[new] = lp->rx_skbuff[new]; }/* now allocate any new buffers needed */ -for (; new < size; new++) { +for (; new < entries; new++) {struct sk_buff *rx_skbuff; new_skb_list[new] =netdev_alloc_skb (dev,PKT_BUF_SKB ); rx_skbuff = new_skb_list[new];
@@ -612,7 +613,7 @@ static void pcnet32_realloc_rx_ring(struct net_device *dev, lp->rx_ring_size, lp->rx_ring, lp->rx_ring_dma_addr); - lp->rx_ring_size = (1 << size); + lp->rx_ring_size = entries; lp->rx_mod_mask = lp->rx_ring_size -1 ; lp->rx_len_bits = (size <<4 ); lp->rx_ring = new_rx_ring;
@@ -634,8 +635,7 @@ free_new_lists:kfree (new_dma_addr_list); free_new_rx_ring:pci_free_consistent (lp->pci_dev, -sizeof (struct pcnet32_rx_head ) * - (1 << size), +sizeof (struct pcnet32_rx_head ) * entries, new_rx_ring, new_ring_dma_addr); }
Signature · 1 line
1.7.2.3
drivers/net/ethernet/amd/pcnet32.c static void pcnet32_realloc_rx_ring(struct net_device *dev, · line 581
Quoted 11 lines · Don Fry
pcnet32_realloc_rx_ring()only worked on the first log2 number of entries in the receive ring instead of the all the entries. Replaced "1 << size" with more descriptive variable.drivers/net/ethernet/amd/pcnet32.c +2
-2/* first copy the current receive buffers */ - overlap =min (size, lp->rx_ring_size); + overlap =min (entries, lp->rx_ring_size);/* now allocate any new buffers needed */ -for (; new < size; new++) { +for (; new < entries; new++) {
Until this fix, lp->rx_skbuff[size...(1<<size)-1] were not allocated by netdev_alloc_skb() and lp->rx_ring_dma_addr[size...(1<<size)-1] were not mapped by pci_map_single(), right?
Since lp->rx_ring_size was set to (1<<size), wasn't there possibility that something bad happens by accessing lp->rx_skbuff[] and lp->rx_ring_dma_addr[] up to (1<<size)-1 ? (In other words, don't we want to backport this fix?)
Don Fry
maintainer-----Original Message-----
From: Tetsuo Handa <[email protected]>
Subject: Re: [PATCH net-next 1/2] pcnet32: fix reallocation error
Date: Tue, 18 Feb 2014 21:29:00 +0900
Quoted 17 lines
drivers/net/ethernet/amd/pcnet32.c static void pcnet32_realloc_rx_ring(struct net_device *dev, · line 581
Quoted 12 lines · Don Fry
pcnet32_realloc_rx_ring()only worked on the first log2 number of entries in the receive ring instead of the all the entries. Replaced "1 << size" with more descriptive variable.drivers/net/ethernet/amd/pcnet32.c +2
-2/* first copy the current receive buffers */ - overlap =min (size, lp->rx_ring_size); + overlap =min (entries, lp->rx_ring_size);/* now allocate any new buffers needed */ -for (; new < size; new++) { +for (; new < entries; new++) {Until this fix, lp->rx_skbuff[size...(1<<size)-1] were not allocated by
netdev_alloc_skb()and lp->rx_ring_dma_addr[size...(1<<size)-1] were not mapped bypci_map_single(), right?
Quoted 4 lines
Since
lp->rx_ring_sizewas set to (1<<size), wasn't there possibility that something bad happens by accessing lp->rx_skbuff[] and lp->rx_ring_dma_addr[] up to (1<<size)-1 ? (In other words, don't we want to backport this fix?)
I looked into this when testing. If the device is operational (netif_running) when the ring is resized, it will do pcnet32_restart and make sure all rx buffers are full. If the device is not operational it will leave the ring partially full until it is opened and then the ring is filled via pcnet32_init_ring(). Another thing that could happen while it is down is that a selftest is done (offline) which also calls pcnet32_restart and makes sure the ring is full. I could not find a case that would cause a problem. No-one has reported this in the last 8 years so it does not look like backporting this fix is needed, in my opinion.
From: Don Fry <[email protected]>
Date: Mon, 17 Feb 2014 20:57:46 -0800
Quoted 7 lines
pcnet32_realloc_rx_ring()only worked on the first log2 number of entries in the receive ring instead of the all the entries. Replaced "1 << size" with more descriptive variable.
This is my original bug from 2006. Found while testing another problem. Tested on 79C972 and 79C976.Signed-off-by Don Fry <[email protected]>
Applied.