eepro100

[Stephane Eranian]

Donald,

> > The way the macro clear_suspend() is defined IS NOT
> > SAFE. We ran into a problem with this driver because
> 
> It has been changed to 
> #define clear_suspend(cmd)   ((char *)(&(cmd)->cmd_status))[3] &= ~0x40
> for little-endian machines.
> 
How is this better than:

#define clear_suspend(cmd)   clear_bit(30, &(cmd)->cmd_status)

with regards to atomicity ?

You clearly need atomicity here, unless I am missing something.
My understanding of the operation is:
	- The suspend bit is the marker for the end-of-transmit list.
	  When the chip reaches this point. It enters the suspended state
	  and interrupts.

	- When you want to add packets to the list, you need to extend the list.
	  Because you're using a fixed ring buffer, you don't need to link in your
	  new list. However, you need to tell the chip that the end of list has now
	  moved (one entry farther). That's why you need to clear the Suspend
	  bit of the previous tail.

	  You need atomicity because the chip is working in parallel on the list
	  otherwise you have a race condition.

	  Is this correct ?

	  The bug we were getting by using the:

#define clear_suspend(cmd)	(cmd)->cmd_status &= cpu_to_le32(~CmdSuspend)

	  form proves the atomicity requirement.


	   The consequence of the operation not being atomic was that we were
	   getting timeouts because no transmission had completed for the 
	   last 2seconds (speedo_timer()). At that point speedo_show_state()
	   was saying:

eth0: Transmit timed out: status 0050  0070 at 3319/3331 command 000c0000.
eth0: Tx ring dump,  Tx queue 3331 / 3319:
eth0:   0 000ca000.
eth0:   1 000ca000.
eth0:   2 400ca000.
eth0:  =3 000ca000.
eth0:   4 000ca000.
eth0:   5 000ca000.
eth0:   6 000ca000.
eth0:   7 000ca000.
eth0:   8 000ca000.
eth0:   9 000ca000.
eth0:   10 000ca000.
eth0:   11 000ca000.
eth0:   12 000ca000.
eth0:   13 000ca000.
eth0:   14 000ca000.
eth0:   15 000ca000.
eth0:   16 000ca000.
eth0:   17 000ca000.
eth0:   18 000ca000.
eth0:   19 000ca000.
eth0:   20 000ca000.
eth0:   21 000ca000.
eth0:   22 000ca000.
eth0: * 23 000c0000.
eth0:   24 000ca000.
eth0:   25 000ca000.
eth0:   26 000ca000.
eth0:   27 000ca000.
eth0:   28 000ca000.
eth0:   29 000ca000.
eth0:   30 000ca000.
eth0:   31 000ca000.
  PHY index 1 register 0 is 3000.
  PHY index 1 register 1 is 782d.
  PHY index 1 register 2 is 02a8.
  PHY index 1 register 3 is 0154.
  PHY index 1 register 4 is 05e1.
  PHY index 1 register 5 is 0081.
  PHY index 1 register 21 is 0000.
eth0: Trying to restart the transmitter...

So I still think the code should rather look like this:


#if defined(__i386__) || defined(__alpha__) /* || defined(__ia64__)*/
#define clear_suspend(cmd)   clear_bit(30, &(cmd)->cmd_status)
#elif defined(__powerpc__)
#define clear_suspend(cmd)      clear_bit(6, &(cmd)->cmd_status)
#else
#error "You need the clear_bit() atomic operation for your platform"
#endif


+--------------------------------------------------------------------+
| Ste'phane ERANIAN                       | Email eranian@hpl.hp.com |
| Hewlett-Packard Laboratories            |                          |
| 1501, Page Mill Road MS 1U-15           |                          |
| Palo  Alto, CA 94303-096                |                          |
| USA                                     |                          |
| Tel : (650) 857-7174                    |                          |
| Fax : (650) 857-5548                    |                          |
+--------------------------------------------------------------------+