Printing the driver name as part of the netdev watchdog message

Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

Hi Dave and co,

a few weeks ago you suggested to print the driver name as part of the WARN_ON for the 
watchdog timeout message; the patch below will do this.

The patch depends on the patch to add WARN(), a WARN_ON() variant taking printk arguments.
With WARN(), the actual message is part of the ==[ cut here ]== so that tools and humans
pick up the message as well when reporting the bugs.

Does this patch look like the right approach for achieving the goal?
If so... on the logistics side, how should we do this, since the WARN() patch is
not in mainline yet. I have a patch series adding it / users of it, and could just fold this patch
into that series assuming you ack the patch. Obviously I'm also open to other ways
of dealing with the logistics


From: Arjan van de Ven <arjan@linux.intel.com>
Subject: Use WARN_ONCE() in the netdev timeout handler (and print module name)

As suggested by Dave:

This patch adds a function to get the driver name from a struct net_device,
and consequently uses this in the watchdog timeout handler to print as 
part of the message. To make this useful, use WARN_ONCE() so that the
message is part of the ===[ cut here ]=== section, meaning that people
are more likely to report it, and automated warning collection will pick it up.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 include/linux/netdevice.h |    3 +++
 net/core/dev.c            |   22 +++++++++++++++++++---
 net/sched/sch_generic.c   |    9 +++++----
 3 files changed, 27 insertions(+), 7 deletions(-)

