another cleanup patch gone wrong

another cleanup patch gone wrong

[Finn Thain]

...but this one was already merged, unfortunately.

> Use printk_once
> Add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> Convert printks without KERN_<level> to pr_info and pr_cont
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> 
> diff --git a/drivers/net/mac8390.c b/drivers/net/mac8390.c
> index 517cee4..8bd09e2 100644 (file)
> --- a/drivers/net/mac8390.c
> +++ b/drivers/net/mac8390.c
> @@ -17,6 +17,8 @@
>  /* 2002-12-30: Try to support more cards, some clues from NetBSD driver */
>  /* 2003-12-26: Make sure Asante cards always work. */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +

Why the macro? You only used it once.

The pr_xxx naming convention belongs to a kernel-wide include file. Is it 
really a good idea to start repurposing it in .c files?

> @@ -545,7 +537,7 @@ static int __init mac8390_initdev(struct net_device * dev, struct nubus_dev * nd
>         case MAC8390_APPLE:
>                 switch (mac8390_testio(dev->mem_start)) {
>                 case ACCESS_UNKNOWN:
> -                       printk("Don't know how to access card memory!\n");
> +                       pr_info("Don't know how to access card memory!\n");

No, this is pr_err. The driver sets dev->mem_start expecting it to work, 
obviously.

>                         return -ENODEV;
>                         break;
>  

> @@ -633,7 +626,7 @@ static int mac8390_open(struct net_device *dev)
>  {
>         __ei_open(dev);
>         if (request_irq(dev->irq, __ei_interrupt, 0, "8390 Ethernet", dev)) {
> -               printk ("%s: unable to get IRQ %d.\n", dev->name, dev->irq);
> +               pr_info("%s: unable to get IRQ %d.\n", dev->name, dev->irq);
>                 return -EAGAIN;

Same here.

>         }
>         return 0;
> @@ -650,7 +643,7 @@ static void mac8390_no_reset(struct net_device *dev)
>  {
>         ei_status.txing = 0;
>         if (ei_debug > 1)
> -               printk("reset not supported\n");
> +               pr_info("reset not supported\n");

I think you meant pr_debug().

>         return;
>  }
>  
> @@ -658,11 +651,11 @@ static void interlan_reset(struct net_device *dev)
>  {
>         unsigned char *target=nubus_slot_addr(IRQ2SLOT(dev->irq));
>         if (ei_debug > 1)
> -               printk("Need to reset the NS8390 t=%lu...", jiffies);
> +               pr_info("Need to reset the NS8390 t=%lu...", jiffies);

Same here.

Finn

Re: another cleanup patch gone wrong

[David Miller]

From: Finn Thain <fthain@telegraphics.com.au>
Date: Fri, 16 Apr 2010 12:34:24 +1000 (EST)

> 
> ...but this one was already merged, unfortunately.
> 
>> Use printk_once
>> Add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> Convert printks without KERN_<level> to pr_info and pr_cont
>> 
>> Signed-off-by: Joe Perches <joe@perches.com>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>> 
>> 
>> diff --git a/drivers/net/mac8390.c b/drivers/net/mac8390.c
>> index 517cee4..8bd09e2 100644 (file)
>> --- a/drivers/net/mac8390.c
>> +++ b/drivers/net/mac8390.c
>> @@ -17,6 +17,8 @@
>>  /* 2002-12-30: Try to support more cards, some clues from NetBSD driver */
>>  /* 2003-12-26: Make sure Asante cards always work. */
>>  
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
> 
> Why the macro? You only used it once.

It gets expanded internally into all of the pr_*() calls.

> The pr_xxx naming convention belongs to a kernel-wide include file. Is it 
> really a good idea to start repurposing it in .c files?

This is exactly how it can be used, and there is much
precedent for this now.

>> -                       printk("Don't know how to access card memory!\n");
>> +                       pr_info("Don't know how to access card memory!\n");
> 
> No, this is pr_err. The driver sets dev->mem_start expecting it to work, 
> obviously.

It was an unspecified printk() so Joe's conversion is equal
and that's a good way for him to have made these changes.

If we want to mark this as KERN_ERR or whatever, that's entirely
a seperate change.

I think your objections to Joe's changes are completely uncalled
for and his changes were good ones.

[PATCH] mac8390: fix pr_info() calls, was Re: another cleanup patch gone wrong

[Finn Thain]

On Thu, 15 Apr 2010, David Miller wrote:

> >> -                       printk("Don't know how to access card memory!\n");
> >> +                       pr_info("Don't know how to access card memory!\n");
> > 
> > No, this is pr_err. The driver sets dev->mem_start expecting it to work, 
> > obviously.
> 
> It was an unspecified printk() so Joe's conversion is equal
> and that's a good way for him to have made these changes.

Seems to me that the code went from unspecified to wrong. But I can see 
your point of view.

> If we want to mark this as KERN_ERR or whatever, that's entirely a 
> seperate change.
>
> I think your objections to Joe's changes are completely uncalled for and 
> his changes were good ones.

Here's a patch, both uncalled-for and untested.


Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

--- a/drivers/net/mac8390.c	2010-04-16 13:31:04.000000000 +1000
+++ b/drivers/net/mac8390.c	2010-04-16 13:35:06.000000000 +1000
@@ -554,7 +554,7 @@
 	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,8 +643,8 @@
 {
 	__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;
+		pr_err("%s: unable to get IRQ %d.\n", dev->name, dev->irq);
+		return -EBUSY;
 	}
 	return 0;
 }
@@ -660,7 +660,7 @@
 {
 	ei_status.txing = 0;
 	if (ei_debug > 1)
-		pr_info("reset not supported\n");
+		pr_debug("reset not supported\n");
 	return;
 }
 
@@ -668,7 +668,7 @@
 {
 	unsigned char *target = nubus_slot_addr(IRQ2SLOT(dev->irq));
 	if (ei_debug > 1)
-		pr_info("Need to reset the NS8390 t=%lu...", jiffies);
+		pr_debug("Need to reset the NS8390 t=%lu...", jiffies);
 	ei_status.txing = 0;
 	target[0xC0000] = 0;
 	if (ei_debug > 1)

Re: [PATCH] mac8390: fix pr_info() calls, was Re: another cleanup patch gone wrong

[Joe Perches]

On Fri, 2010-04-16 at 13:45 +1000, Finn Thain wrote:
> Here's a patch, both uncalled-for and untested.
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> 
> --- a/drivers/net/mac8390.c	2010-04-16 13:31:04.000000000 +1000
> +++ b/drivers/net/mac8390.c	2010-04-16 13:35:06.000000000 +1000
> @@ -643,8 +643,8 @@
>  {
>  	__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;
> +		pr_err("%s: unable to get IRQ %d.\n", dev->name, dev->irq);
> +		return -EBUSY;

You should document this in the changelog.

>  	}
>  	return 0;
>  }
> @@ -660,7 +660,7 @@
>  {
>  	ei_status.txing = 0;
>  	if (ei_debug > 1)
> -		pr_info("reset not supported\n");
> +		pr_debug("reset not supported\n");

You'll need to add
#define DEBUG
for this to print.

> -		pr_info("Need to reset the NS8390 t=%lu...", jiffies);
> +		pr_debug("Need to reset the NS8390 t=%lu...", jiffies);

This also now doesn't print.

cheers, Joe

Re: [PATCH] mac8390: fix pr_info() calls, was Re: another cleanup patch gone wrong

[Finn Thain]

On Thu, 15 Apr 2010, Joe Perches wrote:

> >  	return 0;
> >  }
> > @@ -660,7 +660,7 @@
> >  {
> >  	ei_status.txing = 0;
> >  	if (ei_debug > 1)
> > -		pr_info("reset not supported\n");
> > +		pr_debug("reset not supported\n");
> 
> You'll need to add
> #define DEBUG
> for this to print.
> 
> > -		pr_info("Need to reset the NS8390 t=%lu...", jiffies);
> > +		pr_debug("Need to reset the NS8390 t=%lu...", jiffies);
> 
> This also now doesn't print.
> 

Oops. Thanks for spotting that. I'll resend.

Finn

> cheers, Joe

Re: [PATCH] mac8390: fix pr_info() calls and change return code

[Finn Thain]

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

--- a/drivers/net/mac8390.c	2010-04-16 13:31:04.000000000 +1000
+++ b/drivers/net/mac8390.c	2010-04-16 14:02:29.000000000 +1000
@@ -554,7 +554,7 @@
 	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,8 +643,8 @@
 {
 	__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;
+		pr_err("%s: unable to get IRQ %d.\n", dev->name, dev->irq);
+		return -EBUSY;
 	}
 	return 0;
 }
@@ -660,7 +660,7 @@
 {
 	ei_status.txing = 0;
 	if (ei_debug > 1)
-		pr_info("reset not supported\n");
+		printk(KERN_DEBUG "reset not supported\n");
 	return;
 }
 
@@ -668,11 +668,11 @@
 {
 	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 "Need to reset the NS8390 t=%lu...", jiffies);
 	ei_status.txing = 0;
 	target[0xC0000] = 0;
 	if (ei_debug > 1)
-		pr_cont("reset complete\n");
+		printk(KERN_CONT "reset complete\n");
 	return;
 }
 

Re: [PATCH] mac8390: fix pr_info() calls and change return code

[Joe Perches]

On Fri, 2010-04-16 at 14:21 +1000, Finn Thain wrote:
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> 
> --- a/drivers/net/mac8390.c	2010-04-16 13:31:04.000000000 +1000
> +++ b/drivers/net/mac8390.c	2010-04-16 14:02:29.000000000 +1000
> @@ -554,7 +554,7 @@
>  	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,8 +643,8 @@
>  {
>  	__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;
> +		pr_err("%s: unable to get IRQ %d.\n", dev->name, dev->irq);
> +		return -EBUSY;

You should document the reason for the
return code change in the changelog.
Why is it better to use -EBUSY?

>  	}
>  	return 0;
>  }
> @@ -660,7 +660,7 @@
>  {
>  	ei_status.txing = 0;
>  	if (ei_debug > 1)
> -		pr_info("reset not supported\n");
> +		printk(KERN_DEBUG "reset not supported\n");

It'd be better to prefix this with the driver name
or use something like netdev_dbg with #define DEBUG
otherwise it's "huh? what device emits this message?"
when reading the logs.

Something like:
	printk(KERN_DEBUG pr_fmt("reset not supported\n"));
or
#define DEBUG
	netdev_dbg(dev, "reset not supported\n");
or
#define DEBUG
	pr_debug("reset not supported\n");

>  	if (ei_debug > 1)
> -		pr_cont("reset complete\n");
> +		printk(KERN_CONT "reset complete\n");

unnecessary conversion.

Re: [PATCH] mac8390: fix pr_info() calls and change return code

[Finn Thain]

On Thu, 15 Apr 2010, Joe Perches wrote:

> ...Why is it better to use -EBUSY?

Nubus slots are geographically addressed and their irqs are equally 
inflexible. -EAGAIN is misleading because retrying will not help fix 
whatever bug caused the irq to unavailable.

> ...It'd be better to prefix this with the driver name
> or use something like netdev_dbg with #define DEBUG
> otherwise it's "huh? what device emits this message?"
> when reading the logs.
> 
> Something like:
> 	printk(KERN_DEBUG pr_fmt("reset not supported\n"));

Thanks for the suggestion. I'll resend again.

> ...unnecessary conversion.

I guess some prefer consistency, some prefer symmetry.

Finn

Re: [PATCH] mac8390: fix pr_info() calls and change return code

[David Miller]

From: Finn Thain <fthain@telegraphics.com.au>
Date: Fri, 16 Apr 2010 23:57:34 +1000 (EST)

> 
> On Thu, 15 Apr 2010, Joe Perches wrote:
> 
>> ...Why is it better to use -EBUSY?
> 
> Nubus slots are geographically addressed and their irqs are equally 
> inflexible. -EAGAIN is misleading because retrying will not help fix 
> whatever bug caused the irq to unavailable.

This is exactly the kind of background information and verbose
explanation that belongs in the commit message.

Yet in your recent version of the patch, you're still being extremely
terse as per the reasoning for using -EBUSY

Just saying it's "misleading" doesn't tell anyone anything if they
have to go back in the commit history and try to figure out why this
change was made if it's causing problems later.

Please make the verbose and complete explanation in your commit
message, and resubmit your patch.

I just want to point out that with all the trouble you gave about
Joe's work, you're having one heck of a time even submitting your
changes properly. :-)

Thanks.

Re: [PATCH] mac8390: fix pr_info() calls and change return code

[Finn Thain]

On Fri, 16 Apr 2010, David Miller wrote:

> 
> I just want to point out that with all the trouble you gave about Joe's 
> work, you're having one heck of a time even submitting your changes 
> properly. :-)

You are a thorough reviewer, despite the regrettable implication to the 
contrary in my first message in this thread. And you are quite right, I 
did not understand the pr_* macros at the time. I apologise.

I am hypersensitive to bit rot. I guess that's what happens when one makes 
it one's job to fix regressions in Linux.

Finn

Re: [PATCH] mac8390: fix pr_info() calls and change return code

[David Miller]

From: Finn Thain <fthain@telegraphics.com.au>
Date: Fri, 16 Apr 2010 14:21:00 +1000 (EST)

>  
> @@ -668,11 +668,11 @@
>  {
>  	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 "Need to reset the NS8390 t=%lu...", jiffies);
>  	ei_status.txing = 0;
>  	target[0xC0000] = 0;
>  	if (ei_debug > 1)
> -		pr_cont("reset complete\n");
> +		printk(KERN_CONT "reset complete\n");
>  	return;

You're missing the whole point of using pr_info() et al.  in that it
includes the bits we define for pr_fmt at the top of the file.

Also, you write absolutely no commit log message entry for your
change explaining why you're doing the things you are doing.

And finally you are doing two completely unrelated things at one
(changing error return values and changing log message levels).

[PATCH] mac8390: change an error return code and some cleanup

[Finn Thain]

Change an error return from EAGAIN to EBUSY since the former is 
misleading. Also promote the log message. Likewise some other KERN_INFO 
log messages.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

--- a/drivers/net/mac8390.c	2010-04-16 13:31:04.000000000 +1000
+++ b/drivers/net/mac8390.c	2010-04-16 23:50:39.000000000 +1000
@@ -554,7 +554,7 @@
 	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,8 +643,8 @@
 {
 	__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;
+		pr_err("%s: unable to get IRQ %d.\n", dev->name, dev->irq);
+		return -EBUSY;
 	}
 	return 0;
 }
@@ -660,7 +660,7 @@
 {
 	ei_status.txing = 0;
 	if (ei_debug > 1)
-		pr_info("reset not supported\n");
+		printk(KERN_DEBUG pr_fmt("reset not supported\n"));
 	return;
 }
 
@@ -668,11 +668,11 @@
 {
 	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[0xC0000] = 0;
 	if (ei_debug > 1)
-		pr_cont("reset complete\n");
+		printk(KERN_CONT "reset complete\n");
 	return;
 }
 

[PATCH] mac8390: change an error return code and some cleanup, take 3

[Finn Thain]

Change an error return code from -EAGAIN to -EBUSY since the former is 
misleading.

Nubus slots are geographically addressed and their irqs are equally 
inflexible. -EAGAIN is misleading because retrying will not help fix 
whatever bug it was that made the irq unavailable.

