linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] Net device error logging, revised
@ 2003-08-25 21:31 Jim Keniston
  2003-08-25 21:39 ` Subject: [PATCH 2/4] Net device error logging, revised (e100) Jim Keniston
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Jim Keniston @ 2003-08-25 21:31 UTC (permalink / raw)
  To: LKML, netdev
  Cc: Jeff Garzik, Feldman, Scott, Larry Kessler, Greg KH,
	Randy Dunlap, Alan Cox, Andrew Morton, jkenisto

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

This patch extends the concept of Linux 2.6's dev_* logging macros to
support network devices.  Analogous netdev_* macros are defined.  This
feature is part of an effort to simplify error-log analysis by providing
more consistent and informative messages.

This is a modification of a proposal from May.  The changes reflect
suggestions made on LKML, at the Kernel Summit, and at OLS.

Calls to the netdev_* macros (netdev_printk and wrappers such as
netdev_err) are intended to replace calls to printk in network device
drivers.  These macros have the following characteristics:
- Same format + args as the corresponding printk call.
- Approximately the same amount of text as the corresponding printk call.
- The first arg is a pointer to the net_device struct.
- The second (optional) arg, which is a NETIF_MSG_* message level, can be
used to implement verbosity control.
- Standard message prefixes: verbose (see below) during probe, or just the
interface name once the device is registered.
- The current implementation just calls printk.  However, the netdev_*
interface (and availability of the net_device pointer) opens the door
for logging additional information (via printk, via evlog/netlink, etc.)
as desired, with no change to driver code.

Examples:
        netdev_err(netdev, RX_ERR, "No mem: dropped packet\n");
logs a message such as the following if the NETIF_MSG_RX_ERR bit is set
in netdev->msg_enable.
        eth2: No mem: dropped packet

        netdev_err(netdev,, "The EEPROM Checksum Is Not Valid\n");
unconditionally logs a message such as:
        eth%d (e1000 0000:00:03.0): The EEPROM Checksum Is Not Valid
The message's prefix includes the driver name and bus ID because the
message is logged at probe time, before netdev is registered.

Note that the netdev_* interface can be used in v2.4 drivers as well,
but in a v2.4 implementation, the message prefix would always be just
the interface name.

Three other patches are included in subsequent emails.  These patch
the e100, e1000, and tg3 Ethernet drivers to use the netdev_* macros.
(For e100 and e1000, they also add the necessary scaffolding for verbosity
control via the NETIF_MSG_* message levels).

Jim Keniston
IBM Linux Technology Center
-----

[-- Attachment #2: netdev-2.6.0-test4.patch --]
[-- Type: text/plain, Size: 5417 bytes --]

diff -Naur linux.org/include/linux/netdevice.h linux.netdev.patched/include/linux/netdevice.h
--- linux.org/include/linux/netdevice.h	Mon Aug 25 13:29:38 2003
+++ linux.netdev.patched/include/linux/netdevice.h	Mon Aug 25 13:29:38 2003
@@ -470,6 +470,9 @@
 	struct divert_blk	*divert;
 #endif /* CONFIG_NET_DIVERT */
 
+	/* NETIF_MSG_* flags to control the types of events we log */
+	int msg_enable;
+
 	/* class/net/name entry */
 	struct class_device	class_dev;
 };
@@ -743,6 +746,8 @@
 	NETIF_MSG_PKTDATA	= 0x1000,
 	NETIF_MSG_HW		= 0x2000,
 	NETIF_MSG_WOL		= 0x4000,
+	NETIF_MSG_ALL		= -1,		/* always log message */
+	NETIF_MSG_		= -1		/* always log message */
 };
 
 #define netif_msg_drv(p)	((p)->msg_enable & NETIF_MSG_DRV)
@@ -869,6 +874,36 @@
 extern void		dev_clear_fastroute(struct net_device *dev);
 #endif
 
+/* debugging and troubleshooting/diagnostic helpers. */
+/**
+ * netdev_printk() - Log message with interface name, gated by message level
+ * @sevlevel: severity level -- e.g., KERN_INFO
+ * @netdev: net_device pointer
+ * @msglevel: a standard message-level flag with the NETIF_MSG_ prefix removed.
+ *	Unless msglevel is NETIF_MSG_ALL, or omitted, log the message only if
+ *	that flag is set in netdev->msg_enable.
+ * @format: as with printk
+ * @args: as with printk
+ */
+extern int __netdev_printk(const char *sevlevel,
+	const struct net_device *netdev, int msglevel, const char *format, ...);
+#define netdev_printk(sevlevel, netdev, msglevel, format, arg...)	\
+	__netdev_printk(sevlevel , netdev , NETIF_MSG_##msglevel ,	\
+	format , ## arg)
+
+#ifdef DEBUG
+#define netdev_dbg(netdev, msglevel, format, arg...)		\
+	netdev_printk(KERN_DEBUG , netdev , msglevel , format , ## arg)
+#else
+#define netdev_dbg(netdev, msglevel, format, arg...) do {} while (0)
+#endif
+
+#define netdev_err(netdev, msglevel, format, arg...)		\
+	netdev_printk(KERN_ERR , netdev , msglevel , format , ## arg)
+#define netdev_info(netdev, msglevel, format, arg...)		\
+	netdev_printk(KERN_INFO , netdev , msglevel , format , ## arg)
+#define netdev_warn(netdev, msglevel, format, arg...)		\
+	netdev_printk(KERN_WARNING , netdev , msglevel , format , ## arg)
 
 #endif /* __KERNEL__ */
 
diff -Naur linux.org/net/core/dev.c linux.netdev.patched/net/core/dev.c
--- linux.org/net/core/dev.c	Mon Aug 25 13:29:38 2003
+++ linux.netdev.patched/net/core/dev.c	Mon Aug 25 13:29:38 2003
@@ -3116,5 +3116,71 @@
 out:
 	return rc;
 }
-
 subsys_initcall(net_dev_init);
+
+/**
+ * __netdev_printk() - Log message with interface name, gated by message level
+ * @sevlevel: severity level -- e.g., KERN_INFO
+ * @netdev: net_device pointer
+ * @msglevel: a standard message-level flag such as NETIF_MSG_PROBE.
+ *	Unless msglevel is NETIF_MSG_ALL, log the message only if
+ *	that flag is set in netdev->msg_enable.
+ * @format: as with printk
+ * @args: as with printk
+ *
+ * Does the work for the netdev_printk macro.
+ * For a lot of network drivers, the probe function looks like
+ *	...
+ *	netdev = alloc_netdev(...);	// or alloc_etherdev(...)
+ *	SET_NETDEV_DEV(netdev, dev);
+ *	...
+ *	register_netdev(netdev);
+ *	...
+ * netdev_printk and its wrappers (e.g., netdev_err) can be used as
+ * soon as you have a valid net_device pointer -- e.g., from alloc_netdev,
+ * alloc_etherdev, or init_etherdev.  (Before that, use dev_printk and
+ * its wrappers to report device errors.)  It's common for an interface to
+ * have a name like "eth%d" until the device is successfully configured,
+ * and the call to register_netdev changes it to a "real" name like "eth0".
+ *
+ * If the interface's reg_state is NETREG_REGISTERED, we assume that it has
+ * been successfully set up in sysfs, and we prepend only the interface name
+ * to the message -- e.g., "eth0: NIC Link is Down".  The interface
+ * name can be used to find eth0's driver, bus ID, etc. in sysfs.
+ *
+ * For any other value of reg_state, we prepend the driver name and bus ID
+ * as well as the (possibly incomplete) interface name -- e.g.,
+ * "eth%d (e100 0000:00:03.0): Failed to map PCI address..."
+ *
+ * Probe functions that alloc and register in one step (via init_etherdev),
+ * or otherwise register the device before the probe completes successfully,
+ * may need to take other steps to ensure that the failing device is clearly
+ * identified.
+ */
+int __netdev_printk(const char *sevlevel, const struct net_device *netdev,
+	int msglevel, const char *format, ...)
+{
+	if (!netdev || !format) {
+		return -EINVAL;
+	}
+	if (msglevel == NETIF_MSG_ALL || (netdev->msg_enable & msglevel)) {
+		char msg[512];
+		va_list args;
+		struct device *dev = netdev->class_dev.dev;
+		
+		va_start(args, format);
+		vsnprintf(msg, 512, format, args);
+		va_end(args);
+
+		if (!sevlevel) {
+			sevlevel = "";
+		}
+		if (netdev->reg_state == NETREG_REGISTERED || !dev) {
+			printk("%s%s: %s", sevlevel, netdev->name, msg);
+		} else {
+			printk("%s%s (%s %s): %s", sevlevel, netdev->name,
+				dev->driver->name, dev->bus_id, msg);
+		}
+	}
+	return 0;
+}
diff -Naur linux.org/net/netsyms.c linux.netdev.patched/net/netsyms.c
--- linux.org/net/netsyms.c	Mon Aug 25 13:29:38 2003
+++ linux.netdev.patched/net/netsyms.c	Mon Aug 25 13:29:38 2003
@@ -210,6 +210,7 @@
 EXPORT_SYMBOL(net_ratelimit);
 EXPORT_SYMBOL(net_random);
 EXPORT_SYMBOL(net_srandom);
+EXPORT_SYMBOL(__netdev_printk);
 
 /* Needed by smbfs.o */
 EXPORT_SYMBOL(__scm_destroy);

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Subject: [PATCH 2/4] Net device error logging, revised (e100)
  2003-08-25 21:31 [PATCH 1/4] Net device error logging, revised Jim Keniston
@ 2003-08-25 21:39 ` Jim Keniston
  2003-08-25 21:41 ` Subject: [PATCH 3/4] Net device error logging, revised (e1000) Jim Keniston
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Jim Keniston @ 2003-08-25 21:39 UTC (permalink / raw)
  To: LKML, netdev, Jeff Garzik, Feldman, Scott, Larry Kessler,
	Greg KH, Randy Dunlap, Alan Cox, Andrew Morton, jkenisto

[-- Attachment #1: Type: text/plain, Size: 184 bytes --]

Here's a patch to modify the v2.6.0-test4 e100 driver to use netdev_*
macros and support verbosity control via the NETIF_MSG_* message levels.

Jim Keniston
IBM Linux Technology Center

[-- Attachment #2: e100-2.6.0-test4.patch --]
[-- Type: text/plain, Size: 21326 bytes --]

diff -Naur linux.org/drivers/net/e100/e100_config.c linux.e100.patched/drivers/net/e100/e100_config.c
--- linux.org/drivers/net/e100/e100_config.c	Mon Aug 25 13:29:38 2003
+++ linux.e100.patched/drivers/net/e100/e100_config.c	Mon Aug 25 13:29:38 2003
@@ -565,7 +565,8 @@
 		config_byte = CB_CFIG_LOOPBACK_EXTERNAL;
 		break;
 	default:
-		printk(KERN_NOTICE "e100: e100_config_loopback_mode: "
+		netdev_printk(KERN_NOTICE, bdp->device,,
+		       "e100_config_loopback_mode: "
 		       "Invalid argument 'mode': %d\n", mode);
 		goto exit;
 	}
diff -Naur linux.org/drivers/net/e100/e100_main.c linux.e100.patched/drivers/net/e100/e100_main.c
--- linux.org/drivers/net/e100/e100_main.c	Mon Aug 25 13:29:38 2003
+++ linux.e100.patched/drivers/net/e100/e100_main.c	Mon Aug 25 13:29:38 2003
@@ -78,6 +78,7 @@
 #include <net/checksum.h>
 #include <linux/tcp.h>
 #include <linux/udp.h>
+#include <linux/moduleparam.h>
 #include "e100.h"
 #include "e100_ucode.h"
 #include "e100_config.h"
@@ -215,12 +216,19 @@
 static void e100_dump_stats_cntrs(struct e100_private *);
 
 static void e100_check_options(int board, struct e100_private *bdp);
-static void e100_set_int_option(int *, int, int, int, int, char *);
+static void e100_set_int_option(struct e100_private *bdp, int *,
+				int, int, int, int, char *);
 static void e100_set_bool_option(struct e100_private *bdp, int, u32, int,
 				 char *);
 unsigned char e100_wait_exec_cmplx(struct e100_private *, u32, u8, u8);
 void e100_exec_cmplx(struct e100_private *, u32, u8);
 
+static int debug = -1;
+#define E100_DFLT_MSGLVL (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK)
+
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "e100 debug level: 0 (quiet) to 15 (verbose)");
+
 /**
  * e100_get_rx_struct - retrieve cell to hold skb buff from the pool
  * @bdp: atapter's private data struct
@@ -433,15 +441,12 @@
 e100_wait_exec_simple(struct e100_private *bdp, u8 scb_cmd_low)
 {
 	if (!e100_wait_scb(bdp)) {
-		printk(KERN_DEBUG "e100: %s: e100_wait_exec_simple: failed\n",
-		       bdp->device->name);
+		netdev_dbg(bdp->device,, "e100_wait_exec_simple: failed\n");
 #ifdef E100_CU_DEBUG		
-		printk(KERN_ERR "e100: %s: Last command (%x/%x) "
-			"timeout\n", bdp->device->name, 
+		netdev_err(bdp->device,, "Last command (%x/%x) timeout\n",
 			bdp->last_cmd, bdp->last_sub_cmd);
-		printk(KERN_ERR "e100: %s: Current simple command (%x) "
-			"can't be executed\n", 
-			bdp->device->name, scb_cmd_low);
+		netdev_err(bdp->device,, "Current simple command (%x) "
+			"can't be executed\n", scb_cmd_low);
 #endif		
 		return false;
 	}
@@ -466,12 +471,10 @@
 {
 	if (!e100_wait_scb(bdp)) {
 #ifdef E100_CU_DEBUG		
-		printk(KERN_ERR "e100: %s: Last command (%x/%x) "
-			"timeout\n", bdp->device->name, 
+		netdev_err(bdp->device,, "Last command (%x/%x) timeout\n",
 			bdp->last_cmd, bdp->last_sub_cmd);
-		printk(KERN_ERR "e100: %s: Current complex command "
-			"(%x/%x) can't be executed\n", 
-			bdp->device->name, cmd, sub_cmd);
+		netdev_err(bdp->device,, "Current complex command "
+			"(%x/%x) can't be executed\n", cmd, sub_cmd);
 #endif		
 		return false;
 	}
@@ -562,7 +565,7 @@
 
 	dev = alloc_etherdev(sizeof (struct e100_private));
 	if (dev == NULL) {
-		printk(KERN_ERR "e100: Not able to alloc etherdev struct\n");
+		dev_err(&pcid->dev, "Not able to alloc etherdev struct\n");
 		rc = -ENODEV;
 		goto out;
 	}
@@ -584,6 +587,15 @@
 	pci_set_drvdata(pcid, dev);
 	SET_NETDEV_DEV(dev, &pcid->dev);
 
+	if (debug < 0) {
+		dev->msg_enable = E100_DFLT_MSGLVL;
+	} else if (debug >= 8 * sizeof(int)) {
+		/* All levels enabled */
+		dev->msg_enable = -1;
+	} else {
+		dev->msg_enable = (1 << debug) - 1;
+	}
+
 	if ((rc = e100_alloc_space(bdp)) != 0) {
 		goto err_dev;
 	}
@@ -655,7 +667,7 @@
 	e100_check_options(e100nics, bdp);
 
 	if (!e100_init(bdp)) {
-		printk(KERN_ERR "e100: Failed to initialize, instance #%d\n",
+		netdev_err(dev,, "Failed to initialize, instance #%d\n",
 		       e100nics);
 		rc = -ENODEV;
 		goto err_unregister_netdev;
@@ -665,7 +677,7 @@
 	cal_checksum = e100_eeprom_calculate_chksum(bdp);
 	read_checksum = e100_eeprom_read(bdp, (bdp->eeprom_size - 1));
 	if (cal_checksum != read_checksum) {
-                printk(KERN_ERR "e100: Corrupted EEPROM on instance #%d\n",
+                netdev_err(dev,, "Corrupted EEPROM on instance #%d\n",
 		       e100nics);
                 rc = -ENODEV;
                 goto err_unregister_netdev;
@@ -675,9 +687,8 @@
 
 	e100_get_speed_duplex_caps(bdp);
 
-	printk(KERN_NOTICE
-	       "e100: %s: %s\n", 
-	       bdp->device->name, "Intel(R) PRO/100 Network Connection");
+	netdev_printk(KERN_NOTICE,
+		dev, PROBE, "Intel(R) PRO/100 Network Connection\n");
 	e100_print_brd_conf(bdp);
 
 	bdp->wolsupported = 0;
@@ -694,8 +705,6 @@
 		bdp->wolopts = WAKE_MAGIC;
 	}
 
-	printk(KERN_NOTICE "\n");
-
 	goto out;
 
 err_unregister_netdev:
@@ -839,26 +848,28 @@
 e100_check_options(int board, struct e100_private *bdp)
 {
 	if (board >= E100_MAX_NIC) {
-		printk(KERN_NOTICE 
-		       "e100: No configuration available for board #%d\n",
-		       board);
-		printk(KERN_NOTICE "e100: Using defaults for all values\n");
+		netdev_printk(KERN_NOTICE, bdp->device, PROBE,
+			"No configuration available for board #%d.  "
+			"Using defaults for all values\n", board);
 		board = E100_MAX_NIC;
 	}
 
-	e100_set_int_option(&(bdp->params.TxDescriptors), TxDescriptors[board],
+	e100_set_int_option(bdp, &(bdp->params.TxDescriptors),
+			    TxDescriptors[board],
 			    E100_MIN_TCB, E100_MAX_TCB, E100_DEFAULT_TCB,
 			    "TxDescriptor count");
 
-	e100_set_int_option(&(bdp->params.RxDescriptors), RxDescriptors[board],
+	e100_set_int_option(bdp, &(bdp->params.RxDescriptors),
+			    RxDescriptors[board],
 			    E100_MIN_RFD, E100_MAX_RFD, E100_DEFAULT_RFD,
 			    "RxDescriptor count");
 
-	e100_set_int_option(&(bdp->params.e100_speed_duplex),
+	e100_set_int_option(bdp, &(bdp->params.e100_speed_duplex),
 			    e100_speed_duplex[board], 0, 4,
 			    E100_DEFAULT_SPEED_DUPLEX, "speed/duplex mode");
 
-	e100_set_int_option(&(bdp->params.ber), ber[board], 0, ZLOCK_MAX_ERRORS,
+	e100_set_int_option(bdp, &(bdp->params.ber), ber[board], 0,
+			    ZLOCK_MAX_ERRORS,
 			    E100_DEFAULT_BER, "Bit Error Rate count");
 
 	e100_set_bool_option(bdp, XsumRX[board], PRM_XSUMRX, E100_DEFAULT_XSUM,
@@ -883,11 +894,11 @@
 			     E100_DEFAULT_BUNDLE_SMALL_FR,
 			     "CPU saver bundle small frames value");
 
-	e100_set_int_option(&(bdp->params.IntDelay), IntDelay[board], 0x0,
+	e100_set_int_option(bdp, &(bdp->params.IntDelay), IntDelay[board], 0x0,
 			    0xFFFF, E100_DEFAULT_CPUSAVER_INTERRUPT_DELAY,
 			    "CPU saver interrupt delay value");
 
-	e100_set_int_option(&(bdp->params.BundleMax), BundleMax[board], 0x1,
+	e100_set_int_option(bdp, &(bdp->params.BundleMax), BundleMax[board], 0x1,
 			    0xFFFF, E100_DEFAULT_CPUSAVER_BUNDLE_MAX,
 			    "CPU saver bundle max value");
 
@@ -895,6 +906,7 @@
 
 /**
  * e100_set_int_option - check and set an integer option
+ * @bdp: adapter's private data struct
  * @option: a pointer to the relevant option field
  * @val: the value specified
  * @min: the minimum valid value
@@ -907,22 +919,22 @@
  * Otherwise, if the value is invalid, change it to the default.
  */
 void __devinit
-e100_set_int_option(int *option, int val, int min, int max, int default_val,
-		    char *name)
+e100_set_int_option(struct e100_private *bdp, int *option, int val,
+	int min, int max, int default_val, char *name)
 {
 	if (val == -1) {	/* no value specified. use default */
 		*option = default_val;
 
 	} else if ((val < min) || (val > max)) {
-		printk(KERN_NOTICE
-		       "e100: Invalid %s specified (%i). "
-		       "Valid range is %i-%i\n",
-		       name, val, min, max);
-		printk(KERN_NOTICE "e100: Using default %s of %i\n", name,
-		       default_val);
+		netdev_printk(KERN_NOTICE, bdp->device, PROBE,
+		       "Invalid %s specified (%i). "
+		       "Valid range is %i-%i. "
+		       "Using default %s of %i\n",
+		       name, val, min, max, name, default_val);
 		*option = default_val;
 	} else {
-		printk(KERN_INFO "e100: Using specified %s of %i\n", name, val);
+		netdev_info(bdp->device, PROBE,
+		       "Using specified %s of %i\n", name, val);
 		*option = val;
 	}
 }
@@ -949,17 +961,17 @@
 			bdp->params.b_params |= mask;
 
 	} else if ((val != true) && (val != false)) {
-		printk(KERN_NOTICE
-		       "e100: Invalid %s specified (%i). "
-		       "Valid values are %i/%i\n",
-		       name, val, false, true);
-		printk(KERN_NOTICE "e100: Using default %s of %i\n", name,
-		       default_val);
+		netdev_printk(KERN_NOTICE, bdp->device, PROBE,
+		       "Invalid %s specified (%i). "
+		       "Valid values are %i/%i. "
+		       "Using default %s of %i\n",
+		       name, val, false, true, name, default_val);
 
 		if (default_val)
 			bdp->params.b_params |= mask;
 	} else {
-		printk(KERN_INFO "e100: Using specified %s of %i\n", name, val);
+		netdev_info(bdp->device, PROBE,
+			"Using specified %s of %i\n", name, val);
 		if (val)
 			bdp->params.b_params |= mask;
 	}
@@ -1181,8 +1193,7 @@
 	}
 
 	if (!e100_exec_non_cu_cmd(bdp, cmd)) {
-		printk(KERN_WARNING "e100: %s: Multicast setup failed\n", 
-		       dev->name);
+		netdev_warn(dev,, "Multicast setup failed\n");
 	}
 }
 
@@ -1262,20 +1273,20 @@
 
 	if (!e100_selftest(bdp, &st_timeout, &st_result)) {
         	if (st_timeout) {
-			printk(KERN_ERR "e100: selftest timeout\n");
+			netdev_err(bdp->device,, "selftest timeout\n");
 		} else {
-			printk(KERN_ERR "e100: selftest failed. Results: %x\n",
-					st_result);
+			netdev_err(bdp->device,,
+				"selftest failed. Results: %x\n", st_result);
 		}
 		return false;
 	}
 	else
-		printk(KERN_DEBUG "e100: selftest OK.\n");
+		netdev_dbg(bdp->device,, "selftest OK.\n");
 
 	/* read the MAC address from the eprom */
 	e100_rd_eaddr(bdp);
 	if (!is_valid_ether_addr(bdp->device->dev_addr)) {
-		printk(KERN_ERR "e100: Invalid Ethernet address\n");
+		netdev_err(bdp->device,, "Invalid Ethernet address\n");
 		return false;
 	}
 	/* read NIC's part number */
@@ -1426,7 +1437,7 @@
 
 	return true;
 err:
-	printk(KERN_ERR "e100: hw init failed\n");
+	netdev_err(bdp->device,, "hw init failed\n");
 	return false;
 }
 
@@ -1555,8 +1566,7 @@
 	return 0;
 
 err:
-	printk(KERN_ERR
-	       "e100: Failed to allocate memory\n");
+	netdev_err(bdp->device,, "Failed to allocate memory\n");
 	return -ENOMEM;
 }
 
@@ -1697,8 +1707,7 @@
 
 #ifdef E100_CU_DEBUG
 	if (e100_cu_unknown_state(bdp)) {
-		printk(KERN_ERR "e100: %s: CU unknown state in e100_watchdog\n",
-			dev->name);
+		netdev_err(dev,, "CU unknown state in e100_watchdog\n");
 	}
 #endif	
 	if (!netif_running(dev)) {
@@ -1708,9 +1717,9 @@
 	/* check if link state has changed */
 	if (e100_phy_check(bdp)) {
 		if (netif_carrier_ok(dev)) {
-			printk(KERN_ERR
-			       "e100: %s NIC Link is Up %d Mbps %s duplex\n",
-			       bdp->device->name, bdp->cur_line_speed,
+			netdev_err(dev, LINK,
+			       "NIC Link is Up %d Mbps %s duplex\n",
+			       bdp->cur_line_speed,
 			       (bdp->cur_dplx_mode == HALF_DUPLEX) ?
 			       "Half" : "Full");
 
@@ -1718,8 +1727,7 @@
 			e100_config(bdp);  
 
 		} else {
-			printk(KERN_ERR "e100: %s NIC Link is Down\n",
-			       bdp->device->name);
+			netdev_err(dev, LINK, "NIC Link is Down\n");
 		}
 	}
 
@@ -2289,14 +2297,12 @@
 	case START_WAIT:
 		// The last command was a non_tx CU command
 		if (!e100_wait_cus_idle(bdp))
-			printk(KERN_DEBUG
-			       "e100: %s: cu_start: timeout waiting for cu\n",
-			       bdp->device->name);
+			netdev_dbg(bdp->device,,
+			       "cu_start: timeout waiting for cu\n");
 		if (!e100_wait_exec_cmplx(bdp, (u32) (tcb->tcb_phys),
 					  SCB_CUC_START, CB_TRANSMIT)) {
-			printk(KERN_DEBUG
-			       "e100: %s: cu_start: timeout waiting for scb\n",
-			       bdp->device->name);
+			netdev_dbg(bdp->device,,
+			       "cu_start: timeout waiting for scb\n");
 			e100_exec_cmplx(bdp, (u32) (tcb->tcb_phys),
 					SCB_CUC_START);
 			ret = false;
@@ -2414,8 +2420,7 @@
 
 	res = e100_exec_non_cu_cmd(bdp, cmd);
 	if (!res)
-		printk(KERN_WARNING "e100: %s: IA setup failed\n", 
-		       bdp->device->name);
+		netdev_warn(bdp->device,, "IA setup failed\n");
 
 exit:
 	return res;
@@ -2460,9 +2465,7 @@
 	spin_lock(&bdp->bd_lock);
 
 	if (!e100_wait_exec_cmplx(bdp, rx_struct->dma_addr, SCB_RUC_START, 0)) {
-		printk(KERN_DEBUG
-		       "e100: %s: start_ru: wait_scb failed\n", 
-		       bdp->device->name);
+		netdev_dbg(bdp->device,, "start_ru: wait_scb failed\n");
 		e100_exec_cmplx(bdp, rx_struct->dma_addr, SCB_RUC_START);
 	}
 	if (bdp->next_cu_cmd == RESUME_NO_WAIT) {
@@ -2707,8 +2710,8 @@
 			spin_lock_bh(&(bdp->bd_non_tx_lock));
 		} else {
 #ifdef E100_CU_DEBUG			
-			printk(KERN_ERR "e100: %s: non-TX command (%x) "
-				"timeout\n", bdp->device->name, sub_cmd);
+			netdev_err(bdp->device,, "non-TX command (%x) "
+				"timeout\n", sub_cmd);
 #endif			
 			rc = false;
 			goto exit;
@@ -2961,14 +2964,17 @@
 {
 	/* Print the string if checksum Offloading was enabled */
 	if (bdp->flags & DF_CSUM_OFFLOAD)
-		printk(KERN_NOTICE "  Hardware receive checksums enabled\n");
+		netdev_printk(KERN_NOTICE, bdp->device, PROBE,
+			"  Hardware receive checksums enabled\n");
 	else {
 		if (bdp->rev_id >= D101MA_REV_ID) 
-			printk(KERN_NOTICE "  Hardware receive checksums disabled\n");
+			netdev_printk(KERN_NOTICE, bdp->device, PROBE,
+				"  Hardware receive checksums disabled\n");
 	}
 
 	if ((bdp->flags & DF_UCODE_LOADED))
-		printk(KERN_NOTICE "  cpu cycle saver enabled\n");
+		netdev_printk(KERN_NOTICE, bdp->device, PROBE,
+			"  cpu cycle saver enabled\n");
 }
 
 /**
@@ -3017,8 +3023,8 @@
 	bdp->scb = (scb_t *) ioremap_nocache(dev->mem_start, sizeof (scb_t));
 
 	if (!bdp->scb) {
-		printk(KERN_ERR "e100: %s: Failed to map PCI address 0x%lX\n",
-		       dev->name, pci_resource_start(pcid, 0));
+		netdev_err(dev,, "Failed to map PCI address 0x%lX\n",
+		       pci_resource_start(pcid, 0));
 		rc = -ENOMEM;
 		goto err_region;
 	}
@@ -3092,7 +3098,7 @@
 		return false;
 
 	if (!e100_setup_iaaddr(bdp, bdp->device->dev_addr)) {
-		printk(KERN_ERR "e100: e100_configure_device: "
+		netdev_err(bdp->device,, "e100_configure_device: "
 			"setup iaaddr failed\n");
 		return false;
 	}
@@ -3122,7 +3128,7 @@
 	e100_sw_reset(bdp, cmd);
 	if (cmd == PORT_SOFTWARE_RESET) {
 		if (!e100_configure_device(bdp))
-			printk(KERN_ERR "e100: e100_deisolate_driver:" 
+			netdev_err(bdp->device,, "e100_deisolate_driver:"
 		       		" device configuration failed\n");
 	} 
 
@@ -3206,6 +3212,20 @@
 	case ETHTOOL_PHYS_ID:
 		rc = e100_ethtool_led_blink(dev,ifr);
 		break;
+	case ETHTOOL_GMSGLVL: {
+		struct ethtool_value eval = {ETHTOOL_GMSGLVL};
+		eval.data = dev->msg_enable;
+		if (copy_to_user(ifr->ifr_data, &eval, sizeof(eval)))
+			return -EFAULT;
+		return 0;
+	}
+	case ETHTOOL_SMSGLVL: {
+		struct ethtool_value eval;
+		if (copy_from_user(&eval, ifr->ifr_data, sizeof(eval)))
+			return -EFAULT;
+		dev->msg_enable = eval.data;
+		return 0;
+	}
 #ifdef	ETHTOOL_GRINGPARAM
 	case ETHTOOL_GRINGPARAM: {
 		struct ethtool_ringparam ering;
@@ -3843,8 +3863,7 @@
 
 	res = e100_exec_non_cu_cmd(bdp, cmd);
 	if (!res)
-		printk(KERN_WARNING "e100: %s: Filter setup failed\n",
-		       bdp->device->name);
+		netdev_warn(bdp->device,, "Filter setup failed\n");
 
 exit:
 	return res;
@@ -3859,10 +3878,10 @@
 	if (e100_config(bdp)) {
 		if (bdp->wolopts & (WAKE_UCAST | WAKE_ARP))
 			if (!e100_setup_filter(bdp))
-				printk(KERN_ERR
-				       "e100: WOL options failed\n");
+				netdev_err(bdp->device,,
+				       "WOL options failed\n");
 	} else {
-		printk(KERN_ERR "e100: config WOL failed\n");
+		netdev_err(bdp->device,, "config WOL failed\n");
 	}
 }
 #endif
@@ -4159,9 +4178,9 @@
 #ifdef E100_CU_DEBUG			
 			if (!(non_tx_cmd->cb_status 
 			    & __constant_cpu_to_le16(CB_STATUS_COMPLETE)))
-				printk(KERN_ERR "e100: %s: Queued "
+				netdev_err(bdp->device,, "Queued "
 					"command (%x) timeout\n", 
-					bdp->device->name, sub_cmd);
+					sub_cmd);
 #endif			
 			list_del(&(active_command->list_elem));
 			e100_free_non_tx_cmd(bdp, active_command);
diff -Naur linux.org/drivers/net/e100/e100_phy.c linux.e100.patched/drivers/net/e100/e100_phy.c
--- linux.org/drivers/net/e100/e100_phy.c	Mon Aug 25 13:29:38 2003
+++ linux.e100.patched/drivers/net/e100/e100_phy.c	Mon Aug 25 13:29:38 2003
@@ -72,7 +72,7 @@
 	if (mdi_cntrl & MDI_PHY_READY) 
 		return 0;
 	else {
-		printk(KERN_ERR "e100: MDI write timeout\n");
+		netdev_err(bdp->device,, "MDI write timeout\n");
 		return 1;
 	}
 }
@@ -127,7 +127,7 @@
 		return 0;
 	}
 	else {
-		printk(KERN_ERR "e100: MDI read timeout\n");
+		netdev_err(bdp->device,, "MDI read timeout\n");
 		return 1;
 	}
 }
@@ -236,22 +236,20 @@
 		switch (bdp->params.e100_speed_duplex) {
 		case E100_AUTONEG:
 			/* The adapter can't autoneg. so set to 10/HALF */
-			printk(KERN_INFO
-			       "e100: 503 serial component detected which "
-			       "cannot autonegotiate\n");
-			printk(KERN_INFO
-			       "e100: speed/duplex forced to "
+			netdev_info(bdp->device, PROBE,
+			       "503 serial component detected which "
+			       "cannot autonegotiate. "
+			       "speed/duplex forced to "
 			       "10Mbps / Half duplex\n");
 			bdp->params.e100_speed_duplex = E100_SPEED_10_HALF;
 			break;
 
 		case E100_SPEED_100_HALF:
 		case E100_SPEED_100_FULL:
-			printk(KERN_ERR
-			       "e100: 503 serial component detected "
-			       "which does not support 100Mbps\n");
-			printk(KERN_ERR
-			       "e100: Change the forced speed/duplex "
+			netdev_err(bdp->device,,
+			       "503 serial component detected "
+			       "which does not support 100Mbps. "
+			       "Change the forced speed/duplex "
 			       "to a supported setting\n");
 			return false;
 		}
@@ -269,7 +267,7 @@
 		if ((bdp->params.e100_speed_duplex != E100_AUTONEG) &&
 		    (bdp->params.e100_speed_duplex != E100_SPEED_100_FULL)) {
 			/* just inform user about 100 full */
-			printk(KERN_ERR "e100: NC3133 NIC can only run "
+			netdev_err(bdp->device,, "NC3133 NIC can only run "
 			       "at 100Mbps full duplex\n");
 		}
 
diff -Naur linux.org/drivers/net/e100/e100_test.c linux.e100.patched/drivers/net/e100/e100_test.c
--- linux.org/drivers/net/e100/e100_test.c	Mon Aug 25 13:29:38 2003
+++ linux.e100.patched/drivers/net/e100/e100_test.c	Mon Aug 25 13:29:38 2003
@@ -44,7 +44,7 @@
 static void e100_diag_config_loopback(struct e100_private *, u8, u8, u8 *,u8 *);
 static u8 e100_diag_loopback_alloc(struct e100_private *);
 static void e100_diag_loopback_cu_ru_exec(struct e100_private *);
-static u8 e100_diag_check_pkt(u8 *);
+static u8 e100_diag_check_pkt(struct e100_private *bdp, u8 *);
 static void e100_diag_loopback_free(struct e100_private *);
 static int e100_cable_diag(struct e100_private *bdp);
 
@@ -169,19 +169,19 @@
 {
 	u8 rc = 0;
 
-	printk(KERN_DEBUG "%s: PHY loopback test starts\n", dev->name);
+	netdev_printk(KERN_DEBUG, dev,, "PHY loopback test starts\n");
 	e100_hw_init(dev->priv);
 	if (!e100_diag_one_loopback(dev, PHY_LOOPBACK)) {
 		rc |= PHY_LOOPBACK;
 	}
-	printk(KERN_DEBUG "%s: PHY loopback test ends\n", dev->name);
+	netdev_printk(KERN_DEBUG, dev,, "PHY loopback test ends\n");
 
-	printk(KERN_DEBUG "%s: MAC loopback test starts\n", dev->name);
+	netdev_printk(KERN_DEBUG, dev,, "MAC loopback test starts\n");
 	e100_hw_init(dev->priv);
 	if (!e100_diag_one_loopback(dev, MAC_LOOPBACK)) {
 		rc |= MAC_LOOPBACK;
 	}
-	printk(KERN_DEBUG "%s: MAC loopback test ends\n", dev->name);
+	netdev_printk(KERN_DEBUG, dev,, "MAC loopback test ends\n");
 
 	return rc;
 }
@@ -349,7 +349,7 @@
 {
 	/*load CU & RU base */ 
 	if(!e100_wait_exec_cmplx(bdp, bdp->loopback.dma_handle, SCB_RUC_START, 0))
-		printk(KERN_ERR "e100: SCB_RUC_START failed!\n");
+		netdev_err(bdp->device,, "SCB_RUC_START failed!\n");
 
 	bdp->next_cu_cmd = START_WAIT;
 	e100_start_cu(bdp, bdp->loopback.tcb);
@@ -359,20 +359,23 @@
 /**
  * e100_diag_check_pkt - checks if a given packet is a loopback packet
  * @bdp: atapter's private data struct
+ * @datap: pointer to packet data
  *
  * Returns true if OK false otherwise.
  */
 static u8
-e100_diag_check_pkt(u8 *datap)
+e100_diag_check_pkt(struct e100_private *bdp, u8 *datap)
 {
 	int i;
 	for (i = 0; i<512; i++) {
 		if( !((*datap)==0xFF && (*(datap + 512) == 0xBA)) ) {
-			printk (KERN_ERR "e100: check loopback packet failed at: %x\n", i);
+			netdev_err(bdp->device,,
+				"check loopback packet failed at: %x\n", i);
 			return false;
 			}
 	}
-	printk (KERN_DEBUG "e100: Check received loopback packet OK\n");
+	netdev_printk(KERN_DEBUG, bdp->device,,
+		"Check received loopback packet OK\n");
 	return true;
 }
 
@@ -404,11 +407,13 @@
         }
 
         if (rfd_status & RFD_STATUS_COMPLETE) {
-		printk(KERN_DEBUG "e100: Loopback packet received\n");
-                return e100_diag_check_pkt(((u8 *)rfdp+bdp->rfd_size));
+		netdev_printk(KERN_DEBUG, bdp->device,,
+			"Loopback packet received\n");
+                return e100_diag_check_pkt(bdp,
+			((u8 *)rfdp+bdp->rfd_size));
 	}
 	else {
-		printk(KERN_ERR "e100: Loopback packet not received\n");
+		netdev_err(bdp->device,, "Loopback packet not received\n");
 		return false;
 	}
 }

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Subject: [PATCH 3/4] Net device error logging, revised (e1000)
  2003-08-25 21:31 [PATCH 1/4] Net device error logging, revised Jim Keniston
  2003-08-25 21:39 ` Subject: [PATCH 2/4] Net device error logging, revised (e100) Jim Keniston
