netdev - Re: [PATCH 1/4] asix: Fix checkpatch warnings

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: <1341588334.2011.21.camel@joe2Laptop>
Date:	Fri, 06 Jul 2012 08:25:34 -0700
From:	Joe Perches <joe@...ches.com>
To:	Christian Riesch <christian.riesch@...cron.at>
Cc:	netdev@...r.kernel.org, Oliver Neukum <oneukum@...e.de>,
	Eric Dumazet <edumazet@...gle.com>,
	Allan Chou <allan@...x.com.tw>,
	Mark Lord <kernel@...savvy.com>,
	Grant Grundler <grundler@...omium.org>,
	Ming Lei <tom.leiming@...il.com>,
	Michael Riesch <michael@...sch.at>
Subject: Re: [PATCH 1/4] asix: Fix checkpatch warnings

On Fri, 2012-07-06 at 13:33 +0200, Christian Riesch wrote:
>

Hi Christian.  Just some trivial comments for a
trivial cleanup patch.

> diff --git a/drivers/net/usb/asix.c b/drivers/net/usb/asix.c
> index 3ae80ec..9210f40 100644
> --- a/drivers/net/usb/asix.c
> +++ b/drivers/net/usb/asix.c
> @@ -20,8 +20,8 @@
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>   */
>  
> -// #define	DEBUG			// error path messages, extra info
> -// #define	VERBOSE			// more; success messages
> +/* #define	DEBUG	*/		/* error path messages, extra info */
> +/* #define	VERBOSE	*/		/* more; success messages */

Might as well delete as change the comment style.
It isn't applicable after the patch.

> @@ -253,8 +253,8 @@ static void asix_async_cmd_callback(struct urb *urb)
>  	int status = urb->status;
>  
>  	if (status < 0)
> -		printk(KERN_DEBUG "asix_async_cmd_callback() failed with %d",
> -			status);
> +		pr_debug("asix_async_cmd_callback() failed with %d",
> +			 status);

Probably better with "%s: "..., __func__, ...
Missing a newline too.
There are several other uses of embedded function names
that could be modified.

> @@ -432,7 +433,8 @@ static inline int asix_get_phy_addr(struct usbnet *dev)
>  	netdev_dbg(dev->net, "asix_get_phy_addr()\n");
>  
>  	if (ret < 0) {
> -		netdev_err(dev->net, "Error reading PHYID register: %02x\n", ret);
> +		netdev_err(dev->net, "Error reading PHYID register: %02x\n",
> +			   ret);

80 column zealotry?  If you want, but it's probably past
the time that's really desirable or necessary.

> @@ -575,7 +580,7 @@ static int asix_mdio_read(struct net_device *netdev, int phy_id, int loc)
>  	mutex_lock(&dev->phy_mutex);
>  	asix_set_sw_mii(dev);
>  	asix_read_cmd(dev, AX_CMD_READ_MII_REG, phy_id,
> -				(__u16)loc, 2, &res);
> +		      (__u16)loc, 2, &res);

Fits on 1 line.

> +static void asix_get_drvinfo(struct net_device *net,
> +			     struct ethtool_drvinfo *info)
>  {
>  	struct usbnet *dev = netdev_priv(net);
>  	struct asix_data *data = (struct asix_data *)&dev->data;
>  
>  	/* Inherit standard device info */
>  	usbnet_get_drvinfo(net, info);
> -	strncpy (info->driver, DRIVER_NAME, sizeof info->driver);
> -	strncpy (info->version, DRIVER_VERSION, sizeof info->version);
> +	strncpy(info->driver, DRIVER_NAME, sizeof info->driver);
> +	strncpy(info->version, DRIVER_VERSION, sizeof info->version);

Most every kernel use of sizeof uses parens like:

	strncpy(info->driver, DRIVER_NAME, sizeof(info->driver));
	strncpy(info->version, DRIVER_VERSION, sizeof(info->version));
 
@@ -1510,133 +1520,133 @@ static const struct driver_info ax88178_info = {
>  	.tx_fixup = asix_tx_fixup,
>  };
>  
> -static const struct usb_device_id	products [] = {
> +static const struct usb_device_id	products[] = {

Maybe use a space not a tab after usb_device_id.

>  {
> -	// Linksys USB200M
> -	USB_DEVICE (0x077b, 0x2226),
> +	/* Linksys USB200M */
> +	USB_DEVICE(0x077b, 0x2226),
>  	.driver_info =	(unsigned long) &ax8817x_info,
>  }, {

I think all of these would look more reasonable on single
lines like

	{ USB_DEVICE(0xxxxx, 0xxxxx), .driver_info = (unsigned long)&func },

or maybe add another macro like:

#define ASIX_USB_DEVICE(vendor, product, driver)	\
	USB_DEVICE(vendor, product), .driver_info = (unsigned long)driver)

and make these

	{ ASIX_USB_DEVICE(0xxxxx, 0xxxxx, &func) },	/* description */

Come to think of it, the & for the function address
isn't necessary either.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html