Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1351740 > unrolled thread
| Started by | Sedat Dilek <sedat.dilek@gmail.com> |
|---|---|
| First post | 2016-03-07 17:00 +0100 |
| Last post | 2016-03-07 20:20 +0100 |
| Articles | 15 — 7 participants |
Back to article view | Back to linux.kernel
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-07 17:00 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-07 17:30 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Alan Stern <stern@rowland.harvard.edu> - 2016-03-07 17:50 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Jiri Kosina <jikos@kernel.org> - 2016-03-07 18:10 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-07 18:20 +0100
RE: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning David Laight <David.Laight@ACULAB.COM> - 2016-03-07 18:40 +0100
RE: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Alan Stern <stern@rowland.harvard.edu> - 2016-03-07 19:10 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-07 19:40 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Steven Rostedt <rostedt@goodmis.org> - 2016-03-07 18:10 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Steven Rostedt <rostedt@goodmis.org> - 2016-03-07 18:20 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-07 18:20 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Jiri Kosina <jikos@kernel.org> - 2016-03-07 18:30 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Steven Rostedt <rostedt@goodmis.org> - 2016-03-07 18:40 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Andy Lutomirski <luto@amacapital.net> - 2016-03-07 19:10 +0100
Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning Steven Rostedt <rostedt@goodmis.org> - 2016-03-07 20:20 +0100
| From | Sedat Dilek <sedat.dilek@gmail.com> |
|---|---|
| Date | 2016-03-07 17:00 +0100 |
| Subject | Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning |
| Message-ID | <ra5JE-4Li-7@gated-at.bofh.it> |
[Multipart message — attachments visible in raw view] — view raw
On Sun, Mar 6, 2016 at 6:23 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 5 Mar 2016, Sedat Dilek wrote:
>
>> On Fri, Mar 4, 2016 at 5:04 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Wed, 2 Mar 2016, Sedat Dilek wrote:
>> >
>> >> On 3/1/16, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >> > On Tue, 1 Mar 2016, Sedat Dilek wrote:
>> >> >
>> >> >> On Tue, Oct 13, 2015 at 2:57 AM, Steven Rostedt <rostedt@goodmis.org>
>> >> >> wrote:
>> >> >> > On Sat, 3 Oct 2015 12:05:42 +0200
>> >> >> > Sedat Dilek <sedat.dilek@gmail.com> wrote:
>> >> >> >
>> >> >> >> So, at the beginning... dunno WTF is causing the problems - no
>> >> >> >> workaround for CLANG.
>> >> >> >
>> >> >> > Probably need to compile with gcc and with clang and look at the binary
>> >> >> > differences. Or at least what objdump shows.
>> >> >> >
>> >> >>
>> >> >> [ Hope to address this issue to the correct people - CCed some people
>> >> >> I taped on their nerves ]
>> >> >>
>> >> >> Not sure if I should open a new thread?
>> >> >> Please, some clear statements on this.
>> >> >> Thanks.
>> >> >>
>> >> >> The issue is still visible and alive.
>> >
>> > I think it would be worthwhile to doublecheck the time at which
>> > interrupts get disabled. Sedat, please try your plug/unplug the USB
>> > mouse test with the patch below.
>> >
>> > Alan Stern
>> >
>> >
>> >
>> > Index: usb-4.4/drivers/hid/usbhid/hid-core.c
>> > ===================================================================
>> > --- usb-4.4.orig/drivers/hid/usbhid/hid-core.c
>> > +++ usb-4.4/drivers/hid/usbhid/hid-core.c
>> > @@ -1393,8 +1393,11 @@ static void usbhid_disconnect(struct usb
>> >
>> > static void hid_cancel_delayed_stuff(struct usbhid_device *usbhid)
>> > {
>> > + if (raw_irqs_disabled()) pr_info("usbhid irqs disabled A\n");
>> > del_timer_sync(&usbhid->io_retry);
>> > + if (raw_irqs_disabled()) pr_info("usbhid irqs disabled B\n");
>> > cancel_work_sync(&usbhid->reset_work);
>> > + if (raw_irqs_disabled()) pr_info("usbhid irqs disabled C\n");
>> > }
>> >
>> > static void hid_cease_io(struct usbhid_device *usbhid)
>> >
>>
>> With your patch I get the dmesg attached.
>
>> [ 22.234758] usbhid irqs disabled A
>> [ 22.234857] usbhid irqs disabled B
>> [ 22.234912] BUG: sleeping function called from invalid context atkernel/workqueue.c:2688
>
> That's a smoking gun. It means everyone has been looking in the wrong
> place. Can you provide an objdump listing of usbhid_close()? The
> routine starts like this:
>
> void usbhid_close(struct hid_device *hid)
> {
> struct usbhid_device *usbhid = hid->driver_data;
>
> mutex_lock(&hid_open_mut);
>
> /* protecting hid->open to make sure we don't restart
> * data acquistion due to a resumption we no longer
> * care about
> */
> spin_lock_irq(&usbhid->lock);
> if (!--hid->open) {
> spin_unlock_irq(&usbhid->lock);
> hid_cancel_delayed_stuff(usbhid);
>
> It appears that the spin_unlock_irq() call isn't working.
>
> For extra thoroughness, try putting one of those raw_irqs_disabled()
> checks just before and one just after the spin_lock_irq() line above.
> Maybe also before the mutex_lock() line.
>
> Alan Stern
>
Hmm, we are there where I was looking at...
Please, read the reply of Jiri [1], we did some tweaking.
With CONFIG_FTRACE=n and CONFIG_PROVE_LOCKING=n !
*** Part one: ObjectDump of hid-core.o ***
$ objdump -D drivers/hid/usbhid/hid-core.o | awk '/<[^>]*>:$/ { p=0; }
/<usbhid_close>:/ { p=1; } { if (p) print $0; }' >
../objdump-D_hid-core_o_usbhid_close_$(uname -r).txt
$ cat ../objdump-D_hid-core_o_usbhid_close_4.4.4-1-iniza-small.txt
00000000000002e0 <usbhid_close>:
2e0: 55 push %rbp
2e1: 48 89 e5 mov %rsp,%rbp
2e4: 41 57 push %r15
2e6: 41 56 push %r14
2e8: 41 54 push %r12
2ea: 53 push %rbx
2eb: 49 89 ff mov %rdi,%r15
2ee: 4d 8b b7 e8 1e 00 00 mov 0x1ee8(%r15),%r14
2f5: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
2fc: 31 f6 xor %esi,%esi
2fe: e8 00 00 00 00 callq 303 <usbhid_close+0x23>
303: 49 8d 9e 88 28 00 00 lea 0x2888(%r14),%rbx
30a: 48 89 df mov %rbx,%rdi
30d: e8 00 00 00 00 callq 312 <usbhid_close+0x32>
312: 41 ff 8f e4 1d 00 00 decl 0x1de4(%r15)
319: 9c pushfq
31a: 41 5c pop %r12
31c: 48 89 df mov %rbx,%rdi
31f: e8 00 00 00 00 callq 324 <usbhid_close+0x44>
324: 41 54 push %r12
326: 9d popfq
327: 75 23 jne 34c <usbhid_close+0x6c>
329: 4c 89 f7 mov %r14,%rdi
32c: e8 3f 00 00 00 callq 370 <hid_cancel_delayed_stuff>
331: 41 f6 87 b9 1d 00 00 testb $0x4,0x1db9(%r15)
338: 04
339: 75 11 jne 34c <usbhid_close+0x6c>
33b: 49 8b 7e 18 mov 0x18(%r14),%rdi
33f: e8 00 00 00 00 callq 344 <usbhid_close+0x64>
344: 49 8b 46 08 mov 0x8(%r14),%rax
348: 80 60 28 f7 andb $0xf7,0x28(%rax)
34c: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
353: e8 00 00 00 00 callq 358 <usbhid_close+0x78>
358: 5b pop %rbx
359: 41 5c pop %r12
35b: 41 5e pop %r14
35d: 41 5f pop %r15
35f: 5d pop %rbp
360: c3 retq
361: 66 66 66 66 66 66 2e data32 data32 data32 data32
data32 nopw %cs:0x0(%rax,%rax,1)
368: 0f 1f 84 00 00 00 00
36f: 00
*** Part two: Double-checking (after TV-adds) ***
- Sedat -
[1] https://marc.info/?l=linux-input&m=144359852905747&w=2
[toc] | [next] | [standalone]
| From | Sedat Dilek <sedat.dilek@gmail.com> |
|---|---|
| Date | 2016-03-07 17:30 +0100 |
| Message-ID | <ra6cI-5dM-49@gated-at.bofh.it> |
| In reply to | #1351740 |
On Mon, Mar 7, 2016 at 4:59 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
> On Sun, Mar 6, 2016 at 6:23 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Sat, 5 Mar 2016, Sedat Dilek wrote:
>>
>>> On Fri, Mar 4, 2016 at 5:04 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>> > On Wed, 2 Mar 2016, Sedat Dilek wrote:
>>> >
>>> >> On 3/1/16, Alan Stern <stern@rowland.harvard.edu> wrote:
>>> >> > On Tue, 1 Mar 2016, Sedat Dilek wrote:
>>> >> >
>>> >> >> On Tue, Oct 13, 2015 at 2:57 AM, Steven Rostedt <rostedt@goodmis.org>
>>> >> >> wrote:
>>> >> >> > On Sat, 3 Oct 2015 12:05:42 +0200
>>> >> >> > Sedat Dilek <sedat.dilek@gmail.com> wrote:
>>> >> >> >
>>> >> >> >> So, at the beginning... dunno WTF is causing the problems - no
>>> >> >> >> workaround for CLANG.
>>> >> >> >
>>> >> >> > Probably need to compile with gcc and with clang and look at the binary
>>> >> >> > differences. Or at least what objdump shows.
>>> >> >> >
>>> >> >>
>>> >> >> [ Hope to address this issue to the correct people - CCed some people
>>> >> >> I taped on their nerves ]
>>> >> >>
>>> >> >> Not sure if I should open a new thread?
>>> >> >> Please, some clear statements on this.
>>> >> >> Thanks.
>>> >> >>
>>> >> >> The issue is still visible and alive.
>>> >
>>> > I think it would be worthwhile to doublecheck the time at which
>>> > interrupts get disabled. Sedat, please try your plug/unplug the USB
>>> > mouse test with the patch below.
>>> >
>>> > Alan Stern
>>> >
>>> >
>>> >
>>> > Index: usb-4.4/drivers/hid/usbhid/hid-core.c
>>> > ===================================================================
>>> > --- usb-4.4.orig/drivers/hid/usbhid/hid-core.c
>>> > +++ usb-4.4/drivers/hid/usbhid/hid-core.c
>>> > @@ -1393,8 +1393,11 @@ static void usbhid_disconnect(struct usb
>>> >
>>> > static void hid_cancel_delayed_stuff(struct usbhid_device *usbhid)
>>> > {
>>> > + if (raw_irqs_disabled()) pr_info("usbhid irqs disabled A\n");
>>> > del_timer_sync(&usbhid->io_retry);
>>> > + if (raw_irqs_disabled()) pr_info("usbhid irqs disabled B\n");
>>> > cancel_work_sync(&usbhid->reset_work);
>>> > + if (raw_irqs_disabled()) pr_info("usbhid irqs disabled C\n");
>>> > }
>>> >
>>> > static void hid_cease_io(struct usbhid_device *usbhid)
>>> >
>>>
>>> With your patch I get the dmesg attached.
>>
>>> [ 22.234758] usbhid irqs disabled A
>>> [ 22.234857] usbhid irqs disabled B
>>> [ 22.234912] BUG: sleeping function called from invalid context atkernel/workqueue.c:2688
>>
>> That's a smoking gun. It means everyone has been looking in the wrong
>> place. Can you provide an objdump listing of usbhid_close()? The
>> routine starts like this:
>>
>> void usbhid_close(struct hid_device *hid)
>> {
>> struct usbhid_device *usbhid = hid->driver_data;
>>
>> mutex_lock(&hid_open_mut);
>>
>> /* protecting hid->open to make sure we don't restart
>> * data acquistion due to a resumption we no longer
>> * care about
>> */
>> spin_lock_irq(&usbhid->lock);
>> if (!--hid->open) {
>> spin_unlock_irq(&usbhid->lock);
>> hid_cancel_delayed_stuff(usbhid);
>>
>> It appears that the spin_unlock_irq() call isn't working.
>>
>> For extra thoroughness, try putting one of those raw_irqs_disabled()
>> checks just before and one just after the spin_lock_irq() line above.
>> Maybe also before the mutex_lock() line.
>>
>> Alan Stern
>>
>
> Hmm, we are there where I was looking at...
>
> Please, read the reply of Jiri [1], we did some tweaking.
> With CONFIG_FTRACE=n and CONFIG_PROVE_LOCKING=n !
>
Shall I enable CONFIG_TRACE_IRQFLAGS (CONFIG_PROVE_LOCKING=n disables it)?
- Sedat -
[toc] | [prev] | [next] | [standalone]
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2016-03-07 17:50 +0100 |
| Message-ID | <ra6w3-5kV-21@gated-at.bofh.it> |
| In reply to | #1351740 |
On Mon, 7 Mar 2016, Sedat Dilek wrote:
> Hmm, we are there where I was looking at...
>
> Please, read the reply of Jiri [1], we did some tweaking.
> With CONFIG_FTRACE=n and CONFIG_PROVE_LOCKING=n !
Yes, Jiri was looking more or less at the right place but his
conclusions were wrong.
> *** Part one: ObjectDump of hid-core.o ***
>
> $ objdump -D drivers/hid/usbhid/hid-core.o | awk '/<[^>]*>:$/ { p=0; }
> /<usbhid_close>:/ { p=1; } { if (p) print $0; }' >
> ../objdump-D_hid-core_o_usbhid_close_$(uname -r).txt
This reveals the problem, at last...
> $ cat ../objdump-D_hid-core_o_usbhid_close_4.4.4-1-iniza-small.txt
> 00000000000002e0 <usbhid_close>:
> 2e0: 55 push %rbp
> 2e1: 48 89 e5 mov %rsp,%rbp
> 2e4: 41 57 push %r15
> 2e6: 41 56 push %r14
> 2e8: 41 54 push %r12
> 2ea: 53 push %rbx
> 2eb: 49 89 ff mov %rdi,%r15
> 2ee: 4d 8b b7 e8 1e 00 00 mov 0x1ee8(%r15),%r14
> 2f5: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
> 2fc: 31 f6 xor %esi,%esi
> 2fe: e8 00 00 00 00 callq 303 <usbhid_close+0x23>
mutex_lock(&hid_open_mut);
> 303: 49 8d 9e 88 28 00 00 lea 0x2888(%r14),%rbx
> 30a: 48 89 df mov %rbx,%rdi
> 30d: e8 00 00 00 00 callq 312 <usbhid_close+0x32>
spin_lock_irq(&usbhid->lock);
> 312: 41 ff 8f e4 1d 00 00 decl 0x1de4(%r15)
--hid->open
> 319: 9c pushfq
> 31a: 41 5c pop %r12
> 31c: 48 89 df mov %rbx,%rdi
> 31f: e8 00 00 00 00 callq 324 <usbhid_close+0x44>
> 324: 41 54 push %r12
> 326: 9d popfq
spin_unlock_irq(&usbhid->lock); while attempting to preserve the Z
flag. The problem is that this code sequence will also preserve the
Interrupt Flag!
> 327: 75 23 jne 34c <usbhid_close+0x6c>
if (!--hid->open), testing the Z flag from the decl.
> 329: 4c 89 f7 mov %r14,%rdi
> 32c: e8 3f 00 00 00 callq 370 <hid_cancel_delayed_stuff>
But now hid_cancel_delayed_stuff(usbhid) gets called with interrupts
disabled.
It's hard to call this a compiler bug, but perhaps it is -- I don't
know how programmers are supposed to tell CLANG that a subroutine
modifies the Interrupt Flag in a way that the compiler shouldn't mess
up.
Alan Stern
[toc] | [prev] | [next] | [standalone]
| From | Jiri Kosina <jikos@kernel.org> |
|---|---|
| Date | 2016-03-07 18:10 +0100 |
| Message-ID | <ra6Po-5Ig-1@gated-at.bofh.it> |
| In reply to | #1351790 |
On Mon, 7 Mar 2016, Alan Stern wrote: > > 319: 9c pushfq > > 31a: 41 5c pop %r12 > > 31c: 48 89 df mov %rbx,%rdi > > 31f: e8 00 00 00 00 callq 324 <usbhid_close+0x44> > > 324: 41 54 push %r12 > > 326: 9d popfq > > spin_unlock_irq(&usbhid->lock); while attempting to preserve the Z > flag. The problem is that this code sequence will also preserve the > Interrupt Flag! You are right Alan, thanks a lot, for reason I could not understand I completely missed the pushf/popf last time I was looking at the generated assembly! OK, a little bit of googling revealed related discussion on LLVM mailinglist: http://lists.llvm.org/pipermail/llvm-dev/2015-July/088780.html Seems like it has been reported already, but noone dared to fix it yet. This basically makes LLVM unusable for compiling the kernel. -- Jiri Kosina SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Sedat Dilek <sedat.dilek@gmail.com> |
|---|---|
| Date | 2016-03-07 18:20 +0100 |
| Message-ID | <ra6Z3-5LX-3@gated-at.bofh.it> |
| In reply to | #1351803 |
On Mon, Mar 7, 2016 at 6:05 PM, Jiri Kosina <jikos@kernel.org> wrote: > On Mon, 7 Mar 2016, Alan Stern wrote: > >> > 319: 9c pushfq >> > 31a: 41 5c pop %r12 >> > 31c: 48 89 df mov %rbx,%rdi >> > 31f: e8 00 00 00 00 callq 324 <usbhid_close+0x44> >> > 324: 41 54 push %r12 >> > 326: 9d popfq >> >> spin_unlock_irq(&usbhid->lock); while attempting to preserve the Z >> flag. The problem is that this code sequence will also preserve the >> Interrupt Flag! > > You are right Alan, thanks a lot, for reason I could not understand I > completely missed the pushf/popf last time I was looking at the generated > assembly! > > OK, a little bit of googling revealed related discussion on LLVM > mailinglist: > > http://lists.llvm.org/pipermail/llvm-dev/2015-July/088780.html > > Seems like it has been reported already, but noone dared to fix it yet. > > This basically makes LLVM unusable for compiling the kernel. > OK, OK. Did someone look at the next/follow-ups in this thread? For example: D6629 "x86: Emit LAHF/SAHF instead of PUSHF/POPF" [2]? - Sedat - [1] http://lists.llvm.org/pipermail/llvm-dev/2015-July/088874.html [2] http://reviews.llvm.org/D6629
[toc] | [prev] | [next] | [standalone]
| From | David Laight <David.Laight@ACULAB.COM> |
|---|---|
| Date | 2016-03-07 18:40 +0100 |
| Message-ID | <ra7iq-5Vi-21@gated-at.bofh.it> |
| In reply to | #1351810 |
From: Sedat Dilek ... > Did someone look at the next/follow-ups in this thread? > For example: D6629 "x86: Emit LAHF/SAHF instead of PUSHF/POPF" [2]? LAHF and SAHF come with the following note: This instruction executes as described above in compatibility mode and legacy mode. It is valid in 64-bit mode only if CPUID.80000001H:ECX.LAHF-SAHF[bit 0] = 1. So I suspect they can't be used. David
[toc] | [prev] | [next] | [standalone]
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2016-03-07 19:10 +0100 |
| Message-ID | <ra7Ls-6kn-5@gated-at.bofh.it> |
| In reply to | #1351831 |
On Mon, 7 Mar 2016, David Laight wrote: > From: Sedat Dilek > ... > > Did someone look at the next/follow-ups in this thread? > > For example: D6629 "x86: Emit LAHF/SAHF instead of PUSHF/POPF" [2]? > > LAHF and SAHF come with the following note: > > This instruction executes as described above in compatibility mode and legacy mode. > It is valid in 64-bit mode only if CPUID.80000001H:ECX.LAHF-SAHF[bit 0] = 1. > > So I suspect they can't be used. Of course, there are other ways to save a single flag value (such as setz). It's up to the compiler developers to decide what they think is best. Alan Stern
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-03-07 19:40 +0100 |
| Message-ID | <ra8ev-6wg-41@gated-at.bofh.it> |
| In reply to | #1351843 |
On Mon, Mar 7, 2016 at 10:07 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> Of course, there are other ways to save a single flag value (such as
> setz). It's up to the compiler developers to decide what they think is
> best.
Using 'setcc' to save eflags somewhere is definitely the right thing to do.
Using pushf/popf in generated code is completely insane (unless done
very localized in a controlled area).
It is, in fact, insane and wrong even in user space, since eflags does
contain bits that user space itself might be modifying.
In fact, even IF may be modified with iopl 3 (thing old X server
setups), but ignoring that flag entirely, you have AC that acts in
very similar ways (system-wide alignment control) that user space
might be using to make sure it doesn't have unaligned accesses.
It's rare, yes. But still - this isn't really limited to just the kernel.
But perhaps more importantly, I suspect using pushf/popf isn't just
semantically the wrong thing to do, it's just plain stupid. It's
likely slower than the obvious 'setcc' model. Agner Fog's table shows
it "popf" as being 25-30 uops on several microarchitectures. Looks
like it's often microcode.
Now, pushf/popf may well be fairly cheap on *some* uarchitectures, but
it really sounds like a bad idea to use it when not absolutely
required. And that is completely independent of the fact that is
screws up the IF bit.
But yeah, for the kernel we at a minimum need a way to disable that
code generation, even if the clang guys might have some insane reason
to keep it for other cases.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-03-07 18:10 +0100 |
| Message-ID | <ra6Pp-5Ig-21@gated-at.bofh.it> |
| In reply to | #1351790 |
On Mon, 7 Mar 2016 11:41:37 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:
> It's hard to call this a compiler bug, but perhaps it is -- I don't
> know how programmers are supposed to tell CLANG that a subroutine
> modifies the Interrupt Flag in a way that the compiler shouldn't mess
> up.
Really! This is what's is happening??
Clang takes this:
if (!--hid->open) {
spin_unlock_irq(X);
do_something();
} else {
spin_unlock_irq(X);
}
Thus it's basically doing:
FLAG = !--hid->open;
push flags;
spin_unlock_irq(X)
pop flags;
if (FLAG zero set) {
do_something();
}
OUCH!!! There's gotta be a way to turn that off, otherwise Clang can
not be used to compile the kernel.
Nice detective work.
-- Steve
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-03-07 18:20 +0100 |
| Message-ID | <ra6Z3-5LX-7@gated-at.bofh.it> |
| In reply to | #1351790 |
On Mon, 7 Mar 2016 11:41:37 -0500 (EST) Alan Stern <stern@rowland.harvard.edu> wrote: > It's hard to call this a compiler bug, but perhaps it is -- I don't > know how programmers are supposed to tell CLANG that a subroutine > modifies the Interrupt Flag in a way that the compiler shouldn't mess > up. I would state that this is a compiler bug for any kernel development. Because it's modifying a global variable (IF) that can be modified by asm(). Clang is assuming that this is userspace where IF can't change. But because this is kernel space, the IF can (and does here), which makes this "feature" incompatible with any (Linux or otherwise) kernel programming. -- Steve
[toc] | [prev] | [next] | [standalone]
| From | Sedat Dilek <sedat.dilek@gmail.com> |
|---|---|
| Date | 2016-03-07 18:20 +0100 |
| Message-ID | <ra6Z4-5LX-27@gated-at.bofh.it> |
| In reply to | #1351790 |
On Mon, Mar 7, 2016 at 5:41 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 7 Mar 2016, Sedat Dilek wrote:
>
>> Hmm, we are there where I was looking at...
>>
>> Please, read the reply of Jiri [1], we did some tweaking.
>> With CONFIG_FTRACE=n and CONFIG_PROVE_LOCKING=n !
>
> Yes, Jiri was looking more or less at the right place but his
> conclusions were wrong.
>
>> *** Part one: ObjectDump of hid-core.o ***
>>
>> $ objdump -D drivers/hid/usbhid/hid-core.o | awk '/<[^>]*>:$/ { p=0; }
>> /<usbhid_close>:/ { p=1; } { if (p) print $0; }' >
>> ../objdump-D_hid-core_o_usbhid_close_$(uname -r).txt
>
> This reveals the problem, at last...
>
>> $ cat ../objdump-D_hid-core_o_usbhid_close_4.4.4-1-iniza-small.txt
>> 00000000000002e0 <usbhid_close>:
>> 2e0: 55 push %rbp
>> 2e1: 48 89 e5 mov %rsp,%rbp
>> 2e4: 41 57 push %r15
>> 2e6: 41 56 push %r14
>> 2e8: 41 54 push %r12
>> 2ea: 53 push %rbx
>> 2eb: 49 89 ff mov %rdi,%r15
>> 2ee: 4d 8b b7 e8 1e 00 00 mov 0x1ee8(%r15),%r14
>> 2f5: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
>> 2fc: 31 f6 xor %esi,%esi
>> 2fe: e8 00 00 00 00 callq 303 <usbhid_close+0x23>
>
> mutex_lock(&hid_open_mut);
>
>> 303: 49 8d 9e 88 28 00 00 lea 0x2888(%r14),%rbx
>> 30a: 48 89 df mov %rbx,%rdi
>> 30d: e8 00 00 00 00 callq 312 <usbhid_close+0x32>
>
> spin_lock_irq(&usbhid->lock);
>
>> 312: 41 ff 8f e4 1d 00 00 decl 0x1de4(%r15)
>
> --hid->open
>
>> 319: 9c pushfq
>> 31a: 41 5c pop %r12
>> 31c: 48 89 df mov %rbx,%rdi
>> 31f: e8 00 00 00 00 callq 324 <usbhid_close+0x44>
>> 324: 41 54 push %r12
>> 326: 9d popfq
>
> spin_unlock_irq(&usbhid->lock); while attempting to preserve the Z
> flag. The problem is that this code sequence will also preserve the
> Interrupt Flag!
>
>> 327: 75 23 jne 34c <usbhid_close+0x6c>
>
> if (!--hid->open), testing the Z flag from the decl.
>
>> 329: 4c 89 f7 mov %r14,%rdi
>> 32c: e8 3f 00 00 00 callq 370 <hid_cancel_delayed_stuff>
>
> But now hid_cancel_delayed_stuff(usbhid) gets called with interrupts
> disabled.
>
> It's hard to call this a compiler bug, but perhaps it is -- I don't
> know how programmers are supposed to tell CLANG that a subroutine
> modifies the Interrupt Flag in a way that the compiler shouldn't mess
> up.
>
So, if Clang is producing wrong X86 code here, is it possible to turn
interrupts on/off manually?
But, hmm that affects other places as well in the Linux sources, so.
- Sedat -
[toc] | [prev] | [next] | [standalone]
| From | Jiri Kosina <jikos@kernel.org> |
|---|---|
| Date | 2016-03-07 18:30 +0100 |
| Message-ID | <ra78L-5Qh-31@gated-at.bofh.it> |
| In reply to | #1351817 |
On Mon, 7 Mar 2016, Sedat Dilek wrote: > Did someone look at the next/follow-ups in this thread? > For example: D6629 "x86: Emit LAHF/SAHF instead of PUSHF/POPF" [2]? Using LAHF/SAHF would "solve" it, as IF is at bit #9. The question is whether they need to play with flags here at all. On Mon, 7 Mar 2016, Sedat Dilek wrote: > So, if Clang is producing wrong X86 code here, is it possible to turn > interrupts on/off manually? But, hmm that affects other places as well > in the Linux sources, so. This issue needs to be handled in the compiler. -- Jiri Kosina SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-03-07 18:40 +0100 |
| Message-ID | <ra7iq-5Vi-19@gated-at.bofh.it> |
| In reply to | #1351823 |
On Mon, 7 Mar 2016 18:24:12 +0100 (CET) Jiri Kosina <jikos@kernel.org> wrote: > > So, if Clang is producing wrong X86 code here, is it possible to turn > > interrupts on/off manually? But, hmm that affects other places as well > > in the Linux sources, so. > > This issue needs to be handled in the compiler. > Exactly. The compiler may get away with this in userspace (maybe), but for the kernel, it is definitely a show stopper. Especially if it knows that an asm() may be called. -- Steve
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-03-07 19:10 +0100 |
| Message-ID | <ra7Ls-6kn-13@gated-at.bofh.it> |
| In reply to | #1351828 |
On Mon, Mar 7, 2016 at 9:30 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 7 Mar 2016 18:24:12 +0100 (CET) > Jiri Kosina <jikos@kernel.org> wrote: > >> > So, if Clang is producing wrong X86 code here, is it possible to turn >> > interrupts on/off manually? But, hmm that affects other places as well >> > in the Linux sources, so. >> >> This issue needs to be handled in the compiler. >> > > Exactly. The compiler may get away with this in userspace (maybe), but > for the kernel, it is definitely a show stopper. Especially if it knows > that an asm() may be called. It's broken for user code that fiddles with AC, too. --Andy
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-03-07 20:20 +0100 |
| Message-ID | <ra8Rb-702-1@gated-at.bofh.it> |
| In reply to | #1351847 |
On Mon, 7 Mar 2016 10:04:21 -0800 Andy Lutomirski <luto@amacapital.net> wrote: > > Exactly. The compiler may get away with this in userspace (maybe), but > > for the kernel, it is definitely a show stopper. Especially if it knows > > that an asm() may be called. > > It's broken for user code that fiddles with AC, too. Which is why I added the "(maybe)" ;-) -- Steve
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web