@ 2003-08-25 21:41 ` Jim Keniston
  2003-08-25 21:43 ` [PATCH 4/4] Net device error logging, revised (tg3) Jim Keniston
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Jim Keniston @ 2003-08-25 21:41 UTC (permalink / raw)
  To: LKML, netdev, Jeff Garzik, Feldman, Scott, Larry Kessler,
	Greg KH, Randy Dunlap, Alan Cox, Andrew Morton, jkenisto

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]

Here's a patch to modify the v2.6.0-test4 e1000 driver to use netdev_*
macros and support verbosity control via the NETIF_MSG_* message levels.

Jim Keniston
IBM Linux Technology Center

[-- Attachment #2: e1000-2.6.0-test4.patch --]
[-- Type: text/plain, Size: 16831 bytes --]

diff -Naur linux.org/drivers/net/e1000/e1000.h linux.e1000.patched/drivers/net/e1000/e1000.h
--- linux.org/drivers/net/e1000/e1000.h	Mon Aug 25 13:29:38 2003
+++ linux.e1000.patched/drivers/net/e1000/e1000.h	Mon Aug 25 13:29:38 2003
@@ -35,6 +35,7 @@
 #include <linux/stddef.h>
 #include <linux/config.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/types.h>
 #include <asm/byteorder.h>
 #include <linux/init.h>
@@ -81,14 +82,6 @@
 struct e1000_adapter;
 
 #include "e1000_hw.h"
-
-#if DBG
-#define E1000_DBG(args...) printk(KERN_DEBUG "e1000: " args)
-#else
-#define E1000_DBG(args...)
-#endif
-
-#define E1000_ERR(args...) printk(KERN_ERR "e1000: " args)
 
 #define E1000_MAX_INTR 10
 
diff -Naur linux.org/drivers/net/e1000/e1000_ethtool.c linux.e1000.patched/drivers/net/e1000/e1000_ethtool.c
--- linux.org/drivers/net/e1000/e1000_ethtool.c	Mon Aug 25 13:29:38 2003
+++ linux.e1000.patched/drivers/net/e1000/e1000_ethtool.c	Mon Aug 25 13:29:38 2003
@@ -1576,6 +1576,20 @@
 		return 0;
 	}
 #endif
