Re: [PATCH] forcedeth: new backoff implementation

[Manfred Spraul <manfred@colorfullife.com>]

Andrew Morton wrote:
> On Mon, 07 Apr 2008 17:40:28 -0400 Ayaz Abdulla <aabdulla@nvidia.com> wrote:
>   
>> +static void nv_gear_backoff_reseed(struct net_device *dev)
>> +{
>> +	u8 __iomem *base = get_hwbase(dev);
>> +	u32 miniseed1, miniseed2, miniseed2_reversed, miniseed3, miniseed3_reversed;
>> +	u32 temp, seedset, combinedSeed;
>> +	int i;
>> +
>> +	/* Setup seed for free running LFSR */
>> +	/* We are going to read the time stamp counter 3 times and swizzle bits around to increase randomness */
>>     
>
> I see this driver rigorously observes the 800-column-xterm convention.
>
>   
>> +	rdtscl(miniseed1);
>>     
>
> err, no.  We don't have sufficient Kconfig dependencies in place to be able
> to do this.  You just broke all non-x86.
>
> Suitable fixes would be
>
> a) add #ifdef CONFIG_X86 all over the place
>
> b) use do_gettimeofday() (might be tricky if called from interrupt)
>
> c) use cpu_clock().
>
>   
d) use get_random_bytes()?
It's irq-safe, used e.g. by the network code for the TCP sequence numbers.
>> +	/* Ensure seeds are not the same */
>> +	if ((combinedSeed & NVREG_BKOFFCTRL_SEED_MASK) ==
>> +	    (combinedSeed & (NVREG_BKOFFCTRL_SEED_MASK << NVREG_BKOFFCTRL_GEAR)))
>> +		combinedSeed = 0x3FF3FF;
>> +
>>     
I don't understand this block: The seed consists of two parts, each 12 bits.
Both parts mustn't be identical. If both parts are identical, then both 
are set to 0x3ff.
But if both are 0x3ff, then they are identical. Is that intentional?

--
    Manfred