Message-ID: <20071028043846.GM29176@networkno.de>
Date:	Sun, 28 Oct 2007 04:38:46 +0000
From:	Thiemo Seufer <ths@...workno.de>
To:	linux-mips@...ux-mips.org
Cc:	ralf@...ux-mips.org, netdev@...r.kernel.org
Subject: [PATCH] Fix/Rewrite of the mipsnet driver

Hello All,

currently the mipsnet driver fails after transmitting a number of
packages because SKBs are allocated but never freed. I fixed that
and coudn't refrain from removing the most egregious warts.

- mipsnet.h folded into mipsnet.c, as it doesn't provide any
  useful external interface.
- Free SKB after transmission.
- Call free_irq in mipsnet_close, to balance the request_irq in
  mipsnet_open.
- Removed duplicate read of rxDataCount.
- Some identifiers are now less verbose.
- Removed dead and/or unnecessarily complex code.
- Code formatting fixes.

Tested on Qemu's mipssim emulation, with this patch it can boot a
Debian NFSroot.


Thiemo


Signed-off-by: Thiemo Seufer <ths@...workno.de>
---
diff --git a/drivers/net/mipsnet.c b/drivers/net/mipsnet.c
index 9853c74..28c66c1 100644
--- a/drivers/net/mipsnet.c
+++ b/drivers/net/mipsnet.c
@@ -4,8 +4,6 @@
  * for more details.
  */
 
-#define DEBUG
-
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -16,11 +14,93 @@
 #include <asm/io.h>
 #include <asm/mips-boards/simint.h>
 