+	case ETHTOOL_GMSGLVL: {
+		struct ethtool_value edata = {ETHTOOL_GMSGLVL};
+		edata.data = netdev->msg_enable;
+		if (copy_to_user(addr, &edata, sizeof(edata)))
+			return -EFAULT;
+		return 0;
+	}
+	case ETHTOOL_SMSGLVL: {
+		struct ethtool_value edata;
+		if (copy_from_user(&edata, addr, sizeof(edata)))
+			return -EFAULT;
+		netdev->msg_enable = edata.data;
+		return 0;
+	}
 	default:
 		return -EOPNOTSUPP;
 	}
diff -Naur linux.org/drivers/net/e1000/e1000_main.c linux.e1000.patched/drivers/net/e1000/e1000_main.c
--- linux.org/drivers/net/e1000/e1000_main.c	Mon Aug 25 13:29:38 2003
+++ linux.e1000.patched/drivers/net/e1000/e1000_main.c	Mon Aug 25 13:29:38 2003
@@ -178,9 +178,14 @@
 #endif
 };
 
+static int debug = -1;
+#define E1000_DFLT_MSGLVL (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK)
+
 MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
 MODULE_DESCRIPTION("Intel(R) PRO/1000 Network Driver");
 MODULE_LICENSE("GPL");
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "e1000 debug level: 0 (quiet) to 15 (verbose)");
 
 /**
  * e1000_init_module - Driver Registration Routine
@@ -337,7 +342,8 @@
 		pci_using_dac = 1;
 	} else {
 		if((i = pci_set_dma_mask(pdev, PCI_DMA_32BIT))) {
-			E1000_ERR("No usable DMA configuration, aborting\n");
+			dev_err(&pdev->dev,
+			      "No usable DMA configuration, aborting\n");
 			return i;
 		}
 		pci_using_dac = 0;
@@ -361,6 +367,15 @@
 	adapter->pdev = pdev;
 	adapter->hw.back = adapter;
 
+	if (debug < 0) {
+		netdev->msg_enable = E1000_DFLT_MSGLVL;
+	} else if (debug >= 8 * sizeof(int)) {
+		/* All levels enabled */
+		netdev->msg_enable = -1;
+	} else {
+		netdev->msg_enable = (1 << debug) - 1;
+	}
+
 	mmio_start = pci_resource_start(pdev, BAR_0);
 	mmio_len = pci_resource_len(pdev, BAR_0);
 
@@ -429,7 +444,7 @@
 	/* make sure the EEPROM is good */
 
 	if(e1000_validate_eeprom_checksum(&adapter->hw) < 0) {
-		printk(KERN_ERR "The EEPROM Checksum Is Not Valid\n");
+		netdev_err(netdev,, "The EEPROM Checksum Is Not Valid\n");
 		goto err_eeprom;
 	}
 
@@ -467,8 +482,7 @@
 	netif_carrier_off(netdev);
 	netif_stop_queue(netdev);
 
-	printk(KERN_INFO "%s: Intel(R) PRO/1000 Network Connection\n",
-	       netdev->name);
+	netdev_info(netdev, PROBE, "Intel(R) PRO/1000 Network Connection\n");
 	e1000_check_options(adapter);
 
 	/* Initial Wake on LAN setting
@@ -569,7 +583,7 @@
 	/* identify the MAC */
 
 	if (e1000_set_mac_type(hw)) {
-		E1000_ERR("Unknown MAC Type\n");
+		netdev_err(netdev,, "Unknown MAC Type\n");
 		return -1;
 	}
 
@@ -1329,9 +1343,8 @@
 			                           &adapter->link_speed,
 			                           &adapter->link_duplex);
 
-			printk(KERN_INFO
-			       "e1000: %s NIC Link is Up %d Mbps %s\n",
-			       netdev->name, adapter->link_speed,
+			netdev_info(netdev, LINK, "NIC Link is Up %d Mbps %s\n",
+			       adapter->link_speed,
 			       adapter->link_duplex == FULL_DUPLEX ?
 			       "Full Duplex" : "Half Duplex");
 
@@ -1344,9 +1357,7 @@
 		if(netif_carrier_ok(netdev)) {
 			adapter->link_speed = 0;
 			adapter->link_duplex = 0;
-			printk(KERN_INFO
-			       "e1000: %s NIC Link is Down\n",
-			       netdev->name);
+			netdev_info(netdev, LINK, "NIC Link is Down\n");
 			netif_carrier_off(netdev);
 			netif_stop_queue(netdev);
 			mod_timer(&adapter->phy_info_timer, jiffies + 2 * HZ);
@@ -1758,7 +1769,7 @@
 
 	if((max_frame < MINIMUM_ETHERNET_FRAME_SIZE) ||
 	   (max_frame > MAX_JUMBO_FRAME_SIZE)) {
-		E1000_ERR("Invalid MTU setting\n");
+		netdev_err(netdev,, "Invalid MTU setting\n");
 		return -EINVAL;
 	}
 
@@ -1766,7 +1777,7 @@
 		adapter->rx_buffer_len = E1000_RXBUFFER_2048;
 
 	} else if(adapter->hw.mac_type < e1000_82543) {
-		E1000_ERR("Jumbo Frames not supported on 82542\n");
+		netdev_err(netdev,, "Jumbo Frames not supported on 82542\n");
 		return -EINVAL;
 
 	} else if(max_frame <= E1000_RXBUFFER_4096) {
@@ -2147,7 +2158,8 @@
 
 			/* All receives must fit into a single buffer */
 
-			E1000_DBG("Receive packet consumed multiple buffers\n");
+			netdev_dbg(netdev, RX_ERR,
+			         "Receive packet consumed multiple buffers\n");
 
 			dev_kfree_skb_irq(skb);
 			rx_desc->status = 0;
diff -Naur linux.org/drivers/net/e1000/e1000_param.c linux.e1000.patched/drivers/net/e1000/e1000_param.c
--- linux.org/drivers/net/e1000/e1000_param.c	Mon Aug 25 13:29:38 2003
+++ linux.e1000.patched/drivers/net/e1000/e1000_param.c	Mon Aug 25 13:29:38 2003
@@ -244,7 +244,8 @@
 };
 
 static int __devinit
-e1000_validate_option(int *value, struct e1000_option *opt)
+e1000_validate_option(int *value, struct e1000_option *opt,
+	struct net_device *netdev)
 {
 	if(*value == OPTION_UNSET) {
 		*value = opt->def;
@@ -255,16 +256,17 @@
 	case enable_option:
 		switch (*value) {
 		case OPTION_ENABLED:
-			printk(KERN_INFO "%s Enabled\n", opt->name);
+			netdev_info(netdev, PROBE, "%s Enabled\n", opt->name);
 			return 0;
 		case OPTION_DISABLED:
-			printk(KERN_INFO "%s Disabled\n", opt->name);
+			netdev_info(netdev, PROBE, "%s Disabled\n", opt->name);
 			return 0;
 		}
 		break;
 	case range_option:
 		if(*value >= opt->arg.r.min && *value <= opt->arg.r.max) {
-			printk(KERN_INFO "%s set to %i\n", opt->name, *value);
+			netdev_info(netdev, PROBE,
+			            "%s set to %i\n", opt->name, *value);
 			return 0;
 		}
 		break;
@@ -276,7 +278,8 @@
 			ent = &opt->arg.l.p[i];
 			if(*value == ent->i) {
 				if(ent->str[0] != '\0')
-					printk(KERN_INFO "%s\n", ent->str);
+					netdev_info(netdev, PROBE, "%s\n",
+						ent->str);
 				return 0;
 			}
 		}
@@ -286,8 +289,8 @@
 		BUG();
 	}
 
-	printk(KERN_INFO "Invalid %s specified (%i) %s\n",
-	       opt->name, *value, opt->err);
+	netdev_info(netdev,, "Invalid %s specified (%i) %s\n",
+	            opt->name, *value, opt->err);
 	*value = opt->def;
 	return -1;
 }
@@ -308,11 +311,12 @@
 void __devinit
 e1000_check_options(struct e1000_adapter *adapter)
 {
+	struct net_device *netdev = adapter->netdev;
 	int bd = adapter->bd_number;
 	if(bd >= E1000_MAX_NIC) {
-		printk(KERN_NOTICE
-		       "Warning: no configuration for board #%i\n", bd);
-		printk(KERN_NOTICE "Using defaults for all values\n");
+		netdev_warn(netdev, PROBE,
+			"Warning: no configuration for board #%i.  "
+			"Using defaults for all values\n", bd);
 		bd = E1000_MAX_NIC;
 	}
 
@@ -330,7 +334,7 @@
 			MAX_TXD : MAX_82544_TXD;
 
 		tx_ring->count = TxDescriptors[bd];
-		e1000_validate_option(&tx_ring->count, &opt);
+		e1000_validate_option(&tx_ring->count, &opt, netdev);
 		E1000_ROUNDUP(tx_ring->count, REQ_TX_DESCRIPTOR_MULTIPLE);
 	}
 	{ /* Receive Descriptor Count */
@@ -346,7 +350,7 @@
 		opt.arg.r.max = mac_type < e1000_82544 ? MAX_RXD : MAX_82544_RXD;
 
 		rx_ring->count = RxDescriptors[bd];
-		e1000_validate_option(&rx_ring->count, &opt);
+		e1000_validate_option(&rx_ring->count, &opt, netdev);
 		E1000_ROUNDUP(rx_ring->count, REQ_RX_DESCRIPTOR_MULTIPLE);
 	}
 	{ /* Checksum Offload Enable/Disable */
@@ -358,7 +362,7 @@
 		};
 
 		int rx_csum = XsumRX[bd];
-		e1000_validate_option(&rx_csum, &opt);
+		e1000_validate_option(&rx_csum, &opt, netdev);
 		adapter->rx_csum = rx_csum;
 	}
 	{ /* Flow Control */
@@ -380,7 +384,7 @@
 		};
 
 		int fc = FlowControl[bd];
-		e1000_validate_option(&fc, &opt);
+		e1000_validate_option(&fc, &opt, netdev);
 		adapter->hw.fc = adapter->hw.original_fc = fc;
 	}
 	{ /* Transmit Interrupt Delay */
@@ -394,7 +398,7 @@
 		};
 
 		adapter->tx_int_delay = TxIntDelay[bd];
-		e1000_validate_option(&adapter->tx_int_delay, &opt);
+		e1000_validate_option(&adapter->tx_int_delay, &opt, netdev);
 	}
 	{ /* Transmit Absolute Interrupt Delay */
 		struct e1000_option opt = {
@@ -407,7 +411,7 @@
 		};
 
 		adapter->tx_abs_int_delay = TxAbsIntDelay[bd];
-		e1000_validate_option(&adapter->tx_abs_int_delay, &opt);
+		e1000_validate_option(&adapter->tx_abs_int_delay, &opt, netdev);
 	}
 	{ /* Receive Interrupt Delay */
 		struct e1000_option opt = {
@@ -420,7 +424,7 @@
 		};
 
 		adapter->rx_int_delay = RxIntDelay[bd];
-		e1000_validate_option(&adapter->rx_int_delay, &opt);
+		e1000_validate_option(&adapter->rx_int_delay, &opt, netdev);
 	}
 	{ /* Receive Absolute Interrupt Delay */
 		struct e1000_option opt = {
@@ -433,7 +437,7 @@
 		};
 
 		adapter->rx_abs_int_delay = RxAbsIntDelay[bd];
-		e1000_validate_option(&adapter->rx_abs_int_delay, &opt);
+		e1000_validate_option(&adapter->rx_abs_int_delay, &opt, netdev);
 	}
 	{ /* Interrupt Throttling Rate */
 		struct e1000_option opt = {
@@ -447,12 +451,12 @@
 
 		adapter->itr = InterruptThrottleRate[bd];
 		if(adapter->itr == 0) {
-			printk(KERN_INFO "%s turned off\n", opt.name);
+			netdev_info(netdev, PROBE, "%s turned off\n", opt.name);
 		} else if(adapter->itr == 1 || adapter->itr == -1) {
 			/* Dynamic mode */
 			adapter->itr = 1;
 		} else {
-			e1000_validate_option(&adapter->itr, &opt);
+			e1000_validate_option(&adapter->itr, &opt, netdev);
 		}
 	}
 
@@ -478,20 +482,24 @@
 static void __devinit
 e1000_check_fiber_options(struct e1000_adapter *adapter)
 {
+	struct net_device *netdev = adapter->netdev;
 	int bd = adapter->bd_number;
 	bd = bd > E1000_MAX_NIC ? E1000_MAX_NIC : bd;
 
 	if((Speed[bd] != OPTION_UNSET)) {
-		printk(KERN_INFO "Speed not valid for fiber adapters, "
-		       "parameter ignored\n");
+		netdev_info(netdev, PROBE,
+		            "Speed not valid for fiber adapters, "
+		            "parameter ignored\n");
 	}
 	if((Duplex[bd] != OPTION_UNSET)) {
-		printk(KERN_INFO "Duplex not valid for fiber adapters, "
-		       "parameter ignored\n");
+		netdev_info(netdev, PROBE,
+		            "Duplex not valid for fiber adapters, "
+		            "parameter ignored\n");
 	}
 	if((AutoNeg[bd] != OPTION_UNSET)) {
-		printk(KERN_INFO "AutoNeg not valid for fiber adapters, "
-		       "parameter ignored\n");
+		netdev_info(netdev, PROBE,
+		            "AutoNeg not valid for fiber adapters, "
+		            "parameter ignored\n");
 	}
 }
 
@@ -505,6 +513,7 @@
 static void __devinit
 e1000_check_copper_options(struct e1000_adapter *adapter)
 {
+	struct net_device *netdev = adapter->netdev;
 	int speed, dplx;
 	int bd = adapter->bd_number;
 	bd = bd > E1000_MAX_NIC ? E1000_MAX_NIC : bd;
@@ -525,7 +534,7 @@
 		};
 
 		speed = Speed[bd];
-		e1000_validate_option(&speed, &opt);
+		e1000_validate_option(&speed, &opt, netdev);
 	}
 	{ /* Duplex */
 		struct e1000_opt_list dplx_list[] = {{           0, "" },
@@ -542,13 +551,13 @@
 		};
 
 		dplx = Duplex[bd];
-		e1000_validate_option(&dplx, &opt);
+		e1000_validate_option(&dplx, &opt, netdev);
 	}
 
 	if(AutoNeg[bd] != OPTION_UNSET && (speed != 0 || dplx != 0)) {
-		printk(KERN_INFO
-		       "AutoNeg specified along with Speed or Duplex, "
-		       "parameter ignored\n");
+		netdev_info(netdev, PROBE,
+		            "AutoNeg specified along with Speed or Duplex, "
+		            "parameter ignored\n");
 		adapter->hw.autoneg_advertised = AUTONEG_ADV_DEFAULT;
 	} else { /* Autoneg */
 		struct e1000_opt_list an_list[] =
@@ -595,7 +604,7 @@
 		};
 
 		int an = AutoNeg[bd];
-		e1000_validate_option(&an, &opt);
+		e1000_validate_option(&an, &opt, netdev);
 		adapter->hw.autoneg_advertised = an;
 	}
 
@@ -603,78 +612,82 @@
 	case 0:
 		adapter->hw.autoneg = 1;
 		if(Speed[bd] != OPTION_UNSET || Duplex[bd] != OPTION_UNSET)
-			printk(KERN_INFO
+			netdev_info(netdev, PROBE,
 			       "Speed and duplex autonegotiation enabled\n");
 		break;
 	case HALF_DUPLEX:
-		printk(KERN_INFO "Half Duplex specified without Speed\n");
-		printk(KERN_INFO "Using Autonegotiation at Half Duplex only\n");
+		netdev_info(netdev, PROBE,
+			"Half Duplex specified without Speed.  "
+			"Using Autonegotiation at Half Duplex only\n");
 		adapter->hw.autoneg = 1;
 		adapter->hw.autoneg_advertised = ADVERTISE_10_HALF |
 		                                 ADVERTISE_100_HALF;
 		break;
 	case FULL_DUPLEX:
-		printk(KERN_INFO "Full Duplex specified without Speed\n");
-		printk(KERN_INFO "Using Autonegotiation at Full Duplex only\n");
+		netdev_info(netdev, PROBE,
+			"Full Duplex specified without Speed.  "
+			"Using Autonegotiation at Full Duplex only\n");
 		adapter->hw.autoneg = 1;
 		adapter->hw.autoneg_advertised = ADVERTISE_10_FULL |
 		                                 ADVERTISE_100_FULL |
 		                                 ADVERTISE_1000_FULL;
 		break;
 	case SPEED_10:
-		printk(KERN_INFO "10 Mbps Speed specified without Duplex\n");
-		printk(KERN_INFO "Using Autonegotiation at 10 Mbps only\n");
+		netdev_info(netdev, PROBE,
+			"10 Mbps Speed specified without Duplex.  "
+			"Using Autonegotiation at 10 Mbps only\n");
 		adapter->hw.autoneg = 1;
 		adapter->hw.autoneg_advertised = ADVERTISE_10_HALF |
 		                                 ADVERTISE_10_FULL;
 		break;
 	case SPEED_10 + HALF_DUPLEX:
-		printk(KERN_INFO "Forcing to 10 Mbps Half Duplex\n");
+		netdev_info(netdev, PROBE, "Forcing to 10 Mbps Half Duplex\n");
 		adapter->hw.autoneg = 0;
 		adapter->hw.forced_speed_duplex = e1000_10_half;
 		adapter->hw.autoneg_advertised = 0;
 		break;
 	case SPEED_10 + FULL_DUPLEX:
-		printk(KERN_INFO "Forcing to 10 Mbps Full Duplex\n");
+		netdev_info(netdev, PROBE, "Forcing to 10 Mbps Full Duplex\n");
 		adapter->hw.autoneg = 0;
 		adapter->hw.forced_speed_duplex = e1000_10_full;
 		adapter->hw.autoneg_advertised = 0;
 		break;
 	case SPEED_100:
-		printk(KERN_INFO "100 Mbps Speed specified without Duplex\n");
-		printk(KERN_INFO "Using Autonegotiation at 100 Mbps only\n");
+		netdev_info(netdev, PROBE,
+			"100 Mbps Speed specified without Duplex.  "
+			"Using Autonegotiation at 100 Mbps only\n");
 		adapter->hw.autoneg = 1;
 		adapter->hw.autoneg_advertised = ADVERTISE_100_HALF |
 		                                 ADVERTISE_100_FULL;
 		break;
 	case SPEED_100 + HALF_DUPLEX:
-		printk(KERN_INFO "Forcing to 100 Mbps Half Duplex\n");
+		netdev_info(netdev, PROBE, "Forcing to 100 Mbps Half Duplex\n");
 		adapter->hw.autoneg = 0;
 		adapter->hw.forced_speed_duplex = e1000_100_half;
 		adapter->hw.autoneg_advertised = 0;
 		break;
 	case SPEED_100 + FULL_DUPLEX:
-		printk(KERN_INFO "Forcing to 100 Mbps Full Duplex\n");
+		netdev_info(netdev, PROBE, "Forcing to 100 Mbps Full Duplex\n");
 		adapter->hw.autoneg = 0;
 		adapter->hw.forced_speed_duplex = e1000_100_full;
 		adapter->hw.autoneg_advertised = 0;
 		break;
 	case SPEED_1000:
-		printk(KERN_INFO "1000 Mbps Speed specified without Duplex\n");
-		printk(KERN_INFO
-		       "Using Autonegotiation at 1000 Mbps Full Duplex only\n");
+		netdev_info(netdev, PROBE,
+			"1000 Mbps Speed specified without Duplex.  "
+			"Using Autonegotiation at 1000 Mbps Full Duplex only\n");
 		adapter->hw.autoneg = 1;
 		adapter->hw.autoneg_advertised = ADVERTISE_1000_FULL;
 		break;
 	case SPEED_1000 + HALF_DUPLEX:
-		printk(KERN_INFO "Half Duplex is not supported at 1000 Mbps\n");
-		printk(KERN_INFO
-		       "Using Autonegotiation at 1000 Mbps Full Duplex only\n");
+		netdev_info(netdev, PROBE,
+			"Half Duplex is not supported at 1000 Mbps.  "
+			"Using Autonegotiation at 1000 Mbps Full Duplex only\n");
 		adapter->hw.autoneg = 1;
 		adapter->hw.autoneg_advertised = ADVERTISE_1000_FULL;
 		break;
 	case SPEED_1000 + FULL_DUPLEX:
-		printk(KERN_INFO
+		netdev_info(netdev, PROBE,
 		       "Using Autonegotiation at 1000 Mbps Full Duplex only\n");
 		adapter->hw.autoneg = 1;
 		adapter->hw.autoneg_advertised = ADVERTISE_1000_FULL;
@@ -685,8 +698,9 @@
 
 	/* Speed, AutoNeg and MDI/MDI-X must all play nice */
 	if (e1000_validate_mdi_setting(&(adapter->hw)) < 0) {
-		printk(KERN_INFO "Speed, AutoNeg and MDI-X specifications are "
-		       "incompatible. Setting MDI-X to a compatible value.\n");
+		netdev_info(netdev, PROBE,
+			"Speed, AutoNeg and MDI-X specifications are "
+			"incompatible. Setting MDI-X to a compatible value.\n");
 	}
 }
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 4/4] Net device error logging, revised (tg3)
  2003-08-25 21:31 [PATCH 1/4] Net device error logging, revised Jim Keniston
  2003-08-25 21:39 ` Subject: [PATCH 2/4] Net device error logging, revised (e100) Jim Keniston
  2003-08-25 21:41 ` Subject: [PATCH 3/4] Net device error logging, revised (e1000) Jim Keniston
