netdev - Re: [PATCH 1/2] usbnet: Convert dev(dbg|err|warn|info) macros to 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]
Message-ID: <4B7892AA.7000305@imap.cc>
Date:	Mon, 15 Feb 2010 01:17:46 +0100
From:	Tilman Schmidt <tilman@...p.cc>
To:	Joe Perches <joe@...ches.com>
CC:	David Miller <davem@...emloft.net>,
	David Brownell <david-b@...bell.net>,
	Greg Kroah-Hartman <gregkh@...e.de>, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] usbnet: Convert dev(dbg|err|warn|info) macros to
 netdev_<level>

Am 15.02.2010 00:01 schrieb Joe Perches:
> Some logging messages were in the form:
> 
> 	printk(KERN_<level> "msg %s%s%s%s\n",
> 	       value == 1 ? "one" : "",
> 	       value == 2 ? "two" : "",
> 	       value == 3 ? "three" : "",
> 	       value == 4 ? "four" : "unknown");
> 
> Converted to:
> 
> 	printk(KERN_<level> "msg %s\n",
> 	       value == 1 ? "one" :
> 	       value == 2 ? "two" :
> 	       value == 3 ? "three" :
> 	       value == 4 ? "four" : "unknown");
[...]
> diff --git a/drivers/net/usb/net1080.c b/drivers/net/usb/net1080.c
> index aeb1ab0..aa078f3 100644
> --- a/drivers/net/usb/net1080.c
> +++ b/drivers/net/usb/net1080.c
> @@ -205,23 +205,24 @@ static inline void nc_dump_usbctl(struct usbnet *dev, u16 usbctl)
>  {
>  	if (!netif_msg_link(dev))
>  		return;
> -	devdbg(dev, "net1080 %s-%s usbctl 0x%x:%s%s%s%s%s;"
> -			" this%s%s;"
> -			" other%s%s; r/o 0x%x",
> -		dev->udev->bus->bus_name, dev->udev->devpath,
> -		usbctl,
> -		(usbctl & USBCTL_ENABLE_LANG) ? " lang" : "",
> -		(usbctl & USBCTL_ENABLE_MFGR) ? " mfgr" : "",
> -		(usbctl & USBCTL_ENABLE_PROD) ? " prod" : "",
> -		(usbctl & USBCTL_ENABLE_SERIAL) ? " serial" : "",
> -		(usbctl & USBCTL_ENABLE_DEFAULTS) ? " defaults" : "",
> -
> -		(usbctl & USBCTL_FLUSH_OTHER) ? " FLUSH" : "",
> -		(usbctl & USBCTL_DISCONN_OTHER) ? " DIS" : "",
> -		(usbctl & USBCTL_FLUSH_THIS) ? " FLUSH" : "",
> -		(usbctl & USBCTL_DISCONN_THIS) ? " DIS" : "",
> -		usbctl & ~USBCTL_WRITABLE_MASK
> -		);
> +	netdev_dbg(dev->net, "net1080 %s-%s usbctl 0x%x:%s;"
> +		   " this%s;"
> +		   " other%s; r/o 0x%x\n",
> +		   dev->udev->bus->bus_name, dev->udev->devpath,
> +		   usbctl,
> +		   (usbctl & USBCTL_ENABLE_LANG) ? " lang" :
> +		   (usbctl & USBCTL_ENABLE_MFGR) ? " mfgr" :
> +		   (usbctl & USBCTL_ENABLE_PROD) ? " prod" :
> +		   (usbctl & USBCTL_ENABLE_SERIAL) ? " serial" :
> +		   (usbctl & USBCTL_ENABLE_DEFAULTS) ? " defaults" : "",
> +
> +		   (usbctl & USBCTL_FLUSH_OTHER) ? " FLUSH" :
> +		   (usbctl & USBCTL_DISCONN_OTHER) ? " DIS" : "",
> +
> +		   (usbctl & USBCTL_FLUSH_THIS) ? " FLUSH" :
> +		   (usbctl & USBCTL_DISCONN_THIS) ? " DIS" : "",
> +
> +		   usbctl & ~USBCTL_WRITABLE_MASK);
>  }
>  
>  /*-------------------------------------------------------------------------*/

That doesn't look right. The original code concatenates all the strings
corresponding to the set bits in usbctl. With your patch, it prints only
the string corresponding to the first set bit it encounters within each
group. Eg. with USBCTL_FLUSH_OTHER and USBCTL_DISCONN_OTHER both set,
the original code would print "this FLUSH DIS", while your version would
only print "this FLUSH".

Btw, is it intentional that the _OTHER values are printed after the
label "this", and the _THIS values after the label "other"?

> @@ -250,28 +251,25 @@ static inline void nc_dump_status(struct usbnet *dev, u16 status)
>  {
>  	if (!netif_msg_link(dev))
>  		return;
> -	devdbg(dev, "net1080 %s-%s status 0x%x:"
> -			" this (%c) PKT=%d%s%s%s;"
> -			" other PKT=%d%s%s%s; unspec 0x%x",
> -		dev->udev->bus->bus_name, dev->udev->devpath,
> -		status,
> -
> -		// XXX the packet counts don't seem right
> -		// (1 at reset, not 0); maybe UNSPEC too
> -
> -		(status & STATUS_PORT_A) ? 'A' : 'B',
> -		STATUS_PACKETS_THIS(status),
> -		(status & STATUS_CONN_THIS) ? " CON" : "",
> -		(status & STATUS_SUSPEND_THIS) ? " SUS" : "",
> -		(status & STATUS_MAILBOX_THIS) ? " MBOX" : "",
> -
> -		STATUS_PACKETS_OTHER(status),
> -		(status & STATUS_CONN_OTHER) ? " CON" : "",
> -		(status & STATUS_SUSPEND_OTHER) ? " SUS" : "",
> -		(status & STATUS_MAILBOX_OTHER) ? " MBOX" : "",
> -
> -		status & STATUS_UNSPEC_MASK
> -		);
> +	netdev_dbg(dev->net, "net1080 %s-%s status 0x%x: this (%c) PKT=%d%s; other PKT=%d%s; unspec 0x%x\n",
> +		   dev->udev->bus->bus_name, dev->udev->devpath,
> +		   status,
> +
> +		   // XXX the packet counts don't seem right
> +		   // (1 at reset, not 0); maybe UNSPEC too
> +
> +		   (status & STATUS_PORT_A) ? 'A' : 'B',
> +		   STATUS_PACKETS_THIS(status),
> +		   (status & STATUS_CONN_THIS) ? " CON" :
> +		   (status & STATUS_SUSPEND_THIS) ? " SUS" :
> +		   (status & STATUS_MAILBOX_THIS) ? " MBOX" : "",
> +
> +		   STATUS_PACKETS_OTHER(status),
> +		   (status & STATUS_CONN_OTHER) ? " CON" :
> +		   (status & STATUS_SUSPEND_OTHER) ? " SUS" :
> +		   (status & STATUS_MAILBOX_OTHER) ? " MBOX" : "",
> +
> +		   status & STATUS_UNSPEC_MASK);
>  }
>  
>  /*-------------------------------------------------------------------------*/

Same problem.

HTH
Tilman

-- 
Tilman Schmidt                    E-Mail: tilman@...p.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


Download attachment "signature.asc" of type "application/pgp-signature" (260 bytes)