Also promote the log message. Likewise some other KERN_INFO log messages.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

--- a/drivers/net/mac8390.c	2010-04-16 13:31:04.000000000 +1000
+++ b/drivers/net/mac8390.c	2010-04-16 23:50:39.000000000 +1000
@@ -554,7 +554,7 @@
 	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,8 +643,8 @@
 {
 	__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;
+		pr_err("%s: unable to get IRQ %d.\n", dev->name, dev->irq);
+		return -EBUSY;
 	}
 	return 0;
 }
@@ -660,7 +660,7 @@
 {
 	ei_status.txing = 0;
 	if (ei_debug > 1)
-		pr_info("reset not supported\n");
+		printk(KERN_DEBUG pr_fmt("reset not supported\n"));
 	return;
 }
 
@@ -668,7 +668,7 @@
 {
 	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[0xC0000] = 0;
 	if (ei_debug > 1)
 

Re: [PATCH] mac8390: change an error return code and some cleanup, take 3

[David Miller]

From: Finn Thain <fthain@telegraphics.com.au>
Date: Sat, 17 Apr 2010 13:16:04 +1000 (EST)

> 
> Change an error return code from -EAGAIN to -EBUSY since the former is 
> misleading.
> 
> Nubus slots are geographically addressed and their irqs are equally 
> inflexible. -EAGAIN is misleading because retrying will not help fix 
> whatever bug it was that made the irq unavailable.

request_irq() itself returns an appropriate error code, so the
correct change is to do:

	err = request_irq( ... );
	if (err) {
	...

and return 'err'.

Re: [PATCH] mac8390: change an error return code and some cleanup, take 3

[Finn Thain]

On Wed, 21 Apr 2010, David Miller wrote:

> From: Finn Thain <fthain@telegraphics.com.au>
> Date: Sat, 17 Apr 2010 13:16:04 +1000 (EST)
> 
> > 
> > Change an error return code from -EAGAIN to -EBUSY since the former is 
> > misleading.
> > 
> > Nubus slots are geographically addressed and their irqs are equally 
> > inflexible. -EAGAIN is misleading because retrying will not help fix 
> > whatever bug it was that made the irq unavailable.
> 
> request_irq() itself returns an appropriate error code, so the
> correct change is to do:
> 
> 	err = request_irq( ... );
> 	if (err) {
> 	...
> 
> and return 'err'.

OK. I'll send a new patch once 2.6.34 is out and I have time to test this 
and some other patches.

Finn

[PATCH] mac8390: change an error return code and some cleanup, take 4

[Finn Thain]

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 <fthain@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.000000000 +1000
+++ linux-2.6.34/drivers/net/mac8390.c	2010-05-28 00:27:32.000000000 +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[0xC0000] = 0;
 	if (ei_debug > 1)
@@ -871,5 +874,3 @@ static void word_memcpy_fromcard(void *t
 	while (count--)
 		*to++ = *from++;
 }
-
-

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[David Miller]

From: Finn Thain <fthain@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.

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[fthain]

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

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[Geert Uytterhoeven]

On Mon, May 31, 2010 at 11:21,  <fthain@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 -- geert@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

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[fthain]

On Mon, 31 May 2010, Geert Uytterhoeven wrote:

> 
> `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?

Geert, in the beginning, I decided that it should be KERN_DEBUG, not 
KERN_INFO, and made that change in the first patch submission. I used 
pr_debug().

Then Joe pointed out that this required DEBUG defined, which I'd forgotten 
(I didn't know about CONFIG_DYNAMIC_DEBUG). So, to retain the old 
behaviour, while following the example of other usages of ei_debug in 
lib8390 and drivers, I changed it to printk(KERN_DEBUG ...).

Then Joe pointed out that I should take advantage of pr_fmt(), so the 
third submission made that change.

(Then David said I should propagate the return code from request_irq, so I 
made the present patch submission.)

Apparently David now wants me to submit this again --

if (ei_debug)
	pr_debug(...)

David, if that code is acceptable, please let me know.

If that code is mandatory, why didn't you say so upon the second patch 
submission?

Alternatively, if the following is preferred (as implied by your last 
email):

if (ei_debug)
	pr_info(...)

Then let me know, and I'll quit bothering you.

Or just go ahead and change my patch if you wish to save us both some 
time.

Finn

> 
> Gr{oetje,eeting}s,
> 
> 						Geert

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[David Miller]

From: fthain@telegraphics.com.au
Date: Mon, 31 May 2010 21:07:09 +1000 (EST)

> Apparently David now wants me to submit this again --
> 
> if (ei_debug)
> 	pr_debug(...)
> 
> David, if that code is acceptable, please let me know.

The only thing I care about is at the moment that you don't do
something that ends up dropping the pr_fmt prefix.

The pr_fmt define at the beginning of the driver is for nothing
if we end up adding exceptions that end up eliding it for no
good reason.  And that's what your patch was doing.

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[Finn Thain]

On Mon, 31 May 2010, David Miller wrote:

> From: fthain@telegraphics.com.au
> Date: Mon, 31 May 2010 21:07:09 +1000 (EST)
> 
> > Apparently David now wants me to submit this again --
> > 
> > if (ei_debug)
> > 	pr_debug(...)
> > 
> > David, if that code is acceptable, please let me know.
> 
> The only thing I care about is at the moment that you don't do something 
> that ends up dropping the pr_fmt prefix.
> 
> The pr_fmt define at the beginning of the driver is for nothing if we 
> end up adding exceptions that end up eliding it for no good reason.  
> And that's what your patch was doing.
> 

Since you have rejected my most recent patch submission, which uses pr_fmt 
explicitly, I imagine that what you are trying to say here is that only 
pr_debug or pr_info are acceptable.

Now, so that we don't have to go through pointless resubmission 
iterations, can you tell me which of the following you prefer:

if (ei_debug)
	pr_debug(...)

OR

if (ei_debug)
	pr_info(...)

Thanks.

Finn

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[David Miller]

From: Finn Thain <fthain@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.

[PATCH 1/2] mac8390: propagate error code from request_irq

[Finn Thain]

Use the request_irq() error code as the return value for mac8390_open(). 
EAGAIN doesn't make sense for Nubus slot IRQs. Only this driver can claim 
this IRQ (until the NIC is removed, which means everything is powered 
down).

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Index: linux-2.6.34/drivers/net/mac8390.c
===================================================================
--- linux-2.6.34.orig/drivers/net/mac8390.c	2010-06-01 20:20:29.000000000 +1000
+++ linux-2.6.34/drivers/net/mac8390.c	2010-06-01 20:22:23.000000000 +1000
@@ -643,12 +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;
-	}
-	return 0;
+	err = request_irq(dev->irq, __ei_interrupt, 0, "8390 Ethernet", dev);
+	if (err)
+		pr_info("%s: unable to get IRQ %d\n", dev->name, dev->irq);
+	return err;
 }
 
 static int mac8390_close(struct net_device *dev)

Re: [PATCH 1/2] mac8390: propagate error code from request_irq

[David Miller]

From: Finn Thain <fthain@telegraphics.com.au>
Date: Tue, 1 Jun 2010 22:18:32 +1000 (EST)

> 
> Use the request_irq() error code as the return value for mac8390_open(). 
> EAGAIN doesn't make sense for Nubus slot IRQs. Only this driver can claim 
> this IRQ (until the NIC is removed, which means everything is powered 
> down).
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Applied.

[PATCH 2/2] mac8390: raise error logging priority

[Finn Thain]

Log error conditions using KERN_ERR priority.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

---

This patch doesn't address the KERN_INFO messages conditional on ei_debug. 
I don't know how those can be improved to the satisfaction of all 
interested parties.

These two patches should be applied in sequence.

Index: linux-2.6.34/drivers/net/mac8390.c
===================================================================
--- linux-2.6.34.orig/drivers/net/mac8390.c	2010-06-01 20:22:23.000000000 +1000
+++ linux-2.6.34/drivers/net/mac8390.c	2010-06-01 20:22:25.000000000 +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;
 
@@ -648,7 +648,7 @@ static int mac8390_open(struct net_devic
 	__ei_open(dev);
 	err = request_irq(dev->irq, __ei_interrupt, 0, "8390 Ethernet", dev);
 	if (err)
-		pr_info("%s: unable to get IRQ %d\n", dev->name, dev->irq);
+		pr_err("%s: unable to get IRQ %d\n", dev->name, dev->irq);
 	return err;
 }
 

Re: [PATCH 2/2] mac8390: raise error logging priority

[David Miller]

From: Finn Thain <fthain@telegraphics.com.au>
Date: Tue, 1 Jun 2010 22:18:56 +1000 (EST)

> Log error conditions using KERN_ERR priority.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Applied, thank you.

Re: [PATCH 2/2] mac8390: raise error logging priority

[Finn Thain]

On Wed, 2 Jun 2010, David Miller wrote:

> 
> Applied, thank you.
> 

You're welcome.

And I ought to add, thanks, Joe, for your patches.

It is much harder than I had realised to do them right (or for that 
matter, well enough).

Finn

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[Joe Perches]

On Mon, 2010-05-31 at 11:58 +0200, Geert Uytterhoeven wrote:
> > 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?

There are many uses of KERN_DEBUG that are reasonable to have
always enabled.

There is no pr_<level> macro/function that is always enabled.

David, would you accept a new pr_<level> in kernel.h
for that purpose?

If so, do you have an opinion what it should be named?

I think pr_dbg is not ideal as dev_dbg is already in use
and can get optimized away.

Maybe one of:

	pr_always_dbg
	pr_dbg_always
	pr_dbg_noopt
	pr_tdbg

or something better?  Anyone else?

http://lkml.org/lkml/2009/10/1/399

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[David Miller]

From: Joe Perches <joe@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().

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[Finn Thain]

On Mon, 31 May 2010, David Miller wrote:

> From: Joe Perches <joe@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

Re: [PATCH] mac8390: change an error return code and some cleanup, take 4

[David Miller]

From: fthain@telegraphics.com.au
Date: Mon, 31 May 2010 19:21:26 +1000 (EST)

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

I won't do that, because your change elides the pr_fmt prefix Joe
Perches added to the driver for the purposes of making sure such a
prefix would be added to all log messages output by the driver.

The whole idea is that everything the driver puts into the kernel
log has the driver module name at the beginning.  That's what the
pr_fmt define at the top of the driver does.

It's so you can tell which driver the message came from.

You're undoing that, which is a bug.  It's a step backwards, it's
wrong.

And to top it off we've had to explain this stuff to you multiple
times.

So, as long as the patch is incorrect I absolutely will not apply it.

Re: another cleanup patch gone wrong

[Joe Perches]

On Fri, 2010-04-16 at 12:34 +1000, Finn Thain wrote:
> ...but this one was already merged, unfortunately.
> 
> > Use printk_once
> > Add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > Convert printks without KERN_<level> to pr_info and pr_cont
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> > 
> > 
> > diff --git a/drivers/net/mac8390.c b/drivers/net/mac8390.c
> > index 517cee4..8bd09e2 100644 (file)
> > --- a/drivers/net/mac8390.c
> > +++ b/drivers/net/mac8390.c
> > @@ -17,6 +17,8 @@
> >  /* 2002-12-30: Try to support more cards, some clues from NetBSD driver */
> >  /* 2003-12-26: Make sure Asante cards always work. */
> >  
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> 
> Why the macro? You only used it once.

It's used embedded in the pr_<level> functions.
It's used more than once.

> The pr_xxx naming convention belongs to a kernel-wide include file. Is it 
> really a good idea to start repurposing it in .c files?

It's in kernel.h, and yes, it is.
http://lkml.org/lkml/2008/11/12/297

> No, this is pr_err. The driver sets dev->mem_start expecting it to work, 
> obviously.

I suggest you change the levels to what you desire.

You could add yourself to the MAINTAINERS entry for this file.

cheers, Joe

Re: another cleanup patch gone wrong

[Finn Thain]

On Thu, 15 Apr 2010, Joe Perches wrote:

> > Why the macro? You only used it once.
> 
> It's used embedded in the pr_<level> functions.
> It's used more than once.
> 
> > The pr_xxx naming convention belongs to a kernel-wide include file. Is it 
> > really a good idea to start repurposing it in .c files?
> 
> It's in kernel.h, and yes, it is.
> http://lkml.org/lkml/2008/11/12/297

My mistake.

Finn