`stop' left out usbhid->urb* pointers and so the next `start' thought
it needs to allocate nothing and used the memory pointers previously
pointed to. This led to memory corruption and device malfunction.
Also don't forget to clear disconnect flag on start which was left set
by the previous `stop'.
Signed-off-by: Jiri Slaby <[email protected]>
---
drivers/hid/usbhid/hid-core.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 18e5ddd..f0339ae 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -781,6 +781,8 @@ static int usbhid_start(struct hid_device *hid)
unsigned int n, insize = 0;
int ret;
+ clear_bit(HID_DISCONNECTED, &usbhid->iofl);
+
usbhid->bufsize = HID_MIN_BUFFER_SIZE;
hid_find_max_report(hid, HID_INPUT_REPORT, &usbhid->bufsize);
hid_find_max_report(hid, HID_OUTPUT_REPORT, &usbhid->bufsize);
@@ -888,6 +890,9 @@ fail:
usb_free_urb(usbhid->urbin);
usb_free_urb(usbhid->urbout);
usb_free_urb(usbhid->urbctrl);
+ usbhid->urbin = NULL;
+ usbhid->urbout = NULL;
+ usbhid->urbctrl = NULL;
hid_free_buffers(dev, hid);
mutex_unlock(&usbhid->setup);
return ret;
@@ -924,6 +929,9 @@ static void usbhid_stop(struct hid_device *hid)
usb_free_urb(usbhid->urbin);
usb_free_urb(usbhid->urbctrl);
usb_free_urb(usbhid->urbout);
+ usbhid->urbin = NULL; /* don't mess up next start */
+ usbhid->urbctrl = NULL;
+ usbhid->urbout = NULL;
hid_free_buffers(hid_to_usb_dev(hid), hid);
mutex_unlock(&usbhid->setup);
--
1.6.0.3
On Sat, 1 Nov 2008, Jiri Slaby wrote:
> `stop' left out usbhid->urb* pointers and so the next `start' thought
> it needs to allocate nothing and used the memory pointers previously
> pointed to. This led to memory corruption and device malfunction.
>
> Also don't forget to clear disconnect flag on start which was left set
> by the previous `stop'.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> ---
> drivers/hid/usbhid/hid-core.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 18e5ddd..f0339ae 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -781,6 +781,8 @@ static int usbhid_start(struct hid_device *hid)
> unsigned int n, insize = 0;
> int ret;
>
> + clear_bit(HID_DISCONNECTED, &usbhid->iofl);
> +
> usbhid->bufsize = HID_MIN_BUFFER_SIZE;
> hid_find_max_report(hid, HID_INPUT_REPORT, &usbhid->bufsize);
> hid_find_max_report(hid, HID_OUTPUT_REPORT, &usbhid->bufsize);
> @@ -888,6 +890,9 @@ fail:
> usb_free_urb(usbhid->urbin);
> usb_free_urb(usbhid->urbout);
> usb_free_urb(usbhid->urbctrl);
> + usbhid->urbin = NULL;
> + usbhid->urbout = NULL;
> + usbhid->urbctrl = NULL;
> hid_free_buffers(dev, hid);
> mutex_unlock(&usbhid->setup);
> return ret;
> @@ -924,6 +929,9 @@ static void usbhid_stop(struct hid_device *hid)
> usb_free_urb(usbhid->urbin);
> usb_free_urb(usbhid->urbctrl);
> usb_free_urb(usbhid->urbout);
> + usbhid->urbin = NULL; /* don't mess up next start */
> + usbhid->urbctrl = NULL;
> + usbhid->urbout = NULL;
>
> hid_free_buffers(hid_to_usb_dev(hid), hid);
> mutex_unlock(&usbhid->setup);
Jeroen, Helge,
could you please verify whether this patch fixes the corruption you were
experiencing?
[ I will be offline for the upcoming ~9 days, will push the fix upstream
then, if it is not picked up through different channels in the
meantime ]
Thanks!
--
Jiri Kosina
SUSE Labs
On 11/02/2008 12:02 AM, Jiri Kosina wrote:
> On Sat, 1 Nov 2008, Jiri Slaby wrote:
>
>> `stop' left out usbhid->urb* pointers and so the next `start' thought
>> it needs to allocate nothing and used the memory pointers previously
>> pointed to. This led to memory corruption and device malfunction.
[...]
> could you please verify whether this patch fixes the corruption you were
> experiencing?
btw. this is not expected to fix that, but if it does, the better ;).
This fixes
echo DEVICE > /sys/bus/hid/drivers/DRIVER/unbind
echo DEVICE > /sys/bus/hid/drivers/DRIVER/bind
failures.
But maybe parisc does something differently than x86 in bus handling so that it
triggers...
Jiri Slaby wrote:
> On 11/02/2008 12:02 AM, Jiri Kosina wrote:
>> On Sat, 1 Nov 2008, Jiri Slaby wrote:
>>
>>> `stop' left out usbhid->urb* pointers and so the next `start' thought
>>> it needs to allocate nothing and used the memory pointers previously
>>> pointed to. This led to memory corruption and device malfunction.
> [...]
>> could you please verify whether this patch fixes the corruption you were
>> experiencing?
>
> btw. this is not expected to fix that, but if it does, the better ;).
I tried the patch and sadly it didn't fixed the parisc bug.
Helge
> This fixes
> echo DEVICE > /sys/bus/hid/drivers/DRIVER/unbind
> echo DEVICE > /sys/bus/hid/drivers/DRIVER/bind
> failures.
>
> But maybe parisc does something differently than x86 in bus handling so that it
> triggers...
Helge Deller napsal(a):
> Jiri Slaby wrote:
>> On 11/02/2008 12:02 AM, Jiri Kosina wrote:
>>> On Sat, 1 Nov 2008, Jiri Slaby wrote:
>>>
>>>> `stop' left out usbhid->urb* pointers and so the next `start' thought
>>>> it needs to allocate nothing and used the memory pointers previously
>>>> pointed to. This led to memory corruption and device malfunction.
>> [...]
>>> could you please verify whether this patch fixes the corruption you
>>> were experiencing?
>>
>> btw. this is not expected to fix that, but if it does, the better ;).
>
> I tried the patch and sadly it didn't fixed the parisc bug.
Could you bisect it?
Jiri Slaby wrote:
> Helge Deller napsal(a):
>> Jiri Slaby wrote:
>>> On 11/02/2008 12:02 AM, Jiri Kosina wrote:
>>>> On Sat, 1 Nov 2008, Jiri Slaby wrote:
>>>>
>>>>> `stop' left out usbhid->urb* pointers and so the next `start' thought
>>>>> it needs to allocate nothing and used the memory pointers previously
>>>>> pointed to. This led to memory corruption and device malfunction.
>>> [...]
>>>> could you please verify whether this patch fixes the corruption you
>>>> were experiencing?
>>> btw. this is not expected to fix that, but if it does, the better ;).
>> I tried the patch and sadly it didn't fixed the parisc bug.
>
> Could you bisect it?
I fully bisected it, and the final "buggy" patch seems to have been
Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f
(see
http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f)
Denys: Any reason you removed "!prev" in front of "expand_stack" ? It
seems wrong to remove the prev-check, else it would crash in
expand_upwards(in the CONFIG_STACK_GROWSUP case).
This is parisc architecture (stack grows up, big-endian).
Sadly reverting just this patch, didn't fixed the bugzilla bug either:
http://bugzilla.kernel.org/show_bug.cgi?id=11913
I think I need to redo all of my bisecting again... sigh...
Helge
On Sunday 02 November 2008 17:50, Helge Deller wrote:
> Jiri Slaby wrote:
> >>> btw. this is not expected to fix that, but if it does, the better ;).
> >> I tried the patch and sadly it didn't fixed the parisc bug.
> >
> > Could you bisect it?
>
> I fully bisected it, and the final "buggy" patch seems to have been
> Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f
> (see
> http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f)
> Denys: Any reason you removed "!prev" in front of "expand_stack"?
Looks like I erroneously thought it can't be NULL,
or that expand_upwards() is ok with getting NULL vma parameter.
I looked again and neither is true.
Sorry, looks like I indeed broke this.
--
vda
On Sat, 1 Nov 2008, Jiri Slaby wrote:
> `stop' left out usbhid->urb* pointers and so the next `start' thought
> it needs to allocate nothing and used the memory pointers previously
> pointed to. This led to memory corruption and device malfunction.
>
> Also don't forget to clear disconnect flag on start which was left set
> by the previous `stop'.
Applied, thanks Jiri.
--
Jiri Kosina
SUSE Labs
On Sun, 2 Nov 2008, Denys Vlasenko wrote:
> > I fully bisected it, and the final "buggy" patch seems to have been
> > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
> > http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f)
> > Denys: Any reason you removed "!prev" in front of "expand_stack"?
> Looks like I erroneously thought it can't be NULL,
> or that expand_upwards() is ok with getting NULL vma parameter.
> I looked again and neither is true.
> Sorry, looks like I indeed broke this.
Hmm, so ... ? Seems like this didn't get fixed in Linus' tree yet?
--
Jiri Kosina
SUSE Labs
On Wednesday 12 November 2008 00:22, Jiri Kosina wrote:
> On Sun, 2 Nov 2008, Denys Vlasenko wrote:
> > > I fully bisected it, and the final "buggy" patch seems to have been
> > > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
> > > http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f)
> > > Denys: Any reason you removed "!prev" in front of "expand_stack"?
>
> > Looks like I erroneously thought it can't be NULL,
> > or that expand_upwards() is ok with getting NULL vma parameter.
> > I looked again and neither is true.
> > Sorry, looks like I indeed broke this.
>
> Hmm, so ... ? Seems like this didn't get fixed in Linus' tree yet?
I found my original email in "sent" folder. The patch in that mail
does NOT remove !prev. That change had beed added by someone else.
See attached file with original email.
Ok, I think we are not much interested in who did it, let's
fix it for good.
Signed-off-by: Denys Vlasenko <[email protected]>
--
vda
--- linux-2.6.28-rc4/mm/mmap.c Mon Nov 10 01:36:15 2008
+++ linux-2.6.28-rc4.fix/mm/mmap.c Wed Nov 12 01:21:39 2008
@@ -1704,7 +1704,7 @@
vma = find_vma_prev(mm, addr, &prev);
if (vma && (vma->vm_start <= addr))
return vma;
- if (expand_stack(prev, addr))
+ if (!prev || expand_stack(prev, addr))
return NULL;
if (prev->vm_flags & VM_LOCKED) {
if (mlock_vma_pages_range(prev, addr, prev->vm_end) < 0)
On Wed, 12 Nov 2008, Denys Vlasenko wrote:
> > > > I fully bisected it, and the final "buggy" patch seems to have been
> > > > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
> > > > http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f)
> > > > Denys: Any reason you removed "!prev" in front of "expand_stack"?
> > > Looks like I erroneously thought it can't be NULL,
> > > or that expand_upwards() is ok with getting NULL vma parameter.
> > > I looked again and neither is true.
> > > Sorry, looks like I indeed broke this.
> > Hmm, so ... ? Seems like this didn't get fixed in Linus' tree yet?
> I found my original email in "sent" folder. The patch in that mail does
> NOT remove !prev. That change had beed added by someone else. See
> attached file with original email.
> Ok, I think we are not much interested in who did it,
Hmm, I in fact think we would like to know who removed the check and
folded it into your original patch. I have added all the Signoffs and CCs
to the recepient list.
Andrew usually puts
[[email protected]: added this and that to the patch]
marker into the changelog, but it's not the case here.
Having your name in Signed-off-by field of something you are not aware of
should make you feel at least a little bit nervous IMHO.
> let's fix it for good.
> Signed-off-by: Denys Vlasenko <[email protected]>
> --- linux-2.6.28-rc4/mm/mmap.c Mon Nov 10 01:36:15 2008
> +++ linux-2.6.28-rc4.fix/mm/mmap.c Wed Nov 12 01:21:39 2008
> @@ -1704,7 +1704,7 @@
> vma = find_vma_prev(mm, addr, &prev);
> if (vma && (vma->vm_start <= addr))
> return vma;
> - if (expand_stack(prev, addr))
> + if (!prev || expand_stack(prev, addr))
> return NULL;
> if (prev->vm_flags & VM_LOCKED) {
> if (mlock_vma_pages_range(prev, addr, prev->vm_end) < 0)
Thanks,
--
Jiri Kosina
SUSE Labs
On Wed, 12 Nov 2008 01:34:54 +0100 (CET)
Jiri Kosina <[email protected]> wrote:
> On Wed, 12 Nov 2008, Denys Vlasenko wrote:
>
> > > > > I fully bisected it, and the final "buggy" patch seems to have been
> > > > > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
> > > > > http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f)
> > > > > Denys: Any reason you removed "!prev" in front of "expand_stack"?
> > > > Looks like I erroneously thought it can't be NULL,
> > > > or that expand_upwards() is ok with getting NULL vma parameter.
> > > > I looked again and neither is true.
> > > > Sorry, looks like I indeed broke this.
> > > Hmm, so ... ? Seems like this didn't get fixed in Linus' tree yet?
> > I found my original email in "sent" folder. The patch in that mail does
> > NOT remove !prev. That change had beed added by someone else. See
> > attached file with original email.
> > Ok, I think we are not much interested in who did it,
>
> Hmm, I in fact think we would like to know who removed the check and
> folded it into your original patch. I have added all the Signoffs and CCs
> to the recepient list.
>
> Andrew usually puts
>
> [[email protected]: added this and that to the patch]
>
> marker into the changelog, but it's not the case here.
>
> Having your name in Signed-off-by field of something you are not aware of
> should make you feel at least a little bit nervous IMHO.
>
> > let's fix it for good.
> > Signed-off-by: Denys Vlasenko <[email protected]>
> > --- linux-2.6.28-rc4/mm/mmap.c Mon Nov 10 01:36:15 2008
> > +++ linux-2.6.28-rc4.fix/mm/mmap.c Wed Nov 12 01:21:39 2008
> > @@ -1704,7 +1704,7 @@
> > vma = find_vma_prev(mm, addr, &prev);
> > if (vma && (vma->vm_start <= addr))
> > return vma;
> > - if (expand_stack(prev, addr))
> > + if (!prev || expand_stack(prev, addr))
> > return NULL;
> > if (prev->vm_flags & VM_LOCKED) {
> > if (mlock_vma_pages_range(prev, addr, prev->vm_end) < 0)
>
It looks like this was caused by me fixing rejects. That was the fancy
include-lots-of-context-so-it-wont-apply patch.
On 11/12/2008 01:50 AM, Andrew Morton wrote:
>>> let's fix it for good.
>>> Signed-off-by: Denys Vlasenko <[email protected]>
>>> --- linux-2.6.28-rc4/mm/mmap.c Mon Nov 10 01:36:15 2008
>>> +++ linux-2.6.28-rc4.fix/mm/mmap.c Wed Nov 12 01:21:39 2008
>>> @@ -1704,7 +1704,7 @@
>>> vma = find_vma_prev(mm, addr, &prev);
>>> if (vma && (vma->vm_start <= addr))
>>> return vma;
>>> - if (expand_stack(prev, addr))
>>> + if (!prev || expand_stack(prev, addr))
>>> return NULL;
>>> if (prev->vm_flags & VM_LOCKED) {
>>> if (mlock_vma_pages_range(prev, addr, prev->vm_end) < 0)
>
> It looks like this was caused by me fixing rejects. That was the fancy
> include-lots-of-context-so-it-wont-apply patch.
Great, this one got fixed. Helge, did you proceed with bisecting which another
commit breaks parics while having this one applied?
Thanks.
Jiri Slaby wrote:
> On 11/12/2008 01:50 AM, Andrew Morton wrote:
>>>> let's fix it for good.
>>>> Signed-off-by: Denys Vlasenko <[email protected]>
>>>> --- linux-2.6.28-rc4/mm/mmap.c Mon Nov 10 01:36:15 2008
>>>> +++ linux-2.6.28-rc4.fix/mm/mmap.c Wed Nov 12 01:21:39 2008
>>>> @@ -1704,7 +1704,7 @@
>>>> vma = find_vma_prev(mm, addr, &prev);
>>>> if (vma && (vma->vm_start <= addr))
>>>> return vma;
>>>> - if (expand_stack(prev, addr))
>>>> + if (!prev || expand_stack(prev, addr))
>>>> return NULL;
>>>> if (prev->vm_flags & VM_LOCKED) {
>>>> if (mlock_vma_pages_range(prev, addr, prev->vm_end) < 0)
>> It looks like this was caused by me fixing rejects. That was the fancy
>> include-lots-of-context-so-it-wont-apply patch.
>
> Great, this one got fixed. Helge, did you proceed with bisecting which another
> commit breaks parics while having this one applied?
I bisected twice. Both times I found this one to be the culprit.
Nevertheless, just reverting this (Thanks Denys!) didn't fixed the USB
problem.
I'll retry another bisecting round with -rc4 now, but I have the dumb
feeling the problem is not the USB part, but more in the usbhid driver.
Let's see.
Helge
On Thu, 13 Nov 2008, Helge Deller wrote:
>
> I bisected twice. Both times I found this one to be the culprit.
> Nevertheless, just reverting this (Thanks Denys!) didn't fixed the USB
> problem.
The trivial bisecting approach doesn't work if there are two independent
bugs that have overlapping lifetimes. In fact, bisection doesn't
necessarily work even if the lifetimes of the bugs are clearly disjoint,
because then if you look for bug A, but mark something "bad" because of
bug B, it can easily end up zeroing in on the wrong cause.
So "git bisect" is an absolutely wonderful tool, but it does require you
to be able to be sure about _which_ exact bug you chase down to give
reliable answers.
In the presense of multiple bugs, you have two choices:
(a) either you have to know how to distinguish them reliably in order to
give a clean good/bad for the particular bug you are chasing.
This can be impossible: one bug may make it impossible to even _test_
for the other bug, eg a bug that prevents bootup will obviously make
it impossible to see whether an independent run-time bug exists or
not. In this case, you have to do (b)
(b) Find _one_ bug first (doesn't matter which), and fix it. And then, do
a second bisection run, but before each test, you may need to apply
the fix for the first bug, so that you know that's not the an issue.
This can be automated (check if the broken commit is in the current
tree to be tested, apply a patch to fix it if it is), but it's not as
simple as just saying "let's bisect".
So bisection with multiple bugs is certainly possible, but it's also
unquestionably a lot more work, and more complicated.
Linus