Re: xhci list corruption on sysfs removal

[Mathias Nyman]

On 21.01.2016 20:29, Joe Lawrence wrote:

On 12/23/2015 08:40 AM, Joe Lawrence wrote:

On 12/21/2015 10:07 AM, Mathias Nyman wrote:

Hi

On 18.12.2015 18:48, Joe Lawrence wrote:

Hello Roger and Mathias,

Running with slub_debug=FZPU and removing an XHCI host controller via
sysfs, I've hit a use-after-free that I've bisected to:

8c24d6d7b09deee3036ddc4f2b81b53b28c8f877 is the first bad commit
commit 8c24d6d7b09deee3036ddc4f2b81b53b28c8f877

...


If I revert 8c24d6d7b09d "usb: xhci: stop everything on the first call
to xhci_stop", the warning goes away.

Let me know if any additional instrumentation or information would help
track down this issue.



Thanks for the in-depth analysis.

Seems that the problem is that xhci driver frees data for all devices,
both
usb2 and and usb3 the first time  usb_remove_hcd() is called.

All data includes the all devices all endpoints and endpoint rings,
including the
the td_list in the xhci_ring struct.

When we call usb_remove_hcd() a second time for the second xhci bus, as
part of
usb_hcd_pci_remove() in xhci_pci_remove() it will try to dequeue all
pending urbs,
but td_list is already freed for that endpoint ring.

before commit 8c24d6d7b09d we only halted xhci when first hcd was
removed,
devices were freed only after second hcd removal.

we will also free xhci->devs[slot_id] and set it to null after freeing
the endpoint rings,
so something like this should work in this case (copypaste of git diff):

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 3f91270..dfc11d0 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1549,7 +1549,9 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
  xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
  "HW died, freeing TD.");
  urb_priv = urb->hcpriv;
-   for (i = urb_priv->td_cnt; i < urb_priv->length; i++) {
+   for (i = urb_priv->td_cnt;
+i < urb_priv->length &&
xhci->devs[urb->dev->slot_id];
+i++) {
  td = urb_priv->td[i];
  if (!list_empty(&td->td_list))
  list_del_init(&td->td_list);

There might be other similar cases caused by commit 8c24d6d7b09d.
This whole issue  probably needs more attention.


Hi Mathias,

Thanks for taking a look.  I applied these changes on top of 4.3 and
didn't see any further slub_debug complaints on sysfs or hotplug removal
during overnight testing. :)  Feel free to add my Reported-by or
Tested-by tag.


Gentle ping -- is there anything else I should need to test for this patch?



Somehow I missed this message earlier, sorry about that.
I'm waiting for rc-1 before I can send anything forward.
So hopefully next week

-Mathias

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Drivers: MUSB: Davinci MUSB: added DT support

[Petr Kulhavy]

Hi Rob,

thanks a lot for your comments. I admit I got inspired by the 
usb/omap-usb.txt  and usb/am33xx-usb.txt bindings
which use identical keywords, mode, num-eps, ram-bits, power and 
multipoint as all these chips use the same or similar MUSB core.
From your comments I have the feeling these are not the best bindings 
to take as an example.

What would you recommend as a good example then?

Regards
Petr

On 22.01.2016 00:07, Rob Herring wrote:

On Thu, Jan 21, 2016 at 03:53:19PM +0100, Petr Kulhavy wrote:

TI DaVinci MUSB driver equipped with DeviceTree support.
Tested with AM1808 board and USB2.0 (OTG) in host mode.

Signed-off-by: Petr Kulhavy
---
  .../devicetree/bindings/usb/da8xx-usb.txt  |  52 +++
  drivers/usb/musb/da8xx.c   | 166 +
  include/linux/platform_data/usb-davinci.h  |   3 +-
  3 files changed, 220 insertions(+), 1 deletion(-)
  create mode 100644 Documentation/devicetree/bindings/usb/da8xx-usb.txt

diff --git a/Documentation/devicetree/bindings/usb/da8xx-usb.txt 
b/Documentation/devicetree/bindings/usb/da8xx-usb.txt
new file mode 100644
index 000..c81d665
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/da8xx-usb.txt

Follow the compatible string for the filename (with wildcards is fine).


@@ -0,0 +1,52 @@
+TI DaVinci MUSB
+
+Required properties:
+
+ - compatible : Should be "ti,da850-musb" or "ti,da830-musb"
+
+ - mode : USB mode. "1" signifies HOST, "2" represents PERIPHERAL,
+ "3" represents OTG.

Surely we have a similar property defined already. Don't create
something new.


+
+ - power : This signifies the maximum current the controller can
+ supply in host mode. The unit size is 2mA, the maximum value is 510mA.

You should model this as a regulator.


+ - num-eps : Specifies the number of endpoints. This is also a
+ MUSB configuration-specific setting.

Just spell out endpoints.


+
+ - multipoint : Should be "1" indicating the musb controller supports
+ multipoint. This is a MUSB configuration-specific setting.

What does multipoint mean?


+ - ram-bits : Specifies the ram address size.

Needs a better description. Then it probably needs a better name too,
but without a description I can't tell what that would be.


+
+
+Optional properties:
+
+ - da8xx,phy20-clkmux-cfg: Integer. Defines the USB 2.0 PHY reference clock 
source.
+ Supported values: "0" for external pin, "1" for internal PLL.

How about a boolean property instead. Name it based on the less used
option (external pin I'm guessing).


+ - da8xx,phy20-refclock-frequency : Integer. Defines the USB 2.0 PHY reference 
clock input
+ frequency in Hz in case the clock is generated by the internal PLL.
+ Supported values are 12MHz, 13MHz, 19.2MHz, 20MHz, 24MHz, 26MHz, 38.4MHz, 
40MHz, 48MHz

da8xx is not a vendor.

Rob

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

[Bjørn Mork]

Emilio López  writes:

> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 38ae877c..bf40aa6 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -77,6 +77,8 @@ struct usb_dev_state {
>   unsigned long ifclaimed;
>   u32 secid;
>   u32 disabled_bulk_eps;
> + bool privileges_dropped;
> + unsigned long interface_allowed_mask;
>  };
>  
>  struct async {
> @@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned 
> int ifnum)
>   if (test_bit(ifnum, &ps->ifclaimed))
>   return 0;
>  
> + if (ps->privileges_dropped) {
> + if (ifnum >= 8*sizeof(ps->interface_allowed_mask))
> + return -EINVAL;


I don't think you need this runtime test. You can just make sure that
sizeof(ps->interface_allowed_mask) == sizeof(ps->ifclaimed) at build
time.

I do find this variable and arbitrary limit a bit confusing, but that's
not your fault - I guess it is an indication that ifnums > 31 are rare
:)


> diff --git a/include/uapi/linux/usbdevice_fs.h 
> b/include/uapi/linux/usbdevice_fs.h
> index 019ba1e..9abcb34 100644
> --- a/include/uapi/linux/usbdevice_fs.h
> +++ b/include/uapi/linux/usbdevice_fs.h
> @@ -154,6 +154,10 @@ struct usbdevfs_streams {
>   unsigned char eps[0];
>  };
>  
> +struct usbdevfs_drop_privs {
> + unsigned long interface_allowed_mask;
> +};
> +

"unsigned long" isn't a very good choice here, is it?



Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] USB: core: let USB device know device node

[Arnd Bergmann]

On Friday 22 January 2016 14:59:01 Peter Chen wrote:
> On Thu, Jan 21, 2016 at 11:24:21PM +0100, Arnd Bergmann wrote:
> > On Thursday 21 January 2016 10:21:15 Alan Stern wrote:
> > > On Thu, 21 Jan 2016, Arnd Bergmann wrote:
> > > > On Thursday 21 January 2016 17:48:32 Peter Chen wrote:
> > hub@1 { /* external hub, superspeed mode class 9/subclass 
> > 0/proto 3 */
> > compatible = "usb2109,0812.591",
> >  "usb2109,0812",
> >  "usb2109,class9.0.3",
> >  "usb2109,class9.0",
> >  "usb2109,class9";
> > compatible = "usb2109,0812";
> 
> Do we really need to write "compatible" so complicated?

The binding mandates it this way, but I guess we could decide to
make it a Linux-specific extension that we allow some of them to
be left out.

> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <1>;
> > 
> > communications@4 { /* superspeed ethernet device */
> > compatible = "usb0b95,1790.100",
> >  "usb0b95,1790",
> >  "usb0b95,class255.255.0",
> >  "usb0b95,class255.255",
> >  "usb0b95,class255",
> >  "usbif0b95,1790.100",
> >  "usbif0b95,1790",
> >  "usbif0b95,class255.255.0",
> >  "usbif0b95,class255.255,
> >  "usbif0b95,class255";
> > reg = <4>;
> > };
> > 
> > storage@1 { /* superspeed flash drive */
> > compatible = "usb1234,5678.600",
> >  "usb1234,5678",
> >  "usbif1234,class8.6.80",
> >  "usbif1234,class8.6",
> >  "usbif1234,class8",
> >  "usbif,class8.6.80",
> >  "usbif,class8.6",
> >  "usbif,class8";
> > reg = <1>;
> > };
> > };
> > 
> > hub@3 { /* same external  hub, highspeed mode */
> > compatible = "usb2109,0812.591",
> >  "usb2109,0812",
> >  "usb2109,class9.0.1",
> >  "usb2109,class9.0",
> >  "usb2109,class9";
> > 
> > #address-cells = <1>;
> > #size-cells = <0>;
> > reg = <3>;
> > 
> 
> Why "reg" is 3 here?

My mistake. It should be hub@1 and reg=<1>;

I accidentally confused the port number and the device number.

> > wireless@0,1 { /* bluetooth config 0, if 1 */
> > compatible = 
> > "usbif0a12,0001.134.config0.1",
> > 
> > "usbif0a12,0001.config0.1",
> > 
> > "usbif0a12,class224.1.1",
> > "usbif0a12,class224.1",
> > "usbif0a12,class224",
> > "usbif,class224.1.1",
> > "usbif,class224.1",
> > "usbif,class224";
> > reg = <0 0>;
> > };
> > };
> > };
> > };
> > 
> > In that description, I have included all four kinds of nodes from
> > the spec: host controller, device (wireless@2), interface (wireless@0.1,
> > wireless@0.2) and combined (hub@1, hub@3, storage@1, communications@4).
> > 
> > Peter's example only contained hubs in combined nodes, no device or
> > interface nodes. I wonder if the code is able to parse all four
> > kinds of nodes though, and if we actually need that.
> > 
> 
> My proposal patch only handles the node under the USB device, not include
> the USB interfaces under such device, we can add it after finalize
> how to describe it at device tree.

We should at least handle the case of multiple hubs connected to one
another I think. No need to limit it to directly connected devices
when it's easy enough to do hubs as well.

> > Do we have a 'struct device' for each interface?
> > 
> 
> Yes, we have, but there are different 'struct device' between USB device
> and USB interfaces

Re: USB gadgets with configfs hang reboot

[Andrzej Pietrasiewicz]

Hi Tony,

W dniu 15.01.2016 o 23:48, Tony Lindgren pisze:

Hi all,

Looks like there's some issue with the USB gadgets and configfs.

I'm seeing rmmod of the UDC driver cause a warning and then reboot
hangs the system. This happens at least with v4.4, and I've reproduced
it with dwc3 and musb so it seems to be generic.



I can't reproduce your problem on 4.4-rc4 nor on 4.4 release
with the script you provided

I'm using Odroid XU3, which has dwc3.


I think Felipe is offline for next few days, but anyways here are
the error messages and a shells script to reproduce the issue.

Cheers,

Tony






Test script to reproduce the problem, need to uncomment the reboot part:

8< 
#!/bin/bash

# Change for your UDC controller
udc_module=dwc3-omap
udc_name=4a03.dwc3


for me these are:

udc_module=dwc3-exynos
udc_name=1240.dwc3



