[PATCH 1/1] USBHID: correct start/stop cycle

[PATCH 1/1] USBHID: correct start/stop cycle

[Jiri Slaby]

`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 <jirislaby@gmail.com>
---
 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

Re: [PATCH 1/1] USBHID: correct start/stop cycle

[Jiri Kosina]

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 <jirislaby@gmail.com>
> ---
>  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

Re: [PATCH 1/1] USBHID: correct start/stop cycle

[Jiri Slaby]

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...

Re: [PATCH 1/1] USBHID: correct start/stop cycle

[Helge Deller]

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...

Re: [PATCH 1/1] USBHID: correct start/stop cycle

[Jiri Slaby]

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?

Re: [PATCH 1/1] USBHID: correct start/stop cycle

[Helge Deller]

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

Re: [PATCH 1/1] USBHID: correct start/stop cycle

[Denys Vlasenko]

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

Re: [PATCH 1/1] USBHID: correct start/stop cycle

[Jiri Kosina]

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

Re: [PATCH 1/1] USBHID: correct start/stop cycle

[Jiri Kosina]

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

Re: [PATCH 1/1] USBHID: correct start/stop cycle

[Denys Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

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 <vda.linux@googlemail.com>
--
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)

[-- Attachment #2: deinline_mmap.txt --]
[-- Type: text/plain, Size: 6897 bytes --]

From vda.linux@googlemail.com Sat Jul  5 18:37:30 2008
From: Denys Vlasenko <vda.linux@googlemail.com>
To: linux-mm@kvack.org,
 linux-kernel@vger.kernel.org
Subject: [PATCH] Deinline a few functions in mmap.c
Date: Sat, 5 Jul 2008 18:37:30 +0200
User-Agent: KMail/1.8.2
MIME-Version: 1.0
Content-Type: text/plain;
  charset="us-ascii"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Message-Id: <200807051837.30219.vda.linux@googlemail.com>
Status: RO
X-Status: RSC
X-KMail-EncryptionState:  
X-KMail-SignatureState:  
X-KMail-MDN-Sent:  

__vma_link_file and expand_downwards functions are not small,
yeat they are marked inline. They probably had one callsite
sometime in the past, but now they have more.
In order to prevent similar thing, I also deinlined
expand_upwards, despite it having only pne callsite.
Nowadays gcc auto-inlines such static functions anyway.
In find_extend_vma, I removed one extra level of indirection.

Patch is deliberately generated with -U $BIGNUM to make
it easier to see that functions are big.

Result:

# size */*/mmap.o */vmlinux
   text    data     bss     dec     hex filename
   9514     188      16    9718    25f6 0.org/mm/mmap.o
   9237     188      16    9441    24e1 deinline/mm/mmap.o
6124402  858996  389480 7372878  70804e 0.org/vmlinux
6124113  858996  389480 7372589  707f2d deinline/vmlinux

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
--
vda

--- 0.org/mm/mmap.c	Wed Jul  2 00:40:52 2008
+++ deinline/mm/mmap.c	Sat Jul  5 16:19:30 2008
@@ -389,41 +389,41 @@
 	if (prev) {
 		vma->vm_next = prev->vm_next;
 		prev->vm_next = vma;
 	} else {
 		mm->mmap = vma;
 		if (rb_parent)
 			vma->vm_next = rb_entry(rb_parent,
 					struct vm_area_struct, vm_rb);
 		else
 			vma->vm_next = NULL;
 	}
 }
 
 void __vma_link_rb(struct mm_struct *mm, struct vm_area_struct *vma,
 		struct rb_node **rb_link, struct rb_node *rb_parent)
 {
 	rb_link_node(&vma->vm_rb, rb_parent, rb_link);
 	rb_insert_color(&vma->vm_rb, &mm->mm_rb);
 }
 
-static inline void __vma_link_file(struct vm_area_struct *vma)
+static void __vma_link_file(struct vm_area_struct *vma)
 {
 	struct file * file;
 
 	file = vma->vm_file;
 	if (file) {
 		struct address_space *mapping = file->f_mapping;
 
 		if (vma->vm_flags & VM_DENYWRITE)
 			atomic_dec(&file->f_path.dentry->d_inode->i_writecount);
 		if (vma->vm_flags & VM_SHARED)
 			mapping->i_mmap_writable++;
 
 		flush_dcache_mmap_lock(mapping);
 		if (unlikely(vma->vm_flags & VM_NONLINEAR))
 			vma_nonlinear_insert(vma, &mapping->i_mmap_nonlinear);
 		else
 			vma_prio_tree_insert(vma, &mapping->i_mmap);
 		flush_dcache_mmap_unlock(mapping);
 	}
 }
@@ -1558,41 +1558,41 @@
 	 * Overcommit..  This must be the final test, as it will
 	 * update security statistics.
 	 */
 	if (security_vm_enough_memory(grow))
 		return -ENOMEM;
 
 	/* Ok, everything looks good - let it rip */
 	mm->total_vm += grow;
 	if (vma->vm_flags & VM_LOCKED)
 		mm->locked_vm += grow;
 	vm_stat_account(mm, vma->vm_flags, vma->vm_file, grow);
 	return 0;
 }
 
 #if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
 /*
  * PA-RISC uses this for its stack; IA64 for its Register Backing Store.
  * vma is the last one with address > vma->vm_end.  Have to extend vma.
  */
 #ifndef CONFIG_IA64
-static inline
+static
 #endif
 int expand_upwards(struct vm_area_struct *vma, unsigned long address)
 {
 	int error;
 
 	if (!(vma->vm_flags & VM_GROWSUP))
 		return -EFAULT;
 
 	/*
 	 * We must make sure the anon_vma is allocated
 	 * so that the anon_vma locking is not a noop.
 	 */
 	if (unlikely(anon_vma_prepare(vma)))
 		return -ENOMEM;
 	anon_vma_lock(vma);
 
 	/*
 	 * vma->vm_start/vm_end cannot change under us because the caller
 	 * is required to hold the mmap_sem in read mode.  We need the
 	 * anon_vma lock to serialize against concurrent expand_stacks.
@@ -1608,41 +1608,41 @@
 
 	/* Somebody else might have raced and expanded it already */
 	if (address > vma->vm_end) {
 		unsigned long size, grow;
 
 		size = address - vma->vm_start;
 		grow = (address - vma->vm_end) >> PAGE_SHIFT;
 
 		error = acct_stack_growth(vma, size, grow);
 		if (!error)
 			vma->vm_end = address;
 	}
 	anon_vma_unlock(vma);
 	return error;
 }
 #endif /* CONFIG_STACK_GROWSUP || CONFIG_IA64 */
 
 /*
  * vma is the first one with address < vma->vm_start.  Have to extend vma.
  */
-static inline int expand_downwards(struct vm_area_struct *vma,
+static int expand_downwards(struct vm_area_struct *vma,
 				   unsigned long address)
 {
 	int error;
 
 	/*
 	 * We must make sure the anon_vma is allocated
 	 * so that the anon_vma locking is not a noop.
 	 */
 	if (unlikely(anon_vma_prepare(vma)))
 		return -ENOMEM;
 
 	address &= PAGE_MASK;
 	error = security_file_mmap(NULL, 0, 0, 0, address, 1);
 	if (error)
 		return error;
 
 	anon_vma_lock(vma);
 
 	/*
 	 * vma->vm_start/vm_end cannot change under us because the caller
@@ -1670,68 +1670,68 @@
 int expand_stack_downwards(struct vm_area_struct *vma, unsigned long address)
 {
 	return expand_downwards(vma, address);
 }
 
 #ifdef CONFIG_STACK_GROWSUP
 int expand_stack(struct vm_area_struct *vma, unsigned long address)
 {
 	return expand_upwards(vma, address);
 }
 
 struct vm_area_struct *
 find_extend_vma(struct mm_struct *mm, unsigned long addr)
 {
 	struct vm_area_struct *vma, *prev;
 
 	addr &= PAGE_MASK;
 	vma = find_vma_prev(mm, addr, &prev);
 	if (vma && (vma->vm_start <= addr))
 		return vma;
-	if (!prev || expand_stack(prev, addr))
+	if (!prev || expand_upwards(prev, addr))
 		return NULL;
 	if (prev->vm_flags & VM_LOCKED)
 		make_pages_present(addr, prev->vm_end);
 	return prev;
 }
 #else
 int expand_stack(struct vm_area_struct *vma, unsigned long address)
 {
 	return expand_downwards(vma, address);
 }
 
 struct vm_area_struct *
 find_extend_vma(struct mm_struct * mm, unsigned long addr)
 {
 	struct vm_area_struct * vma;
 	unsigned long start;
 
 	addr &= PAGE_MASK;
 	vma = find_vma(mm,addr);
 	if (!vma)
 		return NULL;
 	if (vma->vm_start <= addr)
 		return vma;
 	if (!(vma->vm_flags & VM_GROWSDOWN))
 		return NULL;
 	start = vma->vm_start;
-	if (expand_stack(vma, addr))
+	if (expand_downwards(vma, addr))
 		return NULL;
 	if (vma->vm_flags & VM_LOCKED)
 		make_pages_present(addr, start);
 	return vma;
 }
 #endif
 
 /*
  * Ok - we have the memory areas we should free on the vma list,
  * so release them, and do the vma updates.
  *
  * Called with the mm semaphore held.
  */
 static void remove_vma_list(struct mm_struct *mm, struct vm_area_struct *vma)
 {
 	/* Update high watermark before we lower total_vm */
 	update_hiwater_vm(mm);
 	do {
 		long nrpages = vma_pages(vma);
 


[-- Attachment #3: linux-2.6.28-rc4.mmap.c.patch --]
[-- Type: text/x-diff, Size: 416 bytes --]

--- 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)

Who broke cb8f488c33 patch? (was Re: [PATCH 1/1] USBHID: correct start/stop cycle)

[Jiri Kosina]

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

	[someone@somewhere.com: 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 <vda.linux@googlemail.com>
> --- 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

Re: Who broke cb8f488c33 patch? (was Re: [PATCH 1/1] USBHID: correct start/stop cycle)

[Andrew Morton]

On Wed, 12 Nov 2008 01:34:54 +0100 (CET)
Jiri Kosina <jkosina@suse.cz> 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
> 
> 	[someone@somewhere.com: 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 <vda.linux@googlemail.com>
> > --- 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.

Re: Who broke cb8f488c33 patch? (was Re: [PATCH 1/1] USBHID: correct start/stop cycle)

[Jiri Slaby]

On 11/12/2008 01:50 AM, Andrew Morton wrote:
>>> let's fix it for good.
>>> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
>>> --- 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.

Re: Who broke cb8f488c33 patch? (was Re: [PATCH 1/1] USBHID: correct start/stop cycle)

[Helge Deller]

Jiri Slaby wrote:
> On 11/12/2008 01:50 AM, Andrew Morton wrote:
>>>> let's fix it for good.
>>>> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
>>>> --- 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

Re: Who broke cb8f488c33 patch? (was Re: [PATCH 1/1] USBHID: correct start/stop cycle)

[Linus Torvalds]

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