linux-kernel - Re: [PATCH net-next 14/15] drivers/net/typhoon.c: Use (pr|netdev)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Feb 2010 21:30:21 -0500
From:	David Dillow <dave@...dillows.org>
To:	Joe Perches <joe@...ches.com>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	davem@...emloft.net
Subject: Re: [PATCH net-next 14/15] drivers/net/typhoon.c: Use
 (pr|netdev)_<level> macro helpers

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/