dwc_otg: Call usb_hcd_unlink_urb_from_ep with lock held in completion ta...

[wmdb]

...sklet

This fixes a regression introduced in c4564d4,
usb_hcd_unlink_urb_from_ep must be called with the lock held. Not doing so
resulted in corruption of the list.

[popcornmix]

@P33M, @ghollingworth
Happy with this fix?

[wmdb]

Yes, we had an application that involved a lot of powering down/replugging USB devices, sometimes in the middle of serving data over a cdc_ether link. We were hitting kernel oopses due to NULL pointer dereferences in this driver quite frequently. These seem to have gone now, we've done a fair bit of stress testing since this patch went in.

[wmdb]

It occurred to me that if this problem was occurring at all then there must be contention in another place in the driver as well, this turned out to be correct. However fixing it reintroduced the problem again, so we worked out a better solution that I believe addresses the underlying problem. It turned out that it went deeper than just the tasklet code, so the NULL pointer dereferences were likely being seen even prior to that change, but the tasklet made the situation more likely to occur.

The update is at: wmdb@521f206

Errors closely resembling this problem, and pre-dating c4564d4, were reported here: http://www.raspberrypi.org/phpBB3/viewtopic.php?t=16280

Let me know your views on this, this is my first time posting commits through github, so if I am doing it all wrong please do say so.

Mike

[popcornmix]

@wmdb
You can update the pull request by editing the git commit and force pushing (git push -f).
You can edit with "git commit --amend" or "git rebase -i HEAD~" (amongst many other ways...)

[ghollingworth]

I'd suggest you also try this with the fiq_split branch because there are other changes there that probably make this bug occur more (the dequeing fix actually means some of those things make themselves through to the task let)

Gordon

[wmdb]

@popcornmix
I thought you might shoot me if I suggested forcing a push, but I have done as you suggested :-)

@ghollingworth
I did try testing against fiq_split but there were too many changes needed in our module environment for me to do this in the time available. Looking at it, I did not see evidence of conflicts with fiq_split, and I agree it does seem possible that code in that branch might have triggered the bug as well.

[popcornmix]

You've pulled in some unwanted commits.
git rebase -i HEAD~6
And you should be able to fix that up.

[wmdb]

rebase upstream/rpi-3.6.y seemed to do the trick, not sure how those commits ended up with new IDs but they are back the way they were now

Mike

[P33M]

Kernel docs say that usb_hcd_unlink_urb_from_ep has to be done with hcd lock held and interrupts disabled locally so the existing implementation does need changing.

To be clear, what was the call stack you were seeing with typical OOPSes? I had seen a few with kill_urbs_in_qh_list popping up but the problem was quite hard to reproduce (disconnects with an active device had a smallish chance to do it).

[wmdb]

I didn't have the ability to capture or read the full call stack in my setup but the primary message was always the same, "Unable to handle kernel NULL pointer dereference", in usb_hcd_giveback_urb. As you said it could sometimes take a while to replicate but our setup was such that we could spray data at it and just leave it replugging at random intervals, originally it would only take 5 or 10 minutes to reproduce.

I have dealt with a very similar problem in the past with another embedded host driver, killing urbs from a different context at unplug was the cause there, but I can't be certain if it was the same here.

I wasn't clear in saying that the second version of the patch also put the access to completed_urb_list inside the critical section, clearly this was needed on the insertion side, not just in the tasklet. The original version only bracketed the tasklet function, though it did seem to make the problem go away.

But I knew that couldn't be the end of the story and I bracketed the insertion side, but that made it suddenly easy to replicate again. Unlinking the endpoint prior to adding to to a queue that other parts of the driver know nothing about seemed like a good idea, as did ensuring that something else somewhere hadn't already unlinked it.

We've gone through thousands of replugs with this now without any issues.

[P33M]

Is this extra locking absolutely necessary within _complete? (line 359) As far as I can tell, this is called from within the driver during which either interrupts are disabled (as in from xfercomp or error handlers) or the spinlock is already held.

Edit: I see what you're saying now - is there evidence that _complete is being called without the lock being held?

Other than that, the rest makes sense to me.

[wmdb]

