* [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
* [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
* [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
* [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
* 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 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
* 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
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).