Index: linux.trees.git/include/linux/netdevice.h
===================================================================
--- linux.trees.git.orig/include/linux/netdevice.h
+++ linux.trees.git/include/linux/netdevice.h
@@ -1514,6 +1514,9 @@ extern void dev_seq_stop(struct seq_file
 extern int netdev_class_create_file(struct class_attribute *class_attr);
 extern void netdev_class_remove_file(struct class_attribute *class_attr);
 
+extern void netdev_drivername(struct net_device *dev, char *buffer, int len);
+
+
 extern void linkwatch_run_queue(void);
 
 extern int netdev_compute_features(unsigned long all, unsigned long one);
Index: linux.trees.git/net/core/dev.c
===================================================================
--- linux.trees.git.orig/net/core/dev.c
+++ linux.trees.git/net/core/dev.c
@@ -3706,10 +3706,8 @@ static void rollback_registered(struct n
 
 	/* Some devices call without registering for initialization unwind. */
 	if (dev->reg_state == NETREG_UNINITIALIZED) {
-		printk(KERN_DEBUG "unregister_netdevice: device %s/%p never "
+		WARN(1, KERN_DEBUG "unregister_netdevice: device %s/%p never "
 				  "was registered\n", dev->name, dev);
-
-		WARN_ON(1);
 		return;
 	}
 
@@ -4554,6 +4552,24 @@ err_name:
 	return -ENOMEM;
 }
 
+void netdev_drivername(struct net_device *dev, char *buffer, int len)
+{
+	struct device_driver *driver;
+	struct device *parent;
+	if (len <= 0)
+		return;
+	buffer[0] = 0;
+
+	parent = dev->dev.parent;
+
+	if (!parent)
+		return;
+
+	driver = parent->driver;
+	if (driver && driver->name)
+		strlcpy(buffer, driver->name, len);
+}
+
 static void __net_exit netdev_exit(struct net *net)
 {
 	kfree(net->dev_name_head);
Index: linux.trees.git/net/sched/sch_generic.c
===================================================================
--- linux.trees.git.orig/net/sched/sch_generic.c
+++ linux.trees.git/net/sched/sch_generic.c
@@ -215,11 +215,12 @@ static void dev_watchdog(unsigned long a
 		    netif_carrier_ok(dev)) {
 			if (netif_queue_stopped(dev) &&
 			    time_after(jiffies, dev->trans_start + dev->watchdog_timeo)) {
-
-				printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n",
-				       dev->name);
+				char drivername[64];
+				netdev_drivername(dev, drivername, 64);
+				WARN_ONCE(1, KERN_INFO
+					"NETDEV WATCHDOG: %s (%s): transmit timed out\n",
+					dev->name, drivername);
 				dev->tx_timeout(dev);
-				WARN_ON_ONCE(1);
 			}
 			if (!mod_timer(&dev->watchdog_timer, round_jiffies(jiffies + dev->watchdog_timeo)))
 				dev_hold(dev);

Re: Printing the driver name as part of the netdev watchdog message

[David Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Sun, 6 Jul 2008 13:08:01 -0700

> Does this patch look like the right approach for achieving the goal?

It's almost there.

You can issue an ethtool_ops->get_drvinfo() call, on devices which
support ethtool, and fetch the driver info from there.

Your current method of obtaining this info can be used as the
fallback when the ethtool op fails.

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Sun, 06 Jul 2008 15:53:18 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Arjan van de Ven <arjan@infradead.org>
> Date: Sun, 6 Jul 2008 13:08:01 -0700
> 
> > Does this patch look like the right approach for achieving the goal?
> 
> It's almost there.
> 
> You can issue an ethtool_ops->get_drvinfo() call, on devices which
> support ethtool, and fetch the driver info from there.
> 
> Your current method of obtaining this info can be used as the
> fallback when the ethtool op fails.

fair enough...

how about this then?
(still leaves the logistics question between when WARN() hits the tree and when this patch go)




From: Arjan van de Ven <arjan@linux.intel.com>
Subject: Use WARN_ONCE() in the netdev timeout handler (and print module name)

As suggested by Dave:

This patch adds a function to get the driver name from a struct net_device,
and consequently uses this in the watchdog timeout handler to print as 
part of the message. To make this useful, use WARN_ONCE() so that the
message is part of the ===[ cut here ]=== section, meaning that people
are more likely to report it, and automated warning collection will pick it up.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 include/linux/netdevice.h |    3 +++
 net/core/dev.c            |   34 +++++++++++++++++++++++++++++++---
 net/sched/sch_generic.c   |    9 +++++----
 3 files changed, 39 insertions(+), 7 deletions(-)

Index: linux.trees.git/include/linux/netdevice.h
===================================================================
--- linux.trees.git.orig/include/linux/netdevice.h
+++ linux.trees.git/include/linux/netdevice.h
@@ -1514,6 +1514,9 @@ extern void dev_seq_stop(struct seq_file
 extern int netdev_class_create_file(struct class_attribute *class_attr);
 extern void netdev_class_remove_file(struct class_attribute *class_attr);
 
+extern void netdev_drivername(struct net_device *dev, char *buffer, int len);
+
+
 extern void linkwatch_run_queue(void);
 
 extern int netdev_compute_features(unsigned long all, unsigned long one);
Index: linux.trees.git/net/core/dev.c
===================================================================
--- linux.trees.git.orig/net/core/dev.c
+++ linux.trees.git/net/core/dev.c
@@ -3706,10 +3706,8 @@ static void rollback_registered(struct n
 
 	/* Some devices call without registering for initialization unwind. */
 	if (dev->reg_state == NETREG_UNINITIALIZED) {
-		printk(KERN_DEBUG "unregister_netdevice: device %s/%p never "
+		WARN(1, KERN_DEBUG "unregister_netdevice: device %s/%p never "
 				  "was registered\n", dev->name, dev);
-
-		WARN_ON(1);
 		return;
 	}
 
@@ -4554,6 +4552,36 @@ err_name:
 	return -ENOMEM;
 }
 
+void netdev_drivername(struct net_device *dev, char *buffer, int len)
+{
+	struct device_driver *driver;
+	struct device *parent;
+	struct ethtool_drvinfo info;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (len <= 0)
+		return;
+	buffer[0] = 0;
+
+	if (ops->get_drvinfo) {
+		memset(&info, 0, sizeof(info));
+		info.cmd = ETHTOOL_GDRVINFO;
+		ops->get_drvinfo(dev, &info);
+		strlcpy(buffer, info.driver, len);
+		if (strlen(info.driver) > 0)
+			return;
+	}
+
+	parent = dev->dev.parent;
+
+	if (!parent)
+		return;
+
+	driver = parent->driver;
+	if (driver && driver->name)
+		strlcpy(buffer, driver->name, len);
+}
+
 static void __net_exit netdev_exit(struct net *net)
 {
 	kfree(net->dev_name_head);
Index: linux.trees.git/net/sched/sch_generic.c
===================================================================
--- linux.trees.git.orig/net/sched/sch_generic.c
+++ linux.trees.git/net/sched/sch_generic.c
@@ -215,11 +215,12 @@ static void dev_watchdog(unsigned long a
 		    netif_carrier_ok(dev)) {
 			if (netif_queue_stopped(dev) &&
 			    time_after(jiffies, dev->trans_start + dev->watchdog_timeo)) {
-
-				printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n",
-				       dev->name);
+				char drivername[64];
+				netdev_drivername(dev, drivername, 64);
+				WARN_ONCE(1, KERN_INFO
+					"NETDEV WATCHDOG: %s (%s): transmit timed out\n",
+					dev->name, drivername);
 				dev->tx_timeout(dev);
-				WARN_ON_ONCE(1);
 			}
 			if (!mod_timer(&dev->watchdog_timer, round_jiffies(jiffies + dev->watchdog_timeo)))
 				dev_hold(dev);



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: Printing the driver name as part of the netdev watchdog message

[Stephen Hemminger]

On Sun, 06 Jul 2008 15:53:18 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Arjan van de Ven <arjan@infradead.org>
> Date: Sun, 6 Jul 2008 13:08:01 -0700
> 
> > Does this patch look like the right approach for achieving the goal?
> 
> It's almost there.
> 
> You can issue an ethtool_ops->get_drvinfo() call, on devices which
> support ethtool, and fetch the driver info from there.
> 
> Your current method of obtaining this info can be used as the
> fallback when the ethtool op fails.

Maybe not. For consistency each call through ethtool_ops should
be holding rtnl mutex. And since dev_watchdog is a timer routine,
it is not safe to acquire a mutex there.

Re: Printing the driver name as part of the netdev watchdog message

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Sun, 6 Jul 2008 18:08:42 -0700

> For consistency each call through ethtool_ops should
> be holding rtnl mutex. And since dev_watchdog is a timer routine,
> it is not safe to acquire a mutex there.

I think "get driver info", which does nothing but copy strings out of
the driver software state, is a safe exception to these strict locking
rules.

Don't you think? :-)

Re: Printing the driver name as part of the netdev watchdog message

[Wang Chen]

Arjan van de Ven said the following on 2008-7-7 7:56:
> +void netdev_drivername(struct net_device *dev, char *buffer, int len)
> +{
> +	struct device_driver *driver;
> +	struct device *parent;
> +	struct ethtool_drvinfo info;
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +
> +	if (len <= 0)
> +		return;
> +	buffer[0] = 0;
> +
> +	if (ops->get_drvinfo) {

if (ops && ops->get_drvinfo)

> +		memset(&info, 0, sizeof(info));
> +		info.cmd = ETHTOOL_GDRVINFO;
> +		ops->get_drvinfo(dev, &info);
> +		strlcpy(buffer, info.driver, len);
> +		if (strlen(info.driver) > 0)
> +			return;
> +	}

seems the logic should be:
if (strlen(info.driver) > 0) {
	strlcpy(buffer, info.driver, len);
	return;
}

Re: Printing the driver name as part of the netdev watchdog message

[Jeff Garzik]

David Miller wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Sun, 6 Jul 2008 18:08:42 -0700
> 
>> For consistency each call through ethtool_ops should
>> be holding rtnl mutex. And since dev_watchdog is a timer routine,
>> it is not safe to acquire a mutex there.
> 
> I think "get driver info", which does nothing but copy strings out of
> the driver software state, is a safe exception to these strict locking
> rules.
> 
> Don't you think? :-)

Indeed...

	Jeff

Re: Printing the driver name as part of the netdev watchdog message

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Mon, 07 Jul 2008 09:51:30 +0800

> Arjan van de Ven said the following on 2008-7-7 7:56:
> > +void netdev_drivername(struct net_device *dev, char *buffer, int len)
> > +{
> > +	struct device_driver *driver;
> > +	struct device *parent;
> > +	struct ethtool_drvinfo info;
> > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > +
> > +	if (len <= 0)
> > +		return;
> > +	buffer[0] = 0;
> > +
> > +	if (ops->get_drvinfo) {
> 
> if (ops && ops->get_drvinfo)

That's right, ethtool_ops could indeed be NULL.  It is not
a mandatory facility.

> > +		memset(&info, 0, sizeof(info));
> > +		info.cmd = ETHTOOL_GDRVINFO;
> > +		ops->get_drvinfo(dev, &info);
> > +		strlcpy(buffer, info.driver, len);
> > +		if (strlen(info.driver) > 0)
> > +			return;
> > +	}
> 
> seems the logic should be:
> if (strlen(info.driver) > 0) {
> 	strlcpy(buffer, info.driver, len);
> 	return;
> }

Also correct.

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Sun, 06 Jul 2008 20:59:05 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> 
> Also correct.

ok updated version below; I also added an explicit comment for the
ethtool locking

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: Use WARN_ONCE() in the netdev timeout handler (and print module name)

As suggested by Dave:

This patch adds a function to get the driver name from a struct net_device,
and consequently uses this in the watchdog timeout handler to print as 
part of the message. To make this useful, use WARN_ONCE() so that the
message is part of the ===[ cut here ]=== section, meaning that people
are more likely to report it, and automated warning collection will pick it up.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 include/linux/netdevice.h |    3 +++
 net/core/dev.c            |   41 ++++++++++++++++++++++++++++++++++++++---
 net/sched/sch_generic.c   |    9 +++++----
 3 files changed, 46 insertions(+), 7 deletions(-)

Index: linux.trees.git/include/linux/netdevice.h
===================================================================
--- linux.trees.git.orig/include/linux/netdevice.h
+++ linux.trees.git/include/linux/netdevice.h
@@ -1514,6 +1514,9 @@ extern void dev_seq_stop(struct seq_file
 extern int netdev_class_create_file(struct class_attribute *class_attr);
 extern void netdev_class_remove_file(struct class_attribute *class_attr);
 
+extern void netdev_drivername(struct net_device *dev, char *buffer, int len);
+
+
 extern void linkwatch_run_queue(void);
 
 extern int netdev_compute_features(unsigned long all, unsigned long one);
Index: linux.trees.git/net/core/dev.c
===================================================================
--- linux.trees.git.orig/net/core/dev.c
+++ linux.trees.git/net/core/dev.c
@@ -3706,10 +3706,8 @@ static void rollback_registered(struct n
 
 	/* Some devices call without registering for initialization unwind. */
 	if (dev->reg_state == NETREG_UNINITIALIZED) {
-		printk(KERN_DEBUG "unregister_netdevice: device %s/%p never "
+		WARN(1, KERN_DEBUG "unregister_netdevice: device %s/%p never "
 				  "was registered\n", dev->name, dev);
-
-		WARN_ON(1);
 		return;
 	}
 
@@ -4554,6 +4552,43 @@ err_name:
 	return -ENOMEM;
 }
 
+void netdev_drivername(struct net_device *dev, char *buffer, int len)
+{
+	struct device_driver *driver;
+	struct device *parent;
+	struct ethtool_drvinfo info;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (len <= 0)
+		return;
+	buffer[0] = 0;
+
+	/*
+	 * Note: in principle ethtool ops need to be called
+	 * with the RTNL mutex held, while this function is called
+	 * from IRQ context. However, get_drvinfo is a special exception
+	 * (confirmed by Dave and Jeff) and doesn't need the mutex.
+	 */
+	if (ops && ops->get_drvinfo) {
+		memset(&info, 0, sizeof(info));
+		info.cmd = ETHTOOL_GDRVINFO;
+		ops->get_drvinfo(dev, &info);
+		if (strlen(info.driver) > 0) {
+			strlcpy(buffer, info.driver, len);
+			return;
+		}
+	}
+
+	parent = dev->dev.parent;
+
+	if (!parent)
+		return;
+
+	driver = parent->driver;
+	if (driver && driver->name)
+		strlcpy(buffer, driver->name, len);
+}
+
 static void __net_exit netdev_exit(struct net *net)
 {
 	kfree(net->dev_name_head);
Index: linux.trees.git/net/sched/sch_generic.c
===================================================================
--- linux.trees.git.orig/net/sched/sch_generic.c
+++ linux.trees.git/net/sched/sch_generic.c
@@ -215,11 +215,12 @@ static void dev_watchdog(unsigned long a
 		    netif_carrier_ok(dev)) {
 			if (netif_queue_stopped(dev) &&
 			    time_after(jiffies, dev->trans_start + dev->watchdog_timeo)) {
-
-				printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n",
-				       dev->name);
+				char drivername[64];
+				netdev_drivername(dev, drivername, 64);
+				WARN_ONCE(1, KERN_INFO
+					"NETDEV WATCHDOG: %s (%s): transmit timed out\n",
+					dev->name, drivername);
 				dev->tx_timeout(dev);
-				WARN_ON_ONCE(1);
 			}
 			if (!mod_timer(&dev->watchdog_timer, round_jiffies(jiffies + dev->watchdog_timeo)))
 				dev_hold(dev);

Re: Printing the driver name as part of the netdev watchdog message

[David Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Sun, 6 Jul 2008 21:34:27 -0700

> On Sun, 06 Jul 2008 20:59:05 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > 
> > Also correct.
> 
> ok updated version below; I also added an explicit comment for the
> ethtool locking
> 
> From: Arjan van de Ven <arjan@linux.intel.com>
> Subject: Use WARN_ONCE() in the netdev timeout handler (and print module name)

I'm basically fine with these changes.

But plain WARN() and WARN_ONCE() are not in any tree I am maintaining
at the moment, so I really can't add it to net-next-2.6 or net-2.6
even if I wanted to.

Do you really need this dependency?

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Sun, 06 Jul 2008 21:49:15 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Arjan van de Ven <arjan@infradead.org>
> Date: Sun, 6 Jul 2008 21:34:27 -0700
> 
> > On Sun, 06 Jul 2008 20:59:05 -0700 (PDT)
> > David Miller <davem@davemloft.net> wrote:
> > 
> > > 
> > > Also correct.
> > 
> > ok updated version below; I also added an explicit comment for the
> > ethtool locking
> > 
> > From: Arjan van de Ven <arjan@linux.intel.com>
> > Subject: Use WARN_ONCE() in the netdev timeout handler (and print
> > module name)
> 
> I'm basically fine with these changes.
> 
> But plain WARN() and WARN_ONCE() are not in any tree I am maintaining
> at the moment, so I really can't add it to net-next-2.6 or net-2.6
> even if I wanted to.

one option is that I carry this patch in the WARN tree (which I plan to
submit for -next tomorrow)
> 
> Do you really need this dependency?

Well I could split it in two, first add this just to the printk
and then have a second patch to turn it into a WARN().

I do need the later; the reason is lame but simple: the kerneloops
collection client, for privacy reasons, only sends the actual warning,
not random printk's around it. WARN() puts the printk inside the
warning so it'll be picked up as part of the whole, rather than not
being picked up as something probably unrelated.



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: Printing the driver name as part of the netdev watchdog message

[David Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Sun, 6 Jul 2008 21:57:11 -0700

> one option is that I carry this patch in the WARN tree (which I plan to
> submit for -next tomorrow)

It's going to be a conflict field day with net-next-2.6 especially
with some changes I have coming in there soon, otherwise I'd say
that yes that would be the way to do.

We're going to have to do this in multiple stages somehow.

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Sun, 06 Jul 2008 23:44:33 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Arjan van de Ven <arjan@infradead.org>
> Date: Sun, 6 Jul 2008 21:57:11 -0700
> 
> > one option is that I carry this patch in the WARN tree (which I
> > plan to submit for -next tomorrow)
> 
> It's going to be a conflict field day with net-next-2.6 especially
> with some changes I have coming in there soon, otherwise I'd say
> that yes that would be the way to do.

ok

if you merge the piece below (which is just the core), I can do the WARN()
part later when the dust has settled (it's a one liner anyway pretty much)

--

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: Print the driver name as part of the WATCHDOG message

As suggested by Dave:

This patch adds a function to get the driver name from a struct net_device,
and consequently uses this in the watchdog timeout handler to print as 
part of the message. 

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 include/linux/netdevice.h |    3 +++
 net/core/dev.c            |   37 +++++++++++++++++++++++++++++++++++++
 net/sched/sch_generic.c   |    7 ++++---
 3 files changed, 44 insertions(+), 3 deletions(-)

Index: linux.trees.git/include/linux/netdevice.h
===================================================================
--- linux.trees.git.orig/include/linux/netdevice.h
+++ linux.trees.git/include/linux/netdevice.h
@@ -1514,6 +1514,9 @@ extern void dev_seq_stop(struct seq_file
 extern int netdev_class_create_file(struct class_attribute *class_attr);
 extern void netdev_class_remove_file(struct class_attribute *class_attr);
 
+extern void netdev_drivername(struct net_device *dev, char *buffer, int len);
+
+
 extern void linkwatch_run_queue(void);
 
 extern int netdev_compute_features(unsigned long all, unsigned long one);
Index: linux.trees.git/net/core/dev.c
===================================================================
--- linux.trees.git.orig/net/core/dev.c
+++ linux.trees.git/net/core/dev.c
@@ -4554,6 +4554,43 @@ err_name:
 	return -ENOMEM;
 }
 
+void netdev_drivername(struct net_device *dev, char *buffer, int len)
+{
+	struct device_driver *driver;
+	struct device *parent;
+	struct ethtool_drvinfo info;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (len <= 0)
+		return;
+	buffer[0] = 0;
+
+	/*
+	 * Note: in principle ethtool ops need to be called
+	 * with the RTNL mutex held, while this function is called
+	 * from IRQ context. However, get_drvinfo is a special exception
+	 * (confirmed by Dave and Jeff) and doesn't need the mutex.
+	 */
+	if (ops && ops->get_drvinfo) {
+		memset(&info, 0, sizeof(info));
+		info.cmd = ETHTOOL_GDRVINFO;
+		ops->get_drvinfo(dev, &info);
+		if (strlen(info.driver) > 0) {
+			strlcpy(buffer, info.driver, len);
+			return;
+		}
+	}
+
+	parent = dev->dev.parent;
+
+	if (!parent)
+		return;
+
+	driver = parent->driver;
+	if (driver && driver->name)
+		strlcpy(buffer, driver->name, len);
+}
+
 static void __net_exit netdev_exit(struct net *net)
 {
 	kfree(net->dev_name_head);
Index: linux.trees.git/net/sched/sch_generic.c
===================================================================
--- linux.trees.git.orig/net/sched/sch_generic.c
+++ linux.trees.git/net/sched/sch_generic.c
@@ -215,9 +215,10 @@ static void dev_watchdog(unsigned long a
 		    netif_carrier_ok(dev)) {
 			if (netif_queue_stopped(dev) &&
 			    time_after(jiffies, dev->trans_start + dev->watchdog_timeo)) {
-
-				printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n",
-				       dev->name);
+				char drivername[64];
+				netdev_drivername(dev, drivername, 64);
+				printk(KERN_INFO "NETDEV WATCHDOG: %s (%s): transmit timed out\n",
+				       dev->name, drivername);
 				dev->tx_timeout(dev);
 				WARN_ON_ONCE(1);
 			}

Re: Printing the driver name as part of the netdev watchdog message

[Stephen Hemminger]

On Sun, 06 Jul 2008 21:53:24 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> David Miller wrote:
> > From: Stephen Hemminger <shemminger@vyatta.com>
> > Date: Sun, 6 Jul 2008 18:08:42 -0700
> > 
> >> For consistency each call through ethtool_ops should
> >> be holding rtnl mutex. And since dev_watchdog is a timer routine,
> >> it is not safe to acquire a mutex there.
> > 
> > I think "get driver info", which does nothing but copy strings out of
> > the driver software state, is a safe exception to these strict locking
> > rules.
> > 
> > Don't you think? :-)
> 
> Indeed...
> 

It is save exception, but documentation needed?

Re: Printing the driver name as part of the netdev watchdog message

[Roland Dreier]

 > > For consistency each call through ethtool_ops should
 > > be holding rtnl mutex. And since dev_watchdog is a timer routine,
 > > it is not safe to acquire a mutex there.
 > 
 > I think "get driver info", which does nothing but copy strings out of
 > the driver software state, is a safe exception to these strict locking
 > rules.

it seems that's not universally the case, eg get_drvinfo() in
drivers/net/cxgb3/cxgb3_main.c does

	t3_get_fw_version(adapter, &fw_vers);
	t3_get_tp_version(adapter, &tp_vers);

and from reading further in the code it's not obvious that this is safe
to do without serialization (it calls into t3_read_flash(), which does a
typical "write address reg, read data reg" sequence to access flash,
without any locking).

 - R.

Re: Printing the driver name as part of the netdev watchdog message

[David Miller]

From: Roland Dreier <rdreier@cisco.com>
Date: Mon, 07 Jul 2008 15:45:38 -0700

>  > > For consistency each call through ethtool_ops should
>  > > be holding rtnl mutex. And since dev_watchdog is a timer routine,
>  > > it is not safe to acquire a mutex there.
>  > 
>  > I think "get driver info", which does nothing but copy strings out of
>  > the driver software state, is a safe exception to these strict locking
>  > rules.
> 
> it seems that's not universally the case, eg get_drvinfo() in
> drivers/net/cxgb3/cxgb3_main.c does
> 
> 	t3_get_fw_version(adapter, &fw_vers);
> 	t3_get_tp_version(adapter, &tp_vers);
> 
> and from reading further in the code it's not obvious that this is safe
> to do without serialization (it calls into t3_read_flash(), which does a
> typical "write address reg, read data reg" sequence to access flash,
> without any locking).

I doubt it uses the RTNL semaphore elsewhere to protect
against this path, which is the only protection these
calls currently have.

Please don't bring up scarecrows, this looks like simply
a bug which already exists.

Re: Printing the driver name as part of the netdev watchdog message

[Roland Dreier]

 > I doubt it uses the RTNL semaphore elsewhere to protect
 > against this path, which is the only protection these
 > calls currently have.

As far as I can tell from reading the code, the only places in cxgb3
that use t3_read_flash() are in the netdevice's open and ioctl methods,
and the ethtool get_drvinfo method.  So as far as I can tell the current
code is fine as long as rtnl is held across get_drvinfo.

 > Please don't bring up scarecrows, this looks like simply
 > a bug which already exists.

I don't even know how to take this.  "Please don't review our changes"??
"Please don't report bugs"??

 - R.

Re: Printing the driver name as part of the netdev watchdog message

[Stephen Hemminger]

On Mon, 07 Jul 2008 16:14:31 -0700
Roland Dreier <rdreier@cisco.com> wrote:

>  > I doubt it uses the RTNL semaphore elsewhere to protect
>  > against this path, which is the only protection these
>  > calls currently have.
> 
> As far as I can tell from reading the code, the only places in cxgb3
> that use t3_read_flash() are in the netdevice's open and ioctl methods,
> and the ethtool get_drvinfo method.  So as far as I can tell the current
> code is fine as long as rtnl is held across get_drvinfo.
> 
>  > Please don't bring up scarecrows, this looks like simply
>  > a bug which already exists.
> 
> I don't even know how to take this.  "Please don't review our changes"??
> "Please don't report bugs"??
> 
>  - R.

Long term, it would be good to move the driver name into device struct?
Easier and could work for all those random other drivers :-)

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Mon, 7 Jul 2008 16:44:25 -0700
Stephen Hemminger <shemminger@vyatta.com> wrote:

> On Mon, 07 Jul 2008 16:14:31 -0700
> Roland Dreier <rdreier@cisco.com> wrote:
> 
> >  > I doubt it uses the RTNL semaphore elsewhere to protect
> >  > against this path, which is the only protection these
> >  > calls currently have.
> > 
> > As far as I can tell from reading the code, the only places in cxgb3
> > that use t3_read_flash() are in the netdevice's open and ioctl
> > methods, and the ethtool get_drvinfo method.  So as far as I can
> > tell the current code is fine as long as rtnl is held across
> > get_drvinfo.
> > 
> >  > Please don't bring up scarecrows, this looks like simply
> >  > a bug which already exists.
> > 
> > I don't even know how to take this.  "Please don't review our
> > changes"?? "Please don't report bugs"??
> > 
> >  - R.
> 
> Long term, it would be good to move the driver name into device
> struct? Easier and could work for all those random other drivers :-)

well we can already get it from the object model ;)
Dave just preferred to get it from ethtool and only get it from object
model if ethtool didn't work out.. (which makes sense)

I can see the point almost of having a generic_ethtool_getdrv which
just fills the struct from the object model data ;)



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: Printing the driver name as part of the netdev watchdog message

[Steve Wise]

Roland Dreier wrote:
>  > I doubt it uses the RTNL semaphore elsewhere to protect
>  > against this path, which is the only protection these
>  > calls currently have.
>
> As far as I can tell from reading the code, the only places in cxgb3
> that use t3_read_flash() are in the netdevice's open and ioctl methods,
> and the ethtool get_drvinfo method.  So as far as I can tell the current
> code is fine as long as rtnl is held across get_drvinfo.
>
>  > Please don't bring up scarecrows, this looks like simply
>  > a bug which already exists.
>
> I don't even know how to take this.  "Please don't review our changes"??
> "Please don't report bugs"??
>
>   

So what is the conclusion here?  If the rtnl doesn't need to be held 
around calls to ethtool ops, then cxgb3 is broken.  In addition, the 
iw_cxgb3 driver is  currently broken too because a recent commit 
(f4e91eb4a81559da87a3843758a641b5cc590b65) accidentally removed 
acquiring of the rtnl before calling an ethtool op in cxgb3.  I want to 
resolve this.

So should I put the rtnl acquisitions back in iw_cxgb3?  Or fix cxgb3 to 
not assume rtnl is held?


Steve.

Re: Printing the driver name as part of the netdev watchdog message

[David Miller]

From: Steve Wise <swise@opengridcomputing.com>
Date: Tue, 08 Jul 2008 14:13:43 -0500

> So should I put the rtnl acquisitions back in iw_cxgb3?  Or fix cxgb3 to 
> not assume rtnl is held?

It appears that the kernel always invokes the ethtool ops with
RTNL held, and therefore that's the environment that needs
to be ensured.

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Tue, 08 Jul 2008 14:31:58 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Steve Wise <swise@opengridcomputing.com>
> Date: Tue, 08 Jul 2008 14:13:43 -0500
> 
> > So should I put the rtnl acquisitions back in iw_cxgb3?  Or fix
> > cxgb3 to not assume rtnl is held?
> 
> It appears that the kernel always invokes the ethtool ops with
> RTNL held, and therefore that's the environment that needs
> to be ensured.

so for the patch that started all this... do I need to go back to the
original where I didn't use ethool?
(since this is called from irq context....)



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: Printing the driver name as part of the netdev watchdog message

[David Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Tue, 8 Jul 2008 14:47:25 -0700

> On Tue, 08 Jul 2008 14:31:58 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Steve Wise <swise@opengridcomputing.com>
> > Date: Tue, 08 Jul 2008 14:13:43 -0500
> > 
> > > So should I put the rtnl acquisitions back in iw_cxgb3?  Or fix
> > > cxgb3 to not assume rtnl is held?
> > 
> > It appears that the kernel always invokes the ethtool ops with
> > RTNL held, and therefore that's the environment that needs
> > to be ensured.
> 
> so for the patch that started all this... do I need to go back to the
> original where I didn't use ethool?
> (since this is called from irq context....)

What we need instead is to cache the info block into the netdev struct
when the driver is ->open()'d, and then you can fetch it out of
there however you like.

Or, at least, that is one possible approach.

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Tue, 08 Jul 2008 14:57:38 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> 
> What we need instead is to cache the info block into the netdev struct
> when the driver is ->open()'d, and then you can fetch it out of
> there however you like.

but.. isn't that like almost the same as using the object model data at
that point?

> 
> Or, at least, that is one possible approach.

I'll look at it; but this begs the question if this shouldn't turned
inside out.
I mean... if we had a "netdev_set_drivername()" thing with appropriate
arguments (well I suck at names, name it whatever you feel like), we
wouldn't need the drivers to implement each their own ethtool method,
since this could just be done in one place and pull the data from the
netdev.

(for the eeprom etc data that's different, but those are already
different ethtool methods last I looked)


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: Printing the driver name as part of the netdev watchdog message

[David Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Tue, 8 Jul 2008 16:48:26 -0700

> On Tue, 08 Jul 2008 14:57:38 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > What we need instead is to cache the info block into the netdev struct
> > when the driver is ->open()'d, and then you can fetch it out of
> > there however you like.
> 
> but.. isn't that like almost the same as using the object model data at
> that point?

You're right, this is getting silly

> I mean... if we had a "netdev_set_drivername()" thing with appropriate
> arguments (well I suck at names, name it whatever you feel like), we
> wouldn't need the drivers to implement each their own ethtool method,
> since this could just be done in one place and pull the data from the
> netdev.
> 
> (for the eeprom etc data that's different, but those are already
> different ethtool methods last I looked)

To be honest, the more I think about this, the driver->name should
be sufficient.

I just checked a bunch of PCI drivers and they provide the
same value for pci_driver->name as ethtool's info->driver

Sure, the ethtool info thing has a driver version etc. but
for your purposes that really doesn't add much.

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Tue, 08 Jul 2008 16:53:04 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Arjan van de Ven <arjan@infradead.org>
> Date: Tue, 8 Jul 2008 16:48:26 -0700
> 
> > On Tue, 08 Jul 2008 14:57:38 -0700 (PDT)
> > David Miller <davem@davemloft.net> wrote:
> > 
> > > What we need instead is to cache the info block into the netdev
> > > struct when the driver is ->open()'d, and then you can fetch it
> > > out of there however you like.
> > 
> > but.. isn't that like almost the same as using the object model
> > data at that point?
> 
> You're right, this is getting silly
> 
> > I mean... if we had a "netdev_set_drivername()" thing with
> > appropriate arguments (well I suck at names, name it whatever you
> > feel like), we wouldn't need the drivers to implement each their
> > own ethtool method, since this could just be done in one place and
> > pull the data from the netdev.
> > 
> > (for the eeprom etc data that's different, but those are already
> > different ethtool methods last I looked)
> 
> To be honest, the more I think about this, the driver->name should
> be sufficient.
> 
> I just checked a bunch of PCI drivers and they provide the
> same value for pci_driver->name as ethtool's info->driver
> 
> Sure, the ethtool info thing has a driver version etc. but
> for your purposes that really doesn't add much.

ok I'll get you a patch after dinner.



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Tue, 08 Jul 2008 16:53:04 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Arjan van de Ven <arjan@infradead.org>
> Date: Tue, 8 Jul 2008 16:48:26 -0700
> 
> > On Tue, 08 Jul 2008 14:57:38 -0700 (PDT)
> > David Miller <davem@davemloft.net> wrote:
> > 
> > > What we need instead is to cache the info block into the netdev
> > > struct when the driver is ->open()'d, and then you can fetch it
> > > out of there however you like.
> > 
> > but.. isn't that like almost the same as using the object model
> > data at that point?
> 
> You're right, this is getting silly
> 
> To be honest, the more I think about this, the driver->name should
> be sufficient.
> 

ok here it is:

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: Print the module name as part of the watchdog message

As suggested by Dave:

This patch adds a function to get the driver name from a struct net_device,
and consequently uses this in the watchdog timeout handler to print as 
part of the message. 

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 include/linux/netdevice.h |    2 ++
 net/core/dev.c            |   19 +++++++++++++++++++
 net/sched/sch_generic.c   |    7 ++++---
 3 files changed, 25 insertions(+), 3 deletions(-)

Index: linux.trees.git/include/linux/netdevice.h
===================================================================
--- linux.trees.git.orig/include/linux/netdevice.h
+++ linux.trees.git/include/linux/netdevice.h
@@ -1516,6 +1516,8 @@ extern void dev_seq_stop(struct seq_file
 extern int netdev_class_create_file(struct class_attribute *class_attr);
 extern void netdev_class_remove_file(struct class_attribute *class_attr);
 
+extern void netdev_drivername(struct net_device *dev, char *buffer, int len);
+
 extern void linkwatch_run_queue(void);
 
 extern int netdev_compute_features(unsigned long all, unsigned long one);
Index: linux.trees.git/net/core/dev.c
===================================================================
--- linux.trees.git.orig/net/core/dev.c
+++ linux.trees.git/net/core/dev.c
@@ -4554,6 +4554,25 @@ err_name:
 	return -ENOMEM;
 }
 
+void netdev_drivername(struct net_device *dev, char *buffer, int len)
+{
+	struct device_driver *driver;
+	struct device *parent;
+
+	if (len <= 0)
+		return;
+	buffer[0] = 0;
+
+	parent = dev->dev.parent;
+
+	if (!parent)
+		return;
+
+	driver = parent->driver;
+	if (driver && driver->name)
+		strlcpy(buffer, driver->name, len);
+}
+
 static void __net_exit netdev_exit(struct net *net)
 {
 	kfree(net->dev_name_head);
Index: linux.trees.git/net/sched/sch_generic.c
===================================================================
--- linux.trees.git.orig/net/sched/sch_generic.c
+++ linux.trees.git/net/sched/sch_generic.c
@@ -215,9 +215,10 @@ static void dev_watchdog(unsigned long a
 		    netif_carrier_ok(dev)) {
 			if (netif_queue_stopped(dev) &&
 			    time_after(jiffies, dev->trans_start + dev->watchdog_timeo)) {
-
-				printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n",
-				       dev->name);
+				char drivername[64];
+				netdev_drivername(dev, drivername, 64);
+				printk(KERN_INFO "NETDEV WATCHDOG: %s (%s): transmit timed out\n",
+				       dev->name, drivername);
 				dev->tx_timeout(dev);
 				WARN_ON_ONCE(1);
 			}

Re: Printing the driver name as part of the netdev watchdog message

[Stephen Hemminger]

On Tue, 8 Jul 2008 18:44:56 -0700
Arjan van de Ven <arjan@infradead.org> wrote:

> On Tue, 08 Jul 2008 16:53:04 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Arjan van de Ven <arjan@infradead.org>
> > Date: Tue, 8 Jul 2008 16:48:26 -0700
> > 
> > > On Tue, 08 Jul 2008 14:57:38 -0700 (PDT)
> > > David Miller <davem@davemloft.net> wrote:
> > > 
> > > > What we need instead is to cache the info block into the netdev
> > > > struct when the driver is ->open()'d, and then you can fetch it
> > > > out of there however you like.
> > > 
> > > but.. isn't that like almost the same as using the object model
> > > data at that point?
> > 
> > You're right, this is getting silly
> > 
> > To be honest, the more I think about this, the driver->name should
> > be sufficient.
> > 
> 
> ok here it is:
> 
> From: Arjan van de Ven <arjan@linux.intel.com>
> Subject: Print the module name as part of the watchdog message
> 
> As suggested by Dave:
> 
> This patch adds a function to get the driver name from a struct net_device,
> and consequently uses this in the watchdog timeout handler to print as 
> part of the message. 
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  include/linux/netdevice.h |    2 ++
>  net/core/dev.c            |   19 +++++++++++++++++++
>  net/sched/sch_generic.c   |    7 ++++---
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> Index: linux.trees.git/include/linux/netdevice.h
> ===================================================================
> --- linux.trees.git.orig/include/linux/netdevice.h
> +++ linux.trees.git/include/linux/netdevice.h
> @@ -1516,6 +1516,8 @@ extern void dev_seq_stop(struct seq_file
>  extern int netdev_class_create_file(struct class_attribute *class_attr);
>  extern void netdev_class_remove_file(struct class_attribute *class_attr);
>  
> +extern void netdev_drivername(struct net_device *dev, char *buffer, int len);
> +
>  extern void linkwatch_run_queue(void);
>  
>  extern int netdev_compute_features(unsigned long all, unsigned long one);
> Index: linux.trees.git/net/core/dev.c
> ===================================================================
> --- linux.trees.git.orig/net/core/dev.c
> +++ linux.trees.git/net/core/dev.c
> @@ -4554,6 +4554,25 @@ err_name:
>  	return -ENOMEM;
>  }
>  
> +void netdev_drivername(struct net_device *dev, char *buffer, int len)

void netdev_drivername(const struct net_device *dev, char *buffer, int len)

since net device not changed.

Re: Printing the driver name as part of the netdev watchdog message

[Joe Perches]

On Tue, 2008-07-08 at 20:16 -0700, Stephen Hemminger wrote:
> > +void netdev_drivername(struct net_device *dev, char *buffer, int len)
> void netdev_drivername(const struct net_device *dev, char *buffer, int len)
> since net device not changed.

char *netdev_drivername(const struct net_dev *dev, char *buffer, size_t len)

size_t len and returns *buffer

allows:

	char drivername[64];
        printk(KERN_INFO "NETDEV WATCHDOG: %s (%s): transmit timed out\n",
               dev->name, netdev_drivername(dev, drivername, sizeof(drivername));

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Wed, 09 Jul 2008 10:20:55 -0700
Joe Perches <joe@perches.com> wrote:

> On Tue, 2008-07-08 at 20:16 -0700, Stephen Hemminger wrote:
> > > +void netdev_drivername(struct net_device *dev, char *buffer, int
> > > len)
> > void netdev_drivername(const struct net_device *dev, char *buffer,
> > int len) since net device not changed.
> 
> char *netdev_drivername(const struct net_dev *dev, char *buffer,
> size_t len)
> 
> size_t len and returns *buffer
> 
> allows:
> 
> 	char drivername[64];
>         printk(KERN_INFO "NETDEV WATCHDOG: %s (%s): transmit timed
> out\n", dev->name, netdev_drivername(dev, drivername,
> sizeof(drivername));

I like the return a char * (updated patch below)
I don't like the size_t... size_t is an abstraction to deal with file
sizes.... but there's nothing else wrong with using an int for numbers.
thanks for the suggestion


From: Arjan van de Ven <arjan@linux.intel.com>
Subject: Print the module name as part of the watchdog message

As suggested by Dave:

This patch adds a function to get the driver name from a struct net_device,
and consequently uses this in the watchdog timeout handler to print as 
part of the message. 

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 include/linux/netdevice.h |    2 ++
 net/core/dev.c            |   20 ++++++++++++++++++++
 net/sched/sch_generic.c   |    6 +++---
 3 files changed, 25 insertions(+), 3 deletions(-)

Index: linux.trees.git/include/linux/netdevice.h
===================================================================
--- linux.trees.git.orig/include/linux/netdevice.h
+++ linux.trees.git/include/linux/netdevice.h
@@ -1516,6 +1516,8 @@ extern void dev_seq_stop(struct seq_file
 extern int netdev_class_create_file(struct class_attribute *class_attr);
 extern void netdev_class_remove_file(struct class_attribute *class_attr);
 
+extern char *netdev_drivername(struct net_device *dev, char *buffer, int len);
+
 extern void linkwatch_run_queue(void);
 
 extern int netdev_compute_features(unsigned long all, unsigned long one);
Index: linux.trees.git/net/core/dev.c
===================================================================
--- linux.trees.git.orig/net/core/dev.c
+++ linux.trees.git/net/core/dev.c
@@ -4554,6 +4554,26 @@ err_name:
 	return -ENOMEM;
 }
 
+char *netdev_drivername(struct net_device *dev, char *buffer, int len)
+{
+	struct device_driver *driver;
+	struct device *parent;
+
+	if (len <= 0 || !buffer)
+		return buffer;
+	buffer[0] = 0;
+
+	parent = dev->dev.parent;
+
+	if (!parent)
+		return buffer;
+
+	driver = parent->driver;
+	if (driver && driver->name)
+		strlcpy(buffer, driver->name, len);
+	return buffer;
+}
+
 static void __net_exit netdev_exit(struct net *net)
 {
 	kfree(net->dev_name_head);
Index: linux.trees.git/net/sched/sch_generic.c
===================================================================
--- linux.trees.git.orig/net/sched/sch_generic.c
+++ linux.trees.git/net/sched/sch_generic.c
@@ -215,9 +215,9 @@ static void dev_watchdog(unsigned long a
 		    netif_carrier_ok(dev)) {
 			if (netif_queue_stopped(dev) &&
 			    time_after(jiffies, dev->trans_start + dev->watchdog_timeo)) {
-
-				printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n",
-				       dev->name);
+				char drivername[64];
+				printk(KERN_INFO "NETDEV WATCHDOG: %s (%s): transmit timed out\n",
+				       dev->name, netdev_drivername(dev, drivername, 64));
 				dev->tx_timeout(dev);
 				WARN_ON_ONCE(1);
 			}

Re: Printing the driver name as part of the netdev watchdog message

[Joe Perches]

On Wed, 2008-07-09 at 10:56 -0700, Arjan van de Ven wrote:
> I don't like the size_t... size_t is an abstraction to deal with file
> sizes.... but there's nothing else wrong with using an int for numbers.

<pedant_mode>

o size_t is an abstraction to deal with arbitrary sizes
o an unnecessary <= 0 test exists
o using sizeof(drivername) reduces a dependency on a magic number
o netdev_drivername calls strlcpy with implicit cast to size_t

$ grep strlcpy include/linux/string.h
size_t strlcpy(char *, const char *, size_t);

Your choice.  Cheers, Joe

Re: Printing the driver name as part of the netdev watchdog message

[Ben Hutchings]

Arjan van de Ven wrote:
> On Wed, 09 Jul 2008 10:20:55 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > On Tue, 2008-07-08 at 20:16 -0700, Stephen Hemminger wrote:
> > > > +void netdev_drivername(struct net_device *dev, char *buffer, int
> > > > len)
> > > void netdev_drivername(const struct net_device *dev, char *buffer,
> > > int len) since net device not changed.
> > 
> > char *netdev_drivername(const struct net_dev *dev, char *buffer,
> > size_t len)
> > 
> > size_t len and returns *buffer
> > 
> > allows:
> > 
> > 	char drivername[64];
> >         printk(KERN_INFO "NETDEV WATCHDOG: %s (%s): transmit timed
> > out\n", dev->name, netdev_drivername(dev, drivername,
> > sizeof(drivername));
> 
> I like the return a char * (updated patch below)
> I don't like the size_t... size_t is an abstraction to deal with file
> sizes.... but there's nothing else wrong with using an int for numbers.
[...]

size_t is for memory sizes - that's why sizeof() expressions have that
type.  File sizes are loff_t.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: Printing the driver name as part of the netdev watchdog message

[Arjan van de Ven]

On Wed, 09 Jul 2008 11:20:49 -0700
Joe Perches <joe@perches.com> wrote:

> On Wed, 2008-07-09 at 10:56 -0700, Arjan van de Ven wrote:
> > I don't like the size_t... size_t is an abstraction to deal with
> > file sizes.... but there's nothing else wrong with using an int for
> > numbers.
> 
> <pedant_mode>
> 
> o size_t is an abstraction to deal with arbitrary sizes

it's an int. abstractions hurt. sorry.

> o an unnecessary <= 0 test exists

it's not. if it was a < test going to an unsigned makes the test go
away, but it's a <=.
And signed actually catches more bugs because unsigned makes things
silent.


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org