rwlock and atomic_sub on ia64

I'm in the process of adding support for the ia64 architecture to the message/fusion/mptlan.c driver (it's previously only been tested with i386 and sparc64 systems) and have come across a couple of issues:

1) there doesn't appear to be support for rwlocks under ia64. At least, I can't find any way to make the above driver link when it's got code that uses rwlocks.

and

2) atomic_sub looks to require that I cast a u8 to (int) when I pass it in as a parameter. Not a major issue, I just found it odd.

I'm most likely missing something (didn't see anything about either of the above when I looked through the list archive), but I was wondering if I could get a pointer or two from those with more knowledge of ia64/linux issues.

Thanks.

I'm not currently subscribed to linux-ia64, so please cc: me on replies.

Noah Romer
Driver Developer, CM gopher and Linux Whipping Boy
Storage Components Firmware
LSI Logic Corp.

Noah> I'm in the process of adding support for the ia64 architecture Noah> to the message/fusion/mptlan.c driver (it's previously only been Noah> tested with i386 and sparc64 systems) and have come across a Noah> couple of issues:

Is this a kernel driver or a userland application? If it's the latter then you are not allowed to rely on kernel definitions etc.

Noah> 2) atomic_sub looks to require that I cast a u8 to (int) when I Noah> pass it in as a parameter. Not a major issue, I just found it Noah> odd.

Noah> I'm most likely missing something (didn't see anything about Noah> either of the above when I looked through the list archive), but Noah> I was wondering if I could get a pointer or two from those with Noah> more knowledge of ia64/linux issues.

You are talking about the subtracted argument I take it? It's int on most architectures, including Linux/i386:

static __inline__ void atomic_sub(int i, atomic_t *v)

{
	__asm__ __volatile__(
		LOCK "subl %1,%0"
		:"=m" (v->counter)
		:"ir" (i), "m" (v->counter));
}

Jes

This is a kernel driver (linux/drivers/message/fusion). The version in 2.4.7 isn't up to date (and doesn't contain the code that uses rwlocks), but it's there, finally.

I've been poking around in the kernel with Source Navigator and have found implementations of rwlocks in Alpha, i386, Mips, Mips64, PArisc and Sparc, but not for ia64, yet. If it's not there yet, I can just ifdef around the code for ia64 (it's in a bit of code that works around QLogic's "alternative" view of RFC 2625 and the Fibre Channel protocol specs), but I thought I'd check first.

Correct.

Yep, but, for some reason, I've never gotten any complaints from the compiler until I tried compiling on ia64. It's not a big issue (especially since the subtracted value in this instance should never be big enough that signed/unsigned will make a diff), as I can just cast it to int. It just struck me as strange that the compiler would now decide to issue a warning. The rwlock bit is what's got me hung up.

Thanks.

Noah Romer
Driver Developer, CM gopher and Linux Whipping Boy
Storage Components Firmware
LSI Logic Corp.

Noah> This is a kernel driver (linux/drivers/message/fusion). The Noah> version in 2.4.7 isn't up to date (and doesn't contain the code Noah> that uses rwlocks), but it's there, finally.

Ok, I saw Dave Miller checked it into his CVS tree yesterday or so.

Noah> Yep, but, for some reason, I've never gotten any complaints from Noah> the compiler until I tried compiling on ia64. It's not a big Noah> issue (especially since the subtracted value in this instance Noah> should never be big enough that signed/unsigned will make a Noah> diff), as I can just cast it to int. It just struck me as Noah> strange that the compiler would now decide to issue a Noah> warning. The rwlock bit is what's got me hung up.

The reason for this is most likely the compiler versions. For Linux/ia64 you have to use at least gcc-2.96 to compile the kernel. This compiler is a lot more picky about data types than the older versions such as egcs-1.1.2/gcc-2.95 etc.

I'll leave the question about rwsem's to David ;)

Cheers
Jes

Ok, I wondered if it had anything to do w/ the newer compiler that came w/ Redhat's ia64 beta. Most of my i386 compiles have been done w/, um, egcs 2.91.66. I guess it's time to update those systems.

Thanks.

Noah Romer
Driver Developer, CM gopher and Linux Whipping Boy
Storage Components Firmware
LSI Logic Corp.

Jes> I'll leave the question about rwsem's to David ;)

We're using the generic implementation on ia64.

	--david