I asked myself the same question and I wasn't sure of the context it was
being called in at the time. In a single processor system like the current
one it may be sufficient just to guarantee IRQs are locked in _complete,
but in the general case I think you would need the lock to be held. It
would be more efficient just to use spin_lock in _complete() if IRQs are
already locked. It is also more efficient just to use spin_lock_irq in the
tasklet as you know IRQs are not locked there (I think I added a comment to
that effect, but there isn't a macro in this driver for that and I kept it
the way it was for consistency and because it is what we had the most
testing time with).

Or something like that, I don't have the code in front of me right now :-)

Mike

On 19 June 2013 07:03, P33M notifications@github.com wrote:

Is this extra locking absolutely necessary within _complete? (line 359) As
far as I can tell, this is called from within the driver during which
either interrupts are disabled (as in from xfercomp or error handlers) or
the spinlock is already held.

Other than that, the rest makes sense to me.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/312#issuecomment-19685781
.

Mike Bradley
Inca Networks Inc.
+1 604 653 3360 | mike.bradley@incanetworks.com
Skype: wmd_bradley

[P33M]

The HCD lock is claimed on entry to the IRQ handler, so all usual calls to _complete are with the lock held. The only potentially concurrency-unsafe use of _complete is in a HCD disconnect (i.e. shutdown) with multiple CPUs present, because for some reason the disconnect interrupt handler will release the lock before doing kill_all_urbs which then _completes them all with an error.

Before the tasklet implementation, the _complete function unlocked before usb_hcd_giveback_urb in case a completion callback resubmitted itself (common for interrupt or isochronous) avoiding a deadlock. With this split off to the tasklet, there's no need as we are in a softirq context with no locks held anyway.

For a minimal fix, all I can see that needs doing is:

• Correct flow of usb_hcd_check_unlink_urb / usb_hcd_unlink_urb_from_ep in _complete
• removal of usb_hcd_unlink_urb_from_ep from the tasklet and back into _complete where the lock is already held.

Would it be possible for you to test without the extra locking? I would prefer it if the completion path does not have any extra complexity added as this is a significant factor in performance and reliability of isoc transfers.

[wmdb]

I agree with this, assuming we have no concerns at this stage about the multi-processor case. The biggest impact comes from unlinking from the endpoint before kicking it over to the tasklet (the original idea of keeping that in the tasklet but with the lock held didn't go far enough).

Do you have a view on the applicability of usb_hcd_check_unlink_urb here? One fear I had with that was that an urb that was for some reason never linked to an endpoint might cause this call to fail. I don't know if such a situation could occur in the completion handler, or if _complete is responsible for giving the urb back if it did.

Since _complete is always run from the IRQ handler, and as far as we know the lock is already held there, I am running stress tests without the DWC_SPINLOCK_IRQSAVE/DWC_SPINUNLOCK_IRQRESTORE in _complete. So far there have been no problems.

I have left the other changes as they are for now, including the unlink check.

[P33M]

In theory check_unlink is called during a dequeue to check that we aren't about to unlink a URB that's been completed (and now sitting in the tasklet queue) or previously dequeued. I suspect that it shouldn't be necessary to check for an unlink in _complete but the original code must have had it in for a reason.

[wmdb]

The commit has been updated with those changes, it tested OK overnight (probably 10,000 replugs in that time).

[P33M]

Looks good to me.

I had a trawl through the kernel mailing lists and it seems someone has RFC'd a patch for putting a completion tasklet into EHCI drivers - with some similarly game-changing preemption latency speedups on low-speed systems. There's a few things mentioned that make me want to poke other bits of the driver a bit further, but as far as this bugfix is concerned I've got nothing further to add.

[popcornmix]

Thanks @wmdb and @P33M.

[qtfp]

Hey~
I have problem compiling. I compiled with the page for my touchscreen with the page:

http://engineering-diy.blogspot.tw/2013/01/adding-7inch-display-with-touchscreen.html

Then, I met problem with the ftdi ft232 usb-serial converter hang. Then, I rpi-updated to fix the ftdi ft232 hang problem.
Here I cant find a way to fix these two problem the same time.
How can I compile a version with the newest kernel version?
Thx