@ 2003-08-25 21:43 ` Jim Keniston
  2003-08-26 18:32 ` [PATCH 1/4] Net device error logging, revised Greg KH
  2003-09-15 23:08 ` [PATCH] " Jim Keniston
  4 siblings, 0 replies; 13+ messages in thread
From: Jim Keniston @ 2003-08-25 21:43 UTC (permalink / raw)
  To: LKML, netdev, Jeff Garzik, Feldman, Scott, Larry Kessler,
	Greg KH, Randy Dunlap, Alan Cox, Andrew Morton, jkenisto

[-- Attachment #1: Type: text/plain, Size: 118 bytes --]

Here's a patch to modify the v2.6.0-test4 tg3 driver to use netdev_* macros.

Jim Keniston
IBM Linux Technology Center

[-- Attachment #2: tg3-2.6.0-test4.patch --]
[-- Type: text/plain, Size: 11786 bytes --]

diff -Naur linux.org/drivers/net/tg3.c linux.tg3.patched/drivers/net/tg3.c
--- linux.org/drivers/net/tg3.c	Mon Aug 25 13:29:39 2003
+++ linux.tg3.patched/drivers/net/tg3.c	Mon Aug 25 13:29:39 2003
@@ -489,9 +489,8 @@
 		break;
 
 	default:
-		printk(KERN_WARNING PFX "%s: Invalid power state (%d) "
-		       "requested.\n",
-		       tp->dev->name, state);
+		netdev_warn(tp->dev,, "Invalid power state (%d) "
+			"requested.\n", state);
 		return -EINVAL;
 	};
 
@@ -639,10 +638,10 @@
 static void tg3_link_report(struct tg3 *tp)
 {
 	if (!netif_carrier_ok(tp->dev)) {
-		printk(KERN_INFO PFX "%s: Link is down.\n", tp->dev->name);
+		netdev_info(tp->dev, LINK, "Link is down.\n");
 	} else {
-		printk(KERN_INFO PFX "%s: Link is up at %d Mbps, %s duplex.\n",
-		       tp->dev->name,
+		netdev_info(tp->dev, LINK,
+		       "Link is up at %d Mbps, %s duplex.\n",
 		       (tp->link_config.active_speed == SPEED_1000 ?
 			1000 :
 			(tp->link_config.active_speed == SPEED_100 ?
@@ -650,9 +649,8 @@
 		       (tp->link_config.active_duplex == DUPLEX_FULL ?
 			"full" : "half"));
 
-		printk(KERN_INFO PFX "%s: Flow control is %s for TX and "
+		netdev_info(tp->dev, LINK, "Flow control is %s for TX and "
 		       "%s for RX.\n",
-		       tp->dev->name,
 		       (tp->tg3_flags & TG3_FLAG_TX_PAUSE) ? "on" : "off",
 		       (tp->tg3_flags & TG3_FLAG_RX_PAUSE) ? "on" : "off");
 	}
@@ -2231,8 +2229,7 @@
 {
 	struct tg3 *tp = dev->priv;
 
-	printk(KERN_ERR PFX "%s: transmit timed out, resetting\n",
-	       dev->name);
+	netdev_err(dev, TX_ERR, "transmit timed out, resetting\n");
 
 	schedule_work(&tp->reset_task);
 }
@@ -2379,8 +2376,7 @@
 	if (unlikely(TX_BUFFS_AVAIL(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
 		netif_stop_queue(dev);
 		spin_unlock_irqrestore(&tp->tx_lock, flags);
-		printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n",
-		       dev->name);
+		netdev_err(dev, TX_ERR, "BUG! Tx Ring full when queue awake!\n");
 		return 1;
 	}
 
@@ -2397,7 +2393,8 @@
 			       TXD_FLAG_CPU_POST_DMA);
 
 		if (times++ < 5) {
-			printk("tg3_xmit: tso_size[%u] tso_segs[%u] len[%u]\n",
+			netdev_info(dev,,
+			       "tg3_xmit: tso_size[%u] tso_segs[%u] len[%u]\n",
 			       (unsigned int) skb_shinfo(skb)->tso_size,
 			       (unsigned int) skb_shinfo(skb)->tso_segs,
 			       skb->len);
@@ -2571,8 +2568,7 @@
 	if (unlikely(TX_BUFFS_AVAIL(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
 		netif_stop_queue(dev);
 		spin_unlock_irqrestore(&tp->tx_lock, flags);
-		printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n",
-		       dev->name);
+		netdev_err(dev, TX_ERR, "BUG! Tx Ring full when queue awake!\n");
 		return 1;
 	}
 
@@ -2593,7 +2589,8 @@
 			       TXD_FLAG_CPU_POST_DMA);
 
 		if (times++ < 5) {
-			printk("tg3_xmit: tso_size[%u] tso_segs[%u] len[%u]\n",
+			netdev_info(dev,,
+			       "tg3_xmit: tso_size[%u] tso_segs[%u] len[%u]\n",
 			       (unsigned int) skb_shinfo(skb)->tso_size,
 			       (unsigned int) skb_shinfo(skb)->tso_segs,
 			       skb->len);
@@ -3017,7 +3014,7 @@
 	}
 
 	if (i == MAX_WAIT_CNT) {
-		printk(KERN_ERR PFX "tg3_stop_block timed out, "
+		netdev_err(tp->dev,, "tg3_stop_block timed out, "
 		       "ofs=%lx enable_bit=%x\n",
 		       ofs, enable_bit);
 		return -ENODEV;
@@ -3070,9 +3067,9 @@
 			break;
 	}
 	if (i >= MAX_WAIT_CNT) {
-		printk(KERN_ERR PFX "tg3_abort_hw timed out for %s, "
+		netdev_err(tp->dev,, "tg3_abort_hw timed out; "
 		       "TX_MODE_ENABLE will not clear MAC_TX_MODE=%08x\n",
-		       tp->dev->name, tr32(MAC_TX_MODE));
+		       tr32(MAC_TX_MODE));
 		return -ENODEV;
 	}
 
@@ -3208,9 +3205,8 @@
 
 	if (i >= 100000 &&
 	    !(tp->tg3_flags2 & TG3_FLG2_SUN_5704)) {
-		printk(KERN_ERR PFX "tg3_halt timed out for %s, "
-		       "firmware will not restart magic=%08x\n",
-		       tp->dev->name, val);
+		netdev_err(tp->dev,, "tg3_halt timed out, "
+		       "firmware will not restart magic=%08x\n", val);
 		return -ENODEV;
 	}
 
@@ -3384,9 +3380,7 @@
 	}
 
 	if (i >= 10000) {
-		printk(KERN_ERR PFX "tg3_reset_cpu timed out for %s, "
-		       "and %s CPU\n",
-		       tp->dev->name,
+		netdev_err(tp->dev,, "tg3_reset_cpu timed out for %s CPU\n",
 		       (offset == RX_CPU_BASE ? "RX" : "TX"));
 		return -ENODEV;
 	}
@@ -3498,9 +3492,9 @@
 		udelay(1000);
 	}
 	if (i >= 5) {
-		printk(KERN_ERR PFX "tg3_load_firmware fails for %s "
+		netdev_err(tp->dev,, "tg3_load_firmware fails "
 		       "to set RX CPU PC, is %08x should be %08x\n",
-		       tp->dev->name, tr32(RX_CPU_BASE + CPU_PC),
+		       tr32(RX_CPU_BASE + CPU_PC),
 		       TG3_FW_TEXT_ADDR);
 		return -ENODEV;
 	}
@@ -3826,9 +3820,9 @@
 		udelay(1000);
 	}
 	if (i >= 5) {
-		printk(KERN_ERR PFX "tg3_load_tso_firmware fails for %s "
+		netdev_err(tp->dev,, "tg3_load_tso_firmware fails "
 		       "to set TX CPU PC, is %08x should be %08x\n",
-		       tp->dev->name, tr32(TX_CPU_BASE + CPU_PC),
+		       tr32(TX_CPU_BASE + CPU_PC),
 		       TG3_TSO_FW_TEXT_ADDR);
 		return -ENODEV;
 	}
@@ -3953,9 +3947,8 @@
 	}
 	if (i >= 100000 &&
 	    !(tp->tg3_flags2 & TG3_FLG2_SUN_5704)) {
-		printk(KERN_ERR PFX "tg3_reset_hw timed out for %s, "
-		       "firmware will not restart magic=%08x\n",
-		       tp->dev->name, val);
+		netdev_err(tp->dev,, "tg3_reset_hw timed out, "
+		       "firmware will not restart magic=%08x\n", val);
 		return -ENODEV;
 	}
 
@@ -4060,8 +4053,7 @@
 		udelay(10);
 	}
 	if (i >= 2000) {
-		printk(KERN_ERR PFX "tg3_reset_hw cannot enable BUFMGR for %s.\n",
-		       tp->dev->name);
+		netdev_err(tp->dev,, "tg3_reset_hw cannot enable BUFMGR.\n");
 		return -ENODEV;
 	}
 
@@ -4073,8 +4065,7 @@
 		udelay(10);
 	}
 	if (i >= 2000) {
-		printk(KERN_ERR PFX "tg3_reset_hw cannot reset FTQ for %s.\n",
-		       tp->dev->name);
+		netdev_err(tp->dev,, "tg3_reset_hw cannot reset FTQ.\n");
 		return -ENODEV;
 	}
 
@@ -5222,14 +5213,12 @@
   
 static u32 tg3_get_msglevel(struct net_device *dev)
 {
-	struct tg3 *tp = dev->priv;
-	return tp->msg_enable;
+	return dev->msg_enable;
 }
   
 static void tg3_set_msglevel(struct net_device *dev, u32 value)
 {
-	struct tg3 *tp = dev->priv;
-	tp->msg_enable = value;
+	dev->msg_enable = value;
 }
   
 static int tg3_nway_reset(struct net_device *dev)
@@ -5550,7 +5539,7 @@
 	int i, saw_done_clear;
 
 	if (tp->tg3_flags2 & TG3_FLG2_SUN_5704) {
-		printk(KERN_ERR PFX "Attempt to do nvram_read on Sun 5704\n");
+		netdev_err(tp->dev,, "Attempt to do nvram_read on Sun 5704\n");
 		return -EINVAL;
 	}
 
@@ -6049,7 +6038,7 @@
 	/* Force the chip into D0. */
 	err = tg3_set_power_state(tp, 0);
 	if (err) {
-		printk(KERN_ERR PFX "(%s) transition to D0 failed\n",
+		netdev_err(tp->dev,, "(%s) transition to D0 failed\n",
 		       pci_name(tp->pdev));
 		return err;
 	}
@@ -6160,7 +6149,8 @@
 
 	err = tg3_phy_probe(tp);
 	if (err) {
-		printk(KERN_ERR PFX "(%s) phy probe failed, err %d\n",
+		netdev_err(tp->dev,,
+		       "(%s) phy probe failed, err %d\n",
 		       pci_name(tp->pdev), err);
 		/* ... but do not return immediately ... */
 	}
@@ -6643,20 +6633,21 @@
 	unsigned long tg3reg_base, tg3reg_len;
 	struct net_device *dev;
 	struct tg3 *tp;
-	int i, err, pci_using_dac, pm_cap;
+	int err, pci_using_dac, pm_cap;
+	unsigned char *mac;
 
 	if (tg3_version_printed++ == 0)
 		printk(KERN_INFO "%s", version);
 
 	err = pci_enable_device(pdev);
 	if (err) {
-		printk(KERN_ERR PFX "Cannot enable PCI device, "
+		dev_err(&pdev->dev, "Cannot enable PCI device, "
 		       "aborting.\n");
 		return err;
 	}
 
 	if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM)) {
-		printk(KERN_ERR PFX "Cannot find proper PCI device "
+		dev_err(&pdev->dev, "Cannot find proper PCI device "
 		       "base address, aborting.\n");
 		err = -ENODEV;
 		goto err_out_disable_pdev;
@@ -6664,7 +6655,7 @@
 
 	err = pci_request_regions(pdev, DRV_MODULE_NAME);
 	if (err) {
-		printk(KERN_ERR PFX "Cannot obtain PCI resources, "
+		dev_err(&pdev->dev, "Cannot obtain PCI resources, "
 		       "aborting.\n");
 		goto err_out_disable_pdev;
 	}
@@ -6674,7 +6665,7 @@
 	/* Find power-management capability. */
 	pm_cap = pci_find_capability(pdev, PCI_CAP_ID_PM);
 	if (pm_cap == 0) {
-		printk(KERN_ERR PFX "Cannot find PowerManagement capability, "
+		dev_err(&pdev->dev, "Cannot find PowerManagement capability, "
 		       "aborting.\n");
 		goto err_out_free_res;
 	}
@@ -6683,14 +6674,14 @@
 	if (!pci_set_dma_mask(pdev, 0xffffffffffffffffULL)) {
 		pci_using_dac = 1;
 		if (pci_set_consistent_dma_mask(pdev, 0xffffffffffffffffULL)) {
-			printk(KERN_ERR PFX "Unable to obtain 64 bit DMA "
+			dev_err(&pdev->dev, "Unable to obtain 64 bit DMA "
 			       "for consistent allocations\n");
 			goto err_out_free_res;
 		}
 	} else {
 		err = pci_set_dma_mask(pdev, (u64) 0xffffffff);
 		if (err) {
-			printk(KERN_ERR PFX "No usable DMA configuration, "
+			dev_err(&pdev->dev, "No usable DMA configuration, "
 			       "aborting.\n");
 			goto err_out_free_res;
 		}
@@ -6702,7 +6693,7 @@
 
 	dev = alloc_etherdev(sizeof(*tp));
 	if (!dev) {
-		printk(KERN_ERR PFX "Etherdev alloc failed, aborting.\n");
+		dev_err(&pdev->dev, "Etherdev alloc failed, aborting.\n");
 		err = -ENOMEM;
 		goto err_out_free_res;
 	}
@@ -6727,9 +6718,9 @@
 	tp->tx_mode = TG3_DEF_TX_MODE;
 	tp->mi_mode = MAC_MI_MODE_BASE;
 	if (tg3_debug > 0)
-		tp->msg_enable = tg3_debug;
+		dev->msg_enable = tg3_debug;
 	else
-		tp->msg_enable = TG3_DEF_MSG_ENABLE;
+		dev->msg_enable = TG3_DEF_MSG_ENABLE;
 
 	/* The word/byte swap controls here control register access byte
 	 * swapping.  DMA data byte swapping is controlled in the GRC_MODE
@@ -6759,7 +6750,7 @@
 
 	tp->regs = (unsigned long) ioremap(tg3reg_base, tg3reg_len);
 	if (tp->regs == 0UL) {
-		printk(KERN_ERR PFX "Cannot map device registers, "
+		netdev_err(dev,, "Cannot map device registers, "
 		       "aborting.\n");
 		err = -ENOMEM;
 		goto err_out_free_dev;
@@ -6789,21 +6780,21 @@
 
 	err = tg3_get_invariants(tp);
 	if (err) {
-		printk(KERN_ERR PFX "Problem fetching invariants of chip, "
+		netdev_err(dev,, "Problem fetching invariants of chip, "
 		       "aborting.\n");
 		goto err_out_iounmap;
 	}
 
 	err = tg3_get_device_address(tp);
 	if (err) {
-		printk(KERN_ERR PFX "Could not obtain valid ethernet address, "
+		netdev_err(dev,, "Could not obtain valid ethernet address, "
 		       "aborting.\n");
 		goto err_out_iounmap;
 	}
 
 	err = tg3_test_dma(tp);
 	if (err) {
-		printk(KERN_ERR PFX "DMA engine test failed, aborting.\n");
+		netdev_err(dev,, "DMA engine test failed, aborting.\n");
 		goto err_out_iounmap;
 	}
 
@@ -6829,7 +6820,7 @@
 
 	err = register_netdev(dev);
 	if (err) {
-		printk(KERN_ERR PFX "Cannot register net device, "
+		netdev_err(dev,, "Cannot register net device, "
 		       "aborting.\n");
 		goto err_out_iounmap;
 	}
@@ -6842,8 +6833,9 @@
 	 */
 	pci_save_state(tp->pdev, tp->pci_cfg_state);
 
-	printk(KERN_INFO "%s: Tigon3 [partno(%s) rev %04x PHY(%s)] (PCI%s:%s:%s) %sBaseT Ethernet ",
-	       dev->name,
+	mac = dev->dev_addr;
+	netdev_info(dev, PROBE, "Tigon3 [partno(%s) rev %04x PHY(%s)] (PCI%s:%s:%s) %sBaseT Ethernet "
+	       "%02x:%02x:%02x:%02x:%02x:%02x\n",
 	       tp->board_part_number,
 	       tp->pci_chip_rev_id,
 	       tg3_phy_string(tp),
@@ -6852,11 +6844,8 @@
 		((tp->tg3_flags & TG3_FLAG_PCIX_MODE) ? "133MHz" : "66MHz") :
 		((tp->tg3_flags & TG3_FLAG_PCIX_MODE) ? "100MHz" : "33MHz")),
 	       ((tp->tg3_flags & TG3_FLAG_PCI_32BIT) ? "32-bit" : "64-bit"),
-	       (tp->tg3_flags & TG3_FLAG_10_100_ONLY) ? "10/100" : "10/100/1000");
-
-	for (i = 0; i < 6; i++)
-		printk("%2.2x%c", dev->dev_addr[i],
-		       i == 5 ? '\n' : ':');
+	       (tp->tg3_flags & TG3_FLAG_10_100_ONLY) ? "10/100" : "10/100/1000",
+	       mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
 
 	return 0;
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] Net device error logging, revised
  2003-08-25 21:31 [PATCH 1/4] Net device error logging, revised Jim Keniston
                   ` (2 preceding siblings ...)
  2003-08-25 21:43 ` [PATCH 4/4] Net device error logging, revised (tg3) Jim Keniston
@ 2003-08-26 18:32 ` Greg KH
  2003-08-26 23:34   ` Jim Keniston
  2003-09-15 23:08 ` [PATCH] " Jim Keniston
  4 siblings, 1 reply; 13+ messages in thread
