netdev - Re: [PATCH] - e1000_ethtool.c - convert macros to functions

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: <47292055.5070107@intel.com>
Date:	Wed, 31 Oct 2007 17:39:49 -0700
From:	"Kok, Auke" <auke-jan.h.kok@...el.com>
To:	Joe Perches <joe@...ches.com>
Cc:	"Kok, Auke" <auke-jan.h.kok@...el.com>,
	netdev <netdev@...r.kernel.org>,
	e1000-devel@...ts.sourceforge.net, Jeff Garzik <jgarzik@...ox.com>
Subject: Re: [PATCH] - e1000_ethtool.c - convert macros to functions

Joe Perches wrote:
> On Wed, 2007-10-31 at 14:30 -0700, Kok, Auke wrote:
>> Joe Perches wrote:
>> that's not a bad idea, however see below:
>> can't we keep the macro here (and just make it call the function instead of
>> expanding). the resulting code is much more lenghty and contains all these logic
>> traps that the previous code didn't have.
>> just have the macro expand to `if (reg_pattern_test(...)) return 1)` and you don't
>> need to change any of the calling lines.
> 
> You could define something like:
> 
> #define REG_PATTERN_TEST(reg, mask, write) \
> 	if (reg_pattern_test(adapter, data, \
> 			     E1000_REG(&adapter->hw, reg), \
> 			     mask, write)) \
> 		return 1;
> 
> But isn't the macro with an embedded return a bit too obfuscating?

in this case it saves a bunch of time reading and makes the code actually more
readable and robust IMO. Now we're explicitly mobming out on an error. with your
patch we need to assure that for every call to reg_pattern_test we check the
return value.

>>> +#define E1000_READ_REG_ARRAY(a, reg, offset)		\
>>> +	(readl((a)->hw_addr +				\
>>> +	       (((a)->mac_type >= e1000_82543)		\
>>> +		? E1000_##reg : E1000_82542_##reg) +	\
>>> +	       ((offset) << 2)))
>> did you have to change these macro's ?
> 
> No.  Your choice to keep/remove.
> I did want to use the E1000_REG or a new E1000_REG_ADDR macro.
> 
>> also, I'm a bit inclined to prefer a patch for e1000e for now as we're about to
>> move the pci-express hardware over, but we can certainly merge something like this
>> in e1000 after the move as well.
> 
> Simple enough.
> 
> When is e1000e scheduled to be part of defconfig?

I don't know about defconfig, but you can already get e1000 in 2.6.24rc1...

Auke

-
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