Re: [patch 2.6.24-git] net/enc28j60: low power mode

[Claudio Lanconelli <lanconelli.claudio@eptar.com>]

David Brownell wrote:
> The driver seemed to already have some goofage there:
>
>   # ifconfig eth1 up
>   net eth1: link down
>   net eth1: link down
>   net eth1: normal mode
>   net eth1: multicast mode
>   net eth1: multicast mode
>   #
>
>   
Without low power patch I have:

# ifconfig eth0 up
net eth0: link down
net eth0: normal mode
net eth0: multicast mode
net eth0: multicast mode
# net eth0: link up - Half duplex

> I'd normally expect it not to go down when told to go up, and
> then only to do the multicast thing once ...
>   
multicast is called by network stacks, no control by the driver. The driver
just print message.
I don't know why enc28j60_set_multicast_list() are called more than once.

When you do an ifconfig up the driver reset the chip, so you see link down
before link up message.

>
>   
>> In such cases If I dump the counters with ifconfig I got rx error 
>> counter > 0 and the RX and TX packets counters are freezed.
>> Actually I don't know what causes the freeze, it needs investigation. 
>> The cause can be the rx error condition or the power down/up commands.
>> May be receiving packets while it's going to wakeup causes problems.
>>     
>
> The enc28j60_setlink() was odd too.  It insists the link be down
> before changing duplex, then brings the link up ... so I had to
> put it down again to maintain the "lowpower if not up" invariant.
>
> But the way it brings the link up is to enc28j60_hw_init(), which
> it also does when bringing the link up.  So there's no need to
> bring the link up when changing duplex ...
>
>
>   
I don't follow you anymore.
To change duplex mode the link must be down.
enc28j60_setlink() reinitialize the chip with new duplex mode,
enc28j60_hw_init() never brings link up.

I propose you to add set_lowpower(true) in the enc28j60_probe() and in 
the enc28j60_net_close() after enc28j60_hw_disable().
Probably we don't need to set_lowpower(false) in enc28j60_net_open() since
it performs a soft reset with enc28j60_hw_init().
Do you agree?


>>
>> use
>> if(netif_msg_drv(priv)) ...
>> Doing so we can switch on/off messages runtime using ethtool.
>>     
>
> OK, although there's still a role for "-DDEBUG" compile-time
> mesage removal.
>
>   
It's ok to use

if(netif_msg_drv(priv)) dev_dbv(...


> This driver also abuses __FUNCTION__ (general policy:  don't use it)
>   
Why?
> Then there should be a single routine for all such busy-wait loops,
> so each such usage doesn't need to be open-coded.  Less space, and
> more obviously correct.  I'll add one and make phy_read() use it too.
>
>   
Ok