On Thu, Sep 21, 2006 at 07:10:02PM -0400, Jeff Garzik wrote:
> >+ if (!(rstat0 & RSTAT0_RFP)) {
> >+ printk(KERN_CRIT "ep93xx_rx: buffer not done "
> >+ " %.8x %.8x\n", rstat0, rstat1);
> >+ BUG();
> >+ }
> >+ if (!(rstat0 & RSTAT0_EOF)) {
> >+ printk(KERN_CRIT "ep93xx_rx: not end-of-frame "
> >+ " %.8x %.8x\n", rstat0, rstat1);
> >+ BUG();
> >+ }
> >+ if (!(rstat0 & RSTAT0_EOB)) {
> >+ printk(KERN_CRIT "ep93xx_rx: not end-of-buffer "
> >+ " %.8x %.8x\n", rstat0, rstat1);
> >+ BUG();
> >+ }
> >+ if (!(rstat1 & RSTAT1_RFP)) {
> >+ printk(KERN_CRIT "ep93xx_rx: buffer1 not done "
> >+ " %.8x %.8x\n", rstat0, rstat1);
> >+ BUG();
> >+ }
> >+ if ((rstat1 & RSTAT1_BUFFER_INDEX) >> 16 != entry) {
> >+ printk(KERN_CRIT "ep93xx_rx: entry mismatch "
> >+ " %.8x %.8x\n", rstat0, rstat1);
> >+ BUG();
> >+ }
>
> NAK all these BUGs.
>
> Very unfriendly
If any of these checks trigger, we are in a very bad state, and
something is likely trampling over random bits of memory, but OK,
removed.
> >+ if (tstat0 & TSTAT0_TXWE) {
> >+ int length = ep->descs->tdesc[entry].tdesc1 & 0xfff;
> >+
> >+ ep->stats.tx_packets++;
> >+ ep->stats.tx_bytes += length;
> >+ } else {
> >+ ep->stats.tx_errors++;
> >+ }
> >+#if 0
> >+ /* This is only valid in half duplex mode. */
> >+ if (tstat0 & TSTAT0_LCRS)
> >+ ep->stats.tx_carrier_errors++;
> >+#endif
>
> why #if 0'd?
The CRS bit will be set in the tx completion entry if the MII CRS
(Carrier Sense) signal wasn't asserted after the first N nibbles have
been transmitted. However, if the PHY is running in full duplex mode,
the CRS signal isn't supposed to assert at all, and so we ended up
counting tx_carrier_errors for every transmitted packet if the
interface was in full duplex mode.
I removed this bit of code rather than #if 0'ing it out.
> >+static irqreturn_t ep93xx_irq(int irq, void *dev_id, struct pt_regs *regs)
> >+{
> >+ struct net_device *dev = dev_id;
> >+ struct ep93xx_priv *ep = netdev_priv(dev);
> >+ u32 status;
> >+
> >+ status = rdl(ep, REG_INTSTSC);
> >+ if (status == 0)
> >+ return IRQ_NONE;
>
> also check for status == 0xffffffff
As the ethernet controller is on the CPU die itself, it's not very
likely to be unplugged?
> >+static int ep93xx_alloc_buffers(struct ep93xx_priv *ep)
> >+{
> >+ int i;
> >+
> >+ ep->descs = dma_alloc_coherent(NULL, sizeof(struct ep93xx_descs),
> >+ &ep->descs_dma_addr, GFP_KERNEL | GFP_DMA);
> >+ if (ep->descs == NULL)
> >+ return 1;
> >+
> >+ for (i = 0; i < RX_QUEUE_ENTRIES; i += 2) {
> >+ void *page;
> >+ dma_addr_t d;
> >+
> >+ page = (void *)get_zeroed_page(GFP_KERNEL | GFP_DMA);
> >+ if (page == NULL)
> >+ goto err;
>
> do you really need a zeroed page?
No, any page will do -- fixed.
> >+static int ep93xx_eth_remove(struct platform_device *pdev)
> >+{
> >+ struct net_device *dev;
> >+ struct ep93xx_priv *ep;
> >+
> >+ dev = platform_get_drvdata(pdev);
> >+ if (dev == NULL)
> >+ return 0;
> >+ platform_set_drvdata(pdev, NULL);
> >+
> >+ ep = netdev_priv(dev);
> >+
> >+ /* @@@ Force down. */
> >+ unregister_netdev(dev);
> >+ ep93xx_free_buffers(ep);
> >+
> >+ if (ep->base_addr != NULL)
> >+ iounmap(ep->base_addr);
> >+
> >+ if (ep->res != NULL) {
> >+ release_resource(ep->res);
> >+ kfree(ep->res);
> >+ }
>
> when will these ever be NULL ?
ep93xx_eth_remove is called from ep93xx_eth_probe's error path (see
below.)
All other issues fixed as well. Thanks for your time.
> >+ free_netdev(dev);
> >+
> >+ return 0;
> >+}
> >+
> >+static int ep93xx_eth_probe(struct platform_device *pdev)
> >+{
> >+ struct ep93xx_eth_data *data;
> >+ struct net_device *dev;
> >+ struct ep93xx_priv *ep;
> >+ int err;
> >+
> >+ data = pdev->dev.platform_data;
> >+ if (pdev == NULL)
> >+ return -ENODEV;
> >+
> >+ dev = ep93xx_dev_alloc(data);
> >+ if (dev == NULL) {
> >+ err = -ENOMEM;
> >+ goto err_out;
> >+ }
> >+ ep = netdev_priv(dev);
> >+
> >+ platform_set_drvdata(pdev, dev);
> >+
> >+ ep->res = request_mem_region(pdev->resource[0].start,
> >+ pdev->resource[0].end - pdev->resource[0].start + 1,
> >+ pdev->dev.bus_id);
> >+ if (ep->res == NULL) {
> >+ dev_err(&pdev->dev, "Could not reserve memory region\n");
> >+ err = -ENOMEM;
> >+ goto err_out;
> >+ }
> >+
> >+ ep->base_addr = ioremap(pdev->resource[0].start,
> >+ pdev->resource[0].end - pdev->resource[0].start);
> >+ if (ep->base_addr == NULL) {
> >+ dev_err(&pdev->dev, "Failed to ioremap ethernet
> >registers\n");
> >+ err = -EIO;
> >+ goto err_out;
> >+ }
> >+ ep->irq = pdev->resource[1].start;
> >+
> >+ ep->mii.phy_id = data->phy_id;
> >+ ep->mii.phy_id_mask = 0x1f;
> >+ ep->mii.reg_num_mask = 0x1f;
> >+ ep->mii.dev = dev;
> >+ ep->mii.mdio_read = ep93xx_mdio_read;
> >+ ep->mii.mdio_write = ep93xx_mdio_write;
> >+ ep->mdc_divisor = 40; /* Max HCLK 100 MHz, min MDIO clk 2.5 MHz.
> >*/
> >+
> >+ err = register_netdev(dev);
> >+ if (err) {
> >+ dev_err(&pdev->dev, "Failed to register netdev\n");
> >+ goto err_out;
> >+ }
> >+
> >+ printk(KERN_INFO "%s: ep93xx on-chip ethernet, IRQ %d, "
> >+ "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x.\n", dev->name,
> >+ ep->irq, data->dev_addr[0], data->dev_addr[1],
> >+ data->dev_addr[2], data->dev_addr[3],
> >+ data->dev_addr[4], data->dev_addr[5]);
> >+
> >+ return 0;
> >+
> >+err_out:
> >+ ep93xx_eth_remove(pdev);
> >+ return err;
> >+}
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