-#include "mipsnet.h"		/* actual device IO mapping */
+#define MIPSNET_VERSION "2007-10-28"
+
+/*
+ * Net status/control block as seen by sw in the core.
+ */
+struct MIPS_T_NetControl {
+	/*
+	 * Device info for probing, reads as MIPSNET%d where %d is some
+	 * form of version.
+	 */
+	uint64_t devId;		/*0x00 */
+
+	/*
+	 * read only busy flag.
+	 * Set and cleared by the Net Device to indicate that an rx or a tx
+	 * is in progress.
+	 */
+	uint32_t busy;		/*0x08 */
+
+	/*
+	 * Set by the Net Device.
+	 * The device will set it once data has been received.
+	 * The value is the number of bytes that should be read from
+	 * rxDataBuffer.  The value will decrease till 0 until all the data
+	 * from rxDataBuffer has been read.
+	 */
+	uint32_t rxDataCount;	/*0x0c */
+#define MIPSNET_MAX_RXTX_DATACOUNT (1<<16)
+
+	/*
+	 * Settable from the MIPS core, cleared by the Net Device.
+	 * The core should set the number of bytes it wants to send,
+	 * then it should write those bytes of data to txDataBuffer.
+	 * The device will clear txDataCount has been processed (not
+	 * necessarily sent).
+	 */
+	uint32_t txDataCount;	/*0x10 */
 
-#define MIPSNET_VERSION "2005-06-20"
+	/*
+	 * Interrupt control
+	 *
+	 * Used to clear the interrupted generated by this dev.
+	 * Write a 1 to clear the interrupt. (except bit31).
+	 *
+	 * Bit0 is set if it was a tx-done interrupt.
+	 * Bit1 is set when new rx-data is available.
+	 *    Until this bit is cleared there will be no other RXs.
+	 *
+	 * Bit31 is used for testing, it clears after a read.
+	 *    Writing 1 to this bit will cause an interrupt to be generated.
+	 *    To clear the test interrupt, write 0 to this register.
+	 */
+	uint32_t interruptControl;	/*0x14 */
+#define MIPSNET_INTCTL_TXDONE     ((uint32_t)(1 << 0))
+#define MIPSNET_INTCTL_RXDONE     ((uint32_t)(1 << 1))
+#define MIPSNET_INTCTL_TESTBIT    ((uint32_t)(1 << 31))
 
-#define mipsnet_reg_address(dev, field) (dev->base_addr + field_offset(field))
+	/*
+	 * Readonly core-specific interrupt info for the device to signal
+	 * the core. The meaning of the contents of this field might change.
+	 */
+	/* XXX: the whole memIntf interrupt scheme is messy: the device
+	 * should have no control what so ever of what VPE/register set is
+	 * being used.
+	 * The MemIntf should only expose interrupt lines, and something in
+	 * the config should be responsible for the line<->core/vpe bindings.
+	 */
+	uint32_t interruptInfo;	/*0x18 */
+
+	/*
+	 * This is where the received data is read out.
+	 * There is more data to read until rxDataReady is 0.
+	 * Only 1 byte at this regs offset is used.
+	 */
+	uint32_t rxDataBuffer;	/*0x1c */
+
+	/*
+	 * This is where the data to transmit is written.
+	 * Data should be written for the amount specified in the
+	 * txDataCount register.
+	 * Only 1 byte at this regs offset is used.
+	 */
+	uint32_t txDataBuffer;	/*0x20 */
+};
+
+#define regaddr(dev, field) \
+  (dev->base_addr + offsetof(MIPS_T_NetControl, field))
 
 struct mipsnet_priv {
 	struct net_device_stats stats;
@@ -34,18 +114,13 @@ static char mipsnet_string[] = "mipsnet";
 static int ioiocpy_frommipsnet(struct net_device *dev, unsigned char *kdata,
 			int len)
 {
-	uint32_t available_len = inl(mipsnet_reg_address(dev, rxDataCount));
-	if (available_len < len)
-		return -EFAULT;
+	for (; len > 0; len--, kdata++)
+		*kdata = inb(regaddr(dev, rxDataBuffer));
 
-	for (; len > 0; len--, kdata++) {
-		*kdata = inb(mipsnet_reg_address(dev, rxDataBuffer));
-	}
-
-	return inl(mipsnet_reg_address(dev, rxDataCount));
+	return inl(regaddr(dev, rxDataCount));
 }
 
-static inline ssize_t mipsnet_put_todevice(struct net_device *dev,
+static inline void mipsnet_put_todevice(struct net_device *dev,
 	struct sk_buff *skb)
 {
 	int count_to_go = skb->len;
@@ -55,19 +130,19 @@ static inline ssize_t mipsnet_put_todevice(struct net_device *dev,
 	pr_debug("%s: %s(): telling MIPSNET txDataCount(%d)\n",
 	         dev->name, __FUNCTION__, skb->len);
 
-	outl(skb->len, mipsnet_reg_address(dev, txDataCount));
+	outl(skb->len, regaddr(dev, txDataCount));
 
 	pr_debug("%s: %s(): sending data to MIPSNET txDataBuffer(%d)\n",
 	         dev->name, __FUNCTION__, skb->len);
 
 	for (; count_to_go; buf_ptr++, count_to_go--) {
-		outb(*buf_ptr, mipsnet_reg_address(dev, txDataBuffer));
+		outb(*buf_ptr, regaddr(dev, txDataBuffer));
 	}
 
 	mp->stats.tx_packets++;
 	mp->stats.tx_bytes += skb->len;
 
-	return skb->len;
+	dev_kfree_skb(skb);
 }
 
 static int mipsnet_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -75,7 +150,8 @@ static int mipsnet_xmit(struct sk_buff *skb, struct net_device *dev)
 	pr_debug("%s:%s(): transmitting %d bytes\n",
 	         dev->name, __FUNCTION__, skb->len);
 
-	/* Only one packet at a time. Once TXDONE interrupt is serviced, the
+	/*
+	 * Only one packet at a time. Once TXDONE interrupt is serviced, the
 	 * queue will be restarted.
 	 */
 	netif_stop_queue(dev);
@@ -84,17 +160,19 @@ static int mipsnet_xmit(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
-static inline ssize_t mipsnet_get_fromdev(struct net_device *dev, size_t count)
+static inline ssize_t mipsnet_get_fromdev(struct net_device *dev, size_t len)
 {
 	struct sk_buff *skb;
-	size_t len = count;
 	struct mipsnet_priv *mp = netdev_priv(dev);
 
-	if (!(skb = alloc_skb(len + 2, GFP_KERNEL))) {
+	if (!len)
+		return len;
+
+	skb = dev_alloc_skb(len + 2);
+	if (!skb) {
 		mp->stats.rx_dropped++;
 		return -ENOMEM;
 	}
-
 	skb_reserve(skb, 2);
 	if (ioiocpy_frommipsnet(dev, skb_put(skb, len), len))
 		return -EFAULT;
@@ -103,70 +181,65 @@ static inline ssize_t mipsnet_get_fromdev(struct net_device *dev, size_t count)
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 	pr_debug("%s:%s(): pushing RXed data to kernel\n",
-	         dev->name, __FUNCTION__);
+		 dev->name, __FUNCTION__);
 	netif_rx(skb);
 
 	mp->stats.rx_packets++;
 	mp->stats.rx_bytes += len;
 
-	return count;
+	return len;
 }
 
 static irqreturn_t mipsnet_interrupt(int irq, void *dev_id)
 {
 	struct net_device *dev = dev_id;
-
-	irqreturn_t retval = IRQ_NONE;
-	uint64_t interruptFlags;
-
-	if (irq == dev->irq) {
-		pr_debug("%s:%s(): irq %d for device\n",
-		         dev->name, __FUNCTION__, irq);
-
-		retval = IRQ_HANDLED;
-
-		interruptFlags =
-		    inl(mipsnet_reg_address(dev, interruptControl));
-		pr_debug("%s:%s(): intCtl=0x%016llx\n", dev->name,
-		         __FUNCTION__, interruptFlags);
-
-		if (interruptFlags & MIPSNET_INTCTL_TXDONE) {
-			pr_debug("%s:%s(): got TXDone\n",
-			         dev->name, __FUNCTION__);
-			outl(MIPSNET_INTCTL_TXDONE,
-			     mipsnet_reg_address(dev, interruptControl));
-			// only one packet at a time, we are done.
-			netif_wake_queue(dev);
-		} else if (interruptFlags & MIPSNET_INTCTL_RXDONE) {
-			pr_debug("%s:%s(): got RX data\n",
-			         dev->name, __FUNCTION__);
-			mipsnet_get_fromdev(dev,
-			            inl(mipsnet_reg_address(dev, rxDataCount)));
-			pr_debug("%s:%s(): clearing RX int\n",
-			         dev->name, __FUNCTION__);
-			outl(MIPSNET_INTCTL_RXDONE,
-			     mipsnet_reg_address(dev, interruptControl));
-
-		} else if (interruptFlags & MIPSNET_INTCTL_TESTBIT) {
-			pr_debug("%s:%s(): got test interrupt\n",
-			         dev->name, __FUNCTION__);
-			// TESTBIT is cleared on read.
-			//    And takes effect after a write with 0
-			outl(0, mipsnet_reg_address(dev, interruptControl));
-		} else {
-			pr_debug("%s:%s(): no valid fags 0x%016llx\n",
-			         dev->name, __FUNCTION__, interruptFlags);
-			// Maybe shared IRQ, just ignore, no clearing.
-			retval = IRQ_NONE;
-		}
-
+	struct mipsnet_priv *mp = netdev_priv(dev);
+	u32 int_flags;
+	int ret = IRQ_NONE;
+
+	if (irq != dev->irq)
+		goto out_badirq;
+
+	/* TESTBIT is cleared on read. */
+	int_flags = inl(regaddr(dev, interruptControl));
+	pr_debug("%s:%s(): irq %d intCtl=0x%x\n", dev->name,
+		 __FUNCTION__, irq, int_flags);
+
+	if (int_flags & MIPSNET_INTCTL_TESTBIT) {
+		pr_debug("%s:%s(): got test interrupt\n",
+			 dev->name, __FUNCTION__);
+		/* TESTBIT takes effect after a write with 0. */
+		outl(0, regaddr(dev, interruptControl));
+		ret = IRQ_HANDLED;
+	} else if (int_flags & MIPSNET_INTCTL_TXDONE) {
+		pr_debug("%s:%s(): got TXDone\n",
+			 dev->name, __FUNCTION__);
+		/* Only one packet at a time, we are done. */
+		mp->stats.tx_packets++;
+		netif_wake_queue(dev);
+		pr_debug("%s:%s(): clearing TX int\n",
+			 dev->name, __FUNCTION__);
+		outl(MIPSNET_INTCTL_TXDONE,
+		     regaddr(dev, interruptControl));
+		ret = IRQ_HANDLED;
+	} else if (int_flags & MIPSNET_INTCTL_RXDONE) {
+		pr_debug("%s:%s(): got RX data\n", dev->name, __FUNCTION__);
+		mipsnet_get_fromdev(dev, inl(regaddr(dev, rxDataCount)));
+		pr_debug("%s:%s(): clearing RX int\n",
+			 dev->name, __FUNCTION__);
+		outl(MIPSNET_INTCTL_RXDONE, regaddr(dev, interruptControl));
+		ret = IRQ_HANDLED;
 	} else {
-		printk(KERN_INFO "%s: %s(): irq %d for unknown device\n",
-		       dev->name, __FUNCTION__, irq);
-		retval = IRQ_NONE;
+		pr_debug("%s:%s(): no valid flags 0x%x\n",
+			 dev->name, __FUNCTION__, int_flags);
 	}
-	return retval;
-}				//mipsnet_interrupt()
+	return ret;
+
+out_badirq:
+	printk(KERN_INFO "%s: %s(): irq %d for unknown device\n",
+	       dev->name, __FUNCTION__, irq);
+	return ret;
+}
 
 static int mipsnet_open(struct net_device *dev)
 {
@@ -179,7 +252,7 @@ static int mipsnet_open(struct net_device *dev)
 	if (err) {
 		pr_debug("%s: %s(): can't get irq %d\n",
 		         dev->name, __FUNCTION__, dev->irq);
-		release_region(dev->base_addr, MIPSNET_IO_EXTENT);
+		release_region(dev->base_addr, sizeof(MIPS_T_NetControl));
 		return err;
 	}
 
@@ -189,9 +262,9 @@ static int mipsnet_open(struct net_device *dev)
 
 	netif_start_queue(dev);
 
-	// test interrupt handler
+	/* test interrupt handler */
 	outl(MIPSNET_INTCTL_TESTBIT,
-	     mipsnet_reg_address(dev, interruptControl));
+	     regaddr(dev, interruptControl));
 
 
 	return 0;
@@ -201,6 +274,7 @@ static int mipsnet_close(struct net_device *dev)
 {
 	pr_debug("%s: %s()\n", dev->name, __FUNCTION__);
 	netif_stop_queue(dev);
+	free_irq(dev->irq, dev);
 	return 0;
 }
 
@@ -213,7 +287,7 @@ static struct net_device_stats *mipsnet_get_stats(struct net_device *dev)
 
 static void mipsnet_set_mclist(struct net_device *dev)
 {
-	// we don't do anything
+	/* we don't do anything */
 	return;
 }
 
@@ -241,13 +315,15 @@ static int __init mipsnet_probe(struct device *dev)
 	 */
 	netdev->base_addr = 0x4200;
 	netdev->irq = MIPS_CPU_IRQ_BASE + MIPSCPU_INT_MB0 +
-	              inl(mipsnet_reg_address(netdev, interruptInfo));
+		      inl(regaddr(netdev, interruptInfo));
 
-	// Get the io region now, get irq on open()
-	if (!request_region(netdev->base_addr, MIPSNET_IO_EXTENT, "mipsnet")) {
+	/* Get the io region now, get irq on open() */
+	if (!request_region(netdev->base_addr, sizeof(MIPS_T_NetControl),
+			    "mipsnet")) {
 		pr_debug("%s: %s(): IO region {start: 0x%04lux, len: %d} "
-		         "for dev is not availble.\n", netdev->name,
-		         __FUNCTION__, netdev->base_addr, MIPSNET_IO_EXTENT);
+			 "for dev is not availble.\n", netdev->name,
+			 __FUNCTION__, netdev->base_addr,
+			 sizeof(MIPS_T_NetControl));
 		err = -EBUSY;
 		goto out_free_netdev;
 	}
@@ -267,7 +343,7 @@ static int __init mipsnet_probe(struct device *dev)
 	return 0;
 
 out_free_region:
-	release_region(netdev->base_addr, MIPSNET_IO_EXTENT);
+	release_region(netdev->base_addr, sizeof(MIPS_T_NetControl));
 
 out_free_netdev:
 	free_netdev(netdev);
@@ -281,7 +357,7 @@ static int __devexit mipsnet_device_remove(struct device *device)
 	struct net_device *dev = dev_get_drvdata(device);
 
 	unregister_netdev(dev);
-	release_region(dev->base_addr, MIPSNET_IO_EXTENT);
+	release_region(dev->base_addr, sizeof(MIPS_T_NetControl));
 	free_netdev(dev);
 	dev_set_drvdata(device, NULL);
 
diff --git a/drivers/net/mipsnet.h b/drivers/net/mipsnet.h
deleted file mode 100644
index 026c732..0000000
--- a/drivers/net/mipsnet.h
+++ /dev/null
@@ -1,107 +0,0 @@
-/*
- * This file is subject to the terms and conditions of the GNU General Public
- * License.  See the file "COPYING" in the main directory of this archive
- * for more details.
- */
-#ifndef __MIPSNET_H
-#define __MIPSNET_H
-
-/*
- *  Id of this Net device, as seen by the core.
- */
-#define MIPS_NET_DEV_ID ((uint64_t)           \
-	                     ((uint64_t)'M'<< 0)| \
-	                     ((uint64_t)'I'<< 8)| \
-	                     ((uint64_t)'P'<<16)| \
-	                     ((uint64_t)'S'<<24)| \
-	                     ((uint64_t)'N'<<32)| \
-	                     ((uint64_t)'E'<<40)| \
-	                     ((uint64_t)'T'<<48)| \
-	                     ((uint64_t)'0'<<56))
-
-/*
- * Net status/control block as seen by sw in the core.
- * (Why not use bit fields? can't be bothered with cross-platform struct
- *  packing.)
- */
-typedef struct _net_control_block {
-	/// dev info for probing
-	///  reads as MIPSNET%d where %d is some form of version
-	uint64_t devId;		/*0x00 */
-
-	/*
-	 * read only busy flag.
-	 * Set and cleared by the Net Device to indicate that an rx or a tx
-	 * is in progress.
-	 */
-	uint32_t busy;		/*0x08 */
-
-	/*
-	 * Set by the Net Device.
-	 * The device will set it once data has been received.
-	 * The value is the number of bytes that should be read from
-	 * rxDataBuffer.  The value will decrease till 0 until all the data
-	 * from rxDataBuffer has been read.
-	 */
-	uint32_t rxDataCount;	/*0x0c */
-#define MIPSNET_MAX_RXTX_DATACOUNT (1<<16)
-
-	/*
-	 * Settable from the MIPS core, cleared by the Net Device.
-	 * The core should set the number of bytes it wants to send,
-	 *   then it should write those bytes of data to txDataBuffer.
-	 * The device will clear txDataCount has been processed (not necessarily sent).
-	 */
-	uint32_t txDataCount;	/*0x10 */
-
-	/*
-	 * Interrupt control
-	 *
-	 * Used to clear the interrupted generated by this dev.
-	 * Write a 1 to clear the interrupt. (except bit31).
-	 *
-	 * Bit0 is set if it was a tx-done interrupt.
-	 * Bit1 is set when new rx-data is available.
-	 *      Until this bit is cleared there will be no other RXs.
-	 *
-	 * Bit31 is used for testing, it clears after a read.
-	 *    Writing 1 to this bit will cause an interrupt to be generated.
-	 *    To clear the test interrupt, write 0 to this register.
-	 */
-	uint32_t interruptControl;	/*0x14 */
-#define MIPSNET_INTCTL_TXDONE     ((uint32_t)(1<< 0))
-#define MIPSNET_INTCTL_RXDONE     ((uint32_t)(1<< 1))
-#define MIPSNET_INTCTL_TESTBIT    ((uint32_t)(1<<31))
-#define MIPSNET_INTCTL_ALLSOURCES (MIPSNET_INTCTL_TXDONE|MIPSNET_INTCTL_RXDONE|MIPSNET_INTCTL_TESTBIT)
-
-	/*
-	 * Readonly core-specific interrupt info for the device to signal the core.
-	 * The meaning of the contents of this field might change.
-	 */
-	/*###\todo: the whole memIntf interrupt scheme is messy: the device should have
-	 *  no control what so ever of what VPE/register set is being used.
-	 *  The MemIntf should only expose interrupt lines, and something in the
-	 *  config should be responsible for the line<->core/vpe bindings.
-	 */
-	uint32_t interruptInfo;	/*0x18 */
-
-	/*
-	 *  This is where the received data is read out.
-	 *  There is more data to read until rxDataReady is 0.
-	 *  Only 1 byte at this regs offset is used.
-	 */
-	uint32_t rxDataBuffer;	/*0x1c */
-
-	/*
-	 * This is where the data to transmit is written.
-	 * Data should be written for the amount specified in the txDataCount register.
-	 *  Only 1 byte at this regs offset is used.
-	 */
-	uint32_t txDataBuffer;	/*0x20 */
-} MIPS_T_NetControl;
-
-#define MIPSNET_IO_EXTENT 0x40	/* being generous */
-
-#define field_offset(field) ((int)&((MIPS_T_NetControl*)(0))->field)
-
-#endif /* __MIPSNET_H */
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists