mac8390: fix pr_info() calls and change return code
in [Kernel]
|
Prev: vmscan: Setup pagevec as late as possible in shrink_inactive_list()
Next: vmscan: page_check_references() check low order lumpy reclaim properly
From: Finn Thain on 16 Apr 2010 00:30 Signed-off-by: Finn Thain <fthain(a)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; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Joe Perches on 16 Apr 2010 00:40 On Fri, 2010-04-16 at 14:21 +1000, Finn Thain wrote: > Signed-off-by: Finn Thain <fthain(a)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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Miller on 16 Apr 2010 02:00 From: Finn Thain <fthain(a)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). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Finn Thain on 16 Apr 2010 10:00 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Miller on 16 Apr 2010 16:30
From: Finn Thain <fthain(a)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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |