Re: [PATCH] new device support for forcedeth.c second try

[Manfred Spraul]

Carl-Daniel Hailfinger wrote:

> Hi,
>
> Brian, thank you very much for contributing to forcedeth.
I agree, thanks a lot.

> 	NvRegOffloadConfig = 0x90,
> #define NVREG_OFFLOAD_HOMEPHY	0x601
> -#define NVREG_OFFLOAD_NORMAL	0x5ee
> +#define NVREG_OFFLOAD_NORMAL	RX_NIC_BUFSIZE
>  
Interesting - does that explain why VLAN doesn't work properly? I have a 
report that maximum sized packets are rejected.

> +		struct {
> +			u32 Length:14;
> +			u32 Flags:18;
> +		} v2;
>  
Bitfields for hw access are evil, it caused problems before. I'd prefer 
a macro with explicit shifts.

> +
> +	//wait for 500ms
> +	mdelay(500);
>  
Waiting for phy reset is also evil - it should be done either in a 
separate thread or asynchroneously. Not urgent, we can fix it later.

> +
> +	// check auto negotiation is complete
> +	mii_status = mii_rw(dev, np->phyaddr, MII_BMSR, MII_READ);
> +	while (!(mii_status & BMSR_ANEGCOMPLETE)) {
> +		udelay(NV_MIIBUSY_DELAY);
> +		mii_status = mii_rw(dev, np->phyaddr, MII_BMSR, MII_READ);
> +		microseconds++;
> +		if (microseconds == 20) {
> +			microseconds = 0;
> +			milliseconds++;
> +		}
> +		if (milliseconds > 1200) {
> +			printk(KERN_INFO "%s: phy init failed to autoneg.\n", dev->name);
> +			return PHY_TIMEOUT;
> +		}
>  
Dito.

The phy code needs a big rewrite and support for ethtool anyway. I'd 
propose to merge the patch after removing the bitfields - everything 
else looks good. Perhaps there is a bit too much code duplication with 
the v1/v2 functions, but that's also not fatal.

--
   Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/