Re: [PATCH] mac8390: change an error return code and some cleanup, take 4
From: Finn Thain fth...@telegraphics.com.au
Date: Sat, 29 May 2010 03:29:01 +1000 (EST)
@@ -662,7 +665,7 @@ static void mac8390_no_reset(struct net_
{
ei_status.txing = 0;
if (ei_debug 1)
- pr_info(reset not supported\n);
+ printk(KERN_DEBUG pr_fmt(reset not supported\n));
return;
Use pr_debug() or pr_devel(). Anything which explicitly codes
out a printk(KERN_* is suspect, we have interfaces for this. The
whole point of defining pr_fmt at the top of the driver is so
that in the driver, we use pr_foo() which includes the pr_fmt
string at the beginning. If you bypass it you avoid the intended
effect of that pr_fmt define.
This is getting tiring Finn.
So we're already past 4 iterations of this silly simple patch, and I
want to remind you yet again what an incredibly hard time you gave Joe
Perches for his changes to this file which in the end were entirely
correct and well formed, and he got it right on his first attempt.
I advise you to think twice before snapping at another developer's
work in the future.
--
To unsubscribe from this list: send the line unsubscribe linux-m68k in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac8390: change an error return code and some cleanup, take 4
On Mon, 31 May 2010, David Miller wrote: This is getting tiring Finn. I agree. My patch addresses all of the criticism of the earlier submissions. To make it plain: there are 25 files or so that use ei_debug. Three of those that now have the KERN_DEBUG printk's suppresed by the DEBUG macro only do so as an apparently unintended side effect of a commit that claims to implement dynmic debug infrastructure. (Go figure.) http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commitdiff;h=dd0fab5b940c0b65f26ac5b01485bac1f690ace6 Your suggestion to use pr_debug is invoking compile time infrastructure (the DEBUG macro), so it is not in the spirit of this commit, and it is not relevant to any criticism from you or Joe of the earlier submissions. Please apply the patch. Finn -- To unsubscribe from this list: send the line unsubscribe linux-m68k in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac8390: change an error return code and some cleanup, take 4
On Mon, May 31, 2010 at 11:21, fth...@telegraphics.com.au wrote:
On Mon, 31 May 2010, David Miller wrote:
This is getting tiring Finn.
I agree. My patch addresses all of the criticism of the earlier
submissions.
To make it plain: there are 25 files or so that use ei_debug. Three of
those that now have the KERN_DEBUG printk's suppresed by the DEBUG macro
only do so as an apparently unintended side effect of a commit that claims
to implement dynmic debug infrastructure. (Go figure.)
http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commitdiff;h=dd0fab5b940c0b65f26ac5b01485bac1f690ace6
Your suggestion to use pr_debug is invoking compile time infrastructure
(the DEBUG macro), so it is not in the spirit of this commit, and it is
not relevant to any criticism from you or Joe of the earlier submissions.
Please apply the patch.
`pr_debug()' indeed now may generate code if DEBUG is not defined,
i.e. if CONFIG_DYNAMIC_DEBUG is enabled.
This is intented for debug infrastructure the user may want to enable later.
If you want the old behavior, you can use `pr_devel()' instead, which
only generates code if DEBUG is defined.
This is intended for debug infrastructure for developers only.
However, you used `printk(KERN_DEBUG pr_fmt()...)`, which always generates code.
I'm still not 100% sure that was intentional?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line unsubscribe linux-m68k in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac8390: change an error return code and some cleanup, take 4
From: Finn Thain fth...@telegraphics.com.au Date: Mon, 31 May 2010 22:55:23 +1000 (EST) if (ei_debug) pr_debug(...) OR if (ei_debug) pr_info(...) Well for the printk in question, it's telling the user that a certain feature can't be enabled. And if the driver has an explicit way to control this message, using ei_debug, it's kind of redundant to slap another layer on top by using the debug printk facility. Changing this to a debug log level printk is only going to make getting the debug message shown harder for the user. So leaving it at pr_info() is just as correct in my eyes. Finally, your patch has so many problems getting applied because you're doing multiple things in one patch. And in fact I've asked you not to do this on several occiaions. Fix the error return codes in one patch, and nothing more. Then in another you can masterbate with printk log levels. -- To unsubscribe from this list: send the line unsubscribe linux-m68k in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac8390: change an error return code and some cleanup, take 4
From: Joe Perches j...@perches.com Date: Mon, 31 May 2010 08:08:13 -0700 There are many uses of KERN_DEBUG that are reasonable to have always enabled. Doubtful. pr_debug() makes a ton of sense as currently implemented. It's for messages that we want both compile time and run-time control over. The case we have here in mac8390 seems like it should stay as pr_info(). Because 1) it's already controlled by a run-time knob so controlling it even further is confusing and 2) the message is informative, it lets the user know a feature cannot be enabled, thus pr_info(). -- To unsubscribe from this list: send the line unsubscribe linux-m68k in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mac8390: change an error return code and some cleanup, take 4
On Mon, 31 May 2010, David Miller wrote: From: Joe Perches j...@perches.com Date: Mon, 31 May 2010 08:08:13 -0700 There are many uses of KERN_DEBUG that are reasonable to have always enabled. Doubtful. pr_debug() makes a ton of sense as currently implemented. It's for messages that we want both compile time and run-time control over. The case we have here in mac8390 seems like it should stay as pr_info(). Because 1) it's already controlled by a run-time knob so controlling it even further is confusing and 2) the message is informative, it lets the user know a feature cannot be enabled, thus pr_info(). If that is true in general, then ei_debug itself becomes questionable. In the case of mac8390 at least, I'm certainly leaning toward changing the pr_info (conditional on ei_debug) to pr_debug (unconditional). Finn -- To unsubscribe from this list: send the line unsubscribe linux-m68k in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mac8390: change an error return code and some cleanup, take 4
Propagate the request_irq() return code. Also promote the log level of the
failure message. Likewise some other KERN_INFO messages.
Signed-off-by: Finn Thain fth...@telegraphics.com.au
Index: linux-2.6.34/drivers/net/mac8390.c
===
--- linux-2.6.34.orig/drivers/net/mac8390.c 2010-05-28 00:27:30.0
+1000
+++ linux-2.6.34/drivers/net/mac8390.c 2010-05-28 00:27:32.0 +1000
@@ -556,7 +556,7 @@ static int __init mac8390_initdev(struct
case MAC8390_APPLE:
switch (mac8390_testio(dev-mem_start)) {
case ACCESS_UNKNOWN:
- pr_info(Don't know how to access card memory!\n);
+ pr_err(Don't know how to access card memory!\n);
return -ENODEV;
break;
@@ -643,10 +643,13 @@ static int __init mac8390_initdev(struct
static int mac8390_open(struct net_device *dev)
{
+ int err;
+
__ei_open(dev);
- if (request_irq(dev-irq, __ei_interrupt, 0, 8390 Ethernet, dev)) {
- pr_info(%s: unable to get IRQ %d.\n, dev-name, dev-irq);
- return -EAGAIN;
+ err = request_irq(dev-irq, __ei_interrupt, 0, 8390 Ethernet, dev);
+ if (err) {
+ pr_err(%s: unable to get IRQ %d\n, dev-name, dev-irq);
+ return err;
}
return 0;
}
@@ -662,7 +665,7 @@ static void mac8390_no_reset(struct net_
{
ei_status.txing = 0;
if (ei_debug 1)
- pr_info(reset not supported\n);
+ printk(KERN_DEBUG pr_fmt(reset not supported\n));
return;
}
@@ -670,7 +673,7 @@ static void interlan_reset(struct net_de
{
unsigned char *target = nubus_slot_addr(IRQ2SLOT(dev-irq));
if (ei_debug 1)
- pr_info(Need to reset the NS8390 t=%lu..., jiffies);
+ printk(KERN_DEBUG pr_fmt(Need to reset the NS8390 t=%lu...),
jiffies);
ei_status.txing = 0;
target[0xC] = 0;
if (ei_debug 1)
@@ -871,5 +874,3 @@ static void word_memcpy_fromcard(void *t
while (count--)
*to++ = *from++;
}
-
-
--
To unsubscribe from this list: send the line unsubscribe linux-m68k in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