From: Greg KH @ 2003-08-26 18:32 UTC (permalink / raw)
  To: Jim Keniston
  Cc: LKML, netdev, Jeff Garzik, Feldman, Scott, Larry Kessler,
	Randy Dunlap, Alan Cox, Andrew Morton

On Mon, Aug 25, 2003 at 02:31:19PM -0700, Jim Keniston wrote:
> +int __netdev_printk(const char *sevlevel, const struct net_device *netdev,
> +	int msglevel, const char *format, ...)
> +{
> +	if (!netdev || !format) {
> +		return -EINVAL;
> +	}
> +	if (msglevel == NETIF_MSG_ALL || (netdev->msg_enable & msglevel)) {
> +		char msg[512];

512 bytes on the stack?  Any way to prevent this from happening?  With
the push to make the stack even smaller in 2.7, people will not like
this.

thanks,

greg k-h


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] Net device error logging, revised
  2003-08-26 18:32 ` [PATCH 1/4] Net device error logging, revised Greg KH
@ 2003-08-26 23:34   ` Jim Keniston
  2003-08-26 23:51     ` Jeff Garzik
  2003-08-27  1:06     ` Stephen Hemminger
  0 siblings, 2 replies; 13+ messages in thread
From: Jim Keniston @ 2003-08-26 23:34 UTC (permalink / raw)
  To: Greg KH
  Cc: LKML, netdev, Jeff Garzik, Feldman, Scott, Larry Kessler,
	Randy Dunlap, Alan Cox, Andrew Morton

Greg KH wrote:
> 
> On Mon, Aug 25, 2003 at 02:31:19PM -0700, Jim Keniston wrote:
> > +int __netdev_printk(const char *sevlevel, const struct net_device *netdev,
> > +     int msglevel, const char *format, ...)
> > +{
> > +     if (!netdev || !format) {
> > +             return -EINVAL;
> > +     }
> > +     if (msglevel == NETIF_MSG_ALL || (netdev->msg_enable & msglevel)) {
> > +             char msg[512];
> 
> 512 bytes on the stack?  Any way to prevent this from happening?  With
> the push to make the stack even smaller in 2.7, people will not like
> this.
> 
> thanks,
> 
> greg k-h

The following options come to mind:
1. Keep the msg buffer, but make it smaller.  Around 120 bytes would probably be
big enough for the vast majority of messages.  (printk() uses a 1024-byte buffer,
but it's static -- see #2.)

2. Use a big, static buffer, protected by a spinlock.  printk() does this.

3. Do the whole thing in a macro, as in previous proposals.  The size of the macro
expansion could be reduced somewhat by doing the encode-prefix step in a function --
something like:

#define netdev_printk(sevlevel, netdev, msglevel, format, arg...)	\
do {									\
if (NETIF_MSG_##msglevel == NETIF_MSG_ALL || ((netdev)->msg_enable & NETIF_MSG_##msglevel)) {	\
	char pfx[40];							\
	printk(sevlevel "%s: " format , make_netdev_msg_prefix(pfx, netdev) , ## arg);	\
}} while (0)

This would make your code bigger, but not that much bigger for the common case where
the msglevel is omitted (and the 'if(...)' is optimized out).

Jim

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] Net device error logging, revised
  2003-08-26 23:34   ` Jim Keniston
@ 2003-08-26 23:51     ` Jeff Garzik
  2003-08-27  1:07       ` Jim Keniston
  2003-08-27  1:06     ` Stephen Hemminger
  1 sibling, 1 reply; 13+ messages in thread
From: Jeff Garzik @ 2003-08-26 23:51 UTC (permalink / raw)
  To: Jim Keniston
  Cc: Greg KH, LKML, netdev, Feldman, Scott, Larry Kessler,
	Randy Dunlap, Alan Cox, Andrew Morton

Jim Keniston wrote:
> #define netdev_printk(sevlevel, netdev, msglevel, format, arg...)	\
> do {									\
> if (NETIF_MSG_##msglevel == NETIF_MSG_ALL || ((netdev)->msg_enable & NETIF_MSG_##msglevel)) {	\
> 	char pfx[40];							\
> 	printk(sevlevel "%s: " format , make_netdev_msg_prefix(pfx, netdev) , ## arg);	\
> }} while (0)
> 
> This would make your code bigger, but not that much bigger for the common case where
> the msglevel is omitted (and the 'if(...)' is optimized out).


"NETIF_MSG_" is silly and should be eliminated.
A separate "NETIF_MSG_ALL" test is not needed, because msg_enable is a 
bitmask.  A msg_enable of 0xffffffff will naturally create a NETIF_MSG_ALL.

Also, whatever mechanism is created, it needs to preserve the feature of 
the existing system:

	if (a quick bitmask test)
		do something

And preferably "do something" is not inlined, because printk'ing -- 
although it may appear in a fast path during debugging -- cannot be 
considered a fast path itself.

	Jeff




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] Net device error logging, revised
  2003-08-26 23:34   ` Jim Keniston
  2003-08-26 23:51     ` Jeff Garzik
@ 2003-08-27  1:06     ` Stephen Hemminger
  2003-08-29 21:22       ` Jim Keniston
  1 sibling, 1 reply; 13+ messages in thread
From: Stephen Hemminger @ 2003-08-27  1:06 UTC (permalink / raw)
  To: Jim Keniston
  Cc: Greg KH, LKML, netdev, Jeff Garzik, Feldman, Scott,
	Larry Kessler, Randy Dunlap, Alan Cox, Andrew Morton

 
> The following options come to mind:
> 1. Keep the msg buffer, but make it smaller.  Around 120 bytes would probably be
> big enough for the vast majority of messages.  (printk() uses a 1024-byte buffer,
> but it's static -- see #2.)
> 
> 2. Use a big, static buffer, protected by a spinlock.  printk() does this.
> 
> 3. Do the whole thing in a macro, as in previous proposals.  The size of the macro
> expansion could be reduced somewhat by doing the encode-prefix step in a function --
> something like:
> 
> #define netdev_printk(sevlevel, netdev, msglevel, format, arg...)	\
> do {									\
> if (NETIF_MSG_##msglevel == NETIF_MSG_ALL || ((netdev)->msg_enable & NETIF_MSG_##msglevel)) {	\
> 	char pfx[40];							\
> 	printk(sevlevel "%s: " format , make_netdev_msg_prefix(pfx, netdev) , ## arg);	\
> }} while (0)
> 
> This would make your code bigger, but not that much bigger for the common case where
> the msglevel is omitted (and the 'if(...)' is optimized out).

Is there some way to tack copy and prepend what you want onto the format
string, and add additional arguments to the call to printk?  That way you
wouldn't need space for the potentially large resulting string, but only
enough room for the expanded format string.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] Net device error logging, revised
  2003-08-26 23:51     ` Jeff Garzik
@ 2003-08-27  1:07       ` Jim Keniston
  2003-09-03 17:32         ` Jeff Garzik
  0 siblings, 1 reply; 13+ messages in thread
From: Jim Keniston @ 2003-08-27  1:07 UTC (permalink / raw)
  To: Jeff Garzik
  Cc: Greg KH, LKML, netdev, Feldman, Scott, Larry Kessler,
	Randy Dunlap, Alan Cox, Andrew Morton

Jeff Garzik wrote:
> 
> Jim Keniston wrote:
> > #define netdev_printk(sevlevel, netdev, msglevel, format, arg...)     \
> > do {                                                                  \
> > if (NETIF_MSG_##msglevel == NETIF_MSG_ALL || ((netdev)->msg_enable & NETIF_MSG_##msglevel)) { \
> >       char pfx[40];                                                   \
> >       printk(sevlevel "%s: " format , make_netdev_msg_prefix(pfx, netdev) , ## arg);  \
> > }} while (0)
> >
> > This would make your code bigger, but not that much bigger for the common case where
> > the msglevel is omitted (and the 'if(...)' is optimized out).
> 
> "NETIF_MSG_" is silly and should be eliminated.

>From this, I infer that you think that the option to "omit" the msglevel arg --
e.g.,
	netdev_err(dev,, "NIC is fried!\n");	/* always logged */
-- is silly.  No big deal.  Its sole purpose is to help keep netdev_* calls terse.

> A separate "NETIF_MSG_ALL" test is not needed, because msg_enable is a
> bitmask.  A msg_enable of 0xffffffff will naturally create a NETIF_MSG_ALL.

But how do you code a netdev_* call where you ALWAYS want the message (including
netdev_printk-style prefix) logged, regardless of the value of msg_enable?  That's
what NETIF_MSG_ALL is for (and why it might be better called NETIF_MSG_ALWAYS)...
	netdev_err(dev, ALL, "NIC is fried!\n");	/* always logged */
or
	netdev_err(dev, ALWAYS, "NIC is fried!\n");	/* always logged */

> 
> Also, whatever mechanism is created, it needs to preserve the feature of
> the existing system:
> 
>         if (a quick bitmask test)
>                 do something
> 
> And preferably "do something" is not inlined, because printk'ing --
> although it may appear in a fast path during debugging -- cannot be
> considered a fast path itself.
> 
>         Jeff

Sorry, I'm not sure what you're getting at here.  netdev_* doesn't prevent
people from using the existing netif_msg_* macros; it just provides shorthand
for the (usual) case where "do something" is "printk".

Jim

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] Net device error logging, revised
  2003-08-27  1:06     ` Stephen Hemminger
@ 2003-08-29 21:22       ` Jim Keniston
  0 siblings, 0 replies; 13+ messages in thread
From: Jim Keniston @ 2003-08-29 21:22 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Greg KH, LKML, netdev, Jeff Garzik, Feldman, Scott,
	Larry Kessler, Randy Dunlap, Alan Cox, Andrew Morton,
	Daniel Stekloff, Hien Nguyen, jkenisto

Stephen Hemminger wrote:
> 
> > The following options come to mind:
> > 1. Keep the msg buffer, but make it smaller.  Around 120 bytes would probably be
> > big enough for the vast majority of messages.  (printk() uses a 1024-byte buffer,
> > but it's static -- see #2.)
> >
> > 2. Use a big, static buffer, protected by a spinlock.  printk() does this.
> >
> > 3. Do the whole thing in a macro, as in previous proposals.  The size of the macro
> > expansion could be reduced somewhat by doing the encode-prefix step in a function --
> > something like:
[more on #3 snipped]
> 
> Is there some way to tack copy and prepend what you want onto the format
> string, and add additional arguments to the call to printk?  That way you
> wouldn't need space for the potentially large resulting string, but only
> enough room for the expanded format string.

Interesting idea.  I pondered this for a while.  But even if you postulate 
a varargs version of printk (which doesn't exist), it's not really
feasible to do this in a function.  There's no way for a function to
prepend args to a va_list.  That means you'd have to encode the text of
the prefix as part of the format string, and that would require you to
allocate room for prefix+format, which is still a lot of stack.  Also,
the fact that the interface name itself may contain "%d" or some such
makes it even messier.

Greg K-H thinks #2 is a reasonable solution (you're about to serialize on printk's
lock anyway), so I'll go with that.

Thanks.
Jim

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] Net device error logging, revised
  2003-08-27  1:07       ` Jim Keniston
@ 2003-09-03 17:32         ` Jeff Garzik
  2003-09-03 20:56           ` Jim Keniston
  0 siblings, 1 reply; 13+ messages in thread
From: Jeff Garzik @ 2003-09-03 17:32 UTC (permalink / raw)
  To: Jim Keniston
  Cc: Greg KH, LKML, netdev, Feldman, Scott, Larry Kessler,
	Randy Dunlap, Alan Cox, Andrew Morton

Jim Keniston wrote:
> Jeff Garzik wrote:
>>"NETIF_MSG_" is silly and should be eliminated.
> 
> 
>>From this, I infer that you think that the option to "omit" the msglevel arg --
> e.g.,
> 	netdev_err(dev,, "NIC is fried!\n");	/* always logged */
> -- is silly.  No big deal.  Its sole purpose is to help keep netdev_* calls terse.

yes


>>A separate "NETIF_MSG_ALL" test is not needed, because msg_enable is a
>>bitmask.  A msg_enable of 0xffffffff will naturally create a NETIF_MSG_ALL.
> 
> 
> But how do you code a netdev_* call where you ALWAYS want the message (including
> netdev_printk-style prefix) logged, regardless of the value of msg_enable?  That's
> what NETIF_MSG_ALL is for (and why it might be better called NETIF_MSG_ALWAYS)...

I understand the purpose of NETIF_MSG_ALL; re-read what I said.  You 
don't need a separate _test_, as your implementation includes.  Defining 
NETIF_MSG_ALL to 0xffffffff will naturally create the effect you seek.


>>Also, whatever mechanism is created, it needs to preserve the feature of
>>the existing system:
>>
>>        if (a quick bitmask test)
>>                do something
>>
>>And preferably "do something" is not inlined, because printk'ing --
>>although it may appear in a fast path during debugging -- cannot be
>>considered a fast path itself.

> Sorry, I'm not sure what you're getting at here.  netdev_* doesn't prevent
> people from using the existing netif_msg_* macros; it just provides shorthand
> for the (usual) case where "do something" is "printk".


I would prefer to be more ambitious.  If we're gonna go in and change 
every printk in a driver, we might as well do it right, and (a) make 
sure the driver does msg_enable, and (b) make the source code a bit more 
clean by hiding the "if (test bitmap)" test in your netdev_xxx stuff.

	Jeff



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] Net device error logging, revised
  2003-09-03 17:32         ` Jeff Garzik
@ 2003-09-03 20:56           ` Jim Keniston
  0 siblings, 0 replies; 13+ messages in thread
From: Jim Keniston @ 2003-09-03 20:56 UTC (permalink / raw)
  To: Jeff Garzik
  Cc: Greg KH, LKML, netdev, Feldman, Scott, Larry Kessler,
	Randy Dunlap, Alan Cox, Andrew Morton

Jeff Garzik wrote:
> 
> Jim Keniston wrote:
> > Jeff Garzik wrote:
> ...
> >>A separate "NETIF_MSG_ALL" test is not needed, because msg_enable is a
> >>bitmask.  A msg_enable of 0xffffffff will naturally create a NETIF_MSG_ALL.
> >
> >
> > But how do you code a netdev_* call where you ALWAYS want the message (including
> > netdev_printk-style prefix) logged, regardless of the value of msg_enable?  That's
> > what NETIF_MSG_ALL is for (and why it might be better called NETIF_MSG_ALWAYS)...
> 
> I understand the purpose of NETIF_MSG_ALL; re-read what I said.  You
> don't need a separate _test_, as your implementation includes.  Defining
> NETIF_MSG_ALL to 0xffffffff will naturally create the effect you seek.
>

So the test becomes
	if (netdev->msg_enable & msglevel) { /* log message */ }
If netdev->msg_enable == 0, the message is suppressed even if msglevel == NETIF_MSG_ALL.
I had intended that "ALL" would override the msg_enable setting (even 0), but we can do
it this way as well.

> ...
> 
>         Jeff

Jim

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] Net device error logging, revised
  2003-08-25 21:31 [PATCH 1/4] Net device error logging, revised Jim Keniston
                   ` (3 preceding siblings ...)
  2003-08-26 18:32 ` [PATCH 1/4] Net device error logging, revised Greg KH
@ 2003-09-15 23:08 ` Jim Keniston
  4 siblings, 0 replies; 13+ messages in thread
From: Jim Keniston @ 2003-09-15 23:08 UTC (permalink / raw)
  To: LKML, netdev, Jeff Garzik, Feldman, Scott, Larry Kessler,
	Greg KH, Randy Dunlap, Alan Cox, Andrew Morton, jkenisto
  Cc: David Brownell, Stephen Hemminger

[-- Attachment #1: Type: text/plain, Size: 2255 bytes --]

This patch extends the concept of Linux 2.6's dev_* logging macros to
support network devices.  This is a modification of a patch posted
last month, and addresses the issues raised since then, namely:

1. To minimize stack usage, the msg[] buffer in __netdev_printk() has been
made static and protected by a spinlock.  (The spinlock shouldn't be a big
performance hit because we're about to serialize on printk's lock anyway.)

2. It is no longer possible to omit the msglevel arg.  For example,
        netdev_err(dev,, "NIC fried!\n");
no longer works.  This must be expressed as
        netdev_err(dev, ALL, "NIC fried!\n");
or (see #3 below) something like
        netdev_fatal(dev, HW, "NIC fried!\n");

3. A new macro, netdev_fatal, is included.  Given the call
        netdev_fatal(dev, HW, "NIC fried!\n");
the indicated message is always logged: the msglevel arg (HW, in this
case) is NOT consulted.  In fact, the msglevel arg to netdev_fatal
is ignored in this implementation.  (As previously discussed, in some
future implementation, the msglevel could be logged to help indicate
the circumstances under which the event was logged.)

4. It was suggested that the netdev_* macros should support message
filtering via simple message levels -- e.g.,
        if (dev->msg_enable > 5) printk(KERN_INFO "Received a packet.\n");
-- in addition to (or instead of) via the NETIF_MSG_* bit masks.  But
Jeff Garzik reiterated his desire to standardize on NETIF_MSG_*, so
I'm leaving things unchanged in that respect.

5. It was suggested that netdev_dbg is not flexible enough to handle all
debugging situations.  This is probably true.  Because of the special
nature of debugging messages, I'd expect the developer to use other
approaches in debug code if netdev_dbg doesn't fill the bill.  But the
netdev_dbg approach appears to be reasonably useful.  (For comparison,
there are currently 188 calls to dev_dbg in Linux drivers.)  No change
here.

6. Certain comments seemed to imply that you should be able to
suppress all messages (even those with a msglevel of ALL) by setting the
msg_enable field to 0.  I chose not to support this, because it seemed
counterintuitive and inconsistent with existing practice.

Jim Keniston
IBM Linux Technology Center

[-- Attachment #2: netdev-2.6.0-test5.patch --]
[-- Type: text/plain, Size: 5765 bytes --]

diff -Naur linux.org/include/linux/netdevice.h linux.netdev.patched/include/linux/netdevice.h
--- linux.org/include/linux/netdevice.h	Mon Sep 15 15:58:06 2003
+++ linux.netdev.patched/include/linux/netdevice.h	Mon Sep 15 15:58:06 2003
@@ -467,6 +467,9 @@
 	struct divert_blk	*divert;
 #endif /* CONFIG_NET_DIVERT */
 
+	/* NETIF_MSG_* flags to control the types of events we log */
+	int msg_enable;
+
 	/* class/net/name entry */
 	struct class_device	class_dev;
 };
@@ -741,6 +744,7 @@
 	NETIF_MSG_PKTDATA	= 0x1000,
 	NETIF_MSG_HW		= 0x2000,
 	NETIF_MSG_WOL		= 0x4000,
+	NETIF_MSG_ALL		= -1,		/* always log message */
 };
 
 #define netif_msg_drv(p)	((p)->msg_enable & NETIF_MSG_DRV)
@@ -899,6 +903,40 @@
 extern void		dev_clear_fastroute(struct net_device *dev);
 #endif
 
+/* debugging and troubleshooting/diagnostic helpers. */
+/**
+ * netdev_printk() - Log message with interface name, gated by message level
+ * @sevlevel: severity level -- e.g., KERN_INFO
+ * @netdev: net_device pointer
+ * @msglevel: a standard message-level flag with the NETIF_MSG_ prefix removed.
+ *	Unless msglevel is NETIF_MSG_ALL, log the message only if that flag
+ *	is set in netdev->msg_enable.
+ * @format: as with printk
+ * @args: as with printk
+ */
+extern int __netdev_printk(const char *sevlevel,
+	const struct net_device *netdev, int msglevel, const char *format, ...);
+#define netdev_printk(sevlevel, netdev, msglevel, format, arg...)	\
+	__netdev_printk(sevlevel , netdev , NETIF_MSG_##msglevel ,	\
+	format , ## arg)
+
+#ifdef DEBUG
+#define netdev_dbg(netdev, msglevel, format, arg...)		\
+	netdev_printk(KERN_DEBUG , netdev , msglevel , format , ## arg)
+#else
+#define netdev_dbg(netdev, msglevel, format, arg...) do {} while (0)
+#endif
+
+#define netdev_err(netdev, msglevel, format, arg...)		\
+	netdev_printk(KERN_ERR , netdev , msglevel , format , ## arg)
+#define netdev_info(netdev, msglevel, format, arg...)		\
+	netdev_printk(KERN_INFO , netdev , msglevel , format , ## arg)
+#define netdev_warn(netdev, msglevel, format, arg...)		\
+	netdev_printk(KERN_WARNING , netdev , msglevel , format , ## arg)
+
+/* report fatal error unconditionally; msglevel ignored for now */
+#define netdev_fatal(netdev, msglevel, format, arg...)		\
+	netdev_printk(KERN_ERR , netdev , ALL , format , ## arg)
 
 #endif /* __KERNEL__ */
 
diff -Naur linux.org/net/core/dev.c linux.netdev.patched/net/core/dev.c
--- linux.org/net/core/dev.c	Mon Sep 15 15:58:07 2003
+++ linux.netdev.patched/net/core/dev.c	Mon Sep 15 15:58:07 2003
@@ -3035,3 +3035,75 @@
 }
 
 subsys_initcall(net_dev_init);
+
+static spinlock_t netdev_printk_lock = SPIN_LOCK_UNLOCKED;
+/**
+ * __netdev_printk() - Log message with interface name, gated by message level
+ * @sevlevel: severity level -- e.g., KERN_INFO
+ * @netdev: net_device pointer
+ * @msglevel: a standard message-level flag such as NETIF_MSG_PROBE.
+ *	Unless msglevel is NETIF_MSG_ALL, log the message only if
+ *	that flag is set in netdev->msg_enable.
+ * @format: as with printk
+ * @args: as with printk
+ *
+ * Does the work for the netdev_printk macro.
+ * For a lot of network drivers, the probe function looks like
+ *	...
+ *	netdev = alloc_netdev(...);	// or alloc_etherdev(...)
+ *	SET_NETDEV_DEV(netdev, dev);
+ *	...
+ *	register_netdev(netdev);
+ *	...
+ * netdev_printk and its wrappers (e.g., netdev_err) can be used as
+ * soon as you have a valid net_device pointer -- e.g., from alloc_netdev,
+ * alloc_etherdev, or init_etherdev.  (Before that, use dev_printk and
+ * its wrappers to report device errors.)  It's common for an interface to
+ * have a name like "eth%d" until the device is successfully configured,
+ * and the call to register_netdev changes it to a "real" name like "eth0".
+ *
+ * If the interface's reg_state is NETREG_REGISTERED, we assume that it has
+ * been successfully set up in sysfs, and we prepend only the interface name
+ * to the message -- e.g., "eth0: NIC Link is Down".  The interface
+ * name can be used to find eth0's driver, bus ID, etc. in sysfs.
+ *
+ * For any other value of reg_state, we prepend the driver name and bus ID
+ * as well as the (possibly incomplete) interface name -- e.g.,
+ * "eth%d (e100 0000:00:03.0): Failed to map PCI address..."
+ *
+ * Probe functions that alloc and register in one step (via init_etherdev),
+ * or otherwise register the device before the probe completes successfully,
+ * may need to take other steps to ensure that the failing device is clearly
+ * identified.
+ */
+int __netdev_printk(const char *sevlevel, const struct net_device *netdev,
+	int msglevel, const char *format, ...)
+{
+	if (!netdev || !format) {
+		return -EINVAL;
+	}
+	if (msglevel == NETIF_MSG_ALL || (netdev->msg_enable & msglevel)) {
+		static char msg[512];	/* protected by netdev_printk_lock */
+		unsigned long flags;
+		va_list args;
+		struct device *dev = netdev->class_dev.dev;
+		
+		spin_lock_irqsave(&netdev_printk_lock, flags);
+		va_start(args, format);
+		vsnprintf(msg, 512, format, args);
+		va_end(args);
+
+		if (!sevlevel) {
+			sevlevel = "";
+		}
+
+		if (netdev->reg_state == NETREG_REGISTERED || !dev) {
+			printk("%s%s: %s", sevlevel, netdev->name, msg);
+		} else {
+			printk("%s%s (%s %s): %s", sevlevel, netdev->name,
+				dev->driver->name, dev->bus_id, msg);
+		}
+		spin_unlock_irqrestore(&netdev_printk_lock, flags);
+	}
+	return 0;
+}
diff -Naur linux.org/net/netsyms.c linux.netdev.patched/net/netsyms.c
--- linux.org/net/netsyms.c	Mon Sep 15 15:58:07 2003
+++ linux.netdev.patched/net/netsyms.c	Mon Sep 15 15:58:07 2003
@@ -210,6 +210,7 @@
 EXPORT_SYMBOL(net_ratelimit);
 EXPORT_SYMBOL(net_random);
 EXPORT_SYMBOL(net_srandom);
+EXPORT_SYMBOL(__netdev_printk);
 
 /* Needed by smbfs.o */
 EXPORT_SYMBOL(__scm_destroy);

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2003-09-15 23:12 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-08-25 21:31 [PATCH 1/4] Net device error logging, revised Jim Keniston
2003-08-25 21:39 ` Subject: [PATCH 2/4] Net device error logging, revised (e100) Jim Keniston
2003-08-25 21:41 ` Subject: [PATCH 3/4] Net device error logging, revised (e1000) Jim Keniston
2003-08-25 21:43 ` [PATCH 4/4] Net device error logging, revised (tg3) Jim Keniston
2003-08-26 18:32 ` [PATCH 1/4] Net device error logging, revised Greg KH
2003-08-26 23:34   ` Jim Keniston
2003-08-26 23:51     ` Jeff Garzik
2003-08-27  1:07       ` Jim Keniston
2003-09-03 17:32         ` Jeff Garzik
2003-09-03 20:56           ` Jim Keniston
2003-08-27  1:06     ` Stephen Hemminger
2003-08-29 21:22       ` Jim Keniston
2003-09-15 23:08 ` [PATCH] " Jim Keniston

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