linux-kernel - Re: [PATCH net-next 03/15] net: sgi: ioc3-eth: remove checkpatch errors/warning

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: <d0fd02c3634d187dcfe5487917099bc1905e3789.camel@perches.com>
Date:   Wed, 28 Aug 2019 10:10:18 -0700
From:   Joe Perches <joe@...ches.com>
To:     Thomas Bogendoerfer <tbogendoerfer@...e.de>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Paul Burton <paul.burton@...s.com>,
        James Hogan <jhogan@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 03/15] net: sgi: ioc3-eth: remove checkpatch
 errors/warning

On Wed, 2019-08-28 at 16:03 +0200, Thomas Bogendoerfer wrote:
> Before massaging the driver further fix oddities found by checkpatch like
> - wrong indention
> - comment formatting
> - use of printk instead or netdev_xxx/pr_xxx

trivial notes:

Please try to make the code better rather than merely
shutting up checkpatch.

> diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
[]
> @@ -209,8 +201,7 @@ static inline void nic_write_bit(u32 __iomem *mcr, int bit)
>  	nic_wait(mcr);
>  }
>  
> -/*
> - * Read a byte from an iButton device
> +/* Read a byte from an iButton device
>   */

These comment styles would be simpler on a single line

/* Read a byte from an iButton device */

>  static u32 nic_read_byte(u32 __iomem *mcr)
>  {
> @@ -223,8 +214,7 @@ static u32 nic_read_byte(u32 __iomem *mcr)
>  	return result;
>  }
>  
> -/*
> - * Write a byte to an iButton device
> +/* Write a byte to an iButton device
>   */

/* Write a byte to an iButton device */

etc...

[]
> @@ -323,16 +315,15 @@ static int nic_init(u32 __iomem *mcr)
>  		break;
>  	}
>  
> -	printk("Found %s NIC", type);
> +	pr_info("Found %s NIC", type);
>  	if (type != unknown)
> -		printk (" registration number %pM, CRC %02x", serial, crc);
> -	printk(".\n");
> +		pr_cont(" registration number %pM, CRC %02x", serial, crc);
> +	pr_cont(".\n");

This code would be more sensible as

	if (type != unknown)
		pr_info("Found %s NIC registration number %pM, CRC %02x\n",
			type, serial, crc);
	else
		pr_info("Found %s NIC\n", type); 

Though I don't know if registration number is actually a MAC
address or something else.  If it's just a 6 byte identifier
that uses colon separation it should probably use "%6phC"
instead of "%pM"

[] 

> @@ -645,22 +636,21 @@ static inline void ioc3_tx(struct net_device *dev)
>  static void ioc3_error(struct net_device *dev, u32 eisr)
>  {
>  	struct ioc3_private *ip = netdev_priv(dev);
> -	unsigned char *iface = dev->name;
>  
>  	spin_lock(&ip->ioc3_lock);
>  
>  	if (eisr & EISR_RXOFLO)
> -		printk(KERN_ERR "%s: RX overflow.\n", iface);
> +		netdev_err(dev, "RX overflow.\n");
>  	if (eisr & EISR_RXBUFOFLO)
> -		printk(KERN_ERR "%s: RX buffer overflow.\n", iface);
> +		netdev_err(dev, "RX buffer overflow.\n");
>  	if (eisr & EISR_RXMEMERR)
> -		printk(KERN_ERR "%s: RX PCI error.\n", iface);
> +		netdev_err(dev, "RX PCI error.\n");
>  	if (eisr & EISR_RXPARERR)
> -		printk(KERN_ERR "%s: RX SSRAM parity error.\n", iface);
> +		netdev_err(dev, "RX SSRAM parity error.\n");
>  	if (eisr & EISR_TXBUFUFLO)
> -		printk(KERN_ERR "%s: TX buffer underflow.\n", iface);
> +		netdev_err(dev, "TX buffer underflow.\n");
>  	if (eisr & EISR_TXMEMERR)
> -		printk(KERN_ERR "%s: TX PCI error.\n", iface);
> +		netdev_err(dev, "TX PCI error.\n");

All of these should probably be ratelimited() output.