Le 24.12.2004 22:22, Alan Stern a écrit :
Thanks to both of you for reporting the "Badness in uhci_map_status" warnings and for sending your disassembled listings. It turns out the problem is caused not by a compiler bug, but by the compiler doing generating unexpected code.
The actual error might have occurred at any of several places, but the mechanism is the same in each case. In uhci_result_control are some lines
that say:
status = uhci_status_bits(td_status(td)); if (status & TD_CTRL_ACTIVE) return -EINPROGRESS;
The corresponding disassembled code from Ivars is:
25cf: 8b 59 04 mov 0x4(%ecx),%ebx 25d2: 81 e3 00 00 f6 00 and $0xf60000,%ebx 25d8: f6 41 06 80 testb $0x80,0x6(%ecx) 25dc: 0f 85 c7 00 00 00 jne 26a9 <uhci_result_control+0x119>
Laurent's code is similar. (And by the way, both are different from the code I get because I'm using an older version of gcc.)
In case you're not familiar with x86 assembler, the first two lines compute uhci_status_bits(td_status(td)) by loading the memory address offset by 4 from the address stored in the ecx register and then masking the value with 0xf60000. The third line computes (status & TD_CTRL_ACTIVE) not by testing the value just computed, as you might expect, but by testing the original location in memory.
The problem arises because the UHCI controller can modify the value in memory between the time it is first loaded and the time of the test. So the TD_CTRL_ACTIVE bit actually is set in the value stored in "status" but it is not set in the value stored in memory. This accounts for the warning messages.
Alan, congratulations on having found the source of this problem. It was far from being obvious.
Doing the test in this unexpected way is a valid compiler optimization, but in this case we want to prevent it. Please try using the patch below. In fact, please post the disassembly listing for uhci-hcd.o after applying the patch; it's the best way to make sure the compiled code does what we want. (There's no need to include the listing for the entire module. To cut down on email traffic, you can send just the portion corresponding to the uhci_result_control routine.)
Alan Stern
Please see attached listing (kernel 2.6.10-rc3-mm1 compiled with -Os). Here is a few relevant lines (I guess) :
1f6a: 8b 46 04 mov 0x4(%esi),%eax
1f6d: 89 c1 mov %eax,%ecx
1f6f: 81 e1 00 00 f6 00 and $0xf60000,%ecx
1f75: a9 00 00 80 00 test $0x800000,%eax
1f7a: 0f 85 86 00 00 00 jne 2006 <uhci_result_control+0x116>I must say the badness never occured again since nov. 15th (kernel 2.6.10-rc1-mm5). And now It will be difficult to trigger since I do not use an USB ADSL modem anymore (I received a new _ethernet_ ADSL modem 3 weeks ago). Maybe with my brand new USB webcam...
--- a/drivers/usb/host/uhci-hcd.c Mon Dec 20 09:45:03 2004
+++ b/drivers/usb/host/uhci-hcd.c Fri Dec 24 15:35:22 2004
@@ -236,11 +236,11 @@
{
struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
struct uhci_td *td;
- u32 *plink;
+ __le32 *plink;
/* Ordering isn't important here yet since the QH hasn't been */
/* inserted into the schedule yet */
- plink = &qh->element;
+ plink = (__le32 *) &qh->element;
list_for_each_entry(td, &urbp->td_list, list) {
*plink = cpu_to_le32(td->dma_handle) | breadth;
plink = &td->link;
--- a/drivers/usb/host/uhci-hcd.h Mon Dec 20 09:45:03 2004
+++ b/drivers/usb/host/uhci-hcd.h Fri Dec 24 15:31:16 2004
@@ -106,7 +106,7 @@
struct uhci_qh {
/* Hardware fields */
__le32 link; /* Next queue */
- __le32 element; /* Queue element pointer */
+ volatile __le32 element; /* Queue element pointer */
/* Software fields */
dma_addr_t dma_handle;
@@ -186,7 +186,7 @@
struct uhci_td {
/* Hardware fields */
__le32 link;
- __le32 status;
+ volatile __le32 status;
__le32 token;
__le32 buffer;
-- laurent
00001ef0 <uhci_result_control>:
1ef0: 55 push %ebp
1ef1: 89 e5 mov %esp,%ebp
1ef3: 57 push %edi
1ef4: 56 push %esi
1ef5: 53 push %ebx
1ef6: 83 ec 0c sub $0xc,%esp
1ef9: 89 55 ec mov %edx,0xffffffec(%ebp)
1efc: 89 45 f0 mov %eax,0xfffffff0(%ebp)
1eff: 8b 42 04 mov 0x4(%edx),%eax
1f02: ba ea ff ff ff mov $0xffffffea,%edx
1f07: 89 c7 mov %eax,%edi
1f09: 89 45 e8 mov %eax,0xffffffe8(%ebp)
1f0c: 8b 40 10 mov 0x10(%eax),%eax
1f0f: 83 c7 10 add $0x10,%edi
1f12: 39 f8 cmp %edi,%eax
1f14: 0f 84 8c 01 00 00 je 20a6 <uhci_result_control+0x1b6>
1f1a: 8b 4d e8 mov 0xffffffe8(%ebp),%ecx
1f1d: f6 41 18 08 testb $0x8,0x18(%ecx)
1f21: 74 08 je 1f2b <uhci_result_control+0x3b>
1f23: 8b 5f 04 mov 0x4(%edi),%ebx
1f26: e9 b1 00 00 00 jmp 1fdc <uhci_result_control+0xec>
1f2b: 8d 70 e4 lea 0xffffffe4(%eax),%esi
1f2e: 89 c3 mov %eax,%ebx
1f30: ba 8d ff ff ff mov $0xffffff8d,%edx
1f35: 8b 46 04 mov 0x4(%esi),%eax
1f38: 89 c1 mov %eax,%ecx
1f3a: 81 e1 00 00 f6 00 and $0xf60000,%ecx
1f40: a9 00 00 80 00 test $0x800000,%eax
1f45: 0f 85 5b 01 00 00 jne 20a6 <uhci_result_control+0x1b6>
1f4b: 85 c9 test %ecx,%ecx
1f4d: 0f 85 c4 00 00 00 jne 2017 <uhci_result_control+0x127>
1f53: 8b 45 ec mov 0xffffffec(%ebp),%eax
1f56: c7 40 38 00 00 00 00 movl $0x0,0x38(%eax)
1f5d: 8b 1b mov (%ebx),%ebx
1f5f: eb 77 jmp 1fd8 <uhci_result_control+0xe8>
1f61: 39 3b cmp %edi,(%ebx)
1f63: 74 77 je 1fdc <uhci_result_control+0xec>
1f65: 8d 73 e4 lea 0xffffffe4(%ebx),%esi
1f68: 8b 1b mov (%ebx),%ebx
1f6a: 8b 46 04 mov 0x4(%esi),%eax
1f6d: 89 c1 mov %eax,%ecx
1f6f: 81 e1 00 00 f6 00 and $0xf60000,%ecx
1f75: a9 00 00 80 00 test $0x800000,%eax
1f7a: 0f 85 86 00 00 00 jne 2006 <uhci_result_control+0x116>
1f80: 8b 46 04 mov 0x4(%esi),%eax
1f83: 8b 55 ec mov 0xffffffec(%ebp),%edx
1f86: 40 inc %eax
1f87: 25 ff 07 00 00 and $0x7ff,%eax
1f8c: 01 42 38 add %eax,0x38(%edx)
1f8f: 85 c9 test %ecx,%ecx
1f91: 0f 85 80 00 00 00 jne 2017 <uhci_result_control+0x127>
1f97: 8b 4e 08 mov 0x8(%esi),%ecx
1f9a: 8b 56 04 mov 0x4(%esi),%edx
1f9d: 89 c8 mov %ecx,%eax
1f9f: 42 inc %edx
1fa0: c1 e8 15 shr $0x15,%eax
1fa3: 81 e2 ff 07 00 00 and $0x7ff,%edx
1fa9: 40 inc %eax
1faa: 25 ff 07 00 00 and $0x7ff,%eax
1faf: 39 c2 cmp %eax,%edx
1fb1: 73 25 jae 1fd8 <uhci_result_control+0xe8>
1fb3: 8b 45 ec mov 0xffffffec(%ebp),%eax
1fb6: f6 40 28 01 testb $0x1,0x28(%eax)
1fba: 75 54 jne 2010 <uhci_result_control+0x120>
1fbc: 31 d2 xor %edx,%edx
1fbe: 80 f9 69 cmp $0x69,%cl
1fc1: 0f 85 df 00 00 00 jne 20a6 <uhci_result_control+0x1b6>
1fc7: 89 c2 mov %eax,%edx
1fc9: 8b 45 f0 mov 0xfffffff0(%ebp),%eax
1fcc: 8d 65 f4 lea 0xfffffff4(%ebp),%esp
1fcf: 5b pop %ebx
1fd0: 5e pop %esi
1fd1: 5f pop %edi
1fd2: c9 leave
1fd3: e9 f8 fe ff ff jmp 1ed0
<usb_control_retrigger_status>
1fd8: 39 fb cmp %edi,%ebx
1fda: 75 85 jne 1f61 <uhci_result_control+0x71>
1fdc: 8d 73 e4 lea 0xffffffe4(%ebx),%esi
1fdf: ba 8d ff ff ff mov $0xffffff8d,%edx
1fe4: 8b 4e 04 mov 0x4(%esi),%ecx
1fe7: 81 e1 00 00 f6 00 and $0xf60000,%ecx
1fed: 89 c8 mov %ecx,%eax
1fef: c1 e8 17 shr $0x17,%eax
1ff2: 85 c0 test %eax,%eax
1ff4: 0f 85 ac 00 00 00 jne 20a6 <uhci_result_control+0x1b6>
1ffa: 31 d2 xor %edx,%edx
1ffc: 85 c9 test %ecx,%ecx
1ffe: 0f 84 a2 00 00 00 je 20a6 <uhci_result_control+0x1b6>
2004: eb 11 jmp 2017 <uhci_result_control+0x127>
2006: ba 8d ff ff ff mov $0xffffff8d,%edx
200b: e9 96 00 00 00 jmp 20a6 <uhci_result_control+0x1b6>
2010: bf 87 ff ff ff mov $0xffffff87,%edi
2015: eb 12 jmp 2029 <uhci_result_control+0x139>
2017: 31 d2 xor %edx,%edx
2019: 80 7e 08 69 cmpb $0x69,0x8(%esi)
201d: 89 c8 mov %ecx,%eax
201f: 0f 95 c2 setne %dl
2022: e8 13 fc ff ff call 1c3a <uhci_map_status>
2027: 89 c7 mov %eax,%edi
2029: 8b 0d 00 00 00 00 mov 0x0,%ecx
202f: 83 f9 01 cmp $0x1,%ecx
2032: 0f 94 c2 sete %dl
2035: 31 c0 xor %eax,%eax
2037: 83 ff e0 cmp $0xffffffe0,%edi
203a: 0f 95 c0 setne %al
203d: 85 c2 test %eax,%edx
203f: 75 03 jne 2044 <uhci_result_control+0x154>
2041: 49 dec %ecx
2042: 7e 60 jle 20a4 <uhci_result_control+0x1b4>
2044: 8b 15 08 00 00 00 mov 0x8,%edx
204a: 85 d2 test %edx,%edx
204c: 74 56 je 20a4 <uhci_result_control+0x1b4>
204e: 8b 4d e8 mov 0xffffffe8(%ebp),%ecx
2051: 8b 41 0c mov 0xc(%ecx),%eax
2054: 6a 00 push $0x0
2056: b9 00 80 00 00 mov $0x8000,%ecx
205b: e8 c4 e5 ff ff call 624 <uhci_show_qh>
2060: 8b 15 08 00 00 00 mov 0x8,%edx
2066: 59 pop %ecx
2067: 85 d2 test %edx,%edx
2069: 74 39 je 20a4 <uhci_result_control+0x1b4>
206b: b8 0a 00 00 00 mov $0xa,%eax
2070: 89 d6 mov %edx,%esi
2072: 88 c4 mov %al,%ah
2074: ac lods %ds:(%esi),%al
2075: 38 e0 cmp %ah,%al
2077: 74 09 je 2082 <uhci_result_control+0x192>
2079: 84 c0 test %al,%al
207b: 75 f7 jne 2074 <uhci_result_control+0x184>
207d: be 01 00 00 00 mov $0x1,%esi
2082: 89 f0 mov %esi,%eax
2084: 48 dec %eax
2085: 85 c0 test %eax,%eax
2087: 89 c3 mov %eax,%ebx
2089: 74 03 je 208e <uhci_result_control+0x19e>
208b: c6 00 00 movb $0x0,(%eax)
208e: 52 push %edx
208f: 68 5f 07 00 00 push $0x75f
2094: e8 fc ff ff ff call 2095 <uhci_result_control+0x1a5>
2099: 85 db test %ebx,%ebx
209b: 58 pop %eax
209c: 5a pop %edx
209d: 89 da mov %ebx,%edx
209f: 74 03 je 20a4 <uhci_result_control+0x1b4>
20a1: 42 inc %edx
20a2: eb c5 jmp 2069 <uhci_result_control+0x179>
20a4: 89 fa mov %edi,%edx
20a6: 8d 65 f4 lea 0xfffffff4(%ebp),%esp
20a9: 89 d0 mov %edx,%eax
20ab: 5b pop %ebx
20ac: 5e pop %esi
20ad: 5f pop %edi
20ae: c9 leave
20af: c3 ret
signature.asc
Description: OpenPGP digital signature
