| 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
| ||
|
Message-ID: <485421C1.8010102@s5r6.in-berlin.de>
Date: Sat, 14 Jun 2008 21:53:37 +0200
From: Stefan Richter <stefanr@...6.in-berlin.de>
To: Németh Márton <nm127@...email.hu>
CC: Jeff Garzik <jgarzik@...ox.com>, netdev@...r.kernel.org,
Trivial Patch Monkey <trivial@...nel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] 8139too: clean up spaces and TABs
Németh Márton wrote:
> From: Márton Németh <nm127@...email.hu>
>
> Clean up the following errors and warnings reported by checkpatch.pl:
Is 8139too in active development or are there people actively fixing
current bugs in it? If not, a whitespace cleanup may be considered a
waste of time. There are even a few valid arguments that such changes
are harmful then.
> - space prohibited between function name and open parenthesis '('
> - space required before the open brace '{'
> - code indent should use tabs where possible
> - line over 80 characters
> - spaces required around that '=' (ctx:VxW)
Did you check that your whitespace changes are indeed only whitespace
changes, i.e. that resulting assembler is unchanged? If you checked it,
it's worth mentioning in the submission.
> Signed-off-by: Márton Németh <nm127@...email.hu>
> ---
> --- a/drivers/net/8139too.c 2008-06-14 00:20:47.000000000 +0200
> +++ b/drivers/net/8139too.c 2008-06-14 20:32:45.000000000 +0200
> @@ -11,7 +11,7 @@
>
> -----<snip>-----
>
> - Written 1997-2001 by Donald Becker.
> + Written 1997-2001 by Donald Becker.
> This software may be used and distributed according to the
> terms of the GNU General Public License (GPL), incorporated
> herein by reference. Drivers based on or derived from this
This is not the canonical multi-line comment style.
> @@ -116,8 +116,8 @@
>
> /* Default Message level */
> #define RTL8139_DEF_MSG_ENABLE (NETIF_MSG_DRV | \
> - NETIF_MSG_PROBE | \
> - NETIF_MSG_LINK)
> + NETIF_MSG_PROBE | \
> + NETIF_MSG_LINK)
>
>
Why change this?
(Besides for the reason that checkpatch says that new patches /should/
use tabs at such occasions, which also isn't universally agreed upon.)
> /* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
> @@ -134,7 +134,8 @@
>
> #if RTL8139_DEBUG
> /* note: prints function name for you */
> -# define DPRINTK(fmt, args...) printk(KERN_DEBUG "%s: " fmt, __FUNCTION__ , ## args)
> +# define DPRINTK(fmt, args...) \
> + printk(KERN_DEBUG "%s: " fmt, __FUNCTION__ , ## args)
> #else
> # define DPRINTK(fmt, args...)
> #endif
This line is merely 6 characters too long and does not contain anything
particularly important. Why bother?
> @@ -143,10 +144,10 @@
> # define assert(expr) do {} while (0)
> #else
> # define assert(expr) \
> - if(unlikely(!(expr))) { \
> - printk(KERN_ERR "Assertion failed! %s,%s,%s,line=%d\n", \
> - #expr,__FILE__,__FUNCTION__,__LINE__); \
> - }
> + if (unlikely(!(expr))) { \
> + printk(KERN_ERR "Assertion failed! %s,%s,%s,line=%d\n", \
> + #expr, __FILE__, __FUNCTION__, __LINE__); \
> + }
> #endif
>
>
> @@ -196,7 +197,9 @@ static int debug = -1;
> Threshold is bytes transferred to chip before transmission starts. */
> #define TX_FIFO_THRESH 256 /* In bytes, rounded down to 32 byte units. */
>
> -/* The following settings are log_2(bytes)-4: 0 == 16 bytes .. 6==1024, 7==end of packet. */
> +/* The following settings are log_2(bytes)-4:
> + * 0 == 16 bytes .. 6==1024, 7==end of packet.
> + */
> #define RX_FIFO_THRESH 7 /* Rx buffer level before first PCI xfer. */
> #define RX_DMA_BURST 7 /* Maximum PCI burst, '6' is 1024 */
> #define TX_DMA_BURST 6 /* Maximum PCI burst, '6' is 1024 */
This is not Linux multi-line comment style. (It comes close to the
present style of 8139too.c though and may therefore be acceptable.)
[...]
> @@ -1253,57 +1280,59 @@ static int mdio_read (struct net_device
> }
>
>
> -static void mdio_write (struct net_device *dev, int phy_id, int location,
> - int value)
> +static void mdio_write(struct net_device *dev, int phy_id, int location,
> + int value)
> {
> struct rtl8139_private *tp = netdev_priv(dev);
> #ifdef CONFIG_8139TOO_8129
> void __iomem *ioaddr = tp->mmio_addr;
> - int mii_cmd = (0x5002 << 16) | (phy_id << 23) | (location << 18) | value;
> + int mii_cmd = (0x5002 << 16) | (phy_id << 23) | (location << 18) |
> + value;
> int i;
This decreases readability. You could leave this merely 81 columns long
line as is. Or remove the superfluous parentheses.
[...]
> @@ -1945,22 +1978,22 @@ static int rtl8139_rx(struct net_device
> rmb();
>
> /* read size+status of next frame from DMA ring buffer */
> - rx_status = le32_to_cpu (*(__le32 *) (rx_ring + ring_offset));
> + rx_status = le32_to_cpu(*(__le32 *) (rx_ring + ring_offset));
> rx_size = rx_status >> 16;
> pkt_size = rx_size - 4;
>
> if (netif_msg_rx_status(tp))
> - printk(KERN_DEBUG "%s: rtl8139_rx() status %4.4x, size %4.4x,"
> - " cur %4.4x.\n", dev->name, rx_status,
> - rx_size, cur_rx);
> + printk(KERN_DEBUG "%s: rtl8139_rx() status %4.4x, "
> + "size %4.4x, cur %4.4x.\n",
> + dev->name, rx_status, rx_size, cur_rx);
> #if RTL8139_DEBUG > 2
> {
> int i;
> - DPRINTK ("%s: Frame contents ", dev->name);
> + DPRINTK("%s: Frame contents ", dev->name);
> for (i = 0; i < 70; i++)
> - printk (" %2.2x",
> - rx_ring[ring_offset + i]);
> - printk (".\n");
> + printk(" %2.2x",
> + rx_ring[ring_offset + i]);
> + printk(".\n");
> }
> #endif
>
This printk trick does not work BTW. But it should of course not be
fixed in a patch which is meant to be a whitespace change only.
[...]
--
Stefan Richter
-=====-==--- -==- -===-
http://arcgraph.de/sr/
--
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
Powered by blists - more mailing lists