| Message ID | 4A7C494B.2060204@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html