Re: [PATCH net-next 14/15] drivers/net/typhoon.c: Use (pr|netdev)_<level> macro helpers

[David Dillow <dave@thedillows.org>]

On Wed, 2010-02-17 at 18:18 -0800, Joe Perches wrote:
> On Wed, 2010-02-17 at 20:59 -0500, David Dillow wrote:
> > On Wed, 2010-02-17 at 17:02 -0800, Joe Perches wrote:
> > > Use pr_<level>
> > > Use netdev_<level>
> > 
> > The way the driver uses tp->name, most of the pr_<level> changes add an
> > extraneous "typhoon:" to the front of the messages, which is not
> > desirable. Your absolute change-over from PFX/ERR_PFX to pr_fmt()
> > KBUILD_MODNAME misses the distinction between the message where the
> > prefix is needed, and where it isn't.
> 
> Doesn't that mean that "%s: ...", tp->name should be removed?

No, because the routines that use tp->name are called both before and
after the netdev is registered. Prior to that time, it contains the PCI
slot name -- "00:01.0" etc -- so that the user can determine which card
is acting up. Once the card is registered, it has "ethX" to use a
commonly expected name for the card.

> > The netdev_<level> changes are much more palatable to me than the
> > pr_<level> ones. I have no problem getting behind those.
> > 
> > > Coalesce long formats
> > 
> > I don't like these changes very much, either. I tend to work in 80 char
> > terminals, and the wrap of a few characters is usually annoying.
> 
> Linus prefers formats not be split across multiple lines.
> http://lkml.org/lkml/2008/2/23/251
> I can change it back if you want, no matter to me.

I'd prefer it, but I don't think it is really worth the effort now.

> > These __func__ conversions need to go.
> > 
> > > @@ -1901,16 +1898,16 @@ typhoon_sleep(struct typhoon *tp, pci_power_t state, __le16 events)
> > >  	xp_cmd.parm1 = events;
> > >  	err = typhoon_issue_command(tp, 1, &xp_cmd, 0, NULL);
> > >  	if(err < 0) {
> > > -		printk(KERN_ERR "%s: typhoon_sleep(): wake events cmd err %d\n",
> > > -				tp->name, err);
> > > +		pr_err("%s: %s(): wake events cmd err %d\n",
> > > +		       tp->name, __func__, err);
> > >  		return err;
> > >  	}
> 
> why?  It makes it harder to get the function name wrong if
> it's rewritten.

This driver is rarely touched, except for API changes and such cleanups
people like to make from time to time. It is unlikely to be renamed,
adds code, and looks ugly. It may make sense for other printks in
functions that are in flux, but not here.

> How about something like this (on top of previous)?

The version change is fine, but you shouldn't get rid of tp->name.