Messages in this thread | | | Date | Tue, 09 Dec 2003 01:22:09 -0800 | From | Paul Menage <> | Subject | ACPI global lock macros |
| |
Hi Andy,
The ACPI_ACQUIRE_GLOBAL_LOCK() macro in include/asm-i386/acpi.h looks a little odd:
#define ACPI_ACQUIRE_GLOBAL_LOCK(GLptr, Acq) \ do { \ int dummy; \ asm("1: movl (%1),%%eax;" \ "movl %%eax,%%edx;" \ "andl %2,%%edx;" \ "btsl $0x1,%%edx;" \ "adcl $0x0,%%edx;" \ "lock; cmpxchgl %%edx,(%1);" \ "jnz 1b;" \ "cmpb $0x3,%%dl;" \ "sbbl %%eax,%%eax" \ :"=a"(Acq),"=c"(dummy):"c"(GLptr),"i"(~1L):"dx"); \ } while(0)
When compiled, it results in:
266: mov 0x0,%ecx 268: R_386_32 acpi_gbl_common_fACS 26c: mov (%ecx),%eax 26e: mov %eax,%edx 270: and %ecx,%edx 272: bts $0x1,%edx 276: adc $0x0,%edx 279: lock cmpxchg %edx,(%ecx) 27d: jne 26c <acpi_ev_acquire_global_lock+0x2f> 27f: cmp $0x3,%dl 282: sbb %eax,%eax
So at location 270 we mask %edx with %ecx, which is the address of the global lock. Unless the global lock is aligned on a 2-byte but not 4-byte boundary, which seems a little unlikely, then this is going to clear both the owned and the pending bits in %edx, so we'll always think that the lock is not owned. Shouldn't the andl be masking with %3 (which is initialised as ~1) rather than %2 (the address of the lock)?
Given the comments above the definition, I'm guessing that the "dummy" parameter was added later for some reason (to tell gcc that ecx would get clobbered? - but it doesn't seem to be clobbered), and the parameter substitutions in the asm weren't updated. Unless I'm missing something fundamental, shouldn't the definition be something more like this:
#define ACPI_ACQUIRE_GLOBAL_LOCK(GLptr, Acq) \ do { \ asm volatile("1:movl (%1),%%eax;" \ "movl %%eax,%%edx;" \ "andl %2,%%edx;" \ "btsl $0x1,%%edx;" \ "adcl $0x0,%%edx;" \ "lock; cmpxchgl %%edx,(%1);" \ "jnz 1b;" \ "cmpb $0x3,%%dl;" \ "sbbl %0,%0" \ :"=r"(Acq):"r"(GLptr),"i"(~1L):"dx", "ax"); \ } while(0)
which compiles to:
2e5: mov 0x0,%ecx 2e7: R_386_32 acpi_gbl_common_fACS 2eb: mov (%ecx),%eax 2ed: mov %eax,%edx 2ef: and $0xfffffffe,%edx 2f2: bts $0x1,%edx 2f6: adc $0x0,%edx 2f9: lock cmpxchg %edx,(%ecx) 2fd: jne 2eb <acpi_ev_acquire_global_lock+0x37> 2ff: cmp $0x3,%dl 302: sbb %cl,%cl
which is identical to the ACPI spec reference implementation, apart from returning the result in %cl rather than %al (since we're cleanly separating clobbered registers from input/output params, and letting gcc choose the param registers).
Alternatively it could be defined in C (as in ia64) which would reduce the likelihood of asm bugs. (Although it wouldn't be safe to use __cmpxchg(), as that uses LOCK_PREFIX which is empty on UP, rather than an explicit "lock").
ACPI_RELEASE_GLOBAL_LOCK(), and the x86_64 variants of these, seem to have similar issues.
Paul
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |