It is possible for the VBUS detect GPIO interrupt to occur before
nop_set_peripheral() is called, in which case otg->gadget is NULL.
Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
bdc_ep_disable() expects to be called with bdc->lock held.
The assumption is met in all the cases except for call from bdc_udc_exit(),
that is called from bdc_remove(). As a result a race can happen or unheld
bdc->lock can be unlocked in bdc_req_complete().
The patch proposes to acquire-release bdc->lock around bdc_ep_disable()
in bdc_udc_exit().
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
The musb_hdrc_platform_data::config was defined as a non-const pointer.
However some drivers (e.g. the ux500) set up this pointer to point to a
static structure, which is potentially dangerous. Since the musb core
uses the pointer in a read-only manner the const qualifier was added to
protect the content of the config.
Signed-off-by: Petr Kulhavy <petr@barix.com>
Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Signed-off-by: Bin Liu <b-liu@ti.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Move host core initialization and host channel routines into hcd.c. This
allows these functions to only be compiled in host-enabled driver
configurations (DRD or host-only).
Tested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Move the register save and restore functions into the host and gadget
specific files.
Tested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Here, free memory is allocated using kmem_cache_zalloc. So, use
kmem_cache_free instead of kfree.
This is done using Coccinelle and semantic patch used
is as follows:
//<smpl>
@@
expression x,E,c;
@@
x =
\(kmem_cache_alloc\|kmem_cache_zalloc\|kmem_cache_alloc_node\)(c,...)
... when != x = E
when != &x
?-kfree(x)
+kmem_cache_free(c,x)
//</smpl>
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
The microframe scheduler figured out exactly how many transfers we need
for a split transaction. Let's use this knowledge to know when to end
things.
Without this I found that certain devices would just keep responding
with tons of NYET resonses on their INT_IN endpoint. These would just
keep going and going and eventually we'd decide to terminate the
transfer (because the whole frame changed), but by that time the
scheduler would decide that we "missed" the start of the next transfer.
I can also imagine that if we blow past the end of our scheduled time we
may mess up other things that were scheduled to happen.
No known test cases are improved by this patch except that the scheduler
code doesn't yell about MISSES constantly anymore.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
This totally reimplements the microframe scheduler in dwc2 to attempt to
handle periodic splits properly. The old code didn't even try, so this
was a significant effort since periodic splits are one of the most
complicated things in USB.
I've attempted to keep the old "don't use the microframe" schduler
around for now, but not sure it's needed. It has also only been lightly
tested.
I think it's pretty certain that this scheduler isn't perfect and might
have some bugs, but it seems much better than what was there before.
With this change my stressful USB test (USB webcam + USB audio + some
keyboards) crackles less.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
When setting up ISO and INT transfers dwc2 needs to specify whether the
transfer is for an even or an odd frame (or microframe if the controller
is running in high speed mode).
The controller appears to use this as a simple way to figure out if a
transfer should happen right away (in the current microframe) or should
happen at the start of the next microframe. Said another way:
- If you set "odd" and the current frame number is odd it appears that
the controller will try to transfer right away. Same thing if you set
"even" and the current frame number is even.
- If the oddness you set and the oddness of the frame number are
_different_, the transfer will be delayed until the frame number
changes.
As I understand it, the above technique allows you to plan ahead of time
where possible by always working on the next frame. ...but it still
allows you to properly respond immediately to things that happened in
the previous frame.
The old dwc2_hc_set_even_odd_frame() didn't really handle this concept.
It always looked at the frame number and setup the transfer to happen in
the next frame. In some cases that meant that certain transactions
would be transferred in the wrong frame.
We'll try our best to set the even / odd to do the transfer in the
scheduled frame. If that fails then we'll do an ugly "schedule ASAP".
We'll also modify the scheduler code to handle this and not try to
schedule a second transfer for the same frame.
Note that this change relies on the work to redo the microframe
scheduler. It can work atop ("usb: dwc2: host: Manage frame nums better
in scheduler") but it works even better after ("usb: dwc2: host: Totally
redo the microframe scheduler").
With this change my stressful USB test (USB webcam + USB audio +
keyboards) has less audio crackling than before.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
As we start getting more exact about our scheduling it's becoming more
and more important to know exactly how far through the current frame we
are. This lets us make decisions about whether there's still time left
to start a new transaction in the current frame.
We'll add dwc2_hcd_get_future_frame_number() which will tell you what
the frame number will be a certain number of microseconds (us) from
now. We can use this information to help decide if there's enough time
left in the frame for a transaction that will take a certain duration.
This is expected to be used by a future change ("usb: dwc2: host:
Properly set even/odd frame").
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
The dwc2 scheduler (contained in hcd_queue.c) was a bit confusing in the
way it initted / kept track of which frames a QH was going to be active
in. Let's clean things up a little bit in preparation for a rewrite of
the microframe scheduler.
Specifically:
* Old code would pick a frame number in dwc2_qh_init() and would try to
pick it "in a slightly future (micro)frame". As far as I can tell the
reason for this was that there was a delay between dwc2_qh_init() and
when we actually wanted to dwc2_hcd_qh_add(). ...but apparently this
attempt to be slightly in the future wasn't enough because
dwc2_hcd_qh_add() then had code to reset things if the frame _wasn't_
in the future. There's no reason not to just pick the frame later.
For non-periodic QH we now pick the frame in dwc2_hcd_qh_add(). For
periodic QH we pick the frame at dwc2_schedule_periodic() time.
* The old "dwc2_qh_init() actually assigned to "hsotg->frame_number".
This doesn't seem like a great idea since that variable is supposed to
be used to keep track of which SOF the interrupt handler has seen.
Let's be clean: anyone who wants the current frame number (instead of
the one as of the last interrupt) should ask for it.
* The old code wasn't terribly consistent about trying to use the frame
that the microframe scheduler assigned to it. In
dwc2_sched_periodic_split() when it was scheduling the first frame it
always "ORed" in 0x7 (!). Since the frame goes on the wire 1 uFrame
after next_active_frame it meant that the SSPLIT would always try for
uFrame 0 and the transaction would happen on the low speed bus during
uFrame 1. This is irregardless of what the microframe scheduler
said.
* The old code assumed it would get called to schedule the next in a
periodic split very quickly. That is if next_active_frame was
0 (transfer on wire in uFrame 1) it assumed it was getting called to
schedule the next uFrame during uFrame 1 too (so it could queue
something up for uFrame 2). It should be possible to actually queue
something up for uFrame 2 while in uFrame 2 (AKA queue up ASAP). To
do this, code needs to look at the previously scheduled frame when
deciding when to next be active, not look at the current frame number.
* If there was no microframe scheduler, the old code would check for
whether we should be active using "qh->next_active_frame ==
frame_number". This seemed like a race waiting to happen. ...plus
there's no way that you wouldn't want to schedule if next_active_frame
was actually less than frame number.
Note that this change doesn't make 100% sense on its own since it's
expecting some sanity in the frame numbers assigned by the microframe
scheduler and (as per the future patch which rewries it) I think that
the current microframe scheduler is quite insane. However, it seems
like splitting this up from the microframe scheduler patch makes things
into smaller chunks and hopefully adds to clarity rather than reduces
it. The two patches could certainly be squashed. Not that in the very
least, I don't see any obvious bad behavior introduced with just this
patch.
I've attempted to keep the config parameter to disable the microframe
scheduler in tact in this change, though I'm not sure it's worth it.
Obviously the code is touched a lot so it's possible I regressed
something when the microframe scheduler is disabled, though I did some
basic testing and it seemed to work OK. I'm still not 100% sure why you
wouldn't want the microframe scheduler (presuming it works), so maybe a
future patch (or a future version of this patch?) could remove that
parameter.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
We'll use the new "scheduler verbose debugging" macro to log missed
SOFs. This is fast enough (assuming you configure it to use the ftrace
buffer) that we can do it without worrying about the speed hit. The
overhead hit if the scheduler tracing is set to "no_printk" should be
near zero.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
This no-op change splits code out of dwc2_schedule_periodic() into a
dwc2_do_reserve() function. This makes it a little easier to follow the
logic.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
This no-op change just reorders a few functions in hcd_queue.c in order
to prepare for future changes. Motivations here:
The functions dwc2_hcd_qh_free() and dwc2_hcd_qh_create() are exported
functions. They are not called within the file. That means that they
should be near the bottom so that they can easily call static helpers.
The function dwc2_qh_init() is only called by dwc2_hcd_qh_create() and
should move near the bottom with it.
The only reason that the dwc2_unreserve_timer_fn() timer function (and
its subroutine dwc2_do_unreserve()) were so high in the file was that
they needed to be above dwc2_qh_init(). Now that dwc2_qh_init() has
been moved down it can be moved down a bit. A later patch will split
the reserve code out of dwc2_schedule_periodic() and the reserve
function should be near the unreserve function. The reserve function
needs to be below dwc2_find_uframe() since it calls that.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
This no-op change just does some renames to simplify a future patch.
1. The "interval" field is renamed to "host_interval" to make it more
obvious that this interval may be 8 times the interval that the
device sees (if we're doing split transactions). A future patch will
also add the "device_interval" field.
2. The "usecs" field is renamed to "host_us" again to make it more
obvious that this is the time for the transaction as seen by the
host. For split transactions the device may see a much longer
transaction time. A future patch will also add "device_us".
3. The "sched_frame" field is renamed to "next_active_frame". The name
"sched_frame" kept confusing me because it felt like something more
permament (the QH's reservation or something). The name
"next_active_frame" makes it more obvious that this field is
constantly changing.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
The old code in dwc2_process_periodic_channels() would only enable the
"periodic empty" interrupt if we weren't using DMA. That wasn't right
since we can still get into cases where we have small FIFOs even on
systems that have DMA (the rk3288 is a prime example).
Let's always enable/disable the "periodic empty" when appropriate. As
part of this:
* Always call dwc2_process_periodic_channels() even if there's nothing
in periodic_sched_assigned (we move the queue empty check so we still
avoid the extra work). That will make extra certain that we will
properly disable the "periodic empty" interrupt even if there's
nothing queued up.
* Move the enable of "periodic empty" due to non-empty
periodic_sched_assigned to be for slave mode (non-DMA mode) only.
Presumably this was the original intention of the check for DMA since
it seems to match the comments above where in slave mode we leave
things on the assigned queue.
Note that even before this change slave mode didn't work for me, so I
can't say for sure that my understanding of slave mode is correct.
However, this shouldn't change anything for slave mode so if slave mode
worked for someone in the past it ought to still work.
With this change, I no longer get constant misses reported by my other
debugging code (and with future patches) when I've got:
* Rockchip rk3288 Chromebook, using port ff540000
-> Pluggable 7-port Hub with Charging (powered)
-> Microsoft Wireless Keyboard 2000 in port 1.
-> Das Keyboard in port 2.
-> Jabra Speaker in port 3
-> Logitech, Inc. Webcam C600 in port 4
-> Microsoft Sidewinder X6 Keyboard in port 5
...and I'm playing music on the USB speaker and capturing video from the
webcam.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
I find that when I plug a full speed (NOT high speed) hub into a dwc2
port and then I plug a bunch of devices into that full speed hub that
dwc2 goes bat guano crazy. Specifically, it just spews errors like this
in the console:
usb usb1: clear tt 1 (9043) error -22
The specific test case I used looks like this:
/: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
|__ Port 1: Dev 17, If 0, Class=Hub, Driver=hub/4p, 12M
|__ Port 2: Dev 19, If 0, ..., Driver=usbhid, 1.5M
|__ Port 4: Dev 20, If 0, ..., Driver=usbhid, 12M
|__ Port 4: Dev 20, If 1, ..., Driver=usbhid, 12M
|__ Port 4: Dev 20, If 2, ..., Driver=usbhid, 12M
Showing VID/PID:
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 001 Device 017: ID 03eb:3301 Atmel Corp. at43301 4-Port Hub
Bus 001 Device 020: ID 045e:0745 Microsoft Corp. Nano Transceiver ...
Bus 001 Device 019: ID 046d:c404 Logitech, Inc. TrackMan Wheel
I spent a bunch of time trying to figure out why there are errors to
begin with. I believe that the issue may be a hardware issue where the
transceiver sometimes accidentally sends a PREAMBLE packet if you send a
packet to a full speed device right after one to a low speed device.
Luckily the USB driver retries and the second time things work OK.
In any case, things kinda seem work despite the errors, except for the
"clear tt" spew mucking up my console. Chalk it up for a win for
retries and robust protocols.
So getting back to the "clear tt" problem, it appears that we get those
because there's not actually a TT here to clear. It's my understanding
that when dwc2 operates in low speed or full speed mode that there's no
real TT out there. That makes all these attempts to "clear the TT"
somewhat meaningless and also causes the spew in the log.
Let's just skip all the useless TT clears. Eventually we should root
cause the errors, but even if we do this is still a proper fix and is
likely to avoid the "clear tt" error in the future.
Note that hooking up a Full Speed USB Audio Device (Jabra 510) to this
same hub with the keyboard / trackball shows that even audio works over
this janky connection. As a point to note, this particular change (skip
bogus TT clears) compared to just commenting out the dev_err() in
hub_tt_work() actually produces better audio.
Note: don't ask me where I got a full speed USB hub or whether the
massive amount of dust that accumulated on it while it was in my junk
box affected its funtionality. Just smile and nod.
Acked-by: John Youn <johnyoun@synopsys.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
According to the most up to date version of the dwc2 databook, the FRINT
field of the HFIR register should be programmed to:
* 125 us * (PHY clock freq for HS) - 1
* 1000 us * (PHY clock freq for FS/LS) - 1
This is opposed to older versions of the doc that claimed it should be:
* 125 us * (PHY clock freq for HS)
* 1000 us * (PHY clock freq for FS/LS)
In case you didn't spot it, the difference is the "- 1".
Let's add the "- 1" to match the newest user manual. It's presumed that
the "- 1" should have always been there and that this was always a
documentation error. If some hardware needs the "- 1" and other
hardware doesn't, we'll have to add a configuration parameter for it in
the future.
I checked things before and after this patch on rk3288 using a Total
Phase Beagle 5000 analyzer.
Before this patch, a low speed mouse shows constant Frame Timing Jitter
errors. After this patch errors have gone away.
Before this patch SOF packets move forward about 1 us per 4 ms. After
this patch the SOF packets move backward about 1 us per 255 ms. Some
specific SOF timestamps from the analyzer are below.
Before:
6.603.790
6.603.916
6.604.041
6.604.166
...
6.607.541
6.607.667
6.607.792
6.607.917
...
6.611.417
6.611.543
6.611.668
6.611.793
After:
6.215.159
6.215.284
6.215.408
6.215.533
6.215.658
...
6.470.658
6.470.783
6.470.907
...
6.726.032
6.726.157
6.725.281
6.725.406
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
In commit 94dfd7edfd ("USB: HCD: support giveback of URB in tasklet
context") support was added to give back the URB in tasklet context.
Let's take advantage of this in dwc2.
This speeds up the dwc2 interrupt handler considerably.
Note that this requires the change ("usb: dwc2: host: Add a delay before
releasing periodic bandwidth") to come first.
Note that, as per Alan Stern in
<https://patchwork.kernel.org/patch/7555771/>, we also need to make sure
that the extra delay before the device drivers submit more data doesn't
break the scheduler. At the moment the scheduler is pretty broken (see
future patches) so it's hard to be 100% certain, but I have yet to see
any new breakage introduced by this delay. ...and speeding up interrupt
processing for dwc2 is a huge deal because it means we've got a better
chance of not missing SOF interrupts. That means we've got an overall
win here.
Note that when playing USB audio and using a USB webcam and having
several USB keyboards plugged in, the crackling on the USB audio device
is noticably reduced with this patch.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
We'd like to be able to use HCD_BH in order to speed up the dwc2 host
interrupt handler quite a bit. However, according to the kernel doc for
usb_submit_urb() (specifically the part about "Reserved Bandwidth
Transfers"), we need to keep a reservation active as long as a device
driver keeps submitting. That was easy to do when we gave back the URB
in the interrupt context: we just looked at when our queue was empty and
released the reserved bandwidth then. ...but now we need a little more
complexity.
We'll follow EHCI's lead in commit 9118f9eb4f ("USB: EHCI: improve
interrupt qh unlink") and add a 5ms delay. Since we don't have a whole
timer infrastructure in dwc2, we'll just add a timer per QH. The
overhead for this is very small.
Note that the dwc2 scheduler is pretty broken (see future patches to fix
it). This patch attempts to replicate all old behavior and just add the
proper delay.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
In preparation for future changes to the scheduler let's add some
tracing that makes it easy for us to see what's happening. By default
this tracing will be off.
By changing "core.h" you can easily trace to ftrace, the console, or
nowhere.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
We're supposed to keep outstanding splits in order. Keep track of a
list of the order of splits and process channel interrupts in that
order.
Without this change and the following setup:
* Rockchip rk3288 Chromebook, using port ff540000
-> Pluggable 7-port Hub with Charging (powered)
-> Microsoft Wireless Keyboard 2000 in port 1.
-> Das Keyboard in port 2.
...I find that I get dropped keys on the Microsoft keyboard (I'm sure
there are other combinations that fail, but this documents my test).
Specifically I've been typing "hahahahahahaha" on the keyboard and often
see keys dropped or repeated.
After this change the above setup works properly. This patch is based
on a previous patch proposed by Yunzhi Li ("usb: dwc2: hcd: fix periodic
transfer schedule sequence")
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Yunzhi Li <lyz@rock-chips.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Kever Yang <kever.yang@rock-chips.com>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
The queues the the dwc2 host controller used are truly queues. That
means FIFO or first in first out.
Unfortunately though the code was iterating through these queues
starting from the head, some places in the code was adding things to the
queue by adding at the head instead of the tail. That means last in
first out. Doh.
Go through and just always add to the tail.
Doing this makes things much happier when I've got:
* 7-port USB 2.0 Single-TT hub
* - Microsoft 2.4 GHz Transceiver v7.0 dongle
* - Jabra speakerphone playing music
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
When poking around with USB devices with slub_debug enabled, I found
another obvious use after free. Turns out that in dwc2_hc_n_intr() I
was in a state when the contents of chan->qh was filled with 0x6b,
indicating that chan->qh was freed but chan still had a reference to
it.
Let's make sure that whenever we free qh we also make sure we remove a
reference from its channel.
The bug fixed here doesn't appear to be new--I believe I just got lucky
and happened to see it while stress testing.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
As documented in dwc2_calculate_dynamic_fifo(), host_rx_fifo_size should
really be:
2 * ((Largest Packet size / 4) + 1 + 1) + n
with n = number of host channel.
We have 9 host channels, so
2 * ((1024/4) + 2) + 9 = 516 + 9 = 525
We've got 960 / 972 total_fifo_size on rk3288 (and presumably on
rk3066) and 525 + 128 + 256 = 909 so we're still under on both ports
even when we increment by 5.
In the future, it would be nice if dwc2_calculate_dynamic_fifo() could
handle the "too small" FIFO case and come up with something more
dynamically. When we do that we can figure out how to allocate the
extra 48 / 60 bytes of FIFO that we're currently wasting.
NOTE: no known bugs are fixed by this patch, but it seems like a simple
fix and ought to fix someone.
Acked-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
All other host controllers who want aligned buffers for DMA do it a
certain way. Let's do that too instead of working behind the USB core's
back. This makes our interrupt handler not take forever and also rips
out a lot of code, simplifying things a bunch.
This also has the side effect of removing the 65535 max transfer size
limit.
NOTE: The actual code to allocate the aligned buffers is ripped almost
completely from the tegra EHCI driver. At some point in the future we
may want to add this functionality to the USB core to share more code
everywhere.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Acked-by: John Youn <johnyoun@synopsys.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: John Youn <johnyoun@synopsys.com>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Previously we needed to set the max_transfer_size to explicitly be 65535
because the old driver would detect that our hardware could support much
bigger transfers and then would try to do them. This wouldn't work
since the DMA alignment code couldn't support it.
Later in commit e8f8c14d9d ("usb: dwc2: clip max_transfer_size to
65535") upstream added support for clipping this automatically. Since
that commit it has been OK to just use "-1" (default), but nobody
bothered to change it.
Let's change it to default now for two reasons:
- It's nice to use autodetected params.
- If we can remove the 65535 limit, we can transfer more!
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Acked-by: John Youn <johnyoun@synopsys.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Check that dwc->maximum_speed is set to a valid value. Also add an error
when we use it later if we encounter an invalid value.
Signed-off-by: John Youn <johnyoun@synopsys.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
MODULE_DEVICE_TABLE() is missing, so the module isn't auto-loading on
sunxi systems using the OTG controller. This commit adds the missing
line so it loads automatically when building it as a module and running
on a system with an USB OTG port.
Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
B-device detects that bus is idle for more than TB_AIDL_BDIS min and
begins HNP by turning off pullup on DP, this allows the bus to discharge
to the SE0 state. This timer was missed and failed with PET test:
6.8.5 B-UUT HNP of USB OTG and EH automated compliance plan v1.2,
this patch is to fix this timing issue.
Acked-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Enable HNP polling support for chipidea gadget and allocate memory
for host request flag when otg fsm init.
Acked-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Set host_request_flag if the current peripheral wants to take host role
via changing a_bus_req or b_bus_req by user application.
Acked-by: Peter Chen <peter.chen@freescale.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
If gadget with HNP polling support receives GetStatus request of otg
status selector, it feedback to host with host request flag to indicate
if it wants to take host role.
Acked-by: Peter Chen <peter.chen@freescale.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Since gadget driver will handle this request, so controller driver bypass
it.
Acked-by: Peter Chen <peter.chen@freescale.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Adds HNP polling timer when transits to host state, the OTG status
request will be sent to peripheral after timeout, if host request flag
is set, it will switch to peripheral state, otherwise it will repeat HNP
polling every 1.5s and maintain the current session.
Acked-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Since commit 855ed04a37 ("usb: gadget:
udc-core: independent registration of gadgets and gadget drivers") gadget
drivers can not assume that UDC drivers are already available on their
initialization. This broke the HACK, which was used in gadgetfs driver,
to get UDC controller name. This patch removes this hack and replaces it
by additional function in the UDC core (which is usefully only for legacy
drivers, please don't use it in the new code).
Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
For every in_substream, there must be a corresponding gmidi_in_port
structure so it is perfectly viable and some might argue sensible to
stash pointer to the input substream in the gmidi_in_port structure.
This has an added benefit that if in_ports < MAX_PORTS, the whole
f_midi structure takes up less space because only in_ports number of
pointers for in_substream are allocated instead of MAX_PORTS lots of
them.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
We added a new error path to this function and we forgot to drop the
lock.
Fixes: e1e3d7ec5d ('usb: gadget: f_midi: pre-allocate IN requests')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
[mina86@mina86.com: rebased on top of refactoring commit]
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Reduce number of allocations, simplify memory management and reduce
memory usage by stacking the gmidi_in_port elements at the end of the
f_midi structure using a flexible array.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
In general case, all of midi->in_port pointers may be non-NULL which
implies that the ‘if (\!port)’ condition will never execute thus never
zeroing midi->in_last_port. Fix by rewriting the loop such that the
field is set to zero if \!port or end of loop has been reached.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Move some of the f_midi_transmit to a separate f_midi_do_transmit
function so the massive indention levels are not so jarring. This
introduces no changes in behaviour.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
remove a field which is unnecessary. No functional changes.
Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
ffs_epfile_io and ffs_epfile_io_complete runs in different context, but
there is no synchronization between them.
consider the following scenario:
1) ffs_epfile_io interrupted by sigal while
wait_for_completion_interruptible
2) then ffs_epfile_io set ret to -EINTR
3) just before or during usb_ep_dequeue, the request completed
4) ffs_epfile_io return with -EINTR
In this case, ffs_epfile_io tell caller no transfer success but actually
it may has been done. This break the caller's pipe.
Below script can help test it (adbd is the process which lies on f_fs).
while true
do
pkill -19 adbd #SIGSTOP
pkill -18 adbd #SIGCONT
sleep 0.1
done
To avoid this, just dequeue the request first. After usb_ep_dequeue, the
request must be done or canceled.
With this change, we can ensure no race condition in f_fs driver. But
actually I found some of the udc driver has analogical issue in its
dequeue implementation. For example,
1) the dequeue function hold the controller's lock.
2) before driver request controller to stop transfer, a request
completed.
3) the controller trigger a interrupt, but its irq handler need wait
dequeue function to release the lock.
4) dequeue function give back the request with negative status, and
release lock.
5) irq handler get lock but the request has already been given back.
So, the dequeue implementation should take care of this case. IMO, it
can be done as below steps to dequeue a already started request,
1) request controller to stop transfer on the given ep. HW know the
actual transfer status.
2) after hw stop transfer, driver scan if there are any completed one.
3) if found, process it with real status. if no, the request can
canceled.
Signed-off-by: "Du, Changbin" <changbin.du@intel.com>
[mina86@mina86.com: rebased on top of refactoring commits]
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Eliminate one of the return paths by using a ‘goto error_mutex’ and
rearrange some if-bodies which results in reduction of the indention level
and thus hopefully makes the function easier to read and reason about.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
In ffs_epfile_io error label points to a return path which includes
a kfree(data) call. However, at the beginning of the function data is
always NULL so some of the early ‘goto error’ can safely be replaced
with a trivial return statement.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
In the AIO path, if allocating of a request failse, the function simply
goes to the error_lock path whose end result is returning value of ret.
However, at this point ret’s value is zero (assigned as return value from
ffs_mutex_lock).
Fix by adding ‘ret = -ENOMEM’ statement.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
In the ffs_epfile_io function, data buffer is allocated for non-halt
requests. Later, after grabing a mutex, the function checks that
epfile->ep is still ep and if it’s not, it set ret to -ESHUTDOWN and
follow a path including spin_unlock_irq (just after ‘ret = -ESHUTDOWN’),
mutex_unlock (after if-else-if-else chain) and returns ret. Noticeably,
this does not include freeing of the data buffer.
Fix by introducing a goto which moves control flow to the the end of the
function where spin_unlock_irq, mutex_unlock and kfree are all called.
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
phy-am335x.c doesn't use any interfaces from linux/regulator/consumer.h, so
stop including it.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
When I wrote the cleanup patch series, it was not clear how
exactly big-endian mode works on ixp4xx, and whether the driver
was doing this correctly. After discussing with Krzysztof Hałasa,
this has been clarified, so I can update the comment let pxa25x
big-endian (which we don't support) work the same way as ixp4xx.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
When dma_addr_t is 64-bit, we get a warning about an invalid cast
in the call to ux500_dma_is_compatible() from ux500_dma_channel_program():
drivers/usb/musb/ux500_dma.c: In function 'ux500_dma_channel_program':
drivers/usb/musb/ux500_dma.c:210:51: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
if (!ux500_dma_is_compatible(channel, packet_sz, (void *)dma_addr, len))
The problem is that ux500_dma_is_compatible() is called from the
main musb driver on the virtual address, but here we pass in a
DMA address, so the types are fundamentally different but it works
because the function only checks the alignment of the buffer and
that is the same.
We could work around this by adding another cast, but I have checked
that the buffer we get passed here is already checked before it
gets mapped, so the second check seems completely unnecessary
and removing it must be the cleanest solution.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Felipe Balbi <balbi@kernel.org>
Maarten ter Huurne