#
# Calling this and then doing rmmod $udc_module
# followed by reboot seems to produce the follow:
# WARNING: CPU: 1 PID: 1617 at lib/debugobjects.c:263
#
start_usb_gadgets() {
vendor=0x1d6b
product=0x0106
file=$1

mount -t configfs none /sys/kernel/config
mkdir /sys/kernel/config/usb_gadget/g1
old_pwd=$(pwd)
cd /sys/kernel/config/usb_gadget/g1

echo $product > idProduct
echo $vendor > idVendor
mkdir strings/0x409
echo 123456789 > strings/0x409/serialnumber
echo foo > strings/0x409/manufacturer
echo "Multi Gadget" > strings/0x409/product

mkdir configs/c.1
echo 120 > configs/c.1/MaxPower
mkdir configs/c.1/strings/0x409
echo "Conf 1" > configs/c.1/strings/0x409/configuration

mkdir configs/c.2
echo 500 > configs/c.2/MaxPower
mkdir configs/c.2/strings/0x409
echo "Conf 2" > configs/c.2/strings/0x409/configuration

mkdir functions/mass_storage.0
echo $file > functions/mass_storage.0/lun.0/file
ln -s functions/mass_storage.0 configs/c.1
ln -s functions/mass_storage.0 configs/c.2

# Not configuring further here just produces
# WARNING: CPU: 1 PID: 1609 at lib/debugobjects.c:263

mkdir functions/acm.0
ln -s functions/acm.0 configs/c.1
ln -s functions/acm.0 configs/c.2

mkdir functions/ecm.0
ln -s functions/acm.0 configs/c.1


I assume you meant ecm.0 here and in the next line.


ln -s functions/acm.0 configs/c.2

# Adding rndis seems to cause alignment trap or some
# random oops on reboot after rmmod $udc_module
mkdir functions/rndis.0
ln -s functions/rndis.0 configs/c.1
ln -s functions/rndis.0 configs/c.2

echo $udc_name > /sys/kernel/config/usb_gadget/g1/UDC
cd $old_pwd
}

function rmmod_and_shutdown() {
rmmod $udc_module
reboot
}

modprobe $udc_module

start_usb_gadgets ""

# uncomment this to reproduce bug
# rmmod_and_shutdown


I have it uncommented, no problems.

But before I run this script I need to modprobe libcomposite,
though.

AP
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: host: xhci-plat: fix NULL pointer in probe for device tree case

[Gregory CLEMENT]

During probe, in the device tree case, the data pointer associated to a
compatible is dereferenced. However, not all the compatibles are
associated to a private data pointer.

The generic-xhci and the xhci-platform don't need them, this patch adds a
test on the data pointer before accessing it, avoiding a kernel crash.

Fixes: 4efb2f694114 ("usb: host: xhci-plat: add struct xhci_plat_priv")
Cc: sta...@vger.kernel.org
Signed-off-by: Gregory CLEMENT 
---
 drivers/usb/host/xhci-plat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 770b6b088797..d39d6bf1d090 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -184,7 +184,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
 
/* Just copy data for now */
-   *priv = *priv_match;
+   if (priv_match)
+   *priv = *priv_match;
}
 
if (xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_MARVELL_ARMADA)) {
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NET2280: Adding PLX usb2380 support (issues encountered)

[Justin DeFields]

It turns out that I had a filesystem issue on my target when testing
my removal of the /* paranoia */ block.  I thought I was writing the
file to NAND, when in fact it was being written over NFS to my Virtual
Machine!!!  After resolving this issue, and with the removal of the
paranoia logic, the transfer completes as expected very quickly and
reliably (20/20 transfers completed successfully).

Can anyone here help me determine if that paranoia loop is still
necessary for the PCIE cards, or what the purpose of it was in the
first place?

Thanks,

Justin D.

On Wed, Jan 20, 2016 at 2:43 PM, Justin DeFields
 wrote:
> System: Custom Xilinx Zynq based system
> Kernel: Xilinx linux v2015.1 (linux 3.18)
>
> I was attempting to add support for the PLX usb2380 chip to the
> net2280.c driver.  As far as I can tell, this chip is the same as the
> 3380, but does not support USB3.0 (2.0 only).
>
> I have changed all checks for the quirk PLX_SUPERSPEED to a new quirk
> called PLX_PCIE_MSI, except for where PLX_SUPERSPEED was used to set
> USB_SPEED_SUPER.
>
> This implementation seemed to work fine, except for when I attached
> the 'g_serial' gadget with ACM.  The issue I have is that any data
> transfer that is not a multiple of the endpoint maxpacket size (512
> bytes), results in the '/* paranoia */' block of code in
> 'scan_dma_completions (struct net2280_ep *ep)' function to be called 8
> times, and then the transfer goes off the rails at that point.  The
> data all got to it's destination, but much more data was processed
> than expected (it seems like the last DMA transaction is being
> processed more than once).
>
> I noticed that the chip has some errata, which specifically called out
> DMA issues when the transfer size is unaligned, but I am using the
> 'AB' revision, and that errata seemed to be listed for 'AA' only.
>
> As a side note, if I remove the  '/* paranoia */' block of code in
> scan_dma_completions, the transfer actually completes as expected with
> no extra data, but it takes a VERY long time for it to complete
> (roughly 10 seconds after the first paranoia loop is entered).
>
> If I use PIO instead of DMA, the transfer sometimes completes,
> sometimes it doesn't, and sometimes it doesn't even start.
>
> I suspect there could be some differences in FIFO sizes, or endpoint
> configuration between the 2380 and 3380, but I've tried many
> permutations and combinations of the various quirk flags in this
> driver, and most didn't change driver operation at all surprisingly
> (or at least not to any degree that I could measure).  I'm looking for
> some assistance or tips in debugging this issue.
>
> Thanks,
>
> Justin DeFields
> Embedded Software Engineer



-- 
Justin DeFields
Embedded Software Engineer
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] USB: core: let USB device know device node

[Alan Stern]

On Fri, 22 Jan 2016, Arnd Bergmann wrote:

> > >   hub@3 { /* same external  hub, highspeed mode */
> > >   compatible = "usb2109,0812.591",
> > >"usb2109,0812",
> > >"usb2109,class9.0.1",
> > >"usb2109,class9.0",
> > >"usb2109,class9";
> > > 
> > >   #address-cells = <1>;
> > >   #size-cells = <0>;
> > >   reg = <3>;
> > > 
> > 
> > Why "reg" is 3 here?
> 
> My mistake. It should be hub@1 and reg=<1>;
> 
> I accidentally confused the port number and the device number.

I thought you did it this way because you were numbering the SS
root-hub ports 1-2 and the HS root-hub ports 3-4.

There's something I should have made clear earlier.  This scheme for
putting SS and HS USB-3 root-hub ports in the same number space is part
of the xHCI spec.  It's not AFAIK required (or even mentioned) by the
USB-3 spec, which means other types of USB-3 host controllers might do
it differently.

The scheme which numbers SS and HS ports separately, both starting from
1, is mandated by the USB-3 spec for non-root hubs.  But since that
spec doesn't say much about root hubs, the OS is free to treat them
however it likes.  We have chosen to make root hubs appear as similar
as possible to non-root hubs; however I believe that Windows (for
example) may do things differently.

At any rate, since DT strives to reflect the actual hardware 
properties, you probably should use the xHCI numbering scheme when 
describing the ports of an xHCI root hub.


> > > Is it possible to have a hub in an interface of a multifunction device
> > > or are they always single-configuration single-interface devices?
> > > 
> > 
> > I have not seen such kinds of devices, but it is possible in theory.
> 
> Ok, so if the USB spec allows it, we should probably try to handle it too.

No, the spec does not allow it.  In fact, the spec divides all USB 
devices into two classes: hubs and functions.  A function is anything 
that isn't a hub.  And a hub is never allowed to contain more than one 
configuration and interface.

The spec does allow for multiple functions to be packaged in the same 
physical device.  In this case, the physical device contains a hub 
along with various functions permanently connected to it.

For example, the old Apple USB keyboards are compound devices.  They
contain an internal 3-port hub; one of the ports is permanently wired
to the keyboard controller and the other two are exposed to the user,
allowing a mouse and something else to be attached.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

[Alan Stern]

On Thu, 21 Jan 2016, Emilio López wrote:

> From: Reilly Grant 
> 
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
> 
> Signed-off-by: Reilly Grant 
> Signed-off-by: Emilio López 


>  static int proc_resetdevice(struct usb_dev_state *ps)
>  {
> + struct usb_host_config *actconfig = ps->dev->actconfig;
> + struct usb_interface *interface;
> + int i, number;
> +
> + /* Don't touch the device if any interfaces are claimed. It
> +  * could interfere with other drivers' operations and this
> +  * process has dropped its privileges to do such things.
> +  */

This comment should be rephrased.  It should say something like:
"Don't allow if the process has dropped its privilege to do such
things and any of the interfaces are claimed."

You also might consider allowing the reset if the interfaces are
claimed only by the current process (or more precisely, by ps).

> +static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
> +{
> + struct usbdevfs_drop_privs data;
> +
> + if (copy_from_user(&data, arg, sizeof(data)))
> + return -EFAULT;
> +
> + /* This is a one way operation. Once privileges were dropped,
> +  * you cannot do it again (Otherwise unprivileged processes
> +  * would be able to change their allowed interfaces mask)
> +  */

If you're going to keep a mask of claimable interfaces then there's no
reason this has to be a one-time operation.  Processes should always be
allowed to shrink the mask, just not to grow it.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] USB: core: let USB device know device node

[Arnd Bergmann]

On Friday 22 January 2016 10:55:01 Alan Stern wrote:
> On Fri, 22 Jan 2016, Arnd Bergmann wrote:
> 
> > > > hub@3 { /* same external  hub, highspeed mode */
> > > > compatible = "usb2109,0812.591",
> > > >  "usb2109,0812",
> > > >  "usb2109,class9.0.1",
> > > >  "usb2109,class9.0",
> > > >  "usb2109,class9";
> > > > 
> > > > #address-cells = <1>;
> > > > #size-cells = <0>;
> > > > reg = <3>;
> > > > 
> > > 
> > > Why "reg" is 3 here?
> > 
> > My mistake. It should be hub@1 and reg=<1>;
> > 
> > I accidentally confused the port number and the device number.
> 
> I thought you did it this way because you were numbering the SS
> root-hub ports 1-2 and the HS root-hub ports 3-4.

Right, I was confusing myself after the question. It was intentional
after all.

> There's something I should have made clear earlier.  This scheme for
> putting SS and HS USB-3 root-hub ports in the same number space is part
> of the xHCI spec.  It's not AFAIK required (or even mentioned) by the
> USB-3 spec, which means other types of USB-3 host controllers might do
> it differently.
> 
> The scheme which numbers SS and HS ports separately, both starting from
> 1, is mandated by the USB-3 spec for non-root hubs.  But since that
> spec doesn't say much about root hubs, the OS is free to treat them
> however it likes.  We have chosen to make root hubs appear as similar
> as possible to non-root hubs; however I believe that Windows (for
> example) may do things differently.
> 
> At any rate, since DT strives to reflect the actual hardware 
> properties, you probably should use the xHCI numbering scheme when 
> describing the ports of an xHCI root hub.

Yes, indeed that is what I was trying to say here.

> > > > Is it possible to have a hub in an interface of a multifunction device
> > > > or are they always single-configuration single-interface devices?
> > > > 
> > > 
> > > I have not seen such kinds of devices, but it is possible in theory.
> > 
> > Ok, so if the USB spec allows it, we should probably try to handle it too.
> 
> No, the spec does not allow it.  In fact, the spec divides all USB 
> devices into two classes: hubs and functions.  A function is anything 
> that isn't a hub.  And a hub is never allowed to contain more than one 
> configuration and interface.

Ok, that helps.

> The spec does allow for multiple functions to be packaged in the same 
> physical device.  In this case, the physical device contains a hub 
> along with various functions permanently connected to it.
> 
> For example, the old Apple USB keyboards are compound devices.  They
> contain an internal 3-port hub; one of the ports is permanently wired
> to the keyboard controller and the other two are exposed to the user,
> allowing a mouse and something else to be attached.

Good, that is straightforward to represent in a way that matches both
the DT binding and the Linux-internal representation.

We just have to decide what to do for non-hub devices that the OF
specification calls "combined nodes" (device class 0, one configuration,
one interface) and that, like hubs do not have one of_node per interface
plus one per device, but only one node.

Should we bind the of_node to the usb_device, the usb_interface or both
in this case? Doing both might be problematic and would need more
testing to be sure it works.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: storage: onetouch: Fixed two space related coding style issues

[Tapan Prakash T]

This patch fixes checkpatch.pl warning in file onetouch.c

Signed-off-by: Tapan Prakash T 
---
 drivers/usb/storage/onetouch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/onetouch.c b/drivers/usb/storage/onetouch.c
index acc3d03..9097bd4 100644
--- a/drivers/usb/storage/onetouch.c
+++ b/drivers/usb/storage/onetouch.c
@@ -69,7 +69,7 @@ struct usb_onetouch {
vendorName, productName, useProtocol, useTransport, \
initFunction, flags) \
 { USB_DEVICE_VER(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax), \
-  .driver_info = (flags) }
+.driver_info = (flags) }
 
 static struct usb_device_id onetouch_usb_ids[] = {
 #  include "unusual_onetouch.h"
@@ -125,7 +125,7 @@ static void usb_onetouch_irq(struct urb *urb)
input_sync(dev);
 
 resubmit:
-   retval = usb_submit_urb (urb, GFP_ATOMIC);
+   retval = usb_submit_urb(urb, GFP_ATOMIC);
if (retval)
dev_err(&dev->dev, "can't resubmit intr, %s-%s/input0, "
"retval %d\n", onetouch->udev->bus->bus_name,
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff, especially with splits

[Douglas Anderson]

This is a bit of catchall series for all the bug fix and performance
patches I've been working on over the last few months.  Note that for
dwc2 we need to do LOTS in software and need super low interrupt
latency, so most performance improvements actually fix real bugs.

Patches are structured to start with no-brainer stuff that could be
applied ASAP, especially things I've already gotten Acks for.  Things
get slightly more RFC / RFT like as we get farther down the series.
Anything that can be landed sooner rather than later (especially those
Acked long ago) would help in re-posts (I'm not biased, of course).

It's been a few months since my last post of this series.  In the
meantime I've added a bunch of small bugfixes to the start of it and
also TOTALLY REWROTE the microframe scheduler.  I'll say up front: I
know nothing about USB.  I haven't read the whole spec.  I'm not
terribly familiar with the OHCI, EHCI, and XHCI drivers in the kernel.
...and I'm pretty clueless overall.  Nevertheless, I've attempted to
write up a fancy scheduler based on the portion of the spec talking
about microframe scheduling requirements.  This rewritten scheduler does
seem to help when I start jamming lots of USB things into a hub, so
presumably the code is a reasonably starting point.  Given my current
understanding of USB the old code was fairly insane, so presumably even
if my new patch isn't perfect it's better than what we had.

Anyway, on to the patches:

1. usb: dwc2: rockchip: Make the max_transfer_size automatic

   No brainer.  Can land any time.

2. usb: dwc2: host: Get aligned DMA in a more supported way

   Although this touches a lot of code, it's mostly just deleting
   stuff.  The way this is working is nearly the same as tegra.  Biggest
   objection I expect is that it has too much duplication with tegra and
   musb.  I'd personally prefer to land it now and remove duplication
   later, but up to others.  Speeding up interrupt handler helps with
   SOF scheduling, so this is not just a dumb optimization.

3. usb: dwc2: host: Set host_rx_fifo_size to 528 for rk3066
4. usb: dwc2: host: Set host_perio_tx_fifo_size to 304 for rk3066

   Seems like a good idea and small impact, but if someone hates it or
   it breaks on some Rockchip SoC, just drop it.  I've only tested on
   rk3288 so it would be nice if someone with access to more Rockchip
   SoCs can give a tested by.

5. usb: dwc2: host: Avoid use of chan->qh after qh freed

   Simple bugfix.  Unrelated to the series but thrown in here.

6. usb: dwc2: host: Always add to the tail of queues

   Big functionality improvement.  Small patch.  Suggest applying ASAP.

7. usb: dwc2: hcd: fix split transfer schedule sequence

   Unless I'm misunderstanding, this should be a no-brainer to fix.
   Could be some bikeshedding on how to fix this.  Let me know if/how
   you want me to spin.  Otherwise I'd say land it and it will fix a
   bunch of stuff.

8. usb: dwc2: host: Add scheduler tracing

   Shouldn't hurt anything.  If you have bikesheds, let me know.

9.  usb: dwc2: host: Add a delay before releasing periodic bandwidth
10. usb: dwc2: host: Giveback URB in tasklet context

   I think we should take these.  They improve things a bunch and I have
   found no regressions due to them.  Additional testing appreciated, of
   course.

11. usb: dwc2: host: Use periodic interrupt even with DMA

   Just came up with this one recently so it's had slightly less
   testing.  ...but it certainly fixed a bunch of stuff.  Could probably
   be moved around in the series to be pretty much anywhere.  I don't
   think this has a huge impact until we fix the scheduler (below) but
   at the same time I'm pretty sure it's something that's been wrong for
   a long time.

12. usb: dwc2: host: Rename some fields in struct dwc2_qh
13. usb: dwc2: host: Reorder things in hcd_queue.c
14. usb: dwc2: host: Split code out to make dwc2_do_reserve()

   Cleanups to make future patches easier to understand.  Bikeshed away.
   All no-op changes.

15. usb: dwc2: host: Add scheduler logging for missed SOFs

   I found this to be quite helpful.  If you hate it, drop it from the
   series.

16. usb: dwc2: host: Manage frame nums better in scheduler

   Doesn't totally make sense on its own, but a good halfway point to
   the microframe scheduler.  ...and shouldn't regress anything.  Allows
   us to do the "Properly set even/odd frame" patch below which
   definitely improves things.

17. usb: dwc2: host: Schedule periodic right away if it's time

   Yet another small change to make scheduling tighter.

18. usb: dwc2: host: Add dwc2_hcd_get_future_frame_number() call

   Prep for ("usb: dwc2: host: Properly set even/odd frame")

19. usb: dwc2: host: Properly set even/odd frame

   Helps quite a bit.  Helps even more after the redone microframe
   scheduler.  Feel free to tidy up if you see easy ways to do this.
   Maybe someone has a better way to estimate time on the wire?

20. usb: dwc2: host: Totally re

[PATCH v5 17/21] usb: dwc2: host: Schedule periodic right away if it's time

[Douglas Anderson]

In dwc2_hcd_qh_deactivate() we will put some things on the
periodic_sched_ready list.  These things won't be taken off the ready
list until the next SOF, which might be a little late.  Let's put them
on right away.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Schedule periodic right away if it's time new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd_queue.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index f235402dfc5e..2b6e57ad2661 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -1078,12 +1078,26 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg, 
struct dwc2_qh *qh,
 * Note: we purposely use the frame_number from the "hsotg" structure
 * since we know SOF interrupt will handle future frames.
 */
-   if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number))
+   if (dwc2_frame_num_le(qh->next_active_frame, hsotg->frame_number)) {
+   enum dwc2_transaction_type tr_type;
+
+   /*
+* We're bypassing the SOF handler which is normally what puts
+* us on the ready list because we're in a hurry and need to
+* try to catch up.
+*/
+   dwc2_sch_vdbg(hsotg, "QH=%p IMM ready fn=%04x, nxt=%04x\n",
+ qh, frame_number, qh->next_active_frame);
list_move_tail(&qh->qh_list_entry,
   &hsotg->periodic_sched_ready);
-   else
+
+   tr_type = dwc2_hcd_select_transactions(hsotg);
+   if (tr_type != DWC2_TRANSACTION_NONE)
+   dwc2_hcd_queue_transactions(hsotg, tr_type);
+   } else {
list_move_tail(&qh->qh_list_entry,
   &hsotg->periodic_sched_inactive);
+   }
 }
 
 /**
-- 
2.7.0.rc3.207.g0ac5344

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 20/21] usb: dwc2: host: Totally redo the microframe scheduler

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson 
---
Changes in v5:
- Moved defines outside of ifdef to avoid gadget-only compile error.

Changes in v4:
- Figured out what the microframe scheduler was supposed to do.
- Microframe rewrite is totally different from v3, hopefully more right.
- Microframe rewrite is later in the series now.

Changes in v3:
- The uframe scheduler patch is folded into optimization series.
- Optimize uframe scheduler "single uframe" case a little.
- uframe scheduler now atop logging patches.
- uframe scheduler now before delayed bandwidth release patches.
- Add defines like EARLY_FRAME_USEC
- Reorder dwc2_deschedule_periodic() in prep for future patches.
- uframe scheduler now shows real usefulness w/ future patches!
- Assuming single_tt is new for v3; not terribly well tested (yet).
- Keep track and use our uframe new for v3.

Changes in v2:
- Totally rewrote uframe scheduler again after writing test code.
- uframe scheduler atop delayed bandwidth release patches.

 drivers/usb/dwc2/core.h  |   85 ++-
 drivers/usb/dwc2/hcd.c   |   87 ++-
 drivers/usb/dwc2/hcd.h   |   79 ++-
 drivers/usb/dwc2/hcd_queue.c | 1208 +++---
 4 files changed, 1271 insertions(+), 188 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 52cbea28d0e9..115925909390 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -592,6 +592,84 @@ struct dwc2_hregs_backup {
bool valid;
 };
 
+/*
+ * Constants related to high speed periodic scheduling
+ *
+ * We have a periodic schedule that is DWC2_HS_SCHEDULE_UFRAMES long.  From a
+ * reservation point of view it's assumed that the schedule goes right back to
+ * the beginning after the end of the schedule.
+ *
+ * What does that mean for scheduling things with a long interval?  It means
+ * we'll reserve time for them in every possible microframe that they could
+ * ever be scheduled in.  ...but we'll still only actually schedule them as
+ * often as they were requested.
+ *
+ * We keep our schedule in a "bitmap" structure.  This simplifies having
+ * to keep track of and merge intervals: we just let the bitmap code do most
+ * of the heavy lifting.  In a way scheduling is much like memory allocation.
+ *
+ * We schedule 100us per uframe or 80% of 125us (the maximum amount you're
+ * supposed to schedule for periodic transfers).  That's according to spec.
+ *
+ * Note that though we only schedule 80% of each microframe, the bitmap that we
+ * keep the schedule in is tightly packed (AKA it doesn't have 100us worth of
+ * space for each uFrame).
+ *
+ * Requirements:
+ * - DWC2_HS_SCHEDULE_UFRAMES must even divide 0x4000 (HFNUM_MAX_FRNUM + 1)
+ * - DWC2_HS_SCHEDULE_UFRAMES must be 8 times DWC2_LS_SCHEDULE_FRAMES (probably
+ *   could be any multiple of 8 times DWC2_LS_SCHEDULE_FRAMES, but there might
+ *   be bugs).  The 8 comes from the USB spec: number of microframes per frame.
+ */
+#define DWC2_US_PER_UFRAME 125
+#define DWC2_HS_PERIODIC_US_PER_UFRAME 100
+
+#define DWC2_HS_SCHEDULE_UFRAMES   8
+#define DWC2_HS_SCHEDULE_US(DWC2_HS_SCHEDULE_UFRAMES * \
+DWC2_HS_PERIODIC_US_PER_UFRAME)
+
+/*
+ * Constants related to low speed scheduling
+ *
+ * For high speed we schedule every 1us.  For low speed that's a bit overkill,
+ * so we make up a unit called a "slice" that's worth 25us.  There are 40
+ * slices in a full frame and we can schedule 36 of those (90%) for periodic
+ * transfers.
+ *
+ * Our low speed schedule can be as short as 1 frame or could be longer.  When
+ * we only schedule 1 frame it means that we'll need to reserve a time every
+ * frame even for things that only transfer very rarely, so something that runs
+ * every 2048 frames will get time reserved in every frame.  Our low speed
+ * schedule can be longer and we'll be able to handle more overlap, but that
+ * will come at increased memory cost and increased time to schedule.
+ *
+ * Note: one other advantage of a short low speed schedule is that if we mess
+ * up and miss scheduling we can jump in and use any of the slots that we
+ * happened to reserve.
+ *
+ * With 25 us per slice and 1 frame in the schedule, we only need 4 bytes for
+ * the schedule.  There will be one schedule per TT.
+ *
+ * Requirements:
+ * - DWC2_US_PER_SLICE

[PATCH v5 16/21] usb: dwc2: host: Manage frame nums better in scheduler

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Manage frame nums better in scheduler new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd.h   |  10 +-
 drivers/usb/dwc2/hcd_queue.c | 355 ---
 2 files changed, 273 insertions(+), 92 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 10c35585a2bd..fd266ac53a28 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -244,8 +244,11 @@ enum dwc2_transaction_type {
  *  the bus.  We'll move the qh to active here.  If the
  *  host is in high speed mode this will be a uframe.  If
  *  the host is in low speed mode this will be a full 
frame.
+ * @start_active_frame: If we are partway through a split transfer, this will 
be
+ * what next_active_frame was when we started.  Otherwise
+ * it should always be the same as next_active_frame.
+ * @assigned_uframe:The uframe (0 -7) assigned by dwc2_find_uframe().
  * @frame_usecs:Internal variable used by the microframe scheduler
- * @start_split_frame:  (Micro)frame at which last start split was initialized
  * @ntd:Actual number of transfer descriptors in a list
  * @qtd_list:   

[PATCH v5 09/21] usb: dwc2: host: Add a delay before releasing periodic bandwidth

[Douglas Anderson]

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 9118f9eb4f1e ("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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Moved periodic bandwidth release delay patch earlier again.

Changes in v3:
- Moved periodic bandwidth release delay patch later in the series.

Changes in v2:
- Periodic bandwidth release delay new for V2

 drivers/usb/dwc2/hcd.h   |   6 ++
 drivers/usb/dwc2/hcd_queue.c | 237 +--
 2 files changed, 187 insertions(+), 56 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 809bc4ff9116..79473ea35bd6 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -215,6 +215,7 @@ enum dwc2_transaction_type {
 /**
  * struct dwc2_qh - Software queue head structure
  *
+ * @hsotg:  The HCD state structure for the DWC OTG controller
  * @ep_type:Endpoint type. One of the following values:
  *   - USB_ENDPOINT_XFER_CONTROL
  *   - USB_ENDPOINT_XFER_BULK
@@ -252,13 +253,16 @@ enum dwc2_transaction_type {
  * @n_bytes:Xfer Bytes array. Each element corresponds to a 
transfer
  *  descriptor and indicates original XferSize value for 
the
  *  descriptor
+ * @unreserve_timer:Timer for releasing periodic reservation.
  * @tt_buffer_dirty True if clear_tt_buffer_complete is pending
+ * @unreserve_pending:  True if we planned to unreserve but haven't yet.
  *
  * A Queue Head (QH) holds the static characteristics of an endpoint and
  * maintains a list of transfers (QTDs) for that endpoint. A QH structure may
  * be entered in either the non-periodic or periodic schedule.
  */
 struct dwc2_qh {
+   struct dwc2_hsotg *hsotg;
u8 ep_type;
u8 ep_is_in;
u16 maxp;
@@ -281,7 +285,9 @@ struct dwc2_qh {
dma_addr_t desc_list_dma;
u32 desc_list_sz;
u32 *n_bytes;
+   struct timer_list unreserve_timer;
unsigned tt_buffer_dirty:1;
+   unsigned unreserve_pending:1;
 };
 
 /**
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 0e9faa75593c..b9e4867e1afd 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -53,6 +53,94 @@
 #include "core.h"
 #include "hcd.h"
 
+/* Wait this long before releasing periodic reservation */
+#define DWC2_UNRESERVE_DELAY (msecs_to_jiffies(5))
+
+/**
+ * dwc2_do_unreserve() - Actually release the periodic reservation
+ *
+ * This function actually releases the periodic bandwidth that was reserved
+ * by the given qh.
+ *
+ * @hsotg: The HCD state structure for the DWC OTG controller
+ * @qh:QH for the periodic transfer.
+ */
+static void dwc2_do_unreserve(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
+{
+   assert_spin_locked(&hsotg->lock);
+
+   WARN_ON(!qh->unreserve_pending);
+
+   /* No more unreserve pending--we're doing it */
+   qh->unreserve_pending = false;
+
+   if (WARN_ON(!list_empty(&qh->qh_list_entry)))
+   list_del_init(&qh->qh_list_entry);
+
+   /* Update claimed usecs per (micro)frame */
+   hsotg->periodic_usecs -= qh->usecs;
+
+   if (hsotg->core_params->uframe_sched > 0) {
+   int i;
+
+   for (i = 0; i < 8; i++) {
+   hsotg->frame_usecs[i] += qh->frame_usecs[i];
+   qh->frame_usecs[i] = 0;
+   }
+   } else {
+   /* Release periodic channel reservation */
+   hsotg->periodic_channels--;
+   }
+}
+
+/**
+ * dwc2_unreserve_timer_fn() - Timer function to release periodic reservation
+ *
+ * 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.  Since we're using HCD_BH to give
+ * back the URB we need to give the driver a little bit of time before we
+ * release the reservation.  This worker is called after the appropriate
+ * delay.
+ *
+ * @work: Pointer to a qh unreserve_work.
+ */
+static void dwc2_unreserve

[PATCH v5 18/21] usb: dwc2: host: Add dwc2_hcd_get_future_frame_number() call

[Douglas Anderson]

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

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Add dwc2_hcd_get_future_frame_number() call new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/core.h |  4 
 drivers/usb/dwc2/hcd.c  | 29 +
 2 files changed, 33 insertions(+)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 64d45a2053bb..52cbea28d0e9 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1235,12 +1235,16 @@ static inline int dwc2_hsotg_set_test_mode(struct 
dwc2_hsotg *hsotg,
 
 #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
 extern int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg);
+extern int dwc2_hcd_get_future_frame_number(struct dwc2_hsotg *hsotg, int us);
 extern void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
 extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force);
 extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
 #else
 static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
 { return 0; }
+static inline int dwc2_hcd_get_future_frame_number(struct dwc2_hsotg *hsotg,
+  int us)
+{ return 0; }
 static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
 static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
 static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 03072d1fd44d..83dc92da10a0 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1947,6 +1947,35 @@ int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
return (hfnum & HFNUM_FRNUM_MASK) >> HFNUM_FRNUM_SHIFT;
 }
 
+int dwc2_hcd_get_future_frame_number(struct dwc2_hsotg *hsotg, int us)
+{
+   u32 hprt = dwc2_readl(hsotg->regs + HPRT0);
+   u32 hfir = dwc2_readl(hsotg->regs + HFIR);
+   u32 hfnum = dwc2_readl(hsotg->regs + HFNUM);
+   unsigned int us_per_frame;
+   unsigned int frame_number;
+   unsigned int remaining;
+   unsigned int interval;
+   unsigned int phy_clks;
+
+   /* High speed has 125 us per (micro) frame; others are 1 ms per */
+   us_per_frame = (hprt & HPRT0_SPD_MASK) ? 1000 : 125;
+
+   /* Extract fields */
+   frame_number = (hfnum & HFNUM_FRNUM_MASK) >> HFNUM_FRNUM_SHIFT;
+   remaining = (hfnum & HFNUM_FRREM_MASK) >> HFNUM_FRREM_SHIFT;
+   interval = (hfir & HFIR_FRINT_MASK) >> HFIR_FRINT_SHIFT;
+
+   /*
+* Number of phy clocks since the last tick of the frame number after
+* "us" has passed.
+*/
+   phy_clks = (interval - remaining) +
+  DIV_ROUND_UP(interval * us, us_per_frame);
+
+   return dwc2_frame_num_inc(frame_number, phy_clks / interval);
+}
+
 int dwc2_hcd_is_b_host(struct dwc2_hsotg *hsotg)
 {
return hsotg->op_state == OTG_STATE_B_HOST;
-- 
2.7.0.rc3.207.g0ac5344

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 21/21] usb: dwc2: host: If using uframe scheduler, end splits better

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- If using uframe scheduler, end splits better new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd_intr.c | 48 +++--
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 1875e4590a3c..2519e1bdad1a 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1353,14 +1353,50 @@ static void dwc2_hc_nyet_intr(struct dwc2_hsotg *hsotg,
 
if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
chan->ep_type == USB_ENDPOINT_XFER_ISOC) {
-   int frnum = dwc2_hcd_get_frame_number(hsotg);
+   struct dwc2_qh *qh = chan->qh;
+   bool past_end;
+
+   if (hsotg->core_params->uframe_sched <= 0) {
+   int frnum = dwc2_hcd_get_frame_number(hsotg);
+
+   /* Don't have num_hs_transfers; simple logic */
+   past_end = dwc2_full_frame_num(frnum) !=
+dwc2_full_frame_num(qh->next_active_frame);
+   } else {
+   int end_frnum;
 
-   if (dwc2_full_frame_num(frnum) !=
-   dwc2_full_frame_num(chan->qh->next_active_frame)) {
/*
-* No longer in the same full speed frame.
-* Treat this as a transaction error.
-*/
+   * Figure out the end frame based on schedule.
+   *
+   * We don't want to go on trying again and again
+   * forever.  Let's stop when we've done all the
+   * transfers that were scheduled.
+   *
+   * We're going to be comparing start_active_frame
+   * and next_active_frame, both of which are 1
+   * before the time the packet goes on the wire,
+   * so that cancels out.  Basically if had 1
+   * transfer and we saw 1 NYET then we're done.
+   * We're getting a NYET here so if next >=
+   * (start + num_transfers) we're done. The
+   * complexity is that for all but ISOC_OUT we
+   * skip one slot.
+   */
+   end_frnum = dwc2_frame_num_inc(
+   qh->start_active_frame,
+   qh->num_hs_transfers);
+
+   if (qh->ep_type != USB_ENDPOINT_XFER_ISOC ||
+   qh->ep_is_in)
+   end_frnum =
+  dwc2_frame_num_inc(end_frnum, 1);
+
+   past_end = dwc2_frame_num_le(
+   end_frnum, qh->next_active_frame);
+   }
+
+   if (past_end) {
+   /* Treat this as a transaction error. */
 #if 0
/*
 * Todo: Fix system performance so this can
-- 
2.7.0.rc3.207.g0ac5344

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 19/21] usb: dwc2: host: Properly set even/odd frame

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Properly set even/odd frame new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/core.c  | 92 +++-
 drivers/usb/dwc2/hcd_queue.c | 11 +-
 2 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index ed73b26818c0..488862e08d59 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -1703,9 +1703,97 @@ static void dwc2_hc_set_even_odd_frame(struct dwc2_hsotg 
*hsotg,
 {
if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
chan->ep_type == USB_ENDPOINT_XFER_ISOC) {
-   /* 1 if _next_ frame is odd, 0 if it's even */
-   if (!(dwc2_hcd_get_frame_number(hsotg) & 0x1))
+   int host_speed;
+   int xfer_ns;
+   int xfer_us;
+   int bytes_in_fifo;
+   u16 fifo_space;
+   u16 frame_number;
+   u16 wire_frame;
+
+   /*
+* Try to figure out if we're an even or odd frame. If we set
+* even and the current frame number is even the the transfer
+* will happen immediately.  Similar if both are odd. If one is
+* even and the other is odd then the transfer will happen when
+* the frame number ticks.
+*
+* There's a bit of a balancing act to get this right.
+* Sometimes we may want to send data in the current frame (AK
+* right away).  We might want to do this if the frame number
+* _just_ ticked, but we might also want to do this in order
+* to continue a split transaction that happened late in a
+* microframe (so we didn't know to queue the next transfer
+* until the frame number had ticked).  The problem is that we
+* need a lot of knowledge to know if there's actually still
+* time to send things or if it would be better to wait until
+* the next frame.
+*
+* We can look at how much time is left in the current frame
+* and make a guess about whether we'll have time to transfer.
+* We'll do that.
+*/
+
+   /* Get speed host is running at */
+   host_speed = (chan->speed != USB_SPEED_HIGH &&
+ !chan->do_split) ? chan->speed : USB_SPEED_HIGH;
+
+   /* See how many bytes are in the periodic FIFO right now */
+   fifo_space = (dwc2_readl(hsotg->regs + HPTXSTS) &
+ TXSTS_FSPCAVAIL_MASK) >> TXSTS_FSPCAVAIL_SHIFT;
+   bytes_in_fifo = sizeof(u32) *
+   (hsotg->core_params->host_perio_tx_fifo_size -
+fifo_space);
+
+   /*
+* Roughly estimate bus time for everything in the periodic
+* queue + our new transfer.  This is "rough" because we're
+* usi

[PATCH v5 15/21] usb: dwc2: host: Add scheduler logging for missed SOFs

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Add scheduler logging for missed SOFs new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/core.h |  3 ++-
 drivers/usb/dwc2/hcd.c  |  2 +-
 drivers/usb/dwc2/hcd_intr.c | 12 
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 18f9e4045643..64d45a2053bb 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -809,9 +809,10 @@ struct dwc2_hsotg {
bool bus_suspended;
bool new_connection;
 
+   u16 last_frame_num;
+
 #ifdef CONFIG_USB_DWC2_TRACK_MISSED_SOFS
 #define FRAME_NUM_ARRAY_SIZE 1000
-   u16 last_frame_num;
u16 *frame_num_array;
u16 *last_frame_num_array;
int frame_num_idx;
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 584c64114a82..03072d1fd44d 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -3082,8 +3082,8 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
FRAME_NUM_ARRAY_SIZE, GFP_KERNEL);
if (!hsotg->last_frame_num_array)
goto error1;
-   hsotg->last_frame_num = HFNUM_MAX_FRNUM;
 #endif
+   hsotg->last_frame_num = HFNUM_MAX_FRNUM;
 
/* Check if the bus driver or platform code has setup a dma_mask */
if (hsotg->core_params->dma_enable > 0 &&
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index b54a0c41d34f..1875e4590a3c 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -55,12 +55,16 @@
 /* This function is for debug only */
 static void dwc2_track_missed_sofs(struct dwc2_hsotg *hsotg)
 {
-#ifdef CONFIG_USB_DWC2_TRACK_MISSED_SOFS
u16 curr_frame_number = hsotg->frame_number;
+   u16 expected = dwc2_frame_num_inc(hsotg->last_frame_num, 1);
+
+   if (expected != curr_frame_number)
+   dwc2_sch_vdbg(hsotg, "MISSED SOF %04x != %04x\n",
+   expected, curr_frame_number);
 
+#ifdef CONFIG_USB_DWC2_TRACK_MISSED_SOFS
if (hsotg->frame_num_idx < FRAME_NUM_ARRAY_SIZE) {
-   if (((hsotg->last_frame_num + 1) & HFNUM_MAX_FRNUM) !=
-   curr_frame_number) {
+   if (expected != curr_frame_number) {
hsotg->frame_num_array[hsotg->frame_num_idx] =
curr_frame_number;
hsotg->last_frame_num_array[hsotg->frame_num_idx] =
@@ -79,8 +83,8 @@ static void dwc2_track_missed_sofs(struct dwc2_hsotg *hsotg)
}
hsotg->dumped_frame_num_array = 1;
}
-   hsotg->last_frame_num = curr_frame_number;
 #endif
+   hsotg->last_frame_num = curr_frame_number;
 }
 
 static void dwc2_hc_handle_tt_clear(struct dwc2_hsotg *hsotg,
-- 
2.7.0.rc3.207.g0ac5344

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 10/21] usb: dwc2: host: Giveback URB in tasklet context

[Douglas Anderson]

In commit 94dfd7edfd5c ("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
, 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.

Signed-off-by: Douglas Anderson 
Tested-by: Heiko Stuebner 
---
Changes in v5: None
Changes in v4:
- A bit earlier in the list of patches than in v3.

Changes in v3: None
Changes in v2:
- Commit message now says that URB giveback change needs delay change.

 drivers/usb/dwc2/hcd.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 87ad5bf2d166..9fd14b74b49c 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -2341,9 +2341,7 @@ void dwc2_host_complete(struct dwc2_hsotg *hsotg, struct 
dwc2_qtd *qtd,
kfree(qtd->urb);
qtd->urb = NULL;
 
-   spin_unlock(&hsotg->lock);
usb_hcd_giveback_urb(dwc2_hsotg_to_hcd(hsotg), urb, status);
-   spin_lock(&hsotg->lock);
 }
 
 /*
@@ -2962,7 +2960,7 @@ static struct hc_driver dwc2_hc_driver = {
.hcd_priv_size = sizeof(struct wrapper_priv_data),
 
.irq = _dwc2_hcd_irq,
-   .flags = HCD_MEMORY | HCD_USB2,
+   .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
 
.start = _dwc2_hcd_start,
.stop = _dwc2_hcd_stop,
-- 
2.7.0.rc3.207.g0ac5344

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 14/21] usb: dwc2: host: Split code out to make dwc2_do_reserve()

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Split code out to make dwc2_do_reserve() new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd_queue.c | 112 ++-
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 8a2067bc1e62..9ce407e5017d 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -245,6 +245,70 @@ static int dwc2_find_uframe(struct dwc2_hsotg *hsotg, 
struct dwc2_qh *qh)
 }
 
 /**
+ * dwc2_do_reserve() - Make a periodic reservation
+ *
+ * Try to allocate space in the periodic schedule.  Depending on parameters
+ * this might use the microframe scheduler or the dumb scheduler.
+ *
+ * @hsotg: The HCD state structure for the DWC OTG controller
+ * @qh:QH for the periodic transfer.
+ *
+ * Returns: 0 upon success; error upon failure.
+ */
+static int dwc2_do_reserve(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
+{
+   int status;
+
+   if (hsotg->core_params->uframe_sched > 0) {
+   int frame = -1;
+
+   status = dwc2_find_uframe(hsotg, qh);
+   if (status == 0)
+   frame = 7;
+   else if (status > 0)
+   frame = status - 1;
+
+   /* Set the new frame up */
+   if (frame >= 0) {
+   qh->next_active_frame &= ~0x7;
+   qh->next_active_frame |= (frame & 7);
+   dwc2_sch_dbg(hsotg,
+"QH=%p sched_p nxt=%04x, uf=%d\n",
+qh, qh->next_active_frame, frame);
+   }
+
+   if (status > 0)
+   status = 0;
+   } else {
+   status = dwc2_periodic_channel_available(hsotg);
+   if (status) {
+   dev_info(hsotg->dev,
+"%s: No host channel available for periodic 
transfer\n",
+__func__);
+   return status;
+   }
+
+   status = dwc2_check_periodic_bandwidth(hsotg, qh);
+   }
+
+   if (status) {
+   dev_dbg(hsotg->dev,
+   "%s: Insufficient periodic bandwidth for periodic 
transfer\n",
+   __func__);
+   return status;
+   }
+
+   if (hsotg->core_params->uframe_sched <= 0)
+   /* Reserve periodic channel */
+   hsotg->periodic_channels++;
+
+   /* Update claimed usecs per (micro)frame */
+   hsotg->periodic_usecs += qh->host_us;
+
+   return 0;
+}
+
+/**
  * dwc2_do_unreserve() - Actually release the periodic reservation
  *
  * This function actually releases the periodic bandwidth that was reserved
@@ -393,51 +457,9 @@ static int dwc2_schedule_periodic(struct dwc2_hsotg 
*hsotg, struct dwc2_qh *qh)
 * that case.
 */
if (!qh->unreserve_pending) {
-   if (hsotg->core_params->uframe_sched > 0) {
-   int frame = -1;
-
-   status = dwc2_find_uframe(hsotg, qh);
-   if (status == 0)
-   frame = 7;
-   else if (status > 0)
-   frame = status - 1;
-
-   /* Set the new frame up */
-   if (frame >= 0) {
-   qh->next_active_frame &= ~0x7;
-   qh->next_active_frame |= (frame & 7);
-   dwc2_sch_dbg(hsotg,
-"QH=%p sched_p nxt=%04x, uf=%d\n",
-qh, qh->next_active_frame, frame);
-   }
-
-   if (status > 0)
-   status = 0;
-   } else {
-   status = dwc2_periodic_channel_available(hsotg);
-   if (status) {
-   dev_info(hsotg->dev,
-   "%s: No host channel available for 
periodic transfer\n",
-   __func__);
-   return status;
-   }
-
-   status = dwc2_check_periodic_bandwidth(hsotg, qh);
-   }
-
-   if (status) {
-   dev_dbg(hsotg->dev,
-   "%s: Insufficient periodic bandwidth for 
periodic transfer\n",
-   __func__);
+   status = dwc2_do_reserve(hsotg, qh);
+   if (status)
return status;
-   }
-
-   if (hsotg

[PATCH v5 05/21] usb: dwc2: host: Avoid use of chan->qh after qh freed

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Avoid use of chan->qh after qh freed new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd.c  |  8 
 drivers/usb/dwc2/hcd_intr.c | 10 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index bc4bdbc1534e..7783c8ba0173 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -164,6 +164,9 @@ static void dwc2_qh_list_free(struct dwc2_hsotg *hsotg,
 qtd_list_entry)
dwc2_hcd_qtd_unlink_and_free(hsotg, qtd, qh);
 
+   if (qh->channel && qh->channel->qh == qh)
+   qh->channel->qh = NULL;
+
spin_unlock_irqrestore(&hsotg->lock, flags);
dwc2_hcd_qh_free(hsotg, qh);
spin_lock_irqsave(&hsotg->lock, flags);
@@ -554,7 +557,12 @@ static int dwc2_hcd_endpoint_disable(struct dwc2_hsotg 
*hsotg,
dwc2_hcd_qtd_unlink_and_free(hsotg, qtd, qh);
 
ep->hcpriv = NULL;
+
+   if (qh->channel && qh->channel->qh == qh)
+   qh->channel->qh = NULL;
+
spin_unlock_irqrestore(&hsotg->lock, flags);
+
dwc2_hcd_qh_free(hsotg, qh);
 
return 0;
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 352c98364317..99efc2bd1617 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1935,6 +1935,16 @@ static void dwc2_hc_n_intr(struct dwc2_hsotg *hsotg, int 
chnum)
}
 
dwc2_writel(hcint, hsotg->regs + HCINT(chnum));
+
+   /*
+* If we got an interrupt after someone called
+* dwc2_hcd_endpoint_disable() we don't want to crash below
+*/
+   if (!chan->qh) {
+   dev_warn(hsotg->dev, "Interrupt on disabled channel\n");
+   return;
+   }
+
chan->hcint = hcint;
hcint &= hcintmsk;
 
-- 
2.7.0.rc3.207.g0ac5344

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 11/21] usb: dwc2: host: Use periodic interrupt even with DMA

[Douglas Anderson]

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

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Use periodic interrupt even with DMA new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd.c | 71 +++---
 1 file changed, 32 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 9fd14b74b49c..584c64114a82 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1109,10 +1109,14 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
u32 fspcavail;
u32 gintmsk;
int status;
-   int no_queue_space = 0;
-   int no_fifo_space = 0;
+   bool no_queue_space = false;
+   bool no_fifo_space = false;
u32 qspcavail;
 
+   /* If empty list then just adjust interrupt enables */
+   if (list_empty(&hsotg->periodic_sched_assigned))
+   goto exit;
+
if (dbg_perio())
dev_vdbg(hsotg->dev, "Queue periodic transactions\n");
 
@@ -1190,42 +1194,32 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
}
}
 
-   if (hsotg->core_params->dma_enable <= 0) {
-   tx_status = dwc2_readl(hsotg->regs + HPTXSTS);
-   qspcavail = (tx_status & TXSTS_QSPCAVAIL_MASK) >>
-   TXSTS_QSPCAVAIL_SHIFT;
-   fspcavail = (tx_status & TXSTS_FSPCAVAIL_MASK) >>
-   TXSTS_FSPCAVAIL_SHIFT;
-   if (dbg_perio()) {
-   dev_vdbg(hsotg->dev,
-"  P Tx Req Queue Space Avail (after queue): 
%d\n",
-qspcavail);
-   dev_vdbg(hsotg->dev,
-"  P Tx FIFO Space Avail (after queue): %d\n",
-fspcavail);
-   }
-
-   if (!list_empty(&hsotg->periodic_sched_assigned) ||
-   no_queue_space || no_fifo_space) {
-   /*
-* May need to queue more transactions as the request
-* queue or Tx FIFO empties. Enable the periodic Tx
-* FIFO empty interrupt. (Always use the half-empty
-* level to ensure that new requests are loaded as
-* soon as possible.)
-*/
-   gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+exit:
+   if (no_queue_space || no_fifo_space ||
+   (hsotg->core_params->dma_enable <= 0 &&
+!list_empty(&hsotg->periodic_sched_assigned))) {
+   /*
+* May need to queue more transactions as the request
+* queue or Tx FIFO empties. Enable the periodic Tx
+* FIFO empty interrupt. (Always use the half-empty
+* level to ensure that new requests are loaded as
+* soon as possible.)
+*/
+   gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+   if (!(gintmsk & GINTSTS_PTXFEMP)) {
gintmsk |= GINTSTS_PTXFEMP;
dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
-   } el

[PATCH v5 08/21] usb: dwc2: host: Add scheduler tracing

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Retooled scheduler tracing a bit, so left off John's Ack from v3.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/core.h  | 20 
 drivers/usb/dwc2/hcd.h   |  5 +
 drivers/usb/dwc2/hcd_intr.c  |  6 +-
 drivers/usb/dwc2/hcd_queue.c | 24 +++-
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 538cf38af0e4..18f9e4045643 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -44,6 +44,26 @@
 #include 
 #include "hw.h"
 
+/*
+ * Suggested defines for tracers:
+ * - no_printk:Disable tracing
+ * - pr_info:  Print this info to the console
+ * - trace_printk: Print this info to trace buffer (good for verbose logging)
+ */
+
+#define DWC2_TRACE_SCHEDULER   no_printk
+#define DWC2_TRACE_SCHEDULER_VBno_printk
+
+/* Detailed scheduler tracing, but won't overwhelm console */
+#define dwc2_sch_dbg(hsotg, fmt, ...)  \
+   DWC2_TRACE_SCHEDULER(pr_fmt("%s: SCH: " fmt),   \
+dev_name(hsotg->dev), ##__VA_ARGS__)
+
+/* Verbose scheduler tracing */
+#define dwc2_sch_vdbg(hsotg, fmt, ...) \
+   DWC2_TRACE_SCHEDULER_VB(pr_fmt("%s: SCH: " fmt),\
+   dev_name(hsotg->dev), ##__VA_ARGS__)
+
 static inline u32 dwc2_readl(const void __iomem *addr)
 {
u32 value = __raw_readl(addr);
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 1b46e2e617cc..809bc4ff9116 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -563,6 +563,11 @@ static inline u16 dwc2_frame_num_inc(u16 frame, u16 inc)
return (frame + inc) & HFNUM_MAX_FRNUM;
 }
 
+static inline u16 dwc2_frame_num_dec(u16 frame, u16 dec)
+{
+   return (frame + HFNUM_MAX_FRNUM + 1 - dec) & HFNUM_MAX_FRNUM;
+}
+
 static inline u16 dwc2_full_frame_num(u16 frame)
 {
return (frame & HFNUM_MAX_FRNUM) >> 3;
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 577c91096a51..5d25a5ec9736 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -138,13 +138,17 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
while (qh_entry != &hsotg->periodic_sched_inactive) {
qh = list_entry(qh_entry, struct dwc2_qh, qh_list_entry);
qh_entry = qh_entry->next;
-   if (dwc2_frame_num_le(qh->sched_frame, hsotg->frame_number))
+   if (dwc2_frame_num_le(qh->sched_frame, hsotg->frame_number)) {
+   dwc2_sch_vdbg(hsotg, "QH=%p ready fn=%04x, sch=%04x\n",
+ qh, hsotg->frame_number, qh->sched_frame);
+
/*
 * Move QH to the ready list to be executed next
 * (micro)frame
 */
list_move_tail(&qh->qh_list_entry,
  &hsotg->periodic_sched_ready);
+   }
}
tr_type = dwc2_hcd_select_transactions(hsotg);
if (tr_type != DWC2_TRANSACTION_NONE)
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index bc632a72f611..0e9faa75593c 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -113,6 +113,9 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct 
dwc2_qh *qh,
qh->sched_frame = dwc2_frame_num_inc(hsotg->frame_number,
 SCHEDULE_SLOP);
qh->interval = urb->interval;
+   dwc2_sch_dbg(hsotg, "QH=%p init sch=%04x, fn=%04x, int=%#x\n",
+qh, qh->sched_frame, hsotg->frame_number,
+qh->interval);
 #if 0
/* Increase interrupt polling rate for debugging */
if (qh->ep_type == USB_ENDPOINT_XFER_INT)
@@ -126,6 +129,11 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct 
dwc2_qh *qh,
qh->interval *= 8;
qh->sched_frame |= 0x7;
qh->start_split_frame = qh->sched_frame;
+   dwc2_sch_dbg(hsotg,
+"QH=%p init*8 sch=%04x, fn=%04x, 
int=%#x\n",
+qh, qh->sched_frame, hsotg->frame_number,
+qh->interval);
+
}
dev_dbg(hsotg->dev, "interval=%d\n", qh->interval);
}
@@ -482,6 +490,8 @@ static int dwc2_schedule_periodic(struct dwc2_

[PATCH v5 13/21] usb: dwc2: host: Reorder things in hcd_queue.c

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Reorder things in hcd_queue.c new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd_queue.c | 600 +--
 1 file changed, 300 insertions(+), 300 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 39f4de6279f8..8a2067bc1e62 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -57,295 +57,6 @@
 #define DWC2_UNRESERVE_DELAY (msecs_to_jiffies(5))
 
 /**
- * dwc2_do_unreserve() - Actually release the periodic reservation
- *
- * This function actually releases the periodic bandwidth that was reserved
- * by the given qh.
- *
- * @hsotg: The HCD state structure for the DWC OTG controller
- * @qh:QH for the periodic transfer.
- */
-static void dwc2_do_unreserve(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
-{
-   assert_spin_locked(&hsotg->lock);
-
-   WARN_ON(!qh->unreserve_pending);
-
-   /* No more unreserve pending--we're doing it */
-   qh->unreserve_pending = false;
-
-   if (WARN_ON(!list_empty(&qh->qh_list_entry)))
-   list_del_init(&qh->qh_list_entry);
-
-   /* Update claimed usecs per (micro)frame */
-   hsotg->periodic_usecs -= qh->host_us;
-
-   if (hsotg->core_params->uframe_sched > 0) {
-   int i;
-
-   for (i = 0; i < 8; i++) {
-   hsotg->frame_usecs[i] += qh->frame_usecs[i];
-   qh->frame_usecs[i] = 0;
-   }
-   } else {
-   /* Release periodic channel reservation */
-   hsotg->periodic_channels--;
-   }
-}
-
-/**
- * dwc2_unreserve_timer_fn() - Timer function to release periodic reservation
- *
- * 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.  Since we're using HCD_BH to give
- * back the URB we need to give the driver a little bit of time before we
- * release the reservation.  This worker is called after the appropriate
- * delay.
- *
- * @work: Pointer to a qh unreserve_work.
- */
-static void dwc2_unreserve_timer_fn(unsigned long data)
-{
-   struct dwc2_qh *qh = (struct dwc2_qh *)data;
-   struct dwc2_hsotg *hsotg = qh->hsotg;
-   unsigned long flags;
-
-   /*
-* Wait for the lock, or for us to be scheduled again.  We
-* could be scheduled again if:
-* - We started executing but didn't get the lock yet.
-* - A new reservation came in, but cancel didn't take effect
-*   because we already started executing.
-* - The timer has been kicked again.
-* In that case cancel and wait for the next call.
-*/
-   while (!spin_trylock_irqsave(&hsotg->lock, flags)) {
-   if (timer_pending(&qh->unreserve_timer))
-   return;
-   }
-
-   /*
-* Might be no more unreserve pending if:
-* - We started executing but didn't get the lock yet.
-* - A new reservation came in, but cancel didn't take effect
-*   because we already started executing.
-*
-* We can't put this in the loop above because unreserve_pending needs
-* to be accessed under lock, so we can only check it once we got the
-* lock.
-*/
-   if (qh->unreserve_pending)
-   dwc2_do_unreserve(hsotg, qh);
-
-   spin_unlock_irqrestore(&hsotg->lock, flags);
-}
-
-/**
- * dwc2_qh_init() - Initializes a QH structure
- *
- * @hsotg: The HCD state structure for the DWC OTG controller
- * @qh:The QH to init
- * @urb:   Holds the information about the device/endpoint needed to initialize
- * the QH
- */
-#define SCHEDULE_SLOP 10
-static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
-struct dwc2_hcd_urb *urb)
-{
-   int dev_speed, hub_addr, hub_port;
-   char *speed, *type;
-
-   de

[PATCH v5 12/21] usb: dwc2: host: Rename some fields in struct dwc2_qh

[Douglas Anderson]

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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Rename some fields in struct dwc2_qh new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd.h   |  20 
 drivers/usb/dwc2/hcd_ddma.c  |  37 ---
 drivers/usb/dwc2/hcd_intr.c  |  10 ++--
 drivers/usb/dwc2/hcd_queue.c | 107 ++-
 4 files changed, 92 insertions(+), 82 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 79473ea35bd6..10c35585a2bd 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -236,10 +236,14 @@ enum dwc2_transaction_type {
  * @do_split:   Full/low speed endpoint on high-speed hub requires 
split
  * @td_first:   Index of first activated isochronous transfer 
descriptor
  * @td_last:Index of last activated isochronous transfer descriptor
- * @usecs:  Bandwidth in microseconds per (micro)frame
- * @interval:   Interval between transfers in (micro)frames
- * @sched_frame:(Micro)frame to initialize a periodic transfer.
- *  The transfer executes in the following (micro)frame.
+ * @host_us:Bandwidth in microseconds per transfer as seen by host
+ * @host_interval:  Interval between transfers as seen by the host.  If
+ *  the host is high speed and the device is low speed this
+ *  will be 8 times device interval.
+ * @next_active_frame:  (Micro)frame before we next need to put something on
+ *  the bus.  We'll move the qh to active here.  If the
+ *  host is in high speed mode this will be a uframe.  If
+ *  the host is in low speed mode this will be a full 
frame.
  * @frame_usecs:Internal variable used by the microframe scheduler
  * @start_split_frame:  (Micro)frame at which last start split was initialized
  * @ntd:Actual number of transfer descriptors in a list
@@ -272,9 +276,9 @@ struct dwc2_qh {
u8 do_split;
u8 td_first;
u8 td_last;
-   u16 usecs;
-   u16 interval;
-   u16 sched_frame;
+   u16 host_us;
+   u16 host_interval;
+   u16 next_active_frame;
u16 frame_usecs[8];
u16 start_split_frame;
u16 ntd;
@@ -651,7 +655,7 @@ static inline u16 dwc2_hcd_get_ep_bandwidth(struct 
dwc2_hsotg *hsotg,
return 0;
}
 
-   return qh->usecs;
+   return qh->host_us;
 }
 
 extern void dwc2_hcd_save_data_toggle(struct dwc2_hsotg *hsotg,
diff --git a/drivers/usb/dwc2/hcd_ddma.c b/drivers/usb/dwc2/hcd_ddma.c
index 16b261cfa92d..26b00270ca0b 100644
--- a/drivers/usb/dwc2/hcd_ddma.c
+++ b/drivers/usb/dwc2/hcd_ddma.c
@@ -81,7 +81,7 @@ static u16 dwc2_max_desc_num(struct dwc2_qh *qh)
 static u16 dwc2_frame_incr_val(struct dwc2_qh *qh)
 {
return qh->dev_speed == USB_SPEED_HIGH ?
-  (qh->interval + 8 - 1) / 8 : qh->interval;
+  (qh->host_interval + 8 - 1) / 8 : qh->host_interval;
 }
 
 static int dwc2_desc_list_alloc(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
@@ -252,7 +252,7 @@ static void dwc2_update_frame_list(struct dwc2_hsotg 
*hsotg, struct dwc2_qh *qh,
chan = qh->channel;
inc = dwc2_frame_incr_val(qh);
if (qh->ep_type == USB_ENDPOINT_XFER_ISOC)
-   i = dwc2_frame_list_idx(qh->sched_frame);
+   i = dwc2_frame_list_idx(qh->next_active_frame);
else
i = 0;
 
@@ -278,13 +278,13 @@ static void dwc2_update_frame_list(struct dwc2_hsotg 
*hsotg, struct dwc2_qh *qh,
return;
 
chan->schinfo = 0;
-   if (chan->speed == USB_SPEED_HIGH && qh->interval) {
+   if (chan->speed == USB_SPEED_HIGH && qh->host_interval) {
j = 1;
/* TODO - check this */
-   inc = (8 + qh->interval - 1) / qh->interval;
+   inc = (8 + qh->host_interval - 1) / qh->host_interval;
for (i = 0; i < inc; i++) {
chan->schinfo |= j;
- 

[PATCH v5 06/21] usb: dwc2: host: Always add to the tail of queues

[Douglas Anderson]

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

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Always add to the tail of queues new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/hcd.c   | 11 ++-
 drivers/usb/dwc2/hcd_ddma.c  |  4 ++--
 drivers/usb/dwc2/hcd_intr.c  |  4 ++--
 drivers/usb/dwc2/hcd_queue.c |  6 --
 4 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 7783c8ba0173..d2daaea88d91 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -969,7 +969,8 @@ enum dwc2_transaction_type dwc2_hcd_select_transactions(
 * periodic assigned schedule
 */
qh_ptr = qh_ptr->next;
-   list_move(&qh->qh_list_entry, &hsotg->periodic_sched_assigned);
+   list_move_tail(&qh->qh_list_entry,
+  &hsotg->periodic_sched_assigned);
ret_val = DWC2_TRANSACTION_PERIODIC;
}
 
@@ -1002,8 +1003,8 @@ enum dwc2_transaction_type dwc2_hcd_select_transactions(
 * non-periodic active schedule
 */
qh_ptr = qh_ptr->next;
-   list_move(&qh->qh_list_entry,
- &hsotg->non_periodic_sched_active);
+   list_move_tail(&qh->qh_list_entry,
+  &hsotg->non_periodic_sched_active);
 
if (ret_val == DWC2_TRANSACTION_NONE)
ret_val = DWC2_TRANSACTION_NON_PERIODIC;
@@ -1176,8 +1177,8 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
 * Move the QH from the periodic assigned schedule to
 * the periodic queued schedule
 */
-   list_move(&qh->qh_list_entry,
- &hsotg->periodic_sched_queued);
+   list_move_tail(&qh->qh_list_entry,
+  &hsotg->periodic_sched_queued);
 
/* done queuing high bandwidth */
hsotg->queuing_high_bandwidth = 0;
diff --git a/drivers/usb/dwc2/hcd_ddma.c b/drivers/usb/dwc2/hcd_ddma.c
index 36606fc33c0d..16b261cfa92d 100644
--- a/drivers/usb/dwc2/hcd_ddma.c
+++ b/drivers/usb/dwc2/hcd_ddma.c
@@ -1327,8 +1327,8 @@ void dwc2_hcd_complete_xfer_ddma(struct dwc2_hsotg *hsotg,
dwc2_hcd_qh_unlink(hsotg, qh);
} else {
/* Keep in assigned schedule to continue transfer */
-   list_move(&qh->qh_list_entry,
- &hsotg->periodic_sched_assigned);
+   list_move_tail(&qh->qh_list_entry,
+  &hsotg->periodic_sched_assigned);
/*
 * If channel has been halted during giveback of urb
 * then prevent any new scheduling.
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 99efc2bd1617..2c521c00e5e0 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -143,7 +143,7 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
 * Move QH to the ready list to be executed next
 * (micro)frame
 */
-   list_move(&qh->qh_list_entry,
+   list_move_tail(&qh->qh_list_entry,
  &hsotg->periodic_sched_ready);
}
tr_type = dwc2_hcd_select_transactions(hsotg);
@@ -794,7 +794,7 @@ static void dwc2_halt_channel(struct dwc2_hsotg *hsotg,
 * halt to be queued when the periodic schedule is
 * processed.
 */
-   list_move(&chan->qh->qh_list_entry,
+   list_move_tail(&chan->qh->qh_list_entry,
  &hsotg->periodic_sched_assigned);
 
/*
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index e0933a9dfad7..bc632a72f611 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -732,9 +732,11 @@ void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg, 
struct dwc2_qh *qh,
 dwc2_frame_num_le(qh->sched_frame, frame_number)) ||
(hsotg->core_params->uframe_sched <= 0 &&
 

[PATCH v5 02/21] usb: dwc2: host: Get aligned DMA in a more supported way

[Douglas Anderson]

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 
Acked-by: John Youn 
Tested-by: Heiko Stuebner 
Tested-by: John Youn 
---
Changes in v5: None
Changes in v4:
- Add John's Acks from 

Changes in v3: None
Changes in v2:
- Add a warn if setup_dma is not aligned (Julius Werner).

 drivers/usb/dwc2/core.c  |  21 +-
 drivers/usb/dwc2/hcd.c   | 170 +--
 drivers/usb/dwc2/hcd.h   |  10 ---
 drivers/usb/dwc2/hcd_intr.c  |  65 -
 drivers/usb/dwc2/hcd_queue.c |   7 +-
 5 files changed, 87 insertions(+), 186 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 39a0fa8a4c0a..73f2771b7740 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -1958,19 +1958,11 @@ void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
}
 
if (hsotg->core_params->dma_enable > 0) {
-   dma_addr_t dma_addr;
-
-   if (chan->align_buf) {
-   if (dbg_hc(chan))
-   dev_vdbg(hsotg->dev, "align_buf\n");
-   dma_addr = chan->align_buf;
-   } else {
-   dma_addr = chan->xfer_dma;
-   }
-   dwc2_writel((u32)dma_addr, hsotg->regs + HCDMA(chan->hc_num));
+   dwc2_writel((u32)chan->xfer_dma,
+   hsotg->regs + HCDMA(chan->hc_num));
if (dbg_hc(chan))
dev_vdbg(hsotg->dev, "Wrote %08lx to HCDMA(%d)\n",
-(unsigned long)dma_addr, chan->hc_num);
+(unsigned long)chan->xfer_dma, chan->hc_num);
}
 
/* Start the split */
@@ -3363,13 +3355,6 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
width = (hwcfg3 & GHWCFG3_XFER_SIZE_CNTR_WIDTH_MASK) >>
GHWCFG3_XFER_SIZE_CNTR_WIDTH_SHIFT;
hw->max_transfer_size = (1 << (width + 11)) - 1;
-   /*
-* Clip max_transfer_size to 65535. dwc2_hc_setup_align_buf() allocates
-* coherent buffers with this size, and if it's too large we can
-* exhaust the coherent DMA pool.
-*/
-   if (hw->max_transfer_size > 65535)
-   hw->max_transfer_size = 65535;
width = (hwcfg3 & GHWCFG3_PACKET_SIZE_CNTR_WIDTH_MASK) >>
GHWCFG3_PACKET_SIZE_CNTR_WIDTH_SHIFT;
hw->max_packet_count = (1 << (width + 4)) - 1;
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 8847c72e55f6..bc4bdbc1534e 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -635,9 +635,9 @@ static void dwc2_hc_init_split(struct dwc2_hsotg *hsotg,
chan->hub_port = (u8)hub_port;
 }
 
-static void *dwc2_hc_init_xfer(struct dwc2_hsotg *hsotg,
-  struct dwc2_host_chan *chan,
-  struct dwc2_qtd *qtd, void *bufptr)
+static void dwc2_hc_init_xfer(struct dwc2_hsotg *hsotg,
+ struct dwc2_host_chan *chan,
+ struct dwc2_qtd *qtd)
 {
struct dwc2_hcd_urb *urb = qtd->urb;
struct dwc2_hcd_iso_packet_desc *frame_desc;
@@ -657,7 +657,6 @@ static void *dwc2_hc_init_xfer(struct dwc2_hsotg *hsotg,
else
chan->xfer_buf = urb->setup_packet;
chan->xfer_len = 8;
-   bufptr = NULL;
break;
 
case DWC2_CONTROL_DATA:
@@ -684,7 +683,6 @@ static void *dwc2_hc_init_xfer(struct dwc2_hsotg *hsotg,
chan->xfer_dma = hsotg->status_buf_dma;
else
chan->xfer_buf = hsotg->status_buf;
-   bufptr = NULL;
break;
}
break;
@@ -717,14 +715,6 @@ static void *dwc2_hc_init_xfer(struct dwc2_hsotg *hsotg,
 
chan->xfer_len = frame_desc->length - qtd->isoc_split_offset;
 
-   /* For non-dword aligned buffers */
-   if (hsotg->core_params->dma_enable > 0 &&
-   (chan->xfer_dma & 0x3))
-   bufptr = (u8 *)urb->buf + frame_desc->offset +
-   qtd->isoc_split_offset;
-   else
-   bufptr = NULL;
-
if (chan->xac

[PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528 for rk3066

[Douglas Anderson]

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.

Since we have space, Kever Yang suggests bumping by 8.  He says this
will meet INCR16 access and next fifo type can start with a aligned
address.

...so let's bump up by 8.  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.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Set host_rx_fifo_size to 528 for rk3066 new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 5008a467ce06..b6d7666e715c 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -126,7 +126,7 @@ static const struct dwc2_core_params params_rk3066 = {
.speed  = -1,
.enable_dynamic_fifo= 1,
.en_multiple_tx_fifo= -1,
-   .host_rx_fifo_size  = 520,  /* 520 DWORDs */
+   .host_rx_fifo_size  = 528,  /* 528 DWORDs */
.host_nperio_tx_fifo_size   = 128,  /* 128 DWORDs */
.host_perio_tx_fifo_size= 256,  /* 256 DWORDs */
.max_transfer_size  = -1,
-- 
2.7.0.rc3.207.g0ac5344

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 07/21] usb: dwc2: hcd: fix split transfer schedule sequence

[Douglas Anderson]

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

Signed-off-by: Douglas Anderson 
Signed-off-by: Yunzhi Li 
---
Changes in v5:
- Move list maintenance to hcd.c to avoid gadget-only compile error

Changes in v4:
- fix split transfer schedule sequence new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/core.c |  2 ++
 drivers/usb/dwc2/core.h |  2 ++
 drivers/usb/dwc2/hcd.c  |  8 
 drivers/usb/dwc2/hcd.h  |  2 ++
 drivers/usb/dwc2/hcd_intr.c | 17 +
 5 files changed, 31 insertions(+)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 73f2771b7740..ed73b26818c0 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -1676,6 +1676,8 @@ void dwc2_hc_cleanup(struct dwc2_hsotg *hsotg, struct 
dwc2_host_chan *chan)
 
chan->xfer_started = 0;
 
+   list_del_init(&chan->split_order_list_entry);
+
/*
 * Clear channel interrupt enables and any unhandled channel interrupt
 * conditions
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 7fb6434f4639..538cf38af0e4 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -657,6 +657,7 @@ struct dwc2_hregs_backup {
  *  periodic_sched_ready because it must be rescheduled for
  *  the next frame. Otherwise, the item moves to
  *  periodic_sched_inactive.
+ * @split_order:List keeping track of channels doing splits, in order.
  * @periodic_usecs: Total bandwidth claimed so far for periodic transfers.
  *  This value is in microseconds per (micro)frame. The
  *  assumption is that all periodic transfers may occur in
@@ -780,6 +781,7 @@ struct dwc2_hsotg {
struct list_head periodic_sched_ready;
struct list_head periodic_sched_assigned;
struct list_head periodic_sched_queued;
+   struct list_head split_order;
u16 periodic_usecs;
u16 frame_usecs[8];
u16 frame_number;
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index d2daaea88d91..87ad5bf2d166 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1045,6 +1045,11 @@ static int dwc2_queue_transaction(struct dwc2_hsotg 
*hsotg,
 {
int retval = 0;
 
+   if (chan->do_split)
+   /* Put ourselves on the list to keep order straight */
+   list_move_tail(&chan->split_order_list_entry,
+  &hsotg->split_order);
+
if (hsotg->core_params->dma_enable > 0) {
if (hsotg->core_params->dma_desc_enable > 0) {
if (!chan->xfer_started ||
@@ -3151,6 +3156,8 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
INIT_LIST_HEAD(&hsotg->periodic_sched_assigned);
INIT_LIST_HEAD(&hsotg->periodic_sched_queued);
 
+   INIT_LIST_HEAD(&hsotg->split_order);
+
/*
 * Create a host channel descriptor for each host channel implemented
 * in the controller. Initialize the channel descriptor array.
@@ -3164,6 +3171,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
if (channel == NULL)
goto error3;
channel->hc_num = i;
+   INIT_LIST_HEAD(&channel->split_order_list_entry);
hsotg->hc_ptr_array[i] = channel;
}
 
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 42f2e4e233da..1b46e2e617cc 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -106,6 +106,7 @@ struct dwc2_qh;
  * @hc_list_entry:  For linking to list of host channels
  * @desc_list_addr: Current QH's descriptor list DMA address
  * @desc_list_sz:   Current QH's descriptor list size
+ * @split_order_list_entry: List entry for keeping track of the order of splits
  *
  * This structure represents the state of a single host channel when acting in
  * host mode. It contains the data items needed to transfer packets to an
@@ -158,6 +159,7 @@ struct dwc2_host_chan {
struct list_head hc_list_entry;
dma_addr_t desc_list_addr;
u32 desc_list_sz;
+   struct list_head split_order_list_entry;
 };
 
 struct dwc2_hcd_pipe_info {

[PATCH v5 01/21] usb: dwc2: rockchip: Make the max_transfer_size automatic

[Douglas Anderson]

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 e8f8c14d9da7 ("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 
Acked-by: John Youn 
Tested-by: Heiko Stuebner 
---
Changes in v5: None
Changes in v4:
- Add John's Acks from 

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 510f787434b3..5008a467ce06 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -129,7 +129,7 @@ static const struct dwc2_core_params params_rk3066 = {
.host_rx_fifo_size  = 520,  /* 520 DWORDs */
.host_nperio_tx_fifo_size   = 128,  /* 128 DWORDs */
.host_perio_tx_fifo_size= 256,  /* 256 DWORDs */
-   .max_transfer_size  = 65535,
+   .max_transfer_size  = -1,
.max_packet_count   = -1,
.host_channels  = -1,
.phy_type   = -1,
-- 
2.7.0.rc3.207.g0ac5344

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 04/21] usb: dwc2: host: Set host_perio_tx_fifo_size to 304 for rk3066

[Douglas Anderson]

Looking at rk3288, there are two dwc2 ports.  One has 960
total_fifo_size and the other 972.

We're currently assigning 528 + 128 + 256 = 912.  That means we're
wasting 48 words on one port and 60 words on the other.  Since we have
one settings block for both ports, let's just eat the 48 words that we
know we have and assign them to what's probably the most
latency-senstive part of the system: periodic transfers.

Presumably other rk3288 parts using the rk3066 settings also have these
extra 48 words as well.

Note: ideally we'd adjust dwc2_calculate_dynamic_fifo() to handle this
more dynamically, but until that's done hardcoding things here seems OK.

NOTE: no known use cases are improved by this patch, but in my stressful
USB tests I certainly see my periodic FIFO filling up both before and
after this change.  Presumably there are some USB configurations where
the periodic FIFO would have been filled before where it no longer is.

Signed-off-by: Douglas Anderson 
---
Changes in v5: None
Changes in v4:
- Set host_perio_tx_fifo_size to 304 for rk3066 new for v4.

Changes in v3: None
Changes in v2: None

 drivers/usb/dwc2/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index b6d7666e715c..afdcdeee266d 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -128,7 +128,7 @@ static const struct dwc2_core_params params_rk3066 = {
.en_multiple_tx_fifo= -1,
.host_rx_fifo_size  = 528,  /* 528 DWORDs */
.host_nperio_tx_fifo_size   = 128,  /* 128 DWORDs */
-   .host_perio_tx_fifo_size= 256,  /* 256 DWORDs */
+   .host_perio_tx_fifo_size= 304,  /* 304 DWORDs */
.max_transfer_size  = -1,
.max_packet_count   = -1,
.host_channels  = -1,
-- 
2.7.0.rc3.207.g0ac5344

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB gadgets with configfs hang reboot

[Tony Lindgren]

* Andrzej Pietrasiewicz  [160122 02:36]:
> Hi Tony,
> 
> W dniu 15.01.2016 o 23:48, Tony Lindgren pisze:
> >Hi all,
> >
> >Looks like there's some issue with the USB gadgets and configfs.
> >
> >I'm seeing rmmod of the UDC driver cause a warning and then reboot
> >hangs the system. This happens at least with v4.4, and I've reproduced
> >it with dwc3 and musb so it seems to be generic.
> >
> 
> I can't reproduce your problem on 4.4-rc4 nor on 4.4 release
> with the script you provided
>
> I'm using Odroid XU3, which has dwc3.

Well thanks for trying. Probably the difference is that
dwc3-exynos.c remove is calling platform_device_unregister()
while dwc3-omap.c remove is calling of_platform_depopulate()?

> > mkdir functions/ecm.0
> > ln -s functions/acm.0 configs/c.1
> 
> I assume you meant ecm.0 here and in the next line.
> 
> > ln -s functions/acm.0 configs/c.2

OK, yes you're right. That does not affect the outcome though.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/1] USB: core: let USB device know device node

[Alan Stern]

On Fri, 22 Jan 2016, Arnd Bergmann wrote:

> We just have to decide what to do for non-hub devices that the OF
> specification calls "combined nodes" (device class 0, one configuration,
> one interface) and that, like hubs do not have one of_node per interface
> plus one per device, but only one node.
> 
> Should we bind the of_node to the usb_device, the usb_interface or both
> in this case? Doing both might be problematic and would need more
> testing to be sure it works.

The purpose of the of_node would generally be to express requirements 
necessary for operating the device, right?  Since the device has to be 
operational before its interfaces can be known, the of_node should be 
bound to the usb_device.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in split transactions on Raspberry Pi

[Alan Stern]

On Thu, 21 Jan 2016, Doug Anderson wrote:

> This doesn't sound familiar to me, but as far as I know all the
> official Raspberry Pi kernels use a pretty different dwc2 driver.  See
> the (non-mainline) code in drivers/usb/host/dwc_otg/ in the Raspberry
> Pi tree at .

Ah, thanks for that pointer.  I also found the page at raspberrypi.org 
which describes how to build and install a custom kernel.

> The Raspberry Pi driver and the mainline driver are pretty different
> (though they have the same heritage).  As far as I remember the
> Raspberry Pi driver has feature to enable FIQ for helping deal with
> some of the dwc2 issues.  That gives it a chunk of code that's totally
> rewritten...

Indeed, dwc_otg is quite different from dwc2.  I found what appears to
be a bug in the dwc_otg's _hub_info() routine.  (This is the subroutine
that gets the upstream hub address and port number for devices behind a
TT.)  It does:

*port_addr = urb->dev->tt->multi ? urb->dev->ttport : 1;

However, the USB spec says that the SPLIT packet's third byte is
supposed to contain the port for both single-TT and multi-TT hubs.  
And sure enough, the upstream hub for this non-working device is
single-TT.  Presumably that explains why the third byte in the SPLIT
packet was 0x01 rather than 0x04.

The corresponding line in the dwc2_host_hub_info() routine simply does:

*hub_port = urb->dev->ttport;

Unfortunately, even after I changed the dwc_otg routine the device 
still didn't work.  My bus analyzer isn't on hand today, so I'll have
to work on it more next week.

> It would be really interesting to see if the patch series I just
> posted helps you (it fixes lots of problems in the mainline driver
> with splits), though it might still not be enough on the Raspberry Pi
> since the CPU clock there is much slower than on my 1.8GHz A12-based
> system and thus maybe FIQ is a must have there...

Can you tell me how to configure a mainline kernel to work on the
Raspberry Pi?  There is no arch/arm/configs/bcmrpi_defconfig.

> Anyway, sorry if that's not terribly helpful.  :(  John may have more
> familiarity with the non-mainline driver (or know people who are more
> familiar with it).

Thanks,

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: dwc2: host: Properly set the HFIR

[Douglas Anderson]

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

Signed-off-by: Douglas Anderson 
---
 drivers/usb/dwc2/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 39a0fa8a4c0a..c7798476e25b 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -2251,10 +2251,10 @@ u32 dwc2_calc_frame_interval(struct dwc2_hsotg *hsotg)
 
if ((hprt0 & HPRT0_SPD_MASK) >> HPRT0_SPD_SHIFT == HPRT0_SPD_HIGH_SPEED)
/* High speed case */
-   return 125 * clock;
+   return 125 * clock - 1;
else
/* FS/LS case */
-   return 1000 * clock;
+   return 1000 * clock - 1;
 }
 
 /**
-- 
2.7.0.rc3.207.g0ac5344

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug in split transactions on Raspberry Pi

[Doug Anderson]

Alan,

On Fri, Jan 22, 2016 at 11:48 AM, Alan Stern  wrote:
> On Thu, 21 Jan 2016, Doug Anderson wrote:
>
>> This doesn't sound familiar to me, but as far as I know all the
>> official Raspberry Pi kernels use a pretty different dwc2 driver.  See
>> the (non-mainline) code in drivers/usb/host/dwc_otg/ in the Raspberry
>> Pi tree at .
>
> Ah, thanks for that pointer.  I also found the page at raspberrypi.org
> which describes how to build and install a custom kernel.
>
>> The Raspberry Pi driver and the mainline driver are pretty different
>> (though they have the same heritage).  As far as I remember the
>> Raspberry Pi driver has feature to enable FIQ for helping deal with
>> some of the dwc2 issues.  That gives it a chunk of code that's totally
>> rewritten...
>
> Indeed, dwc_otg is quite different from dwc2.  I found what appears to
> be a bug in the dwc_otg's _hub_info() routine.  (This is the subroutine
> that gets the upstream hub address and port number for devices behind a
> TT.)  It does:
>
> *port_addr = urb->dev->tt->multi ? urb->dev->ttport : 1;

Hrm.  Yeah, in the mainline driver it looks like we just have:

void dwc2_host_hub_info(struct dwc2_hsotg *hsotg, void *context, int *hub_addr,
int *hub_port)
{
struct urb *urb = context;

if (urb->dev->tt)
*hub_addr = urb->dev->tt->hub->devnum;
else
*hub_addr = 0;
*hub_port = urb->dev->ttport;
}


> However, the USB spec says that the SPLIT packet's third byte is
> supposed to contain the port for both single-TT and multi-TT hubs.
> And sure enough, the upstream hub for this non-working device is
> single-TT.  Presumably that explains why the third byte in the SPLIT
> packet was 0x01 rather than 0x04.
>
> The corresponding line in the dwc2_host_hub_info() routine simply does:
>
> *hub_port = urb->dev->ttport;
>
> Unfortunately, even after I changed the dwc_otg routine the device
> still didn't work.  My bus analyzer isn't on hand today, so I'll have
> to work on it more next week.
>
>> It would be really interesting to see if the patch series I just
>> posted helps you (it fixes lots of problems in the mainline driver
>> with splits), though it might still not be enough on the Raspberry Pi
>> since the CPU clock there is much slower than on my 1.8GHz A12-based
>> system and thus maybe FIQ is a must have there...
>
> Can you tell me how to configure a mainline kernel to work on the
> Raspberry Pi?  There is no arch/arm/configs/bcmrpi_defconfig.

Not offhand.  Again, perhaps someone else on this thread will have
more.  I did do a quick Google search and came up with:

http://elinux.org/RPi_Upstream_Kernel_Compilation

...but the Raspberry Pi I have at home is running some home automation
stuff so I don't want to give that a try.  :-P

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html