* [PATCH] korina: Read buffer overflow @ 2009-08-07 15:33 Roel Kluin 2009-08-08 0:48 ` Phil Sutter 0 siblings, 1 reply; 13+ messages in thread From: Roel Kluin @ 2009-08-07 15:33 UTC (permalink / raw) To: netdev, Andrew Morton, David S. Miller, n0-1, florian If the loop breaks with an i of 0, then we read lp->rd_ring[-1]. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- Should we clean up like this? please review diff --git a/drivers/net/korina.c b/drivers/net/korina.c index b4cf602..b965b2b 100644 --- a/drivers/net/korina.c +++ b/drivers/net/korina.c @@ -754,7 +754,7 @@ static void korina_alloc_ring(struct net_device *dev) { struct korina_private *lp = netdev_priv(dev); struct sk_buff *skb; - int i; + int i, j; /* Initialize the transmit descriptors */ for (i = 0; i < KORINA_NUM_TDS; i++) { @@ -771,7 +771,7 @@ static void korina_alloc_ring(struct net_device *dev) for (i = 0; i < KORINA_NUM_RDS; i++) { skb = dev_alloc_skb(KORINA_RBSIZE + 2); if (!skb) - break; + goto err_free; skb_reserve(skb, 2); lp->rx_skb[i] = skb; lp->rd_ring[i].control = DMA_DESC_IOD | @@ -790,6 +790,12 @@ static void korina_alloc_ring(struct net_device *dev) lp->rx_chain_head = 0; lp->rx_chain_tail = 0; lp->rx_chain_status = desc_empty; +err_free: + for (j = 0; j < i; j++) { + lp->rd_ring[j].control = 0; + dev_kfree_skb_any(lp->rx_skb[j]); + lp->rx_skb[j] = NULL; + } } static void korina_free_ring(struct net_device *dev) ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] korina: Read buffer overflow 2009-08-07 15:33 [PATCH] korina: Read buffer overflow Roel Kluin @ 2009-08-08 0:48 ` Phil Sutter 2009-08-08 13:14 ` roel kluin 0 siblings, 1 reply; 13+ messages in thread From: Phil Sutter @ 2009-08-08 0:48 UTC (permalink / raw) To: Roel Kluin; +Cc: netdev, Andrew Morton, David S. Miller, florian Hi, On Fri, Aug 07, 2009 at 05:33:31PM +0200, Roel Kluin wrote: > If the loop breaks with an i of 0, then we read lp->rd_ring[-1]. Indeed. > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> > --- > Should we clean up like this? please review > > diff --git a/drivers/net/korina.c b/drivers/net/korina.c > index b4cf602..b965b2b 100644 > --- a/drivers/net/korina.c > +++ b/drivers/net/korina.c > @@ -754,7 +754,7 @@ static void korina_alloc_ring(struct net_device *dev) > { > struct korina_private *lp = netdev_priv(dev); > struct sk_buff *skb; > - int i; > + int i, j; > > /* Initialize the transmit descriptors */ > for (i = 0; i < KORINA_NUM_TDS; i++) { > @@ -771,7 +771,7 @@ static void korina_alloc_ring(struct net_device *dev) > for (i = 0; i < KORINA_NUM_RDS; i++) { > skb = dev_alloc_skb(KORINA_RBSIZE + 2); > if (!skb) > - break; > + goto err_free; This implies that all KORINA_NUM_TDS receive descriptors need to be available for the driver to work. > skb_reserve(skb, 2); > lp->rx_skb[i] = skb; > lp->rd_ring[i].control = DMA_DESC_IOD | > @@ -790,6 +790,12 @@ static void korina_alloc_ring(struct net_device *dev) > lp->rx_chain_head = 0; > lp->rx_chain_tail = 0; > lp->rx_chain_status = desc_empty; > +err_free: > + for (j = 0; j < i; j++) { > + lp->rd_ring[j].control = 0; > + dev_kfree_skb_any(lp->rx_skb[j]); > + lp->rx_skb[j] = NULL; > + } > } Also I guess there should be some error handling, as the driver probably wont be useful without a single receive descriptor. What do you think about the following: | diff --git a/drivers/net/korina.c b/drivers/net/korina.c | index a2701f5..d1c5276 100644 | --- a/drivers/net/korina.c | +++ b/drivers/net/korina.c | @@ -741,7 +741,7 @@ static struct ethtool_ops netdev_ethtool_ops = { | .get_link = netdev_get_link, | }; | | -static void korina_alloc_ring(struct net_device *dev) | +static int korina_alloc_ring(struct net_device *dev) | { | struct korina_private *lp = netdev_priv(dev); | struct sk_buff *skb; | @@ -772,6 +772,13 @@ static void korina_alloc_ring(struct net_device *dev) | lp->rd_ring[i].ca = CPHYSADDR(skb->data); | lp->rd_ring[i].link = CPHYSADDR(&lp->rd_ring[i+1]); | } | + if (!i) { | + printk(KERN_ERR DRV_NAME "%s: could not allocate a single" | + " receive descriptor\n", dev->name) | + return 1; | + } | + printk(KERN_DEBUG DRV_NAME "%s: allocated %d receive descriptors\n", | + dev->name, i); | | /* loop back receive descriptors, so the last | * descriptor points to the first one */ | @@ -782,6 +789,8 @@ static void korina_alloc_ring(struct net_device *dev) | lp->rx_chain_head = 0; | lp->rx_chain_tail = 0; | lp->rx_chain_status = desc_empty; | + | + return 0; | } | | static void korina_free_ring(struct net_device *dev) | @@ -824,7 +833,8 @@ static int korina_init(struct net_device *dev) | writel(ETH_INT_FC_EN, &lp->eth_regs->ethintfc); | | /* Allocate rings */ | - korina_alloc_ring(dev); | + if (korina_alloc_ring(dev)) | + return 1; | | writel(0, &lp->rx_dma_regs->dmas); | /* Start Rx DMA */ Greetings, Phil ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] korina: Read buffer overflow 2009-08-08 0:48 ` Phil Sutter @ 2009-08-08 13:14 ` roel kluin 2009-08-08 16:45 ` Phil Sutter 0 siblings, 1 reply; 13+ messages in thread From: roel kluin @ 2009-08-08 13:14 UTC (permalink / raw) To: Roel Kluin, netdev, Andrew Morton, David S. Miller, florian >> If the loop breaks with an i of 0, then we read lp->rd_ring[-1]. >> @@ -771,7 +771,7 @@ static void korina_alloc_ring(struct net_device *dev) >> for (i = 0; i < KORINA_NUM_RDS; i++) { >> skb = dev_alloc_skb(KORINA_RBSIZE + 2); >> if (!skb) >> - break; >> + goto err_free; > > This implies that all KORINA_NUM_TDS receive descriptors need to be > available for the driver to work. > Also I guess there should be some error handling, as the driver probably > wont be useful without a single receive descriptor. What do you think > about the following: A few comments: > | diff --git a/drivers/net/korina.c b/drivers/net/korina.c > | index a2701f5..d1c5276 100644 > | --- a/drivers/net/korina.c > | +++ b/drivers/net/korina.c > | @@ -772,6 +772,13 @@ static void korina_alloc_ring(struct net_device *dev) > | lp->rd_ring[i].ca = CPHYSADDR(skb->data); > | lp->rd_ring[i].link = CPHYSADDR(&lp->rd_ring[i+1]); > | } > | + if (!i) { > | + printk(KERN_ERR DRV_NAME "%s: could not allocate a single" > | + " receive descriptor\n", dev->name) printk(KERN_ERR "%s: could not allocate a single receive descriptor\n", dev->name); Don't interrupt strings, they are an exception in the 80 character limit (since they are easier to grep that way). Also I think the DRV_NAME should be omitted, Doesn't `DRV_NAME "%s:...\n", dev->name)' result in "korinakorina:..."? It occurs at more places in the file. Also a semicolon was missing. > | + return 1; I think -ENOMEM is more appropriate. > | + } > | + printk(KERN_DEBUG DRV_NAME "%s: allocated %d receive descriptors\n", > | + dev->name, i); same `DRV_NAME "%s:...\n", dev->name)' issue. > | @@ -824,7 +833,8 @@ static int korina_init(struct net_device *dev) > | writel(ETH_INT_FC_EN, &lp->eth_regs->ethintfc); > | > | /* Allocate rings */ > | - korina_alloc_ring(dev); > | + if (korina_alloc_ring(dev)) > | + return 1; -ENOMEM > | > | writel(0, &lp->rx_dma_regs->dmas); > | /* Start Rx DMA */ > > Greetings, Phil Otherwise looks fine to me. Thanks, Roel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] korina: Read buffer overflow 2009-08-08 13:14 ` roel kluin @ 2009-08-08 16:45 ` Phil Sutter 2009-08-09 0:06 ` Phil Sutter [not found] ` <1249776388-4626-1-git-send-email-n0-1@freewrt.org> 0 siblings, 2 replies; 13+ messages in thread From: Phil Sutter @ 2009-08-08 16:45 UTC (permalink / raw) To: roel kluin; +Cc: netdev, Andrew Morton, David S. Miller, florian Hi, On Sat, Aug 08, 2009 at 03:14:57PM +0200, roel kluin wrote: > A few comments: > > > | diff --git a/drivers/net/korina.c b/drivers/net/korina.c > > | index a2701f5..d1c5276 100644 > > | --- a/drivers/net/korina.c > > | +++ b/drivers/net/korina.c > > > | @@ -772,6 +772,13 @@ static void korina_alloc_ring(struct net_device *dev) > > | lp->rd_ring[i].ca = CPHYSADDR(skb->data); > > | lp->rd_ring[i].link = CPHYSADDR(&lp->rd_ring[i+1]); > > | } > > | + if (!i) { > > | + printk(KERN_ERR DRV_NAME "%s: could not allocate a single" > > | + " receive descriptor\n", dev->name) > > printk(KERN_ERR "%s: could not allocate a single receive > descriptor\n", > dev->name); > > Don't interrupt strings, they are an exception in the 80 character > limit (since they are easier to grep that way). Ah, good to know. > Also I think the DRV_NAME should be omitted, Doesn't > `DRV_NAME "%s:...\n", dev->name)' result in "korinakorina:..."? It > occurs at more places in the file. Also a semicolon was missing. Sounds like a valid point, I will check this. This patch was just a quick hack to illustrate what I had in mind, without even compile-testing it. > Otherwise looks fine to me. Great you like it, and many thanks for reviewing it. I'm still not completely sure the NIC will still work with less than 64 receive descriptors, but since the documentation wasn't really enlightening I'll add some run-time test to my TODO. Maybe one of the other readers has some advice here, I'll prepare a complete patch in the meantime. Greetings, Phil ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] korina: Read buffer overflow 2009-08-08 16:45 ` Phil Sutter @ 2009-08-09 0:06 ` Phil Sutter 2009-08-12 22:15 ` Phil Sutter [not found] ` <1249776388-4626-1-git-send-email-n0-1@freewrt.org> 1 sibling, 1 reply; 13+ messages in thread From: Phil Sutter @ 2009-08-09 0:06 UTC (permalink / raw) To: netdev; +Cc: Andrew Morton, David S. Miller, florian, Roel Kluin Hi, Testing of my approach to solve the buffer overrun issue showed that living with a subset of the requested receive descriptors is not as easy as initially assumed: uppon allocation failure the number of descriptors would have to be reduced to the next lower power of two (as also stated in the corresponding define's comment), which I consider too much overhead. A better solution would be to export the ring parameters via ethtool. For now, let's go Roel's way aborting completely and cleaning up in this case. The following series fixes the incorrect printk formatting we already discussed, implements the solution from above and makes the driver use netdev_ops. Greetings, Phil ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] korina: Read buffer overflow 2009-08-09 0:06 ` Phil Sutter @ 2009-08-12 22:15 ` Phil Sutter 2009-08-12 22:22 ` [PATCH 1/2] korina: fix printk formatting, add final info line Phil Sutter 0 siblings, 1 reply; 13+ messages in thread From: Phil Sutter @ 2009-08-12 22:15 UTC (permalink / raw) To: netdev; +Cc: Andrew Morton, David S. Miller, florian, Roel Kluin Hi, On Sun, Aug 09, 2009 at 02:06:25AM +0200, Phil Sutter wrote: > The following series fixes the incorrect printk formatting we already > discussed, implements the solution from above and makes the driver use > netdev_ops. Obviously, I took the chance to mess things up again. These three patches were accidentially written on top of the linux-mips tree, right before Ralf pulled from Linus. So they do not apply cleanly to the netdev tree, and even worse the last one is completely useless since it's changes have already been implemented. I will follow up to this email with an updated series of the two remaining, valid patches. Sorry for the inconvenience. Greetings, Phil ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] korina: fix printk formatting, add final info line 2009-08-12 22:15 ` Phil Sutter @ 2009-08-12 22:22 ` Phil Sutter 2009-08-12 22:52 ` [PATCH 2/2] korina: add error-handling to korina_alloc_ring Phil Sutter 2009-08-13 23:27 ` [PATCH 1/2] korina: fix printk formatting, add final info line David Miller 0 siblings, 2 replies; 13+ messages in thread From: Phil Sutter @ 2009-08-12 22:22 UTC (permalink / raw) To: netdev; +Cc: Andrew Morton, David S. Miller, florian, Roel Kluin The macro DRV_NAME contains "korina", the field dev->name points to the actual interface name. So messages were formerly prefixed with 'korinaeth2:' (on my system). Signed-off-by: Phil Sutter <n0-1@freewrt.org> --- drivers/net/korina.c | 32 +++++++++++++++++--------------- 1 files changed, 17 insertions(+), 15 deletions(-) diff --git a/drivers/net/korina.c b/drivers/net/korina.c index b4cf602..6df9d25 100644 --- a/drivers/net/korina.c +++ b/drivers/net/korina.c @@ -338,7 +338,7 @@ static irqreturn_t korina_rx_dma_interrupt(int irq, void *dev_id) napi_schedule(&lp->napi); if (dmas & DMA_STAT_ERR) - printk(KERN_ERR DRV_NAME "%s: DMA error\n", dev->name); + printk(KERN_ERR "%s: DMA error\n", dev->name); retval = IRQ_HANDLED; } else @@ -555,7 +555,7 @@ static void korina_tx(struct net_device *dev) dev->stats.tx_dropped++; /* Should never happen */ - printk(KERN_ERR DRV_NAME "%s: split tx ignored\n", + printk(KERN_ERR "%s: split tx ignored\n", dev->name); } else if (devcs & ETH_TX_TOK) { dev->stats.tx_packets++; @@ -641,7 +641,7 @@ korina_tx_dma_interrupt(int irq, void *dev_id) dev->trans_start = jiffies; } if (dmas & DMA_STAT_ERR) - printk(KERN_ERR DRV_NAME "%s: DMA error\n", dev->name); + printk(KERN_ERR "%s: DMA error\n", dev->name); retval = IRQ_HANDLED; } else @@ -917,8 +917,7 @@ static int korina_restart(struct net_device *dev) ret = korina_init(dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME "%s: cannot restart device\n", - dev->name); + printk(KERN_ERR "%s: cannot restart device\n", dev->name); return ret; } korina_multicast_list(dev); @@ -1005,7 +1004,7 @@ static int korina_open(struct net_device *dev) /* Initialize */ ret = korina_init(dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME "%s: cannot open device\n", dev->name); + printk(KERN_ERR "%s: cannot open device\n", dev->name); goto out; } @@ -1015,14 +1014,14 @@ static int korina_open(struct net_device *dev) ret = request_irq(lp->rx_irq, &korina_rx_dma_interrupt, IRQF_DISABLED, "Korina ethernet Rx", dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME "%s: unable to get Rx DMA IRQ %d\n", + printk(KERN_ERR "%s: unable to get Rx DMA IRQ %d\n", dev->name, lp->rx_irq); goto err_release; } ret = request_irq(lp->tx_irq, &korina_tx_dma_interrupt, IRQF_DISABLED, "Korina ethernet Tx", dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME "%s: unable to get Tx DMA IRQ %d\n", + printk(KERN_ERR "%s: unable to get Tx DMA IRQ %d\n", dev->name, lp->tx_irq); goto err_free_rx_irq; } @@ -1031,7 +1030,7 @@ static int korina_open(struct net_device *dev) ret = request_irq(lp->ovr_irq, &korina_ovr_interrupt, IRQF_DISABLED, "Ethernet Overflow", dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME"%s: unable to get OVR IRQ %d\n", + printk(KERN_ERR "%s: unable to get OVR IRQ %d\n", dev->name, lp->ovr_irq); goto err_free_tx_irq; } @@ -1040,7 +1039,7 @@ static int korina_open(struct net_device *dev) ret = request_irq(lp->und_irq, &korina_und_interrupt, IRQF_DISABLED, "Ethernet Underflow", dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME "%s: unable to get UND IRQ %d\n", + printk(KERN_ERR "%s: unable to get UND IRQ %d\n", dev->name, lp->und_irq); goto err_free_ovr_irq; } @@ -1137,7 +1136,7 @@ static int korina_probe(struct platform_device *pdev) dev->base_addr = r->start; lp->eth_regs = ioremap_nocache(r->start, r->end - r->start); if (!lp->eth_regs) { - printk(KERN_ERR DRV_NAME "cannot remap registers\n"); + printk(KERN_ERR DRV_NAME ": cannot remap registers\n"); rc = -ENXIO; goto probe_err_out; } @@ -1145,7 +1144,7 @@ static int korina_probe(struct platform_device *pdev) r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "korina_dma_rx"); lp->rx_dma_regs = ioremap_nocache(r->start, r->end - r->start); if (!lp->rx_dma_regs) { - printk(KERN_ERR DRV_NAME "cannot remap Rx DMA registers\n"); + printk(KERN_ERR DRV_NAME ": cannot remap Rx DMA registers\n"); rc = -ENXIO; goto probe_err_dma_rx; } @@ -1153,14 +1152,14 @@ static int korina_probe(struct platform_device *pdev) r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "korina_dma_tx"); lp->tx_dma_regs = ioremap_nocache(r->start, r->end - r->start); if (!lp->tx_dma_regs) { - printk(KERN_ERR DRV_NAME "cannot remap Tx DMA registers\n"); + printk(KERN_ERR DRV_NAME ": cannot remap Tx DMA registers\n"); rc = -ENXIO; goto probe_err_dma_tx; } lp->td_ring = kmalloc(TD_RING_SIZE + RD_RING_SIZE, GFP_KERNEL); if (!lp->td_ring) { - printk(KERN_ERR DRV_NAME "cannot allocate descriptors\n"); + printk(KERN_ERR DRV_NAME ": cannot allocate descriptors\n"); rc = -ENXIO; goto probe_err_td_ring; } @@ -1193,10 +1192,13 @@ static int korina_probe(struct platform_device *pdev) rc = register_netdev(dev); if (rc < 0) { printk(KERN_ERR DRV_NAME - ": cannot register net device %d\n", rc); + ": cannot register net device: %d\n", rc); goto probe_err_register; } setup_timer(&lp->media_check_timer, korina_poll_media, (unsigned long) dev); + + printk(KERN_INFO "%s: " DRV_NAME "-" DRV_VERSION " " DRV_RELDATE "\n", + dev->name); out: return rc; -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] korina: add error-handling to korina_alloc_ring 2009-08-12 22:22 ` [PATCH 1/2] korina: fix printk formatting, add final info line Phil Sutter @ 2009-08-12 22:52 ` Phil Sutter 2009-08-13 23:27 ` David Miller 2009-08-13 23:27 ` [PATCH 1/2] korina: fix printk formatting, add final info line David Miller 1 sibling, 1 reply; 13+ messages in thread From: Phil Sutter @ 2009-08-12 22:52 UTC (permalink / raw) To: netdev; +Cc: Andrew Morton, David S. Miller, florian, Roel Kluin This also avoids a potential buffer overflow in case the very first receive descriptor fails to allocate, as an index of -1 would be used afterwards. Kudos to Roel Kluin for pointing this out and providing an initial patch. Signed-off-by: Phil Sutter <n0-1@freewrt.org> --- drivers/net/korina.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/net/korina.c b/drivers/net/korina.c index 6df9d25..51ca54c 100644 --- a/drivers/net/korina.c +++ b/drivers/net/korina.c @@ -750,7 +750,7 @@ static struct ethtool_ops netdev_ethtool_ops = { .get_link = netdev_get_link, }; -static void korina_alloc_ring(struct net_device *dev) +static int korina_alloc_ring(struct net_device *dev) { struct korina_private *lp = netdev_priv(dev); struct sk_buff *skb; @@ -771,7 +771,7 @@ static void korina_alloc_ring(struct net_device *dev) for (i = 0; i < KORINA_NUM_RDS; i++) { skb = dev_alloc_skb(KORINA_RBSIZE + 2); if (!skb) - break; + return -ENOMEM; skb_reserve(skb, 2); lp->rx_skb[i] = skb; lp->rd_ring[i].control = DMA_DESC_IOD | @@ -790,6 +790,8 @@ static void korina_alloc_ring(struct net_device *dev) lp->rx_chain_head = 0; lp->rx_chain_tail = 0; lp->rx_chain_status = desc_empty; + + return 0; } static void korina_free_ring(struct net_device *dev) @@ -832,7 +834,11 @@ static int korina_init(struct net_device *dev) writel(ETH_INT_FC_EN, &lp->eth_regs->ethintfc); /* Allocate rings */ - korina_alloc_ring(dev); + if (korina_alloc_ring(dev)) { + printk(KERN_ERR "%s: descriptor allocation failed\n", dev->name); + korina_free_ring(dev); + return -ENOMEM; + } writel(0, &lp->rx_dma_regs->dmas); /* Start Rx DMA */ -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] korina: add error-handling to korina_alloc_ring 2009-08-12 22:52 ` [PATCH 2/2] korina: add error-handling to korina_alloc_ring Phil Sutter @ 2009-08-13 23:27 ` David Miller 0 siblings, 0 replies; 13+ messages in thread From: David Miller @ 2009-08-13 23:27 UTC (permalink / raw) To: n0-1; +Cc: netdev, akpm, florian, roel.kluin From: Phil Sutter <n0-1@freewrt.org> Date: Thu, 13 Aug 2009 00:52:32 +0200 > This also avoids a potential buffer overflow in case the very first > receive descriptor fails to allocate, as an index of -1 would be used > afterwards. Kudos to Roel Kluin for pointing this out and providing an > initial patch. > > Signed-off-by: Phil Sutter <n0-1@freewrt.org> Applied to net-next-2.6 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] korina: fix printk formatting, add final info line 2009-08-12 22:22 ` [PATCH 1/2] korina: fix printk formatting, add final info line Phil Sutter 2009-08-12 22:52 ` [PATCH 2/2] korina: add error-handling to korina_alloc_ring Phil Sutter @ 2009-08-13 23:27 ` David Miller 1 sibling, 0 replies; 13+ messages in thread From: David Miller @ 2009-08-13 23:27 UTC (permalink / raw) To: n0-1; +Cc: netdev, akpm, florian, roel.kluin From: Phil Sutter <n0-1@freewrt.org> Date: Thu, 13 Aug 2009 00:22:46 +0200 > The macro DRV_NAME contains "korina", the field dev->name points to the > actual interface name. So messages were formerly prefixed with > 'korinaeth2:' (on my system). > > Signed-off-by: Phil Sutter <n0-1@freewrt.org> Applied to net-next-2.6 ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <1249776388-4626-1-git-send-email-n0-1@freewrt.org>]
* [PATCH 1/3] korina: fix printk formatting, add final info line [not found] ` <1249776388-4626-1-git-send-email-n0-1@freewrt.org> @ 2009-08-09 0:06 ` Phil Sutter [not found] ` <1249776388-4626-2-git-send-email-n0-1@freewrt.org> 1 sibling, 0 replies; 13+ messages in thread From: Phil Sutter @ 2009-08-09 0:06 UTC (permalink / raw) To: netdev; +Cc: Andrew Morton, David S. Miller, florian, Roel Kluin The macro DRV_NAME contains "korina", the field dev->name points to the actual interface name. So messages were formerly prefixed with 'korinaeth2:' (on my system). Signed-off-by: Phil Sutter <n0-1@freewrt.org> --- drivers/net/korina.c | 31 ++++++++++++++++--------------- 1 files changed, 16 insertions(+), 15 deletions(-) diff --git a/drivers/net/korina.c b/drivers/net/korina.c index a2701f5..c09e2ce 100644 --- a/drivers/net/korina.c +++ b/drivers/net/korina.c @@ -337,7 +337,7 @@ static irqreturn_t korina_rx_dma_interrupt(int irq, void *dev_id) napi_schedule(&lp->napi); if (dmas & DMA_STAT_ERR) - printk(KERN_ERR DRV_NAME "%s: DMA error\n", dev->name); + printk(KERN_ERR "%s: DMA error\n", dev->name); retval = IRQ_HANDLED; } else @@ -555,7 +555,7 @@ static void korina_tx(struct net_device *dev) dev->stats.tx_dropped++; /* Should never happen */ - printk(KERN_ERR DRV_NAME "%s: split tx ignored\n", + printk(KERN_ERR "%s: split tx ignored\n", dev->name); } else if (devcs & ETH_TX_TOK) { dev->stats.tx_packets++; @@ -641,7 +641,7 @@ korina_tx_dma_interrupt(int irq, void *dev_id) dev->trans_start = jiffies; } if (dmas & DMA_STAT_ERR) - printk(KERN_ERR DRV_NAME "%s: DMA error\n", dev->name); + printk(KERN_ERR "%s: DMA error\n", dev->name); retval = IRQ_HANDLED; } else @@ -909,8 +909,7 @@ static int korina_restart(struct net_device *dev) ret = korina_init(dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME "%s: cannot restart device\n", - dev->name); + printk(KERN_ERR "%s: cannot restart device\n", dev->name); return ret; } korina_multicast_list(dev); @@ -997,7 +996,7 @@ static int korina_open(struct net_device *dev) /* Initialize */ ret = korina_init(dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME "%s: cannot open device\n", dev->name); + printk(KERN_ERR "%s: cannot open device\n", dev->name); goto out; } @@ -1007,14 +1006,14 @@ static int korina_open(struct net_device *dev) ret = request_irq(lp->rx_irq, &korina_rx_dma_interrupt, IRQF_DISABLED, "Korina ethernet Rx", dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME "%s: unable to get Rx DMA IRQ %d\n", + printk(KERN_ERR "%s: unable to get Rx DMA IRQ %d\n", dev->name, lp->rx_irq); goto err_release; } ret = request_irq(lp->tx_irq, &korina_tx_dma_interrupt, IRQF_DISABLED, "Korina ethernet Tx", dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME "%s: unable to get Tx DMA IRQ %d\n", + printk(KERN_ERR "%s: unable to get Tx DMA IRQ %d\n", dev->name, lp->tx_irq); goto err_free_rx_irq; } @@ -1023,7 +1022,7 @@ static int korina_open(struct net_device *dev) ret = request_irq(lp->ovr_irq, &korina_ovr_interrupt, IRQF_DISABLED, "Ethernet Overflow", dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME"%s: unable to get OVR IRQ %d\n", + printk(KERN_ERR "%s: unable to get OVR IRQ %d\n", dev->name, lp->ovr_irq); goto err_free_tx_irq; } @@ -1032,7 +1031,7 @@ static int korina_open(struct net_device *dev) ret = request_irq(lp->und_irq, &korina_und_interrupt, IRQF_DISABLED, "Ethernet Underflow", dev); if (ret < 0) { - printk(KERN_ERR DRV_NAME "%s: unable to get UND IRQ %d\n", + printk(KERN_ERR "%s: unable to get UND IRQ %d\n", dev->name, lp->und_irq); goto err_free_ovr_irq; } @@ -1111,7 +1110,7 @@ static int korina_probe(struct platform_device *pdev) dev->base_addr = r->start; lp->eth_regs = ioremap_nocache(r->start, r->end - r->start); if (!lp->eth_regs) { - printk(KERN_ERR DRV_NAME "cannot remap registers\n"); + printk(KERN_ERR DRV_NAME ": cannot remap registers\n"); rc = -ENXIO; goto probe_err_out; } @@ -1119,7 +1118,7 @@ static int korina_probe(struct platform_device *pdev) r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "korina_dma_rx"); lp->rx_dma_regs = ioremap_nocache(r->start, r->end - r->start); if (!lp->rx_dma_regs) { - printk(KERN_ERR DRV_NAME "cannot remap Rx DMA registers\n"); + printk(KERN_ERR DRV_NAME ": cannot remap Rx DMA registers\n"); rc = -ENXIO; goto probe_err_dma_rx; } @@ -1127,14 +1126,14 @@ static int korina_probe(struct platform_device *pdev) r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "korina_dma_tx"); lp->tx_dma_regs = ioremap_nocache(r->start, r->end - r->start); if (!lp->tx_dma_regs) { - printk(KERN_ERR DRV_NAME "cannot remap Tx DMA registers\n"); + printk(KERN_ERR DRV_NAME ": cannot remap Tx DMA registers\n"); rc = -ENXIO; goto probe_err_dma_tx; } lp->td_ring = kmalloc(TD_RING_SIZE + RD_RING_SIZE, GFP_KERNEL); if (!lp->td_ring) { - printk(KERN_ERR DRV_NAME "cannot allocate descriptors\n"); + printk(KERN_ERR DRV_NAME ": cannot allocate descriptors\n"); rc = -ENXIO; goto probe_err_td_ring; } @@ -1175,9 +1174,11 @@ static int korina_probe(struct platform_device *pdev) rc = register_netdev(dev); if (rc < 0) { printk(KERN_ERR DRV_NAME - ": cannot register net device %d\n", rc); + ": cannot register net device: %d\n", rc); goto probe_err_register; } + printk(KERN_INFO "%s: " DRV_NAME "-" DRV_VERSION " " DRV_RELDATE "\n", + dev->name); out: return rc; -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 13+ messages in thread
[parent not found: <1249776388-4626-2-git-send-email-n0-1@freewrt.org>]
* [PATCH 2/3] korina: add error-handling to korina_alloc_ring [not found] ` <1249776388-4626-2-git-send-email-n0-1@freewrt.org> @ 2009-08-09 0:06 ` Phil Sutter [not found] ` <1249776388-4626-3-git-send-email-n0-1@freewrt.org> 1 sibling, 0 replies; 13+ messages in thread From: Phil Sutter @ 2009-08-09 0:06 UTC (permalink / raw) To: netdev; +Cc: Andrew Morton, David S. Miller, florian, Roel Kluin This also avoids a potential buffer overflow in case the very first receive descriptor fails to allocate, as an index of -1 would be used afterwards. Kudos to Roel Kluin for pointing this out and providing an initial patch. Signed-off-by: Phil Sutter <n0-1@freewrt.org> --- drivers/net/korina.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/net/korina.c b/drivers/net/korina.c index c09e2ce..d256022 100644 --- a/drivers/net/korina.c +++ b/drivers/net/korina.c @@ -741,7 +741,7 @@ static struct ethtool_ops netdev_ethtool_ops = { .get_link = netdev_get_link, }; -static void korina_alloc_ring(struct net_device *dev) +static int korina_alloc_ring(struct net_device *dev) { struct korina_private *lp = netdev_priv(dev); struct sk_buff *skb; @@ -762,7 +762,7 @@ static void korina_alloc_ring(struct net_device *dev) for (i = 0; i < KORINA_NUM_RDS; i++) { skb = dev_alloc_skb(KORINA_RBSIZE + 2); if (!skb) - break; + return -ENOMEM; skb_reserve(skb, 2); skb->dev = dev; lp->rx_skb[i] = skb; @@ -782,6 +782,8 @@ static void korina_alloc_ring(struct net_device *dev) lp->rx_chain_head = 0; lp->rx_chain_tail = 0; lp->rx_chain_status = desc_empty; + + return 0; } static void korina_free_ring(struct net_device *dev) @@ -824,7 +826,11 @@ static int korina_init(struct net_device *dev) writel(ETH_INT_FC_EN, &lp->eth_regs->ethintfc); /* Allocate rings */ - korina_alloc_ring(dev); + if (korina_alloc_ring(dev)) { + printk(KERN_ERR "%s: descriptor allocation failed\n", dev->name); + korina_free_ring(dev); + return -ENOMEM; + } writel(0, &lp->rx_dma_regs->dmas); /* Start Rx DMA */ -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 13+ messages in thread
[parent not found: <1249776388-4626-3-git-send-email-n0-1@freewrt.org>]
* [PATCH 3/3] korina: convert to net_device_ops [not found] ` <1249776388-4626-3-git-send-email-n0-1@freewrt.org> @ 2009-08-09 0:06 ` Phil Sutter 0 siblings, 0 replies; 13+ messages in thread From: Phil Sutter @ 2009-08-09 0:06 UTC (permalink / raw) To: netdev; +Cc: Andrew Morton, David S. Miller, florian, Roel Kluin Signed-off-by: Phil Sutter <n0-1@freewrt.org> --- drivers/net/korina.c | 23 ++++++++++++++--------- 1 files changed, 14 insertions(+), 9 deletions(-) diff --git a/drivers/net/korina.c b/drivers/net/korina.c index d256022..894f686 100644 --- a/drivers/net/korina.c +++ b/drivers/net/korina.c @@ -1088,6 +1088,18 @@ static int korina_close(struct net_device *dev) return 0; } +static const struct net_device_ops korina_netdev_ops = { + .ndo_open = korina_open, + .ndo_stop = korina_close, + .ndo_start_xmit = korina_send_packet, + .ndo_set_multicast_list = korina_multicast_list, + .ndo_tx_timeout = korina_tx_timeout, + .ndo_do_ioctl = korina_ioctl, +#ifdef CONFIG_NET_POLL_CONTROLLER + .ndo_poll_controller = korina_poll_controller, +#endif +}; + static int korina_probe(struct platform_device *pdev) { struct korina_device *bif = platform_get_drvdata(pdev); @@ -1156,17 +1168,10 @@ static int korina_probe(struct platform_device *pdev) dev->irq = lp->rx_irq; lp->dev = dev; - dev->open = korina_open; - dev->stop = korina_close; - dev->hard_start_xmit = korina_send_packet; - dev->set_multicast_list = &korina_multicast_list; + dev->netdev_ops = &korina_netdev_ops; dev->ethtool_ops = &netdev_ethtool_ops; - dev->tx_timeout = korina_tx_timeout; dev->watchdog_timeo = TX_TIMEOUT; - dev->do_ioctl = &korina_ioctl; -#ifdef CONFIG_NET_POLL_CONTROLLER - dev->poll_controller = korina_poll_controller; -#endif + netif_napi_add(dev, &lp->napi, korina_poll, 64); lp->phy_addr = (((lp->rx_irq == 0x2c? 1:0) << 8) | 0x05); -- 1.6.0.6 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2009-08-13 23:27 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-08-07 15:33 [PATCH] korina: Read buffer overflow Roel Kluin
2009-08-08 0:48 ` Phil Sutter
2009-08-08 13:14 ` roel kluin
2009-08-08 16:45 ` Phil Sutter
2009-08-09 0:06 ` Phil Sutter
2009-08-12 22:15 ` Phil Sutter
2009-08-12 22:22 ` [PATCH 1/2] korina: fix printk formatting, add final info line Phil Sutter
2009-08-12 22:52 ` [PATCH 2/2] korina: add error-handling to korina_alloc_ring Phil Sutter
2009-08-13 23:27 ` David Miller
2009-08-13 23:27 ` [PATCH 1/2] korina: fix printk formatting, add final info line David Miller
[not found] ` <1249776388-4626-1-git-send-email-n0-1@freewrt.org>
2009-08-09 0:06 ` [PATCH 1/3] " Phil Sutter
[not found] ` <1249776388-4626-2-git-send-email-n0-1@freewrt.org>
2009-08-09 0:06 ` [PATCH 2/3] korina: add error-handling to korina_alloc_ring Phil Sutter
[not found] ` <1249776388-4626-3-git-send-email-n0-1@freewrt.org>
2009-08-09 0:06 ` [PATCH 3/3] korina: convert to net_device_ops Phil Sutter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).