netdev - Re: [PATCH]: Preliminary release of Sun Neptune driver

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, 19 Sep 2007 15:07:55 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	shemminger@...ux-foundation.org
Cc:	netdev@...r.kernel.org, Ariel.Hendel@....com, greg.onufer@....com,
	jeff@...zik.org
Subject: Re: [PATCH]: Preliminary release of Sun Neptune driver

From: Stephen Hemminger <shemminger@...ux-foundation.org>
Date: Wed, 19 Sep 2007 14:59:00 -0700

> > +#ifndef PCI_DEVICE_ID_SUN_NEPTUNE
> > +#define PCI_DEVICE_ID_SUN_NEPTUNE	0xabcd
> > +#endif
> 
> Why bother defining the ID, and what good does driver_data do you?

Good point, I'll put this 0xabcd thing directly into the
table.

> > +#define nr64(reg)		readq(np->regs + (reg))
> > +#define nw64(val, reg)		writeq((val), np->regs + (reg))
> 
> Macro's that make assumptions about context (ie variable name np)
> are evil and bad style.

Too bad, it saves developers who write 20,000 line drivers from carpel
tunnel symdrome.

It's so repetitive to state things over and over again, and this macro
scheme is 1) established widely (tg3 amonst many other drivers do it)
and 2) forces the driver to use a consistent naming convention for
primary driver private struct variables.

> Wrapping locking is poor style and makes code review harder.

I disagree, because in this case the macro says exactly what the thing
is doing, locking the parent of the port.  The implementation is
arbitrary and can be reviewed independently of the uses.

> > +static int niu_wait_bits_clear_mac(struct niu *np, unsigned long reg, u64 bits,
> > +				   int limit, int delay)
> > +{
> > +	BUILD_BUG_ON(limit <= 0 || delay < 0);
> 
> There is no way compiler can evaluate limit or delay.

Check the callers, they all pass contant values.

> Why are you using __u32?  That is reserved for values going out
> to user space.

Right, I'll fix those up, thanks.
-
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