Yes, but how do I get to it? I finally found the ia64 implementation of write_lock and read_lock for SMP systems (which is what I'm compiling on).(1) However, when I try to compile the driver I get a

warning: implicit declaration of function `rwlock_init'

message. I have <linux/spinlock.h> included, and that's been enough up until now. Is there someother include file I need under the ia64 arch?

Thanks.

(1) Source Navigator doesn't seem to find it when I just have it look for functions/macros w/ those names, but it does find the rwlock_t declaration in include/asm-ia64/spinlock.h.

Noah Romer
Driver Developer, CM gopher and Linux Whipping Boy
Storage Components Firmware
LSI Logic Corp.

Noah> David Mosberger wrote:

Jes> I'll leave the question about rwsem's to David ;)

Oops, Noah was actually talking about the rwlocks, not about rwsem. For rwlocks, we have our own implementation.

Noah> Yes, but how do I get to it? I finally found the ia64 Noah> implementation of write_lock and read_lock for SMP systems Noah> (which is what I'm compiling on).(1) However, when I try to Noah> compile the driver I get a

Noah> warning: implicit declaration of function `rwlock_init'

Noah> message. I have <linux/spinlock.h> included, and that's been Noah> enough up until now. Is there someother include file I need Noah> under the ia64 arch?

No, that seems to be a genuine omission. rwlock_init() was added in the 2.4.0 test series and I seem to have missed that. There aren't too many drivers that use it. Anyhow, the fix is trivial. Something along the lines of:

@@ -108,6 +108,8 @@
 } rwlock_t;
 #define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }
 
+#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+
 #define read_lock(rw)								\
 do {										\
 	int tmp = 0;								\

should do the trick.

	--david

Yep, that fixes it. I never even noticed that rwlock_init wasn't defined, just saw read_lock, write_lock and rwlock_t and assumed rwlock_init was there. My bad. Thanks.

On another note, I'm still a little puzzled by the warnings I got when using atomic_sub. I was passing a u8 value in as the amount to be subtracted, and got four

mptlan.c:1135: warning: comparison is always false due to limited
range of data type

Now, I can make the warning go away by changing the variable to an int (but not by casting the u8 to an int), but it seems odd that gcc would complain about the size of a u8 variable making the comparison always false. I started poking around in the output for other drivers, and found the above warning for all sorts of code (a lot of it involving bh data types). This is probably a question for the gcc folks, but . . . ;)

Noah Romer
Driver Developer, CM gopher and Linux Whipping Boy
Storage Components Firmware
LSI Logic Corp.

This is because atomic_sub is actually a macro defined in asm-ia64/atomic.h and it compares value against negative values to get optimized code.

So, four comparison against negative numbers makes four warnings for the single atomic_sub.

Here is the quote of atomic_sub

#define atomic_sub_return(i,v)						\
	((__builtin_constant_p(i) &&					\
	  (    ( i =  1) || ( i =  4) || ( i =  8) || ( i = 16)	\
	    || ( i = -1) || ( i = -4) || ( i = -8) || ( i = -16)))	\
	 ? ia64_fetch_and_add(-(i), &(v)->counter)			\
	 : ia64_atomic_sub(i, v))

#define atomic_sub(i,v) atomic_sub_return((i), (v))

KOCHI Takayoshi <[email protected]/[email protected]>

Kochi> This is because atomic_sub is actually a macro defined in Kochi> asm-ia64/atomic.h and it compares value against negative values Kochi> to get optimized code.

Kochi> So, four comparison against negative numbers makes four warnings Kochi> for the single atomic_sub.

Kochi> Here is the quote of atomic_sub

  Kochi> #define atomic_sub_return(i,v)						\
  Kochi> ((__builtin_constant_p(i) &&					\
  Kochi> (    ( i =  1) || ( i =  4) || ( i =  8) || ( i = 16)	\
  Kochi> || ( i = -1) || ( i = -4) || ( i = -8) || ( i = -16)))	\
  Kochi> ? ia64_fetch_and_add(-(i), &(v)->counter)			\
  Kochi> : ia64_atomic_sub(i, v))

Kochi> #define atomic_sub(i,v) atomic_sub_return((i), (v))

That's correct. We could fix this with appropriate casting and then you could use fetchadd1 to increment unsigned variables by 224, 240, 252, 255. However, getting this 100% is rather tricky and I'm not sure I want to play with this in the 2.4 series. For now, I'd recommend to just live with the warning (it's harmless, other than being annoying).

	--david