eepro100 macro danger

[Stephane Eranian]

[This message has been sent directly to Donald. I repost it here for
 the broader audience].

Hi,

I am working on the Linux/ia64 port of Linux. You
may recall me as we talked at the HP booth during
the OpenSource conference this summer. 

I found the following danger in the current code of
the eepro100 driver in 2.3.26 (and older versions).

The way the macro clear_suspend() is defined IS NOT
SAFE. We ran into a problem with this driver because
the macro defaults to a non-atomic operation is case
we don't compile for i386 or alpha:

/* Do atomically if possible. */
#if defined(__i386__) || defined(__alpha__)
#define clear_suspend(cmd)   clear_bit(30, &(cmd)->cmd_status)
#elif defined(__powerpc__)
#define clear_suspend(cmd)	clear_bit(6, &(cmd)->cmd_status)
#else
#define clear_suspend(cmd)	(cmd)->cmd_status &= cpu_to_le32(~CmdSuspend)
#endif


This code is extremely dangerous. You either REQUIRE atomicity or you DON'T 
but it's not 'maybe if I can do it'. So on IA-64 we were falling into the
3 case which is clearly not atomic. While I understand why you need
the #if/#else statement, I think it would be better to write it:

#if defined(__i386__) || defined(__alpha__)
#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() operation for your platform"
#endif   

Or at least you should print a warning message.

What do you think ?

+--------------------------------------------------------------------+
| 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                    |                          |
+--------------------------------------------------------------------+