Re: USB scanner stops working with xhci_hcd URB transfer length is wrong, xHC issue? req. len = 0, act. len = 4294967288

[Orion Poplawski]

On 11/06/2015 10:34 AM, Felipe Balbi wrote:


Hi,

Orion Poplawski  writes:

See https://bugzilla.kernel.org/show_bug.cgi?id=107331

Trying to use my scanner.  Worked for a while, but now access fails.  Get this
in log:


okay, first things first. Which kernel version are you using ? Care to
capture a usbmon trace ? (see Documentation/usb/usbmon.txt for instructions)


kernel is 4.2.5-201.fc22.x86_64

Output from:

cat /sys/kernel/debug/usb/usbmon/1u > usb-scanner.log

is attached.  Thanks.


Nov 05 20:25:51 pacas.cora.nwra.com kernel: usb 1-3: new full-speed USB device
number 3 using xhci_hcd
Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: New USB device found,
idVendor=04a9, idProduct=2206
Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: New USB device strings:
Mfr=64, Product=77, SerialNumber=0
Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: Product: CanoScan
Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: Manufacturer: Canon
Nov 05 20:27:55 pacas.cora.nwra.com kernel: xhci_hcd :00:14.0: URB
transfer length is wrong, xHC issue? req. len = 0, act. len = 4294967288


this looks like the length variable wrapped around ? that value is
0xfff8 which is -8 on 32-bits.

Are you, perhaps, using an older kernel which still has this issue with
wrapping which has been fixed before ?


How reproducible:
First time so far.

Additional info:
00:14.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset Family
USB xHCI Host Controller (rev 04) (prog-if 30 [XHCI])
 Subsystem: Acer Incorporated [ALI] Device 0748
 Flags: bus master, medium devsel, latency 0, IRQ 25
 Memory at c060 (64-bit, non-prefetchable) [size=64K]
 Capabilities: [70] Power Management version 2
 Capabilities: [80] MSI: Enable+ Count=1/8 Maskable- 64bit+
 Kernel driver in use: xhci_hcd

Possibly related:
https://www.mail-archive.com/linux-usb@vger.kernel.org/msg50230.html



--
Orion Poplawski
Technical Manager 303-415-9701 x222
NWRA/CoRA DivisionFAX: 303-415-9702
3380 Mitchell Lane  or...@cora.nwra.com
Boulder, CO 80301  http://www.cora.nwra.com
880220628480 1003709312 S Ci:1:001:0 s a3 00  0001 0004 4 <
880220628480 1003709366 C Ci:1:001:0 0 4 = 0001
880220628480 1003709386 S Ci:1:001:0 s a3 00  0002 0004 4 <
880220628480 1003709400 C Ci:1:001:0 0 4 = 0001
880220628480 1003709411 S Ci:1:001:0 s a3 00  0003 0004 4 <
880220628480 1003709419 C Ci:1:001:0 0 4 = 0001
880220628480 1003709427 S Ci:1:001:0 s a3 00  0004 0004 4 <
880220628480 1003709437 C Ci:1:001:0 0 4 = 01010100
880220628480 1003709449 S Co:1:001:0 s 23 01 0010 0004  0
880220628480 1003709461 C Co:1:001:0 0 0
880243136cc0 1003809748 S Ii:1:001:1 -115:2048 4 <
880220628480 1003809799 S Ci:1:001:0 s a3 00  0004 0004 4 <
880220628480 1003809830 C Ci:1:001:0 0 4 = 0101
880220628480 1003809932 S Co:1:001:0 s 23 03 0004 0004  0
880220628480 1003809960 C Co:1:001:0 0 0
880220628480 1003860747 S Ci:1:001:0 s a3 00  0004 0004 4 <
880220628480 1003860801 C Ci:1:001:0 0 4 = 1101
880220628480 1003911757 S Ci:1:001:0 s a3 00  0004 0004 4 <
880220628480 1003911800 C Ci:1:001:0 0 4 = 03011000
880220628480 1003911818 S Co:1:001:0 s 23 01 0014 0004  0
880220628480 1003911834 C Co:1:001:0 0 0
880220628480 1003962809 S Ci:1:000:0 s 80 06 0100  0040 64 <
880220628480 1003964844 C Ci:1:000:0 0 8 = 12010001 0008
880220628480 1003964986 S Co:1:001:0 s 23 03 0004 0004  0
880220628480 1003965018 C Co:1:001:0 0 0
880220628480 1004015756 S Ci:1:001:0 s a3 00  0004 0004 4 <
880220628480 1004015793 C Ci:1:001:0 0 4 = 1101
880220628480 1004066749 S Ci:1:001:0 s a3 00  0004 0004 4 <
880220628480 1004066789 C Ci:1:001:0 0 4 = 03011000
880220628480 1004066804 S Co:1:001:0 s 23 01 0014 0004  0
880220628480 1004066816 C Co:1:001:0 0 0
880220628480 1004128769 S Ci:1:005:0 s 80 06 0100  0012 18 <
880220628480 1004131030 C Ci:1:005:0 0 18 = 12010001 0008 a9040622 0001404d 0001
880220628480 1004131175 S Ci:1:005:0 s 80 06 0200  0009 9 <
880220628480 1004132568 C Ci:1:005:0 0 9 = 09022700 01010080 fa
880220628480 1004132751 S Ci:1:005:0 s 80 06 0200  0027 39 <
880220628480 1004137124 C Ci:1:005:0 0 39 = 09022700 01010080 fa090400 0003ff00 ff000705 81030100 10070582 0240
880220628480 1004137187 S Ci:1:005:0 s 80 06 0300  00ff 255 <
880220628480 1004137960 C Ci:1:005:0 0 4 = 04030904
880220628480 1004138016 S Ci:1:005:0 s 80 06 034d 0409 00ff 255 <
880220628480 1004140210 C Ci:1:005:0 0 18 = 12034300 61006e00 6f005300 63006100 6e00
880220628480 1004140269 S Ci:1:005:0 s 80 06 0340 0409 00ff 255 <
880220628480 1004141857 C Ci:1:005:0 0 12 = 0c034300 61006e00 6f006e00
8802206280c0 1004142587 S Co:1

[RFC PATCH] usb: dwc2: host: Rewrite the microframe scheduler

[Douglas Anderson]

The old microframe scheduler was terribly hard to follow and (it seemed
to me) that it had some bugs in it.

Let's re-write it in a simpler, easier-to-read way.  Hopefully this will
work better.

Note: no known problems are fixed by this patch, and in fact I can see
very little impact of the microframe scheduler overall.

Signed-off-by: Douglas Anderson 
---
 drivers/usb/dwc2/hcd_queue.c | 72 
 1 file changed, 32 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 7d8d06cfe3c1..d6c24decee08 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -359,57 +359,49 @@ static int dwc2_find_single_uframe(struct dwc2_hsotg 
*hsotg, struct dwc2_qh *qh)
  */
 static int dwc2_find_multi_uframe(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 {
-   unsigned short utime = qh->usecs;
-   unsigned short xtime;
-   int t_left;
+   int utime;
int i;
int j;
-   int k;
 
for (i = 0; i < 8; i++) {
if (hsotg->frame_usecs[i] <= 0)
continue;
 
-   /*
-* we need n consecutive slots so use j as a start slot
-* j plus j+1 must be enough time (for now)
-*/
-   xtime = hsotg->frame_usecs[i];
-   for (j = i + 1; j < 8; j++) {
+   for (utime = qh->usecs, j = i; utime > 0 && j < 8; j++) {
+   /* Give the available time from this uframe */
+   utime -= hsotg->frame_usecs[j];
+
/*
-* if we add this frame remaining time to xtime we may
-* be OK, if not we need to test j for a complete frame
+* Except for first frame, we can't continue past this
+* frame if it wasn't full, so bail now.  We might still
+* be successful the above subtract made utime <= 0.
 */
-   if (xtime + hsotg->frame_usecs[j] < utime) {
-   if (hsotg->frame_usecs[j] <
-   max_uframe_usecs[j])
-   continue;
-   }
-   if (xtime >= utime) {
-   t_left = utime;
-   for (k = i; k < 8; k++) {
-   t_left -= hsotg->frame_usecs[k];
-   if (t_left <= 0) {
-   qh->frame_usecs[k] +=
-   hsotg->frame_usecs[k]
-   + t_left;
-   hsotg->frame_usecs[k] = -t_left;
-   return i;
-   } else {
-   qh->frame_usecs[k] +=
-   hsotg->frame_usecs[k];
-   hsotg->frame_usecs[k] = 0;
-   }
-   }
-   }
-   /* add the frame time to x time */
-   xtime += hsotg->frame_usecs[j];
-   /* we must have a fully available next frame or break */
-   if (xtime < utime &&
-  hsotg->frame_usecs[j] == max_uframe_usecs[j])
-   continue;
+   if ((i != j) &&
+   (hsotg->frame_usecs[j] < max_uframe_usecs[j]))
+   break;
+   }
+
+   /* If utime > 0 after above loop, try a different start (i) */
+   if (utime > 0)
+   continue;
+
+   dev_dbg(hsotg->dev, "Assigned %d us starting at i=%d + %d us\n",
+   qh->usecs, i,
+   max_uframe_usecs[i] - hsotg->frame_usecs[i]);
+
+   /* We've got success, so allocate */
+   for (utime = qh->usecs, j = i; utime > 0 && j < 8; j++) {
+   qh->frame_usecs[i] = min_t(u16, utime,
+  hsotg->frame_usecs[j]);
+   utime -= qh->frame_usecs[i];
+   hsotg->frame_usecs[j] -= qh->frame_usecs[i];
}
+
+   return i;
}
+
+   dev_dbg(hsotg->dev, "Failed to assign %d us\n", qh->usecs);
+
return -ENOSPC;
 }
 
-- 
2.6.0.rc2.230.g3dd15c0

--
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 v2 1/4] 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 
Tested-by: Heiko Stuebner 
---
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 5859b0fa19ee..f26e0c31c07e 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -95,7 +95,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.6.0.rc2.230.g3dd15c0

--
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/3] usb: dwc2: host: Giveback URB in tasklet context

[Doug Anderson]

Alan,

On Fri, Nov 6, 2015 at 7:40 AM, Alan Stern  wrote:
> On Thu, 5 Nov 2015, Doug Anderson wrote:
>
>> Alan,
>>
>> On Thu, Nov 5, 2015 at 7:19 AM, Alan Stern  wrote:
>> > On Wed, 4 Nov 2015, Doug Anderson wrote:
>> >
>> >> In the ChromeOS gerrit
>> >>  Julius Werner
>> >> points out that for EHCI it was good to take the optimization from
>> >> commit 9118f9eb4f1e ("USB: EHCI: improve interrupt qh unlink") before
>> >> this one.  I'm still trying to learn USB / dwc2 so it's unclear to me
>> >> whether we also need a similar change before landing.
>> >>
>> >> I'll see if I can do some investigation about this and also some
>> >> benchmarking before and after.  Certainly profiling the interrupt
>> >> handler itself showed a huge improvement, but I'd hate to see a
>> >> regression elsewhere.
>> >>
>> >> If anyone else knows better than I, please speak up!  :)
>> >
>> > This is a matter of both efficiency and correctness.  Giving back URBs
>> > in a tasklet is not a simple change.
>> >
>> > Have you read the kerneldoc for usb_submit_urb() in
>> > drivers/usb/core/urb.c?  The portion about "Reserved Bandwidth
>> > Transfers" is highly relevant.  I don't know how dwc2 goes about
>> > reserving bandwidth for periodic transfers, but if it relies on the
>> > endpoint queue being non-empty to maintain a reservation then it will
>> > be affected by this change.
>>
>> It does look as if you are right and the reservation will end up being
>> released.  It looks to me like dwc2_deschedule_periodic() is in charge
>> of releasing the reservation.  I'll work on trying to actually confirm
>> this.  I guess I need to find a USB test setup where there are enough
>> devices that I exceed the available time so I can see the brokenness
>> of my old solution...
>
> You may not need that.  Try a single USB keyboard and see what happens
> when the interrupt URB is given back.

I haven't been able to reproduce any new errors with my patch, but I
have with trace_printk I have proven that it does cause reservations
to be lost and then re-gained, which isn't good.

Note that dwc2 is currently having scheduling problems anyway (even
before my patch).  l...@rock-chips.com posted an RFC patch to fix
problems he was seeing, but I have a setup that has problems too and
his patch doesn't fix it.  :(  ...so possibly if everything was
working then I could see my patch break things, but as it is now it's
hard to say.

In any case, I've finished testing the patch that adds a 5us delay
before releasing the reservation.  I'll post that so we can be on the
same page.


>> I hadn't realized that this was a correctness problem and not just an
>> optimization problem, so thank you very much for the info!  :)  I ran
>> with a bunch of USB devices and it worked fine (and performance
>> improved!) so I figured I was good to go...  Now I've read the
>> kerneldoc you pointed at and it was very helpful.  As I understand it,
>> it's considered OK if I copy what EHCI did and release the reservation
>> if nothing has been scheduled for 5 ms.
>
> You might also look into the issues surrounding isochronous URBs.  In
> particular, when an URB is submitted, which frames or microframes
> should it be scheduled in?  The problem is that when the submission
> occurs, some of the slots following the previous URB may already have
> expired.  The explanations for EXDEV and EFBIG in
> Documentation/usb/error-codes.txt are relevant here, although terse.
>
> The host controller drivers that I maintain work like this:
>
> If the endpoint's queue is empty and none of the endpoint's
> URBs are still being given back by the tasklet, pretend that
> the URB_ISO_ASAP flag is set.  Note that this involves
> testing hcd_periodic_completion_in_progress() -- that's
> where switching over to tasklets makes a difference.
>
> If the URB_ISO_ASAP flag is set, the URB is scheduled for
> the first unallocated slot that hasn't already expired.
>
> If the flag isn't set, try to schedule the URB for the first
> unallocated slot.  If that means all the isoc packets in the
> URB would be assigned to expired slots, return -EXDEV.  If
> some but not all of the packets would be assigned to expired
> slots, skip them -- only schedule the packets whose slots
> haven't expired yet.

You're talking to someone who has only been looking at the details of
USB for about 2 days now.  :-P  ...I'm trying to grok all of that, but
I'm not sure I got it all...

I will say that "URB_ISO_ASAP" is not referenced anywhere in dwc2.
Presumably that's a bug (or missing feature).  Would it be terrible if
I just ignored that for now and said that if you try to add something
to the queue and we're currently in the process of waiting for our
timer to expire then you'll just get back -ENOSPC?  dwc2 won't deal
perfectly well if you've very close to exhausting the avai

[PATCH v2 4/4] 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.

Signed-off-by: Douglas Anderson 
Tested-by: Heiko Stuebner 
---
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 33495b235b3c..b899b06b41cc 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -2266,9 +2266,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);
 }
 
 /*
@@ -2881,7 +2879,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.6.0.rc2.230.g3dd15c0

--
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 v2 3/4] 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.

Signed-off-by: Douglas Anderson 
---
Changes in v2:
- Periodic bandwidth release delay new for V2

 drivers/usb/dwc2/hcd.h   |   6 ++
 drivers/usb/dwc2/hcd_queue.c | 232 +--
 2 files changed, 184 insertions(+), 54 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 8a4486e1a542..b75a8b116f6e 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -211,6 +211,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
@@ -247,13 +248,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;
@@ -275,7 +279,9 @@ struct dwc2_qh {
struct dwc2_hcd_dma_desc *desc_list;
dma_addr_t desc_list_dma;
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 3e1edafc593c..10f27b594e92 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);
+
+   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--;
+   }
+
+   /* No more unreserve pending--we're did it */
+   qh->unreserve_pending = false;
+}
+
+/**
+ * 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 

[PATCH v2 2/4] 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 
Tested-by: Heiko Stuebner 
---
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 ef73e498e98f..7e28cfafcfd8 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -1830,19 +1830,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 */
@@ -3137,13 +3129,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 e79baf73c234..33495b235b3c 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -598,9 +598,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;
@@ -620,7 +620,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:
@@ -647,7 +646,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;
@@ -680,14 +678,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->xact_pos == DWC2_HCSPLT_XACTPOS_ALL) {
if (chan->xfer_len <= 188)
chan->xact_pos = DWC2_HCSPLT_XACTPOS_ALL;
@@ -69

[PATCH v2 0/4] dwc2: Speed up the interrupt handler quite a bit

[Douglas Anderson]

The dwc2 interrupt handler is quite slow.  On rk3288 with a few things
plugged into the ports and with cpufreq locked at 696MHz (to simulate
real world idle system), I can easily observe dwc2_handle_hcd_intr()
taking > 120 us, sometimes > 150 us.  Note that SOF interrupts come
every 125 us with high speed USB, so taking > 120 us in the interrupt
handler is a big deal.

The patches here will speed up the interrupt controller significantly.
After this series, I have a hard time seeing the interrupt controller
taking > 20 us and I don't ever see it taking > 30 us ever in my tests
unless I bring the cpufreq back down.  With the cpufreq at 126 MHz I can
still see the interrupt handler take > 50 us, so I'm sure we could
improve this further.  ...but hey, it's a start.

This series also shows big speed improvements when testing with a USB
Gigabit Ethernet adapter.  Previously the tested adapter would top out
at about 15MB/s.  After these changes it gets about 23MB/s.

In addition to the speedup, this series also has the advantage of
simplifying dwc2 and making it more like everyone else (introducing the
possibility of future simplifications).  Picking this series up will
help your diffstat and likely win you friends.  ;)

===

Steps for gathering data with ftrace:

cd /sys/devices/system/cpu/cpu0/cpufreq/
echo userspace > scaling_governor
echo 696000 > scaling_setspeed

cd /sys/kernel/debug/tracing
echo 0 > tracing_on
echo "" > trace
echo nop > current_tracer
echo function_graph > current_tracer
echo dwc2_handle_hcd_intr > set_graph_function
echo dwc2_handle_common_intr >> set_graph_function
echo dwc2_handle_hcd_intr > set_ftrace_filter
echo dwc2_handle_common_intr >> set_ftrace_filter
echo funcgraph-abstime > trace_options
echo 70 > tracing_thresh
echo 1 > /sys/kernel/debug/tracing/tracing_on

sleep 2
cat trace

===
NOTE: This series doesn't replace any other patches I've submitted
recently, it merely adds another set of changes that upstream could
benefit from.

Changes in v2:
- Add a warn if setup_dma is not aligned (Julius Werner).
- Periodic bandwidth release delay new for V2
- Commit message now says that URB giveback change needs delay change.

Douglas Anderson (4):
  usb: dwc2: rockchip: Make the max_transfer_size automatic
  usb: dwc2: host: Get aligned DMA in a more supported way
  usb: dwc2: host: Add a delay before releasing periodic bandwidth
  usb: dwc2: host: Giveback URB in tasklet context

 drivers/usb/dwc2/core.c  |  21 +---
 drivers/usb/dwc2/hcd.c   | 174 +++
 drivers/usb/dwc2/hcd.h   |  16 ++-
 drivers/usb/dwc2/hcd_intr.c  |  65 
 drivers/usb/dwc2/hcd_queue.c | 239 ---
 drivers/usb/dwc2/platform.c  |   2 +-
 6 files changed, 273 insertions(+), 244 deletions(-)

-- 
2.6.0.rc2.230.g3dd15c0

--
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] USB: DCW3: GADGET: Set Correct Max Speed

[John Youn]

On 11/6/2015 9:55 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> "McCauley, Ben"  writes:
>> Felipe,
>>
>>  -Original Message-
>>  From: Felipe Balbi [mailto:ba...@ti.com]
>>  Sent: Friday, November 06, 2015 10:06 AM
>>  To: McCauley, Ben 
>>  Cc: linux-usb@vger.kernel.org; Schroeder, Jay 
>> Subject: Re: [PATCH] USB: DCW3: GADGET: Set Correct Max Speed
>>>
>>>
>>> Hi,
>>>
>>> "McCauley, Ben"  writes:
 The max speed was always being set to USB_SUPER_SPEED even when the
 phy was only capable of HIGH_SPEED. This
>>>
>>> this statement is untrue
>>>
 uses the value set for the phy to set the max for the gadget. It
 resolves an issue with Window PCs where they report that using a
 USB3.0 port would result in better performance even when connecting
 through a 3.0 port.
>>>
>>> man, who gave you this kernel ? Which kernel version are you using ?
>>> Have you even tested mainline ?
>>
>> This Kernel is from 
>> http://omapzoom.org/?p=kernel/omap.git;a=shortlog;h=refs/heads/p-ti-linux-3.14.y-common
>> at branch  "development/omapzoom/glsdk-7.-2.00.02".
>> I have not tested it on main line.
> 
> seems like you should be talking to your FAE or using TI's e2e forum for
> this.
> 
 Signed-off-by: McCauley, Ben 
 ---

 diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
 index 1eaf31c..0a388e3 100644
 --- a/drivers/usb/dwc3/gadget.c
 +++ b/drivers/usb/dwc3/gadget.c
 @@ -2899,7 +2899,7 @@
>>>
>>> which kernel are you using ? This file is 2868 lines, so you're patching 
>>> outside
>>> the file.
>>>
 }

 dwc->gadget.ops = &dwc3_gadget_ops;
 -   dwc->gadget.max_speed   = USB_SPEED_SUPER;
 +   dwc->gadget.max_speed   = dwc->maximum_speed;
>>>
>>> this is corrected at dwc3_gadget_start():
> 
> and I see that in that kernel, this is corrected at
> dwc3_gadget_restart() instead.
> 
>>>   /**
>>>* WORKAROUND: DWC3 revision < 2.20a have an issue
>>>* which would cause metastability state on Run/Stop
>>>* bit if we try to force the IP to USB2-only mode.
>>>*
>>>* Because of that, we cannot configure the IP to any
>>>* speed other than the SuperSpeed
>>>*
>>>* Refers to:
>>>*
>>>* STAR#9000525659: Clock Domain Crossing on DCTL in
>>>* USB 2.0 Mode
>>>*/
>>>   if (dwc->revision < DWC3_REVISION_220A) {
>>>   reg |= DWC3_DCFG_SUPERSPEED;
>>>   } else {
>>>   switch (dwc->maximum_speed) {
>>>   case USB_SPEED_LOW:
>>>   reg |= DWC3_DSTS_LOWSPEED;
>>>   break;
>>>   case USB_SPEED_FULL:
>>>   reg |= DWC3_DSTS_FULLSPEED1;
>>>   break;
>>>   case USB_SPEED_HIGH:
>>>   reg |= DWC3_DSTS_HIGHSPEED;
>>>   break;
>>>   case USB_SPEED_SUPER:   /* FALLTHROUGH */
>>>   case USB_SPEED_UNKNOWN: /* FALTHROUGH */
>>>   default:
>>>   reg |= DWC3_DSTS_SUPERSPEED;
>>>   }
>>>   }
>>>
>>
>> I am on 3.14 LTS though even on 4.3 I do not see dwc->gadget.max_speed
>> being updated to reflect the actual max speed as set in
>> drivers/usb/dwc3/core.c [dwc3_probe()]. dwc->gadget.max_speed is used
>> in a number of locations to determine what response is sent to the
>> host.
>>
>> dwc3_gadget_start() only appears to set the max speed to the register
>> and not update the gadget.
> 
> but the IP _is_ super-speed capable. Max speed should only be used to
> determine if $this gadget can run with $this controller; speed, instead,
> is used for all sorts of different things and _that_ is updated on
> Connection Done IRQ.
> 
> Can you point one place where you think it's wrong ? And, please, refer
> to mainline code. We can't support a v3.14 kernel which might contain a
> ton of additions which are not in mainline (for example, mainline
> doesn't have a dwc3_gadget_restart() function).
> 


Hi Felipe,

There are 3 related speed settings which may be confusing the
matter.

dwc->maximum_speed

A user-supplied parameter that determines how to program the
DCFG.speed. This is the code you're referring to above.

gadget->speed

The current connected speed of the gadget. Used in various ways
by the upper layer.

gadget->max_speed

Reports the maximum speed supported by the gadget.

This is used by upper layer to determine which BOS descriptors to
send and the settings within those descriptors. It's also used to
set the bcdUSB appropriately.

Ben is talking about setting the gadget->max_speed, which is
hard-coded in dwc3 to USB_SPEED_SUPER.

For this core, the maximum supported speed may not actually be
super speed. Many customers choose to use the 3.0 core in
2.0-only mode with a HS phy.

And with the 3.1 core, it my be integrated in a system that is
HS-only, SS (gen1), or SS

Re: [RFC] usb: dwc2: hcd: fix split schedule issue

[Doug Anderson]

lyz@,

On Fri, Nov 6, 2015 at 1:36 AM, Yunzhi Li  wrote:
> Fix dwc2 split schedule sequence issue. Not schedule a SSPLIT_IN
> packet between SSPLIT-begin and SSPLIT-end.
>
> Signed-off-by: Yunzhi Li 
> ---
>  drivers/usb/dwc2/hcd.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index e79baf7..a32ed01 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -1122,6 +1122,10 @@ static void dwc2_process_periodic_channels(struct 
> dwc2_hsotg *hsotg)
> break;
> }
>
> +   if (qh->channel->xact_pos == DWC2_HCSPLT_XACTPOS_BEGIN ||
> +   qh->channel->xact_pos == DWC2_HCSPLT_XACTPOS_MID)
> +   break;
> +
> /*
>  * In Slave mode, stay on the current transfer until there is
>  * nothing more to do or the high-bandwidth request count is

Just a quite note to say that this doesn't seem to resolve the weird
errors I'm seeing with the "Microsoft Wireless Keyboard 2000" that I
have.  I see split transaction errors in a USB analyzer with just that
hooked up behind a hub (don't even need any other USB devices).

...it is possible that there are two unrelated problems here, but it's
also possible that there's some bigger root cause that will fix both
of our problems.  I've been poking at things a bit too, but so far no
luck...

-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

Re: [PATCH v5 1/5] gadget: Introduce the notifier functions

[Mark Brown]

On Fri, Nov 06, 2015 at 08:56:44AM -0800, Greg KH wrote:
> On Fri, Nov 06, 2015 at 07:35:10PM +0800, Baolin Wang wrote:

> > Thus this patch adds a notifier mechanism for usb gadget to report a
> > event to usb charger when the usb gadget state is changed.

> I thought we said we did not want another notifier chain in the previous
> versions of this patch?

Did we come up with anything better?


signature.asc
Description: PGP signature

Re: [PATCH] Revert "usb: musb: musb_host: Enable HCD_BH flag to handle urb return in bottom half"

[Bin Liu]

Hi,

On 11/06/2015 01:23 PM, Alan Stern wrote:

On Fri, 6 Nov 2015, Bin Liu wrote:


This reverts commit 2035772010db634ec8566b658fb1cd87ec47ac77.

Commit 20357720 claims throughput improvement for MSC/UVC, but I
don't see much improvement. Following are the MSC measurement using
dd on AM335x GP EVM.

with BCD_BH:read: 14.9MB/s, write: 20.9MB/s
without BCD_BH: read: 15.2MB/s, write: 21.2MB/s

However with this commit the following regressions have been observed.

1. ASIX usb-ethernet dongle is completely broken on UDP RX.

2. Unpluging a 3G modem, which uses option driver, behind a hub causes
console log flooding with the following message.

option_instat_callback: error -71


Did you do any debugging to figure out why these problems occur with
HCD_BH but don't occur without it?

No, I never got a chance to debug it, but I am wondering if something 
causes the BH does not get a chance to execute...


I should have mentioned that both cases failed in PIO mode too, so not 
CPPI related.


BTY, the TI kernel prior to mainline 3.12+ does not use HCD_BH in musb 
driver.


Regards,
-Bin.


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] Revert "usb: musb: musb_host: Enable HCD_BH flag to handle urb return in bottom half"

[Alan Stern]

On Fri, 6 Nov 2015, Bin Liu wrote:

> This reverts commit 2035772010db634ec8566b658fb1cd87ec47ac77.
> 
> Commit 20357720 claims throughput improvement for MSC/UVC, but I
> don't see much improvement. Following are the MSC measurement using
> dd on AM335x GP EVM.
> 
> with BCD_BH:read: 14.9MB/s, write: 20.9MB/s
> without BCD_BH: read: 15.2MB/s, write: 21.2MB/s
> 
> However with this commit the following regressions have been observed.
> 
> 1. ASIX usb-ethernet dongle is completely broken on UDP RX.
> 
> 2. Unpluging a 3G modem, which uses option driver, behind a hub causes
>console log flooding with the following message.
> 
>option_instat_callback: error -71

Did you do any debugging to figure out why these problems occur with 
HCD_BH but don't occur without 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] Revert "usb: musb: musb_host: Enable HCD_BH flag to handle urb return in bottom half"

[Bin Liu]

On 11/06/2015 01:08 PM, Felipe Balbi wrote:


Hi,

Bin Liu  writes:

Hi,

On 11/06/2015 11:56 AM, Felipe Balbi wrote:


Hi,

Bin Liu  writes:

This reverts commit 2035772010db634ec8566b658fb1cd87ec47ac77.

Commit 20357720 claims throughput improvement for MSC/UVC, but I
don't see much improvement. Following are the MSC measurement using
dd on AM335x GP EVM.

with BCD_BH:read: 14.9MB/s, write: 20.9MB/s
without BCD_BH: read: 15.2MB/s, write: 21.2MB/s

However with this commit the following regressions have been observed.

1. ASIX usb-ethernet dongle is completely broken on UDP RX.

2. Unpluging a 3G modem, which uses option driver, behind a hub causes
 console log flooding with the following message.

 option_instat_callback: error -71

Signed-off-by: Bin Liu 
---
   drivers/usb/musb/musb_host.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 0e29184..1c7f858 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2613,7 +2613,7 @@ static const struct hc_driver musb_hc_driver = {
.description= "musb-hcd",
.product_desc   = "MUSB HDRC host driver",
.hcd_priv_size  = sizeof(struct musb *),
-   .flags  = HCD_USB2 | HCD_MEMORY | HCD_BH,
+   .flags  = HCD_USB2 | HCD_MEMORY,


reverting will revert isochronous devices, IIRC. Can you check that
cameras still work ? That commit actually mentions that with this commit
we got 640x480@30fps working. Can you make sure it works without HCD_BH
as well ?


I don't see Isoch improvement with HCD_BH either.
All UVC cameras that I have do not work at YUV 640x480@30fps with or
without HCD_BH, mainly due the 50% bandwidth utilization in Isoch
transfer, which I talked to you months ago. The best I can get is YUV
640x480@20fps, which does not change with or without HCD_BH.


fair enough. Unless someone else has any complaints, I'll take this patch.


Thanks Felipe.

--
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] Revert "usb: musb: musb_host: Enable HCD_BH flag to handle urb return in bottom half"

[Felipe Balbi]

Hi,

Bin Liu  writes:
> Hi,
>
> On 11/06/2015 11:56 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Bin Liu  writes:
>>> This reverts commit 2035772010db634ec8566b658fb1cd87ec47ac77.
>>>
>>> Commit 20357720 claims throughput improvement for MSC/UVC, but I
>>> don't see much improvement. Following are the MSC measurement using
>>> dd on AM335x GP EVM.
>>>
>>> with BCD_BH:read: 14.9MB/s, write: 20.9MB/s
>>> without BCD_BH: read: 15.2MB/s, write: 21.2MB/s
>>>
>>> However with this commit the following regressions have been observed.
>>>
>>> 1. ASIX usb-ethernet dongle is completely broken on UDP RX.
>>>
>>> 2. Unpluging a 3G modem, which uses option driver, behind a hub causes
>>> console log flooding with the following message.
>>>
>>> option_instat_callback: error -71
>>>
>>> Signed-off-by: Bin Liu 
>>> ---
>>>   drivers/usb/musb/musb_host.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>>> index 0e29184..1c7f858 100644
>>> --- a/drivers/usb/musb/musb_host.c
>>> +++ b/drivers/usb/musb/musb_host.c
>>> @@ -2613,7 +2613,7 @@ static const struct hc_driver musb_hc_driver = {
>>> .description= "musb-hcd",
>>> .product_desc   = "MUSB HDRC host driver",
>>> .hcd_priv_size  = sizeof(struct musb *),
>>> -   .flags  = HCD_USB2 | HCD_MEMORY | HCD_BH,
>>> +   .flags  = HCD_USB2 | HCD_MEMORY,
>>
>> reverting will revert isochronous devices, IIRC. Can you check that
>> cameras still work ? That commit actually mentions that with this commit
>> we got 640x480@30fps working. Can you make sure it works without HCD_BH
>> as well ?
>>
> I don't see Isoch improvement with HCD_BH either.
> All UVC cameras that I have do not work at YUV 640x480@30fps with or 
> without HCD_BH, mainly due the 50% bandwidth utilization in Isoch 
> transfer, which I talked to you months ago. The best I can get is YUV 
> 640x480@20fps, which does not change with or without HCD_BH.

fair enough. Unless someone else has any complaints, I'll take this patch.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Sergei Shtylyov]

Hello.

On 11/06/2015 12:44 AM, Felipe Balbi wrote:


Here are a few changes in musb_h_tx_flush_fifo().

- Refering to 2ccc6d30a (usb: musb: fix bit mask for CSR in
 musb_h_tx_flush_fifo()), the datasheet says that
MUSB_TXCSR_FLUSHFIFO
 is only valid when MUSB_TXCSR_TXPKTRDY is set as well. It means
 MUSB_TXCSR_TXPKTRDY should be checked, not set.


  Hum, my copy of the MUSBHDRC  programmer's guide says about
TXCSR.FlushFIFO in host mode:

<<
The CPU writes a 1 to this bit to flush the latest packet from the
endpoint Tx FIFO. The FIFO pointer is reset, the TxPktRdy bit (below) is
cleared and an interrupt is generated. May be set simultaneously with
TxPktRdy to abort the packet that is currently being loaded into the
FIFO. Note: FlushFIFO should only be used when TxPktRdy is set. At other
times, it may cause data to be corrupted. Also note that, if the FIFO is
double-buffered, FlushFIFO may need to be set twice to completely clear
the FIFO.
   >>


So how to interpret this message? FLUSHFIFO and TXPKTRDY should be set
at the
same time? If so, I will drop this change in V3.


  Yes, I think it's rather clear in this respect.


In either case, musb is unable to flush the tx fifo in urb dequque for
device
disconnect, but harmless, so the next two changes are important to
give better
user experience.


  Hm, looks like some errata... and hiding this from users while
there's no workaround doesn't seem a good policy.


Well, based on the programmer's guide, the driver should set the flushFIFO
bit, and wait for the interrupt.


  That's oversimplified, consider the double-buffered FIFO. ;-)


Seems like we have some misinterpretation here and IMO Bin's patch is
almost complete. The way I read MUSB's docs, it tells me three different
things:

1. FIFONOTEMPTY is valid *only* when TXPKTRDY is also set

Bin's new while condition solves this part:

+ while ((csr & MUSB_TXCSR_FIFONOTEMPTY) && (csr & MUSB_TXCSR_TXPKTRDY)) {

2. If we have Double Buffering, we need to set MUSB_TXCSR_FLUSHFIFO
twice.

This does not seem to be taken care of, but it would be
something below:

csr |= MUSB_TXCSR_FLUSH_FIFO;
musb_writew(epio, MUSB_TXCSR, csr);

if (ep->double_buffering_enabled)
musb_write(epio, MUSB_TXCSR, csr);

3. We can set TXPKTRDY *together* with MUSB_TXCSR_FLUSH_FIFO to cancel
the packet which is being loaded in the fifo right now.

This seems to be a regression with current patch, however I
don't think current code is perfect either. It unconditionally
sets FLUSH_FIFO and TXPKTRDY without caring for the consequences
of doing that. What happens if we set both of them but there's
no packet being currently loaded into the FIFO ?

To minimize changes, I think Bin just needs to keep the original csr
unconditionally setting both FLUSHFIFO and TXPKTRDY alone while also
making sure to check that TKPKTRDY is set when checking FIFONOTEMPTY.



I am not sure this is any different, since TXPKTRDY is checked, setting
it again along with FLUSHFIFO does not change TXPKTRDY bit, isn't it?


you're missing one detail:

If you set only FLUSHFIFO, TXPKTRDY is cleared and the packet *already*
in the FIFO gets flushed.


   That's exactly what we want, no? Note that the CPU cannot clear the 
TxPktRdy bit, only MUSB itself can.



If you set them both together, you're telling MUSB that you want to
flush a packet which is *still* being loaded into the FIFO. After that
TXPKTRDY will be cleared by MUSB.


   This is not really different from the first case.


I have a few different test cases, as long as a tx urb is queued, the
WARN() will be triggered when dequque called by device disconnect, that
is why I concluded that currently musb is never able to flush tx fifo.

If we are doing it wrong, I am not what the problem is...


And that's where debugging comes in, right ? MUSB's docs are not the
best out there and they haven't been updated for 8 years at least. There
will be points where it's lacking and we just have to deal with it and
figure out what the IP is actually doing.


   BTW, about MUSB docs. I have encountered the issue with the SendStall bit 
on more than one TI SoC (namely, DaVincis, OMAP-L1x, and AM35x). This bit 
seems to get clearead all by itself so that USB test 13 can never succed. Are 
you aware of this? AFAIR, I have tried to instrument this issue and couldn't 
detect a "rogue" register write that cleared the bit... This issue hadn't been 
documented anywhere (at least back then in 20008-2009)...


MBR, Sergei

--
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] Revert "usb: musb: musb_host: Enable HCD_BH flag to handle urb return in bottom half"

[Bin Liu]

Hi,

On 11/06/2015 11:56 AM, Felipe Balbi wrote:


Hi,

Bin Liu  writes:

This reverts commit 2035772010db634ec8566b658fb1cd87ec47ac77.

Commit 20357720 claims throughput improvement for MSC/UVC, but I
don't see much improvement. Following are the MSC measurement using
dd on AM335x GP EVM.

with BCD_BH:read: 14.9MB/s, write: 20.9MB/s
without BCD_BH: read: 15.2MB/s, write: 21.2MB/s

However with this commit the following regressions have been observed.

1. ASIX usb-ethernet dongle is completely broken on UDP RX.

2. Unpluging a 3G modem, which uses option driver, behind a hub causes
console log flooding with the following message.

option_instat_callback: error -71

Signed-off-by: Bin Liu 
---
  drivers/usb/musb/musb_host.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 0e29184..1c7f858 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2613,7 +2613,7 @@ static const struct hc_driver musb_hc_driver = {
.description= "musb-hcd",
.product_desc   = "MUSB HDRC host driver",
.hcd_priv_size  = sizeof(struct musb *),
-   .flags  = HCD_USB2 | HCD_MEMORY | HCD_BH,
+   .flags  = HCD_USB2 | HCD_MEMORY,


reverting will revert isochronous devices, IIRC. Can you check that
cameras still work ? That commit actually mentions that with this commit
we got 640x480@30fps working. Can you make sure it works without HCD_BH
as well ?


I don't see Isoch improvement with HCD_BH either.
All UVC cameras that I have do not work at YUV 640x480@30fps with or 
without HCD_BH, mainly due the 50% bandwidth utilization in Isoch 
transfer, which I talked to you months ago. The best I can get is YUV 
640x480@20fps, which does not change with or without HCD_BH.


Regards,
-Bin.
--
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 v4] usb: musb: fix tx fifo flush handling

[Bin Liu]

Here are a few changes in musb_h_tx_flush_fifo().

- It has been observed that sometimes (if not always) musb is unable
  to flush tx fifo during urb dequeue when disconnect a device. But
  it seems to be harmless, since the tx fifo flush is done again in
  musb_ep_program() when re-use the hw_ep.

  But the WARN() floods the console in the case when multiple tx urbs
  are queued, so change it to dev_WARN_ONCE().

- applications could queue up many tx urbs, then the 1ms delay could
  causes minutes of delay in device disconnect. So remove it to get
  better user experience. The 1ms delay does not help the flushing
  anyway.

- cleanup the debug code - related to lastcsr.


Note: The tx fifo flush issue has been observed during device disconnect
on AM335x.

To reproduce the issue, ensure tx urb(s) are queued when unplug the usb
device which is connected to AM335x usb host port.

I found using a usb-ethernet device and running iperf (client on AM335x)
has very high chance to trigger the problem.

Better to turn on dev_dbg() in musb_cleanup_urb() with CPPI enabled to
see the issue when aborting the tx channel.

Signed-off-by: Bin Liu 
---
 drivers/usb/musb/musb_host.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 883a9ad..7d890ec 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -112,22 +112,32 @@ static void musb_h_tx_flush_fifo(struct musb_hw_ep *ep)
struct musb *musb = ep->musb;
void __iomem*epio = ep->regs;
u16 csr;
-   u16 lastcsr = 0;
int retries = 1000;
 
csr = musb_readw(epio, MUSB_TXCSR);
while (csr & MUSB_TXCSR_FIFONOTEMPTY) {
-   if (csr != lastcsr)
-   dev_dbg(musb->controller, "Host TX FIFONOTEMPTY csr: 
%02x\n", csr);
-   lastcsr = csr;
csr |= MUSB_TXCSR_FLUSHFIFO | MUSB_TXCSR_TXPKTRDY;
musb_writew(epio, MUSB_TXCSR, csr);
csr = musb_readw(epio, MUSB_TXCSR);
-   if (WARN(retries-- < 1,
+   
+   /*
+* FIXME: sometimes the tx fifo flush failed, it has been
+* observed during device disconnect on AM335x.
+*
+* To reproduce the issue, ensure tx urb(s) are queued when
+* unplug the usb device which is connected to AM335x usb
+* host port.
+*
+* I found using a usb-ethernet device and running iperf
+* (client on AM335x) has very high chance to trigger it.
+*
+* Better to turn on dev_dbg() in musb_cleanup_urb() with
+* CPPI enabled to see the issue when aborting the tx channel.
+*/
+   if (dev_WARN_ONCE(musb->controller, retries-- < 1,
"Could not flush host TX%d fifo: csr: %04x\n",
ep->epnum, csr))
return;
-   mdelay(1);
}
 }
 
-- 
1.8.4

--
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] Revert "usb: musb: musb_host: Enable HCD_BH flag to handle urb return in bottom half"

[Felipe Balbi]

Hi,

Bin Liu  writes:
> This reverts commit 2035772010db634ec8566b658fb1cd87ec47ac77.
>
> Commit 20357720 claims throughput improvement for MSC/UVC, but I
> don't see much improvement. Following are the MSC measurement using
> dd on AM335x GP EVM.
>
> with BCD_BH:read: 14.9MB/s, write: 20.9MB/s
> without BCD_BH: read: 15.2MB/s, write: 21.2MB/s
>
> However with this commit the following regressions have been observed.
>
> 1. ASIX usb-ethernet dongle is completely broken on UDP RX.
>
> 2. Unpluging a 3G modem, which uses option driver, behind a hub causes
>console log flooding with the following message.
>
>option_instat_callback: error -71
>
> Signed-off-by: Bin Liu 
> ---
>  drivers/usb/musb/musb_host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index 0e29184..1c7f858 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -2613,7 +2613,7 @@ static const struct hc_driver musb_hc_driver = {
>   .description= "musb-hcd",
>   .product_desc   = "MUSB HDRC host driver",
>   .hcd_priv_size  = sizeof(struct musb *),
> - .flags  = HCD_USB2 | HCD_MEMORY | HCD_BH,
> + .flags  = HCD_USB2 | HCD_MEMORY,

reverting will revert isochronous devices, IIRC. Can you check that
cameras still work ? That commit actually mentions that with this commit
we got 640x480@30fps working. Can you make sure it works without HCD_BH
as well ?

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] USB: DCW3: GADGET: Set Correct Max Speed

[Felipe Balbi]

Hi,

"McCauley, Ben"  writes:
> Felipe,
>
>  -Original Message-
>  From: Felipe Balbi [mailto:ba...@ti.com]
>  Sent: Friday, November 06, 2015 10:06 AM
>  To: McCauley, Ben 
>  Cc: linux-usb@vger.kernel.org; Schroeder, Jay 
> Subject: Re: [PATCH] USB: DCW3: GADGET: Set Correct Max Speed
>>
>>
>> Hi,
>>
>> "McCauley, Ben"  writes:
>> > The max speed was always being set to USB_SUPER_SPEED even when the
>> > phy was only capable of HIGH_SPEED. This
>>
>> this statement is untrue
>>
>> > uses the value set for the phy to set the max for the gadget. It
>> > resolves an issue with Window PCs where they report that using a
>> > USB3.0 port would result in better performance even when connecting
>> > through a 3.0 port.
>>
>> man, who gave you this kernel ? Which kernel version are you using ?
>> Have you even tested mainline ?
>
> This Kernel is from 
> http://omapzoom.org/?p=kernel/omap.git;a=shortlog;h=refs/heads/p-ti-linux-3.14.y-common
> at branch  "development/omapzoom/glsdk-7.-2.00.02".
> I have not tested it on main line.

seems like you should be talking to your FAE or using TI's e2e forum for
this.

>> > Signed-off-by: McCauley, Ben 
>> > ---
>> >
>> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> > index 1eaf31c..0a388e3 100644
>> > --- a/drivers/usb/dwc3/gadget.c
>> > +++ b/drivers/usb/dwc3/gadget.c
>> > @@ -2899,7 +2899,7 @@
>>
>> which kernel are you using ? This file is 2868 lines, so you're patching 
>> outside
>> the file.
>>
>> > }
>> >
>> > dwc->gadget.ops = &dwc3_gadget_ops;
>> > -   dwc->gadget.max_speed   = USB_SPEED_SUPER;
>> > +   dwc->gadget.max_speed   = dwc->maximum_speed;
>>
>> this is corrected at dwc3_gadget_start():

and I see that in that kernel, this is corrected at
dwc3_gadget_restart() instead.

>>   /**
>>* WORKAROUND: DWC3 revision < 2.20a have an issue
>>* which would cause metastability state on Run/Stop
>>* bit if we try to force the IP to USB2-only mode.
>>*
>>* Because of that, we cannot configure the IP to any
>>* speed other than the SuperSpeed
>>*
>>* Refers to:
>>*
>>* STAR#9000525659: Clock Domain Crossing on DCTL in
>>* USB 2.0 Mode
>>*/
>>   if (dwc->revision < DWC3_REVISION_220A) {
>>   reg |= DWC3_DCFG_SUPERSPEED;
>>   } else {
>>   switch (dwc->maximum_speed) {
>>   case USB_SPEED_LOW:
>>   reg |= DWC3_DSTS_LOWSPEED;
>>   break;
>>   case USB_SPEED_FULL:
>>   reg |= DWC3_DSTS_FULLSPEED1;
>>   break;
>>   case USB_SPEED_HIGH:
>>   reg |= DWC3_DSTS_HIGHSPEED;
>>   break;
>>   case USB_SPEED_SUPER:   /* FALLTHROUGH */
>>   case USB_SPEED_UNKNOWN: /* FALTHROUGH */
>>   default:
>>   reg |= DWC3_DSTS_SUPERSPEED;
>>   }
>>   }
>>
>
> I am on 3.14 LTS though even on 4.3 I do not see dwc->gadget.max_speed
> being updated to reflect the actual max speed as set in
> drivers/usb/dwc3/core.c [dwc3_probe()]. dwc->gadget.max_speed is used
> in a number of locations to determine what response is sent to the
> host.
>
> dwc3_gadget_start() only appears to set the max speed to the register
> and not update the gadget.

but the IP _is_ super-speed capable. Max speed should only be used to
determine if $this gadget can run with $this controller; speed, instead,
is used for all sorts of different things and _that_ is updated on
Connection Done IRQ.

Can you point one place where you think it's wrong ? And, please, refer
to mainline code. We can't support a v3.14 kernel which might contain a
ton of additions which are not in mainline (for example, mainline
doesn't have a dwc3_gadget_restart() function).

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] USB: DCW3: GADGET: Set Correct Max Speed

[McCauley, Ben]

Felipe,

 -Original Message-
 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Friday, November 06, 2015 10:06 AM
 To: McCauley, Ben 
 Cc: linux-usb@vger.kernel.org; Schroeder, Jay 
Subject: Re: [PATCH] USB: DCW3: GADGET: Set Correct Max Speed
>
>
> Hi,
>
> "McCauley, Ben"  writes:
> > The max speed was always being set to USB_SUPER_SPEED even when the
> > phy was only capable of HIGH_SPEED. This
>
> this statement is untrue
>
> > uses the value set for the phy to set the max for the gadget. It
> > resolves an issue with Window PCs where they report that using a
> > USB3.0 port would result in better performance even when connecting
> > through a 3.0 port.
>
> man, who gave you this kernel ? Which kernel version are you using ?
> Have you even tested mainline ?

This Kernel is from 
http://omapzoom.org/?p=kernel/omap.git;a=shortlog;h=refs/heads/p-ti-linux-3.14.y-common
at branch  "development/omapzoom/glsdk-7.-2.00.02".
I have not tested it on main line.

>
> > Signed-off-by: McCauley, Ben 
> > ---
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 1eaf31c..0a388e3 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2899,7 +2899,7 @@
>
> which kernel are you using ? This file is 2868 lines, so you're patching 
> outside
> the file.
>
> > }
> >
> > dwc->gadget.ops = &dwc3_gadget_ops;
> > -   dwc->gadget.max_speed   = USB_SPEED_SUPER;
> > +   dwc->gadget.max_speed   = dwc->maximum_speed;
>
> this is corrected at dwc3_gadget_start():
>
>   /**
>* WORKAROUND: DWC3 revision < 2.20a have an issue
>* which would cause metastability state on Run/Stop
>* bit if we try to force the IP to USB2-only mode.
>*
>* Because of that, we cannot configure the IP to any
>* speed other than the SuperSpeed
>*
>* Refers to:
>*
>* STAR#9000525659: Clock Domain Crossing on DCTL in
>* USB 2.0 Mode
>*/
>   if (dwc->revision < DWC3_REVISION_220A) {
>   reg |= DWC3_DCFG_SUPERSPEED;
>   } else {
>   switch (dwc->maximum_speed) {
>   case USB_SPEED_LOW:
>   reg |= DWC3_DSTS_LOWSPEED;
>   break;
>   case USB_SPEED_FULL:
>   reg |= DWC3_DSTS_FULLSPEED1;
>   break;
>   case USB_SPEED_HIGH:
>   reg |= DWC3_DSTS_HIGHSPEED;
>   break;
>   case USB_SPEED_SUPER:   /* FALLTHROUGH */
>   case USB_SPEED_UNKNOWN: /* FALTHROUGH */
>   default:
>   reg |= DWC3_DSTS_SUPERSPEED;
>   }
>   }
>

I am on 3.14 LTS though even on 4.3 I do not see dwc->gadget.max_speed being 
updated to reflect the actual max speed as set in
drivers/usb/dwc3/core.c [dwc3_probe()]. dwc->gadget.max_speed is used in a 
number of locations to determine what
response is sent to the host.

dwc3_gadget_start() only appears to set the max speed to the register and not 
update the gadget.


> > CONFIDENTIALITY NOTICE: This email and any attachments are for the
> > sole use of the intended recipient(s) and contain information that may
> > be confidential and/or legally privileged. If you have received this
> > email in error, please notify the sender by reply email and delete the
> > message. Any disclosure, copying, distribution or use of this
> > communication (including attachments) by someone other than the
> > intended recipient is prohibited. Thank you.
>
> when sending emails to a public mailing list, you should make sure to drop
> these sort of notices. The content of the email *must* be public.
>

Thanks for pointing that out I will work to get it removed in the future.

> --
> balbi

-Ben



CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of 
the intended recipient(s) and contain information that may be confidential 
and/or legally privileged. If you have received this email in error, please 
notify the sender by reply email and delete the message. Any disclosure, 
copying, distribution or use of this communication (including attachments) by 
someone other than the intended recipient is prohibited. Thank you.
--
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] usb: musb: fix tx fifo flush handling

[Felipe Balbi]

Hi,

Bin Liu  writes:
> Hi,
>
> On 11/06/2015 11:11 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Bin Liu  writes:
>>> Here are a few changes in musb_h_tx_flush_fifo().
>>>
>>> - It has been observed that sometimes (if not always) musb is unable
>>>to flush tx fifo during urb dequeue when disconnect a device. But
>>>it seems to be harmless, since the tx fifo flush is done again in
>>>musb_ep_program() when re-use the hw_ep.
>>>
>>>But the WARN() floods the console in the case when multiple tx urbs
>>>are queued, so change it to dev_WARN_ONCE().
>>>
>>> - applications could queue up many tx urbs, then the 1ms delay could
>>>causes minutes of delay in device disconnect. So remove it to get
>>>better user experience. The 1ms delay does not help the flushing
>>>anyway.
>>>
>>> - cleanup the debug code - related to lastcsr.
>>>
>>> Signed-off-by: Bin Liu 
>>> ---
>>>   drivers/usb/musb/musb_host.c | 12 ++--
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>>> index 883a9ad..1c7f858 100644
>>> --- a/drivers/usb/musb/musb_host.c
>>> +++ b/drivers/usb/musb/musb_host.c
>>> @@ -112,22 +112,22 @@ static void musb_h_tx_flush_fifo(struct musb_hw_ep 
>>> *ep)
>>> struct musb *musb = ep->musb;
>>> void __iomem*epio = ep->regs;
>>> u16 csr;
>>> -   u16 lastcsr = 0;
>>> int retries = 1000;
>>>
>>> csr = musb_readw(epio, MUSB_TXCSR);
>>> while (csr & MUSB_TXCSR_FIFONOTEMPTY) {
>>
>> are you not going to check that TXPKTRDY is still set ? I guess that
>> could be another WARN(), actually, since it should never happen. But
>> maybe it's subject to a separate patch:
>
> Since the check does not help the flush issue, I would prefer leave it 
> for the future patch which does the proper fix.
>
>>
>>  dev_WARN_ONCE(dev, !(csr & MUSB_TXCSR_TXPKTRDY),
>>  "TxPktRdy should still be set!);
>
> I am not sure the WARN() here is right, it is not a error condition when 
> FIFONOTEMPTY is set but TXPKTRDY is not.

if TXPKTRDY has been cleared by MUSB, it means the packet has been
transferred and, thus, there's nothing to left to flush. So it is a race
indeed and I'd like to know if it ever happens.

>>> -   if (csr != lastcsr)
>>> -   dev_dbg(musb->controller, "Host TX FIFONOTEMPTY csr: 
>>> %02x\n", csr);
>>> -   lastcsr = csr;
>>> csr |= MUSB_TXCSR_FLUSHFIFO | MUSB_TXCSR_TXPKTRDY;
>>> musb_writew(epio, MUSB_TXCSR, csr);
>>> csr = musb_readw(epio, MUSB_TXCSR);
>>> -   if (WARN(retries-- < 1,
>>> +   
>>> +   /*
>>> +* FIXME: sometimes the tx fifo flush failed, it has been
>>> +* seeing during device disconnect.
>> seen ?
>>
>> also, you probably want to describe here how you reproduced
>> this. Something along the lines of:
>>
>>  Reproduced at least with AM335x Evaluation Module when an input
>>  device (using $device - VID:PID - here) is attached to its host
>>  port.
>>
>> That way, it's clear that the thing has really been reproduced and it
>> gives people instructions on how to reproduce.
>>
> Better put this explanation in the commit message area, not here
> in-line?

I'd say both. When I'm looking at the code, it's not always that I go
look at the commit log to figure out what happened for the function to
get to this state; sometimes I do, but not always.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3] usb: musb: fix tx fifo flush handling

[Bin Liu]

On 11/06/2015 11:29 AM, Bin Liu wrote:

Hi,

On 11/06/2015 11:11 AM, Felipe Balbi wrote:


Hi,

Bin Liu  writes:

Here are a few changes in musb_h_tx_flush_fifo().

- It has been observed that sometimes (if not always) musb is unable
   to flush tx fifo during urb dequeue when disconnect a device. But
   it seems to be harmless, since the tx fifo flush is done again in
   musb_ep_program() when re-use the hw_ep.

   But the WARN() floods the console in the case when multiple tx urbs
   are queued, so change it to dev_WARN_ONCE().

- applications could queue up many tx urbs, then the 1ms delay could
   causes minutes of delay in device disconnect. So remove it to get
   better user experience. The 1ms delay does not help the flushing
   anyway.

- cleanup the debug code - related to lastcsr.

Signed-off-by: Bin Liu 
---
  drivers/usb/musb/musb_host.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 883a9ad..1c7f858 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -112,22 +112,22 @@ static void musb_h_tx_flush_fifo(struct
musb_hw_ep *ep)
  struct musb*musb = ep->musb;
  void __iomem*epio = ep->regs;
  u16csr;
-u16lastcsr = 0;
  intretries = 1000;

  csr = musb_readw(epio, MUSB_TXCSR);
  while (csr & MUSB_TXCSR_FIFONOTEMPTY) {


are you not going to check that TXPKTRDY is still set ? I guess that
could be another WARN(), actually, since it should never happen. But
maybe it's subject to a separate patch:


Since the check does not help the flush issue, I would prefer leave it
for the future patch which does the proper fix.



dev_WARN_ONCE(dev, !(csr & MUSB_TXCSR_TXPKTRDY),
"TxPktRdy should still be set!);


I am not sure the WARN() here is right, it is not a error condition when
FIFONOTEMPTY is set but TXPKTRDY is not.




-if (csr != lastcsr)
-dev_dbg(musb->controller, "Host TX FIFONOTEMPTY csr:
%02x\n", csr);
-lastcsr = csr;
  csr |= MUSB_TXCSR_FLUSHFIFO | MUSB_TXCSR_TXPKTRDY;
  musb_writew(epio, MUSB_TXCSR, csr);
  csr = musb_readw(epio, MUSB_TXCSR);
-if (WARN(retries-- < 1,
+
+/*
+ * FIXME: sometimes the tx fifo flush failed, it has been
+ * seeing during device disconnect.

   seen ?

also, you probably want to describe here how you reproduced
this. Something along the lines of:

Reproduced at least with AM335x Evaluation Module when an input
device (using $device - VID:PID - here) is attached to its host
port.

That way, it's clear that the thing has really been reproduced and it
gives people instructions on how to reproduce.


Better put this explanation in the commit message area, not here in-line?


Never mind, I will put it here inline, where I think is the better place.



Regards,
-Bin.

--
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 scanner stops working with xhci_hcd URB transfer length is wrong, xHC issue? req. len = 0, act. len = 4294967288

[Felipe Balbi]

Hi,

Orion Poplawski  writes:
> See https://bugzilla.kernel.org/show_bug.cgi?id=107331
>
> Trying to use my scanner.  Worked for a while, but now access fails.  Get this
> in log:

okay, first things first. Which kernel version are you using ? Care to
capture a usbmon trace ? (see Documentation/usb/usbmon.txt for instructions)

> Nov 05 20:25:51 pacas.cora.nwra.com kernel: usb 1-3: new full-speed USB device
> number 3 using xhci_hcd
> Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: New USB device found,
> idVendor=04a9, idProduct=2206
> Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: New USB device strings:
> Mfr=64, Product=77, SerialNumber=0
> Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: Product: CanoScan
> Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: Manufacturer: Canon
> Nov 05 20:27:55 pacas.cora.nwra.com kernel: xhci_hcd :00:14.0: URB
> transfer length is wrong, xHC issue? req. len = 0, act. len = 4294967288

this looks like the length variable wrapped around ? that value is
0xfff8 which is -8 on 32-bits.

Are you, perhaps, using an older kernel which still has this issue with
wrapping which has been fixed before ?

> How reproducible:
> First time so far.
>
> Additional info:
> 00:14.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset Family
> USB xHCI Host Controller (rev 04) (prog-if 30 [XHCI])
> Subsystem: Acer Incorporated [ALI] Device 0748
> Flags: bus master, medium devsel, latency 0, IRQ 25
> Memory at c060 (64-bit, non-prefetchable) [size=64K]
> Capabilities: [70] Power Management version 2
> Capabilities: [80] MSI: Enable+ Count=1/8 Maskable- 64bit+
> Kernel driver in use: xhci_hcd
>
> Possibly related:
> https://www.mail-archive.com/linux-usb@vger.kernel.org/msg50230.html
>
> -- 
> Orion Poplawski
> Technical Manager 303-415-9701 x222
> NWRA, Boulder/CoRA Office FAX: 303-415-9702
> 3380 Mitchell Lane   or...@nwra.com
> Boulder, CO 80301   http://www.nwra.com
> --
> 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

-- 
balbi


signature.asc
Description: PGP signature

[PATCH] Revert "usb: musb: musb_host: Enable HCD_BH flag to handle urb return in bottom half"

[Bin Liu]

This reverts commit 2035772010db634ec8566b658fb1cd87ec47ac77.

Commit 20357720 claims throughput improvement for MSC/UVC, but I
don't see much improvement. Following are the MSC measurement using
dd on AM335x GP EVM.

with BCD_BH:read: 14.9MB/s, write: 20.9MB/s
without BCD_BH: read: 15.2MB/s, write: 21.2MB/s

However with this commit the following regressions have been observed.

1. ASIX usb-ethernet dongle is completely broken on UDP RX.

2. Unpluging a 3G modem, which uses option driver, behind a hub causes
   console log flooding with the following message.

   option_instat_callback: error -71

Signed-off-by: Bin Liu 
---
 drivers/usb/musb/musb_host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 0e29184..1c7f858 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2613,7 +2613,7 @@ static const struct hc_driver musb_hc_driver = {
.description= "musb-hcd",
.product_desc   = "MUSB HDRC host driver",
.hcd_priv_size  = sizeof(struct musb *),
-   .flags  = HCD_USB2 | HCD_MEMORY | HCD_BH,
+   .flags  = HCD_USB2 | HCD_MEMORY,
 
/* not using irq handler or reset hooks from usbcore, since
 * those must be shared with peripheral code for OTG configs
-- 
1.8.4

--
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] usb: musb: fix tx fifo flush handling

[Bin Liu]

Hi,

On 11/06/2015 11:11 AM, Felipe Balbi wrote:


Hi,

Bin Liu  writes:

Here are a few changes in musb_h_tx_flush_fifo().

- It has been observed that sometimes (if not always) musb is unable
   to flush tx fifo during urb dequeue when disconnect a device. But
   it seems to be harmless, since the tx fifo flush is done again in
   musb_ep_program() when re-use the hw_ep.

   But the WARN() floods the console in the case when multiple tx urbs
   are queued, so change it to dev_WARN_ONCE().

- applications could queue up many tx urbs, then the 1ms delay could
   causes minutes of delay in device disconnect. So remove it to get
   better user experience. The 1ms delay does not help the flushing
   anyway.

- cleanup the debug code - related to lastcsr.

Signed-off-by: Bin Liu 
---
  drivers/usb/musb/musb_host.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 883a9ad..1c7f858 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -112,22 +112,22 @@ static void musb_h_tx_flush_fifo(struct musb_hw_ep *ep)
struct musb *musb = ep->musb;
void __iomem*epio = ep->regs;
u16 csr;
-   u16 lastcsr = 0;
int retries = 1000;

csr = musb_readw(epio, MUSB_TXCSR);
while (csr & MUSB_TXCSR_FIFONOTEMPTY) {


are you not going to check that TXPKTRDY is still set ? I guess that
could be another WARN(), actually, since it should never happen. But
maybe it's subject to a separate patch:


Since the check does not help the flush issue, I would prefer leave it 
for the future patch which does the proper fix.




dev_WARN_ONCE(dev, !(csr & MUSB_TXCSR_TXPKTRDY),
"TxPktRdy should still be set!);


I am not sure the WARN() here is right, it is not a error condition when 
FIFONOTEMPTY is set but TXPKTRDY is not.





-   if (csr != lastcsr)
-   dev_dbg(musb->controller, "Host TX FIFONOTEMPTY csr: 
%02x\n", csr);
-   lastcsr = csr;
csr |= MUSB_TXCSR_FLUSHFIFO | MUSB_TXCSR_TXPKTRDY;
musb_writew(epio, MUSB_TXCSR, csr);
csr = musb_readw(epio, MUSB_TXCSR);
-   if (WARN(retries-- < 1,
+   
+   /*
+* FIXME: sometimes the tx fifo flush failed, it has been
+* seeing during device disconnect.

   seen ?

also, you probably want to describe here how you reproduced
this. Something along the lines of:

Reproduced at least with AM335x Evaluation Module when an input
device (using $device - VID:PID - here) is attached to its host
port.

That way, it's clear that the thing has really been reproduced and it
gives people instructions on how to reproduce.


Better put this explanation in the commit message area, not here in-line?

Regards,
-Bin.
--
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

USB scanner stops working with xhci_hcd URB transfer length is wrong, xHC issue? req. len = 0, act. len = 4294967288

[Orion Poplawski]

See https://bugzilla.kernel.org/show_bug.cgi?id=107331

Trying to use my scanner.  Worked for a while, but now access fails.  Get this
in log:

Nov 05 20:25:51 pacas.cora.nwra.com kernel: usb 1-3: new full-speed USB device
number 3 using xhci_hcd
Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: New USB device found,
idVendor=04a9, idProduct=2206
Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: New USB device strings:
Mfr=64, Product=77, SerialNumber=0
Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: Product: CanoScan
Nov 05 20:25:52 pacas.cora.nwra.com kernel: usb 1-3: Manufacturer: Canon
Nov 05 20:27:55 pacas.cora.nwra.com kernel: xhci_hcd :00:14.0: URB
transfer length is wrong, xHC issue? req. len = 0, act. len = 4294967288

How reproducible:
First time so far.

Additional info:
00:14.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset Family
USB xHCI Host Controller (rev 04) (prog-if 30 [XHCI])
Subsystem: Acer Incorporated [ALI] Device 0748
Flags: bus master, medium devsel, latency 0, IRQ 25
Memory at c060 (64-bit, non-prefetchable) [size=64K]
Capabilities: [70] Power Management version 2
Capabilities: [80] MSI: Enable+ Count=1/8 Maskable- 64bit+
Kernel driver in use: xhci_hcd

Possibly related:
https://www.mail-archive.com/linux-usb@vger.kernel.org/msg50230.html

-- 
Orion Poplawski
Technical Manager 303-415-9701 x222
NWRA, Boulder/CoRA Office FAX: 303-415-9702
3380 Mitchell Lane   or...@nwra.com
Boulder, CO 80301   http://www.nwra.com
--
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] usb: musb: fix tx fifo flush handling

[Felipe Balbi]

Hi,

Bin Liu  writes:
> Here are a few changes in musb_h_tx_flush_fifo().
>
> - It has been observed that sometimes (if not always) musb is unable
>   to flush tx fifo during urb dequeue when disconnect a device. But
>   it seems to be harmless, since the tx fifo flush is done again in
>   musb_ep_program() when re-use the hw_ep.
>
>   But the WARN() floods the console in the case when multiple tx urbs
>   are queued, so change it to dev_WARN_ONCE().
>
> - applications could queue up many tx urbs, then the 1ms delay could
>   causes minutes of delay in device disconnect. So remove it to get
>   better user experience. The 1ms delay does not help the flushing
>   anyway.
>
> - cleanup the debug code - related to lastcsr.
>
> Signed-off-by: Bin Liu 
> ---
>  drivers/usb/musb/musb_host.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index 883a9ad..1c7f858 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -112,22 +112,22 @@ static void musb_h_tx_flush_fifo(struct musb_hw_ep *ep)
>   struct musb *musb = ep->musb;
>   void __iomem*epio = ep->regs;
>   u16 csr;
> - u16 lastcsr = 0;
>   int retries = 1000;
>  
>   csr = musb_readw(epio, MUSB_TXCSR);
>   while (csr & MUSB_TXCSR_FIFONOTEMPTY) {

are you not going to check that TXPKTRDY is still set ? I guess that
could be another WARN(), actually, since it should never happen. But
maybe it's subject to a separate patch:

dev_WARN_ONCE(dev, !(csr & MUSB_TXCSR_TXPKTRDY),
"TxPktRdy should still be set!);

> - if (csr != lastcsr)
> - dev_dbg(musb->controller, "Host TX FIFONOTEMPTY csr: 
> %02x\n", csr);
> - lastcsr = csr;
>   csr |= MUSB_TXCSR_FLUSHFIFO | MUSB_TXCSR_TXPKTRDY;
>   musb_writew(epio, MUSB_TXCSR, csr);
>   csr = musb_readw(epio, MUSB_TXCSR);
> - if (WARN(retries-- < 1,
> + 
> + /*
> +  * FIXME: sometimes the tx fifo flush failed, it has been
> +  * seeing during device disconnect.
   seen ?

also, you probably want to describe here how you reproduced
this. Something along the lines of:

Reproduced at least with AM335x Evaluation Module when an input
device (using $device - VID:PID - here) is attached to its host
port.

That way, it's clear that the thing has really been reproduced and it
gives people instructions on how to reproduce.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v5 1/5] gadget: Introduce the notifier functions

[Greg KH]

On Fri, Nov 06, 2015 at 07:35:10PM +0800, Baolin Wang wrote:
> The usb charger framework is based on usb gadget. The usb charger
> need to be notified the state changing of usb gadget to confirm the
> usb charger state.
> 
> Thus this patch adds a notifier mechanism for usb gadget to report a
> event to usb charger when the usb gadget state is changed.

I thought we said we did not want another notifier chain in the previous
versions of this patch?

> 
> Signed-off-by: Baolin Wang 
> ---
>  drivers/usb/gadget/udc/udc-core.c |   32 
>  include/linux/usb/gadget.h|   18 ++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c 
> b/drivers/usb/gadget/udc/udc-core.c
> index f660afb..4238fc3 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
>  }
>  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
>  
> +int usb_gadget_register_notify(struct usb_gadget *gadget,
> +struct notifier_block *nb)
> +{
> + int ret;
> +
> + mutex_lock(&gadget->lock);
> + ret = raw_notifier_chain_register(&gadget->nh, nb);
> + mutex_unlock(&gadget->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
> +
> +int usb_gadget_unregister_notify(struct usb_gadget *gadget,
> +  struct notifier_block *nb)
> +{
> + int ret;
> +
> + mutex_lock(&gadget->lock);
> + ret = raw_notifier_chain_unregister(&gadget->nh, nb);
> + mutex_unlock(&gadget->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_unregister_notify);
> +
>  /* - 
> */
>  
>  /**
> @@ -226,6 +252,10 @@ static void usb_gadget_state_work(struct work_struct 
> *work)
>   struct usb_gadget *gadget = work_to_gadget(work);
>   struct usb_udc *udc = gadget->udc;
>  
> + mutex_lock(&gadget->lock);
> + raw_notifier_call_chain(&gadget->nh, gadget->state, gadget);
> + mutex_unlock(&gadget->lock);
> +
>   if (udc)
>   sysfs_notify(&udc->dev.kobj, NULL, "state");
>  }
> @@ -364,6 +394,8 @@ int usb_add_gadget_udc_release(struct device *parent, 
> struct usb_gadget *gadget,
>  
>   dev_set_name(&gadget->dev, "gadget");
>   INIT_WORK(&gadget->work, usb_gadget_state_work);
> + RAW_INIT_NOTIFIER_HEAD(&gadget->nh);
> + mutex_init(&gadget->lock);
>   gadget->dev.parent = parent;
>  
>  #ifdef   CONFIG_HAS_DMA
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index c14a69b..755e8bc 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -609,6 +609,8 @@ struct usb_gadget {
>   unsignedout_epnum;
>   unsignedin_epnum;
>   struct usb_otg_caps *otg_caps;
> + struct raw_notifier_headnh;
> + struct mutexlock;

You have to document what this lock protects.


>  
>   unsignedsg_supported:1;
>   unsignedis_otg:1;
> @@ -1183,6 +1185,22 @@ extern void usb_gadget_unmap_request(struct usb_gadget 
> *gadget,
>  
>  /*-*/
>  
> +/**
> + * Register a notifiee to get notified by any attach status changes from
> + * the usb gadget
> + */

kerneldoc does not belong in a .h file.

And the kbuild system found lots of problems with this series, please
fix those at the very least :(

thanks,

greg k-h
--
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 v3] usb: musb: fix tx fifo flush handling

[Bin Liu]

Here are a few changes in musb_h_tx_flush_fifo().

- It has been observed that sometimes (if not always) musb is unable
  to flush tx fifo during urb dequeue when disconnect a device. But
  it seems to be harmless, since the tx fifo flush is done again in
  musb_ep_program() when re-use the hw_ep.

  But the WARN() floods the console in the case when multiple tx urbs
  are queued, so change it to dev_WARN_ONCE().

- applications could queue up many tx urbs, then the 1ms delay could
  causes minutes of delay in device disconnect. So remove it to get
  better user experience. The 1ms delay does not help the flushing
  anyway.

- cleanup the debug code - related to lastcsr.

Signed-off-by: Bin Liu 
---
 drivers/usb/musb/musb_host.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 883a9ad..1c7f858 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -112,22 +112,22 @@ static void musb_h_tx_flush_fifo(struct musb_hw_ep *ep)
struct musb *musb = ep->musb;
void __iomem*epio = ep->regs;
u16 csr;
-   u16 lastcsr = 0;
int retries = 1000;
 
csr = musb_readw(epio, MUSB_TXCSR);
while (csr & MUSB_TXCSR_FIFONOTEMPTY) {
-   if (csr != lastcsr)
-   dev_dbg(musb->controller, "Host TX FIFONOTEMPTY csr: 
%02x\n", csr);
-   lastcsr = csr;
csr |= MUSB_TXCSR_FLUSHFIFO | MUSB_TXCSR_TXPKTRDY;
musb_writew(epio, MUSB_TXCSR, csr);
csr = musb_readw(epio, MUSB_TXCSR);
-   if (WARN(retries-- < 1,
+   
+   /*
+* FIXME: sometimes the tx fifo flush failed, it has been
+* seeing during device disconnect.
+*/
+   if (dev_WARN_ONCE(musb->controller, retries-- < 1,
"Could not flush host TX%d fifo: csr: %04x\n",
ep->epnum, csr))
return;
-   mdelay(1);
}
 }
 
-- 
1.8.4

--
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: musb: fix tx fifo flush handling

[Felipe Balbi]

Hi,

Bin Liu  writes:
> I would also add a very verbose and descriptive comment on that
> particular location so we never forget about these MUSB oddities next
> time someone's looking at this.

 Also, leave the WARN() alone. We don't want FIFOFLUSH to ever fail. If
 it does, we're doing it wrong.

>>>
>>> Well, I have tried the changes you mentioned above, but the WARN() was
>>> still triggered for dequeue all the tx urbs while disconnect the
>>> device.
>>
>> then we need to figure out why isn't the patcket being flushed. Are you
>> using double buffering ? Is a packet being loaded in the FIFO right now?
>> Does it change if you set *only* FLUSHFIFO but *not* TXKPKTRDY ?
>
> Right now here have 3 problems:
>
> 1. tx fifo flush failed during device disconnect, it is harmless though;
> 2. WARN() causes console log flooding in the case which multi (100+) tx 
> urbs queued;
> 3. the 1ms delay causes disconnect delay in the case which multi tx urbs 
> queued;
>
> We know if #1 was solved, #2 & #3 would not happen, but the handling of 
> #2 & #3 in the driver is not correct.
>
> How about to address the #2 & #3 first, and leave #1 for another patch 
> since it would take some effort to find the root cause?
>
> To solve #2 & #3,
>
> 1. change WARN() to dev_err() so that kernel log by default still
> shows

not dev_err(), try dev_WARN_ONCE(). I still want the verbosity of a
WARN(), but we can live with only one WARN().

> the issue but does not flood the console;
> 2. add inline comment that #1 needs revisit;
> 3. remove 1ms delay.

yeah, 1ms does sound like a lot.

> Please let me know if this is acceptable then I will send v3.

if you use dev_WARN_ONCE() I can accept. sure.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Bin Liu]

Hi,

On 11/05/2015 03:44 PM, Felipe Balbi wrote:


Hi,

Bin Liu  writes:

Hi,

On 11/05/2015 03:07 PM, Felipe Balbi wrote:


Hi again,

Felipe Balbi  writes:

Hi,

Sergei Shtylyov  writes:

On 11/05/2015 11:34 PM, Bin Liu wrote:


Here are a few changes in musb_h_tx_flush_fifo().

- Refering to 2ccc6d30a (usb: musb: fix bit mask for CSR in
 musb_h_tx_flush_fifo()), the datasheet says that
MUSB_TXCSR_FLUSHFIFO
 is only valid when MUSB_TXCSR_TXPKTRDY is set as well. It means
 MUSB_TXCSR_TXPKTRDY should be checked, not set.


  Hum, my copy of the MUSBHDRC  programmer's guide says about
TXCSR.FlushFIFO in host mode:

<<
The CPU writes a 1 to this bit to flush the latest packet from the
endpoint Tx FIFO. The FIFO pointer is reset, the TxPktRdy bit (below) is
cleared and an interrupt is generated. May be set simultaneously with
TxPktRdy to abort the packet that is currently being loaded into the
FIFO. Note: FlushFIFO should only be used when TxPktRdy is set. At other
times, it may cause data to be corrupted. Also note that, if the FIFO is
double-buffered, FlushFIFO may need to be set twice to completely clear
the FIFO.
   >>


So how to interpret this message? FLUSHFIFO and TXPKTRDY should be set
at the
same time? If so, I will drop this change in V3.


  Yes, I think it's rather clear in this respect.


In either case, musb is unable to flush the tx fifo in urb dequque for
device
disconnect, but harmless, so the next two changes are important to
give better
user experience.


  Hm, looks like some errata... and hiding this from users while
there's no workaround doesn't seem a good policy.


Well, based on the programmer's guide, the driver should set the flushFIFO
bit, and wait for the interrupt.


  That's oversimplified, consider the double-buffered FIFO. ;-)


Seems like we have some misinterpretation here and IMO Bin's patch is
almost complete. The way I read MUSB's docs, it tells me three different
things:

1. FIFONOTEMPTY is valid *only* when TXPKTRDY is also set

Bin's new while condition solves this part:

+ while ((csr & MUSB_TXCSR_FIFONOTEMPTY) && (csr & MUSB_TXCSR_TXPKTRDY)) {

2. If we have Double Buffering, we need to set MUSB_TXCSR_FLUSHFIFO
twice.

This does not seem to be taken care of, but it would be
something below:

csr |= MUSB_TXCSR_FLUSH_FIFO;
musb_writew(epio, MUSB_TXCSR, csr);

if (ep->double_buffering_enabled)
musb_write(epio, MUSB_TXCSR, csr);

3. We can set TXPKTRDY *together* with MUSB_TXCSR_FLUSH_FIFO to cancel
the packet which is being loaded in the fifo right now.

This seems to be a regression with current patch, however I
don't think current code is perfect either. It unconditionally
sets FLUSH_FIFO and TXPKTRDY without caring for the consequences
of doing that. What happens if we set both of them but there's
no packet being currently loaded into the FIFO ?

To minimize changes, I think Bin just needs to keep the original csr
unconditionally setting both FLUSHFIFO and TXPKTRDY alone while also
making sure to check that TKPKTRDY is set when checking FIFONOTEMPTY.



I am not sure this is any different, since TXPKTRDY is checked, setting
it again along with FLUSHFIFO does not change TXPKTRDY bit, isn't it?


you're missing one detail:

If you set only FLUSHFIFO, TXPKTRDY is cleared and the packet *already*
in the FIFO gets flushed.

If you set them both together, you're telling MUSB that you want to
flush a packet which is *still* being loaded into the FIFO. After that
TXPKTRDY will be cleared by MUSB.


I would also add a very verbose and descriptive comment on that
particular location so we never forget about these MUSB oddities next
time someone's looking at this.


Also, leave the WARN() alone. We don't want FIFOFLUSH to ever fail. If
it does, we're doing it wrong.



Well, I have tried the changes you mentioned above, but the WARN() was
still triggered for dequeue all the tx urbs while disconnect the
device.


then we need to figure out why isn't the patcket being flushed. Are you
using double buffering ? Is a packet being loaded in the FIFO right now?
Does it change if you set *only* FLUSHFIFO but *not* TXKPKTRDY ?


Right now here have 3 problems:

1. tx fifo flush failed during device disconnect, it is harmless though;
2. WARN() causes console log flooding in the case which multi (100+) tx 
urbs queued;
3. the 1ms delay causes disconnect delay in the case which multi tx urbs 
queued;


We know if #1 was solved, #2 & #3 would not happen, but the handling of 
#2 & #3 in the driver is not correct.


How about to address the #2 & #3 first, and leave #1 for another patch 
since it would take some effort to find the root cause?


To solve #2 & #3,

1. change WARN() to dev_err() so that kernel log by default still shows 
the issue but does not flood the console;

2. add inline comment that #1 needs revisit;
3. remove 1ms delay.

Please let me know if this is acceptable then I

[PATCH] USB: DCW3: GADGET: Set Correct Max Speed

[McCauley, Ben]

The max speed was always being set to USB_SUPER_SPEED
even when the phy was only capable of HIGH_SPEED. This
uses the value set for the phy to set the max for the
gadget. It resolves an issue with Window PCs where they
report that using a USB3.0 port would result in better
performance even when connecting through a 3.0 port.

Signed-off-by: McCauley, Ben 
---

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1eaf31c..0a388e3 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2899,7 +2899,7 @@
}

dwc->gadget.ops = &dwc3_gadget_ops;
-   dwc->gadget.max_speed   = USB_SPEED_SUPER;
+   dwc->gadget.max_speed   = dwc->maximum_speed;
dwc->gadget.speed   = USB_SPEED_UNKNOWN;
dwc->gadget.sg_supported= true;
dwc->gadget.name= "dwc3-gadget";



CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of 
the intended recipient(s) and contain information that may be confidential 
and/or legally privileged. If you have received this email in error, please 
notify the sender by reply email and delete the message. Any disclosure, 
copying, distribution or use of this communication (including attachments) by 
someone other than the intended recipient is prohibited. Thank you.
--
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] USB: DCW3: GADGET: Set Correct Max Speed

[Felipe Balbi]

Hi,

"McCauley, Ben"  writes:
> The max speed was always being set to USB_SUPER_SPEED
> even when the phy was only capable of HIGH_SPEED. This

this statement is untrue

> uses the value set for the phy to set the max for the
> gadget. It resolves an issue with Window PCs where they
> report that using a USB3.0 port would result in better
> performance even when connecting through a 3.0 port.

man, who gave you this kernel ? Which kernel version are you using ?
Have you even tested mainline ?

> Signed-off-by: McCauley, Ben 
> ---
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 1eaf31c..0a388e3 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2899,7 +2899,7 @@

which kernel are you using ? This file is 2868 lines, so you're patching
outside the file.

> }
>
> dwc->gadget.ops = &dwc3_gadget_ops;
> -   dwc->gadget.max_speed   = USB_SPEED_SUPER;
> +   dwc->gadget.max_speed   = dwc->maximum_speed;

this is corrected at dwc3_gadget_start():

/**
 * WORKAROUND: DWC3 revision < 2.20a have an issue
 * which would cause metastability state on Run/Stop
 * bit if we try to force the IP to USB2-only mode.
 *
 * Because of that, we cannot configure the IP to any
 * speed other than the SuperSpeed
 *
 * Refers to:
 *
 * STAR#9000525659: Clock Domain Crossing on DCTL in
 * USB 2.0 Mode
 */
if (dwc->revision < DWC3_REVISION_220A) {
reg |= DWC3_DCFG_SUPERSPEED;
} else {
switch (dwc->maximum_speed) {
case USB_SPEED_LOW:
reg |= DWC3_DSTS_LOWSPEED;
break;
case USB_SPEED_FULL:
reg |= DWC3_DSTS_FULLSPEED1;
break;
case USB_SPEED_HIGH:
reg |= DWC3_DSTS_HIGHSPEED;
break;
case USB_SPEED_SUPER:   /* FALLTHROUGH */
case USB_SPEED_UNKNOWN: /* FALTHROUGH */
default:
reg |= DWC3_DSTS_SUPERSPEED;
}
}

> CONFIDENTIALITY NOTICE: This email and any attachments are for the
> sole use of the intended recipient(s) and contain information that may
> be confidential and/or legally privileged. If you have received this
> email in error, please notify the sender by reply email and delete the
> message. Any disclosure, copying, distribution or use of this
> communication (including attachments) by someone other than the
> intended recipient is prohibited. Thank you.

when sending emails to a public mailing list, you should make sure to
drop these sort of notices. The content of the email *must* be public.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/2] usb: host: ehci-msm: Use posted data writes on AHB

[Alan Stern]

On Fri, 6 Nov 2015, Georgi Djakov wrote:

> On 11/06/2015 08:04 AM, Andy Gross wrote:
> > This patch sets the AHBMODE to allow for posted data writes.  This
> > results in higher performance.
> > 
> > Signed-off-by: Andy Gross 
> 
> With these patches I see significant improvement in throughput
> on my db410c board.
> 
> Tested-by: Georgi Djakov 
> 
> > ---
> >  drivers/usb/host/ehci-msm.c |4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
> > index c4f84c8..c23e285 100644
> > --- a/drivers/usb/host/ehci-msm.c
> > +++ b/drivers/usb/host/ehci-msm.c
> > @@ -57,8 +57,8 @@ static int ehci_msm_reset(struct usb_hcd *hcd)
> >  
> > /* bursts of unspecified length. */
> > writel(0, USB_AHBBURST);
> > -   /* Use the AHB transactor */
> > -   writel(0, USB_AHBMODE);
> > +   /* Use the AHB transactor, allow posted data writes */
> > +   writel(0x8, USB_AHBMODE);
> > /* Disable streaming mode and select host mode */
> > writel(0x13, USB_USBMODE);

Acked-by: 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 2/3] usb: dwc2: host: Giveback URB in tasklet context

[Alan Stern]

On Thu, 5 Nov 2015, Doug Anderson wrote:

> Alan,
> 
> On Thu, Nov 5, 2015 at 7:19 AM, Alan Stern  wrote:
> > On Wed, 4 Nov 2015, Doug Anderson wrote:
> >
> >> In the ChromeOS gerrit
> >>  Julius Werner
> >> points out that for EHCI it was good to take the optimization from
> >> commit 9118f9eb4f1e ("USB: EHCI: improve interrupt qh unlink") before
> >> this one.  I'm still trying to learn USB / dwc2 so it's unclear to me
> >> whether we also need a similar change before landing.
> >>
> >> I'll see if I can do some investigation about this and also some
> >> benchmarking before and after.  Certainly profiling the interrupt
> >> handler itself showed a huge improvement, but I'd hate to see a
> >> regression elsewhere.
> >>
> >> If anyone else knows better than I, please speak up!  :)
> >
> > This is a matter of both efficiency and correctness.  Giving back URBs
> > in a tasklet is not a simple change.
> >
> > Have you read the kerneldoc for usb_submit_urb() in
> > drivers/usb/core/urb.c?  The portion about "Reserved Bandwidth
> > Transfers" is highly relevant.  I don't know how dwc2 goes about
> > reserving bandwidth for periodic transfers, but if it relies on the
> > endpoint queue being non-empty to maintain a reservation then it will
> > be affected by this change.
> 
> It does look as if you are right and the reservation will end up being
> released.  It looks to me like dwc2_deschedule_periodic() is in charge
> of releasing the reservation.  I'll work on trying to actually confirm
> this.  I guess I need to find a USB test setup where there are enough
> devices that I exceed the available time so I can see the brokenness
> of my old solution...

You may not need that.  Try a single USB keyboard and see what happens
when the interrupt URB is given back.

> I hadn't realized that this was a correctness problem and not just an
> optimization problem, so thank you very much for the info!  :)  I ran
> with a bunch of USB devices and it worked fine (and performance
> improved!) so I figured I was good to go...  Now I've read the
> kerneldoc you pointed at and it was very helpful.  As I understand it,
> it's considered OK if I copy what EHCI did and release the reservation
> if nothing has been scheduled for 5 ms.

You might also look into the issues surrounding isochronous URBs.  In 
particular, when an URB is submitted, which frames or microframes 
should it be scheduled in?  The problem is that when the submission 
occurs, some of the slots following the previous URB may already have 
expired.  The explanations for EXDEV and EFBIG in 
Documentation/usb/error-codes.txt are relevant here, although terse.

The host controller drivers that I maintain work like this:

If the endpoint's queue is empty and none of the endpoint's
URBs are still being given back by the tasklet, pretend that
the URB_ISO_ASAP flag is set.  Note that this involves
testing hcd_periodic_completion_in_progress() -- that's
where switching over to tasklets makes a difference.

If the URB_ISO_ASAP flag is set, the URB is scheduled for
the first unallocated slot that hasn't already expired.

If the flag isn't set, try to schedule the URB for the first
unallocated slot.  If that means all the isoc packets in the
URB would be assigned to expired slots, return -EXDEV.  If
some but not all of the packets would be assigned to expired
slots, skip them -- only schedule the packets whose slots
haven't expired yet.

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] Revert "usb: dwc3: gadget: drop unnecessary loop when cleaning up TRBs"

[Felipe Balbi]

Hi,

Heikki Krogerus  writes:
> On Mon, Oct 12, 2015 at 01:37:40PM -0500, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Felipe Balbi  writes:
>> > On Wed, Sep 02, 2015 at 05:09:39PM +0900, Masakazu Mokuno wrote:
>> >> Hi,
>> >> 
>> >> On Mon, 31 Aug 2015 11:54:13 -0500
>> >> Felipe Balbi  wrote:
>> >> 
>> >> > On Mon, Aug 31, 2015 at 07:48:28PM +0300, ville.syrj...@linux.intel.com 
>> >> > wrote:
>> >> > > From: Ville Syrj舁・
>> >> > > 
>> >> > > This reverts commit 8f2c9544aba636134303105ecb164190a39dece4.
>> >> > > 
>> >> > > As it breaks g_ether on my Baytrail FFRD8 device. Everything starts 
>> >> > > out
>> >> > > fine, but after a bit of data has been transferred it just stops
>> >> > > flowing.
>> >> > > 
>> >> > > Note that I do get a bunch of these "NOHZ: local_softirq_pending 08"
>> >> > > when booting the machine, but I'm not really sure if they're related
>> >> > > to this problem.
>> >> > 
>> >> > I have a feeling your problem is elsewhere. We *are* completing one TRB
>> >> > at a time. 
>> >> 
>> >> If usb_request.no_interrupt is flagged, it seems dwc3 does not set IOC
>> >> on the corresponding TRB.  Does it break the assumption every TRB
>> >> (without SG) will trigger one corresponding EP event?
>> >> u_ether is the function module that utilizes 'no_interrupt' flag.
>> >
>> > XferInProgress should still trigger. Besides, I tested with the exact
>> > same setup (different SoC though), just look at the thread.
>> 
>> I found a way to reproduce this on my end. What I was missing was the
>> use of request.no_interrupt. We won't get XferInProgress for all TRBs if
>> IOC isn't set in all of them.
>> 
>> I'll apply this patch ASAP as it fixes the problem I managed to
>> reproduce (ping -s 4 makes it fail here)
>
> I can see the commit in your next branch, but shouldn't it go in as a
> fix? I guess it should also be tagged with the stable tag.
>
> I got an other guy who hit this issue who I think is using the stable
> tree.

it was kinda late to merge it during the -rc.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v5 3/5] gadget: Support for the usb charger framework

[kbuild test robot]

Hi Baolin,

[auto build test WARNING on v4.3-rc7]
[also build test WARNING on next-20151106]

url:
https://github.com/0day-ci/linux/commits/Baolin-Wang/Introduce-usb-charger-framework-to-deal-with-the-usb-gadget-power-negotation/20151106-194008
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/linux/usb/gadget.h:227: warning: No description found for parameter 
'claimed'
   include/linux/usb/gadget.h:631: warning: No description found for parameter 
'nh'
   include/linux/usb/gadget.h:631: warning: No description found for parameter 
'lock'
>> include/linux/usb/gadget.h:631: warning: No description found for parameter 
>> 'charger'
   include/linux/usb/gadget.h:631: warning: No description found for parameter 
'quirk_altset_not_supp'
   include/linux/usb/gadget.h:631: warning: No description found for parameter 
'quirk_stall_not_supp'
   include/linux/usb/gadget.h:631: warning: No description found for parameter 
'quirk_zlp_not_supp'
   include/linux/usb/gadget.h:1202: warning: No description found for parameter 
'gadget'
   include/linux/usb/gadget.h:1202: warning: No description found for parameter 
'nb'
   include/linux/usb/composite.h:501: warning: Excess struct/union/enum/typedef 
member 'setup_pending' description in 'usb_composite_dev'
   include/linux/usb/composite.h:501: warning: Excess struct/union/enum/typedef 
member 'os_desc_pending' description in 'usb_composite_dev'
   drivers/usb/gadget/function/f_acm.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_ecm.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_subset.c:1: warning: no structured comments 
found
   drivers/usb/gadget/function/f_obex.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_serial.c:1: warning: no structured comments 
found

vim +/charger +631 include/linux/usb/gadget.h

a64cbb7e92 include/linux/usb/gadget.h Baolin Wang 2015-11-06  615   struct 
mutexlock;
a8d202b50d include/linux/usb/gadget.h Baolin Wang 2015-11-06  616   struct 
usb_charger  *charger;
d8318d7f6b include/linux/usb/gadget.h David Cohen 2013-12-09  617  
898c608678 include/linux/usb/gadget.h Felipe Balbi2011-11-22  618   
unsignedsg_supported:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  619   
unsignedis_otg:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  620   
unsignedis_a_peripheral:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  621   
unsignedb_hnp_enable:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  622   
unsigneda_hnp_support:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  623   
unsigneda_alt_hnp_support:1;
0b2d2bbade include/linux/usb/gadget.h David Cohen 2013-12-09  624   
unsignedquirk_ep_out_aligned_size:1;
ffd9a0fcbb include/linux/usb/gadget.h Robert Baldyga  2015-07-28  625   
unsignedquirk_altset_not_supp:1;
02ded1b0d8 include/linux/usb/gadget.h Robert Baldyga  2015-07-28  626   
unsignedquirk_stall_not_supp:1;
ca1023c81d include/linux/usb/gadget.h Robert Baldyga  2015-07-28  627   
unsignedquirk_zlp_not_supp:1;
80b2502cea include/linux/usb/gadget.h Peter Chen  2015-01-28  628   
unsignedis_selfpowered:1;
ccdf138fe3 include/linux/usb/gadget.h Robert Baldyga  2015-05-04  629   
unsigneddeactivated:1;
ccdf138fe3 include/linux/usb/gadget.h Robert Baldyga  2015-05-04  630   
unsignedconnected:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16 @631  };
5702f75375 include/linux/usb/gadget.h Felipe Balbi2013-07-17  632  #define 
work_to_gadget(w)(container_of((w), struct usb_gadget, work))
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  633  
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  634  static 
inline void set_gadget_data(struct usb_gadget *gadget, void *data)
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  635   { 
dev_set_drvdata(&gadget->dev, data); }
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  636  static 
inline void *get_gadget_data(struct usb_gadget *gadget)
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  637   { 
return dev_get_drvdata(&gadget->dev); }
f48cf80f93 include/linux/usb/gadget.h Fabien Chouteau 2010-04-23  638  static 
inline struct usb_gadget *dev_to_usb_gadget(struct device *dev)
f48cf80f93 include/linux/usb/gadget.h Fabien Chouteau 2010-04-23  639  {

::

Re: [PATCH] Revert "usb: dwc3: gadget: drop unnecessary loop when cleaning up TRBs"

[Heikki Krogerus]

On Fri, Nov 06, 2015 at 02:48:00PM +0200, Heikki Krogerus wrote:
> On Mon, Oct 12, 2015 at 01:37:40PM -0500, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > Felipe Balbi  writes:
> > > On Wed, Sep 02, 2015 at 05:09:39PM +0900, Masakazu Mokuno wrote:
> > >> Hi,
> > >> 
> > >> On Mon, 31 Aug 2015 11:54:13 -0500
> > >> Felipe Balbi  wrote:
> > >> 
> > >> > On Mon, Aug 31, 2015 at 07:48:28PM +0300, 
> > >> > ville.syrj...@linux.intel.com wrote:
> > >> > > From: Ville Syrj舁・
> > >> > > 
> > >> > > This reverts commit 8f2c9544aba636134303105ecb164190a39dece4.
> > >> > > 
> > >> > > As it breaks g_ether on my Baytrail FFRD8 device. Everything starts 
> > >> > > out
> > >> > > fine, but after a bit of data has been transferred it just stops
> > >> > > flowing.
> > >> > > 
> > >> > > Note that I do get a bunch of these "NOHZ: local_softirq_pending 08"
> > >> > > when booting the machine, but I'm not really sure if they're related
> > >> > > to this problem.
> > >> > 
> > >> > I have a feeling your problem is elsewhere. We *are* completing one TRB
> > >> > at a time. 
> > >> 
> > >> If usb_request.no_interrupt is flagged, it seems dwc3 does not set IOC
> > >> on the corresponding TRB.  Does it break the assumption every TRB
> > >> (without SG) will trigger one corresponding EP event?
> > >> u_ether is the function module that utilizes 'no_interrupt' flag.
> > >
> > > XferInProgress should still trigger. Besides, I tested with the exact
> > > same setup (different SoC though), just look at the thread.
> > 
> > I found a way to reproduce this on my end. What I was missing was the
> > use of request.no_interrupt. We won't get XferInProgress for all TRBs if
> > IOC isn't set in all of them.
> > 
> > I'll apply this patch ASAP as it fixes the problem I managed to
> > reproduce (ping -s 4 makes it fail here)
> 
> I can see the commit in your next branch, but shouldn't it go in as a
> fix? I guess it should also be tagged with the stable tag.

It does have the "Cc: sta...@vger.kernel.org". Sorry about the noise
:-).


Thanks,

-- 
heikki
--
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] Revert "usb: dwc3: gadget: drop unnecessary loop when cleaning up TRBs"

[Heikki Krogerus]

On Mon, Oct 12, 2015 at 01:37:40PM -0500, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Balbi  writes:
> > On Wed, Sep 02, 2015 at 05:09:39PM +0900, Masakazu Mokuno wrote:
> >> Hi,
> >> 
> >> On Mon, 31 Aug 2015 11:54:13 -0500
> >> Felipe Balbi  wrote:
> >> 
> >> > On Mon, Aug 31, 2015 at 07:48:28PM +0300, ville.syrj...@linux.intel.com 
> >> > wrote:
> >> > > From: Ville Syrj舁・
> >> > > 
> >> > > This reverts commit 8f2c9544aba636134303105ecb164190a39dece4.
> >> > > 
> >> > > As it breaks g_ether on my Baytrail FFRD8 device. Everything starts out
> >> > > fine, but after a bit of data has been transferred it just stops
> >> > > flowing.
> >> > > 
> >> > > Note that I do get a bunch of these "NOHZ: local_softirq_pending 08"
> >> > > when booting the machine, but I'm not really sure if they're related
> >> > > to this problem.
> >> > 
> >> > I have a feeling your problem is elsewhere. We *are* completing one TRB
> >> > at a time. 
> >> 
> >> If usb_request.no_interrupt is flagged, it seems dwc3 does not set IOC
> >> on the corresponding TRB.  Does it break the assumption every TRB
> >> (without SG) will trigger one corresponding EP event?
> >> u_ether is the function module that utilizes 'no_interrupt' flag.
> >
> > XferInProgress should still trigger. Besides, I tested with the exact
> > same setup (different SoC though), just look at the thread.
> 
> I found a way to reproduce this on my end. What I was missing was the
> use of request.no_interrupt. We won't get XferInProgress for all TRBs if
> IOC isn't set in all of them.
> 
> I'll apply this patch ASAP as it fixes the problem I managed to
> reproduce (ping -s 4 makes it fail here)

I can see the commit in your next branch, but shouldn't it go in as a
fix? I guess it should also be tagged with the stable tag.

I got an other guy who hit this issue who I think is using the stable
tree.


Thanks,

-- 
heikki
--
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 v5 1/5] gadget: Introduce the notifier functions

[kbuild test robot]

Hi Baolin,

[auto build test WARNING on v4.3-rc7]
[also build test WARNING on next-20151106]

url:
https://github.com/0day-ci/linux/commits/Baolin-Wang/Introduce-usb-charger-framework-to-deal-with-the-usb-gadget-power-negotation/20151106-194008
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/linux/usb/gadget.h:226: warning: No description found for parameter 
'claimed'
>> include/linux/usb/gadget.h:628: warning: No description found for parameter 
>> 'nh'
>> include/linux/usb/gadget.h:628: warning: No description found for parameter 
>> 'lock'
   include/linux/usb/gadget.h:628: warning: No description found for parameter 
'quirk_altset_not_supp'
   include/linux/usb/gadget.h:628: warning: No description found for parameter 
'quirk_stall_not_supp'
   include/linux/usb/gadget.h:628: warning: No description found for parameter 
'quirk_zlp_not_supp'
>> include/linux/usb/gadget.h:1193: warning: No description found for parameter 
>> 'gadget'
>> include/linux/usb/gadget.h:1193: warning: No description found for parameter 
>> 'nb'
   include/linux/usb/composite.h:501: warning: Excess struct/union/enum/typedef 
member 'setup_pending' description in 'usb_composite_dev'
   include/linux/usb/composite.h:501: warning: Excess struct/union/enum/typedef 
member 'os_desc_pending' description in 'usb_composite_dev'
   drivers/usb/gadget/function/f_acm.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_ecm.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_subset.c:1: warning: no structured comments 
found
   drivers/usb/gadget/function/f_obex.c:1: warning: no structured comments found
   drivers/usb/gadget/function/f_serial.c:1: warning: no structured comments 
found

vim +/nh +628 include/linux/usb/gadget.h

a64cbb7e92 include/linux/usb/gadget.h Baolin Wang 2015-11-06  612   struct 
raw_notifier_headnh;
a64cbb7e92 include/linux/usb/gadget.h Baolin Wang 2015-11-06  613   struct 
mutexlock;
d8318d7f6b include/linux/usb/gadget.h David Cohen 2013-12-09  614  
898c608678 include/linux/usb/gadget.h Felipe Balbi2011-11-22  615   
unsignedsg_supported:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  616   
unsignedis_otg:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  617   
unsignedis_a_peripheral:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  618   
unsignedb_hnp_enable:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  619   
unsigneda_hnp_support:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  620   
unsigneda_alt_hnp_support:1;
0b2d2bbade include/linux/usb/gadget.h David Cohen 2013-12-09  621   
unsignedquirk_ep_out_aligned_size:1;
ffd9a0fcbb include/linux/usb/gadget.h Robert Baldyga  2015-07-28  622   
unsignedquirk_altset_not_supp:1;
02ded1b0d8 include/linux/usb/gadget.h Robert Baldyga  2015-07-28  623   
unsignedquirk_stall_not_supp:1;
ca1023c81d include/linux/usb/gadget.h Robert Baldyga  2015-07-28  624   
unsignedquirk_zlp_not_supp:1;
80b2502cea include/linux/usb/gadget.h Peter Chen  2015-01-28  625   
unsignedis_selfpowered:1;
ccdf138fe3 include/linux/usb/gadget.h Robert Baldyga  2015-05-04  626   
unsigneddeactivated:1;
ccdf138fe3 include/linux/usb/gadget.h Robert Baldyga  2015-05-04  627   
unsignedconnected:1;
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16 @628  };
5702f75375 include/linux/usb/gadget.h Felipe Balbi2013-07-17  629  #define 
work_to_gadget(w)(container_of((w), struct usb_gadget, work))
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  630  
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  631  static 
inline void set_gadget_data(struct usb_gadget *gadget, void *data)
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  632   { 
dev_set_drvdata(&gadget->dev, data); }
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  633  static 
inline void *get_gadget_data(struct usb_gadget *gadget)
^1da177e4c include/linux/usb_gadget.h Linus Torvalds  2005-04-16  634   { 
return dev_get_drvdata(&gadget->dev); }
f48cf80f93 include/linux/usb/gadget.h Fabien Chouteau 2010-04-23  635  static 
inline struct usb_gadget *dev_to_usb_gadget(struct device *dev)
f48cf80f93 include/linux/usb/gadget.h Fabien Chouteau 2010-04-23  636  {

:: The code at line 628 was first introduced by commit
:: 1da177e4c3f41

[PATCH] usb: host: pci_quirks: fix memory leak, by adding iounmap

[Saurabh Sengar]

added iounmap inorder to free memory mapped to base before returning

Signed-off-by: Saurabh Sengar 
---
 drivers/usb/host/pci-quirks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index f940056..332f687 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -990,7 +990,7 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
/* We're reading garbage from the controller */
dev_warn(&pdev->dev,
 "xHCI controller failing to respond");
-   return;
+   goto iounmap;
}
 
if (!ext_cap_offset)
@@ -1061,7 +1061,7 @@ hc_init:
 "xHCI HW did not halt within %d usec status = 0x%x\n",
 XHCI_MAX_HALT_USEC, val);
}
-
+iounmap:
iounmap(base);
 }
 
-- 
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

Re: [PATCH v5 5/5] power: wm831x_power: Support USB charger current limit management

[kbuild test robot]

Hi Baolin,

[auto build test ERROR on v4.3-rc7]
[also build test ERROR on next-20151106]

url:
https://github.com/0day-ci/linux/commits/Baolin-Wang/Introduce-usb-charger-framework-to-deal-with-the-usb-gadget-power-negotation/20151106-194008
config: i386-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from drivers/power/wm831x_power.c:16:0:
>> include/linux/usb/usb_charger.h:53:24: error: field 'old_gadget_state' has 
>> incomplete type
 enum usb_device_state old_gadget_state;
   ^

vim +/old_gadget_state +53 include/linux/usb/usb_charger.h

714a0d15 Baolin Wang 2015-11-06  47 /* for supporting extcon usb gpio */
714a0d15 Baolin Wang 2015-11-06  48 struct extcon_dev   *extcon_dev;
714a0d15 Baolin Wang 2015-11-06  49 struct usb_charger_nb   extcon_nb;
714a0d15 Baolin Wang 2015-11-06  50  
714a0d15 Baolin Wang 2015-11-06  51 /* for supporting usb gadget */
714a0d15 Baolin Wang 2015-11-06  52 struct usb_gadget   *gadget;
714a0d15 Baolin Wang 2015-11-06 @53 enum usb_device_state   
old_gadget_state;
714a0d15 Baolin Wang 2015-11-06  54 struct notifier_block   gadget_nb;
714a0d15 Baolin Wang 2015-11-06  55  
714a0d15 Baolin Wang 2015-11-06  56 /* for supporting power supply */

:: The code at line 53 was first introduced by commit
:: 714a0d150788929d583a0c0961bfaa4242be83f3 gadget: Introduce the usb 
charger framework

:: TO: Baolin Wang 
:: CC: 0day robot 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

[PATCH v5 4/5] gadget: Integrate with the usb gadget supporting for usb charger

[Baolin Wang]

When the usb gadget supporting for usb charger is ready, the usb charger
should get the type by the 'get_charger_type' callback which is implemented
by the usb gadget operations, and get the usb charger pointer from struct
'usb_gadget'.

Signed-off-by: Baolin Wang 
---
 drivers/usb/gadget/charger.c |   45 --
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
index 8387820..c260317 100644
--- a/drivers/usb/gadget/charger.c
+++ b/drivers/usb/gadget/charger.c
@@ -183,7 +183,11 @@ int usb_charger_unregister_notify(struct usb_charger 
*uchger,
 enum usb_charger_type
 usb_charger_detect_type(struct usb_charger *uchger)
 {
-   if (uchger->psy) {
+   if (uchger->gadget && uchger->gadget->ops
+   && uchger->gadget->ops->get_charger_type) {
+   uchger->type =
+   uchger->gadget->ops->get_charger_type(uchger->gadget);
+   } else if (uchger->psy) {
union power_supply_propval val;
 
power_supply_get_property(uchger->psy,
@@ -387,6 +391,30 @@ static int
 usb_charger_plug_by_gadget(struct notifier_block *nb,
   unsigned long state, void *data)
 {
+   struct usb_gadget *gadget = (struct usb_gadget *)data;
+   struct usb_charger *uchger = gadget->charger;
+   enum usb_charger_state uchger_state;
+
+   if (!uchger)
+   return NOTIFY_BAD;
+
+   /* Report event to power to setting the current limitation
+* for this usb charger when one usb charger state is changed
+* with detecting by usb gadget state.
+*/
+   if (uchger->old_gadget_state != state) {
+   uchger->old_gadget_state = state;
+
+   if (state >= USB_STATE_ATTACHED)
+   uchger_state = USB_CHARGER_PRESENT;
+   else if (state == USB_STATE_NOTATTACHED)
+   uchger_state = USB_CHARGER_REMOVE;
+   else
+   uchger_state = USB_CHARGER_DEFAULT;
+
+   usb_charger_notify_others(uchger, uchger_state);
+   }
+
return NOTIFY_OK;
 }
 
@@ -542,6 +570,7 @@ int usb_charger_init(struct usb_gadget *ugadget)
 
/* register a notifier on a usb gadget device */
uchger->gadget = ugadget;
+   ugadget->charger = uchger;
uchger->old_gadget_state = ugadget->state;
uchger->gadget_nb.notifier_call = usb_charger_plug_by_gadget;
usb_gadget_register_notify(ugadget, &uchger->gadget_nb);
@@ -565,7 +594,19 @@ fail:
 
 int usb_charger_exit(struct usb_gadget *ugadget)
 {
-   return 0;
+   struct usb_charger *uchger = ugadget->charger;
+
+   if (!uchger)
+   return -EINVAL;
+
+   if (uchger->extcon_dev)
+   extcon_unregister_notifier(uchger->extcon_dev,
+  EXTCON_USB, &uchger->extcon_nb.nb);
+
+   usb_gadget_unregister_notify(uchger->gadget, &uchger->gadget_nb);
+   ida_simple_remove(&usb_charger_ida, uchger->id);
+
+   return usb_charger_unregister(uchger);
 }
 
 static int __init usb_charger_sysfs_init(void)
-- 
1.7.9.5

--
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 1/5] gadget: Introduce the notifier functions

[Baolin Wang]

The usb charger framework is based on usb gadget. The usb charger
need to be notified the state changing of usb gadget to confirm the
usb charger state.

Thus this patch adds a notifier mechanism for usb gadget to report a
event to usb charger when the usb gadget state is changed.

Signed-off-by: Baolin Wang 
---
 drivers/usb/gadget/udc/udc-core.c |   32 
 include/linux/usb/gadget.h|   18 ++
 2 files changed, 50 insertions(+)

diff --git a/drivers/usb/gadget/udc/udc-core.c 
b/drivers/usb/gadget/udc/udc-core.c
index f660afb..4238fc3 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
 }
 EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
 
+int usb_gadget_register_notify(struct usb_gadget *gadget,
+  struct notifier_block *nb)
+{
+   int ret;
+
+   mutex_lock(&gadget->lock);
+   ret = raw_notifier_chain_register(&gadget->nh, nb);
+   mutex_unlock(&gadget->lock);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
+
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+struct notifier_block *nb)
+{
+   int ret;
+
+   mutex_lock(&gadget->lock);
+   ret = raw_notifier_chain_unregister(&gadget->nh, nb);
+   mutex_unlock(&gadget->lock);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_unregister_notify);
+
 /* - */
 
 /**
@@ -226,6 +252,10 @@ static void usb_gadget_state_work(struct work_struct *work)
struct usb_gadget *gadget = work_to_gadget(work);
struct usb_udc *udc = gadget->udc;
 
+   mutex_lock(&gadget->lock);
+   raw_notifier_call_chain(&gadget->nh, gadget->state, gadget);
+   mutex_unlock(&gadget->lock);
+
if (udc)
sysfs_notify(&udc->dev.kobj, NULL, "state");
 }
@@ -364,6 +394,8 @@ int usb_add_gadget_udc_release(struct device *parent, 
struct usb_gadget *gadget,
 
dev_set_name(&gadget->dev, "gadget");
INIT_WORK(&gadget->work, usb_gadget_state_work);
+   RAW_INIT_NOTIFIER_HEAD(&gadget->nh);
+   mutex_init(&gadget->lock);
gadget->dev.parent = parent;
 
 #ifdef CONFIG_HAS_DMA
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index c14a69b..755e8bc 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -609,6 +609,8 @@ struct usb_gadget {
unsignedout_epnum;
unsignedin_epnum;
struct usb_otg_caps *otg_caps;
+   struct raw_notifier_headnh;
+   struct mutexlock;
 
unsignedsg_supported:1;
unsignedis_otg:1;
@@ -1183,6 +1185,22 @@ extern void usb_gadget_unmap_request(struct usb_gadget 
*gadget,
 
 /*-*/
 
+/**
+ * Register a notifiee to get notified by any attach status changes from
+ * the usb gadget
+ */
+int usb_gadget_register_notify(struct usb_gadget *gadget,
+  struct notifier_block *nb);
+
+/*-*/
+
+
+/* Unregister a notifiee from the usb gadget */
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+struct notifier_block *nb);
+
+/*-*/
+
 /* utility to set gadget state properly */
 
 extern void usb_gadget_set_state(struct usb_gadget *gadget,
-- 
1.7.9.5

--
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 3/5] gadget: Support for the usb charger framework

[Baolin Wang]

For supporting the usb charger, it adds the usb_charger_init() and
usb_charger_exit() functions for usb charger initialization and exit.

Introduce a callback 'get_charger_type' which will implemented by
user for usb gadget operations to get the usb charger type.

Signed-off-by: Baolin Wang 
---
 drivers/usb/gadget/udc/udc-core.c |8 
 include/linux/usb/gadget.h|9 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/usb/gadget/udc/udc-core.c 
b/drivers/usb/gadget/udc/udc-core.c
index 4238fc3..370376e 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /**
  * struct usb_udc - describes one usb device controller
@@ -437,8 +438,14 @@ int usb_add_gadget_udc_release(struct device *parent, 
struct usb_gadget *gadget,
 
mutex_unlock(&udc_lock);
 
+   ret = usb_charger_init(gadget);
+   if (ret)
+   goto err5;
+
return 0;
 
+err5:
+   device_del(&udc->dev);
 err4:
list_del(&udc->list);
mutex_unlock(&udc_lock);
@@ -513,6 +520,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
flush_work(&gadget->work);
device_unregister(&udc->dev);
+   usb_charger_exit(gadget);
device_unregister(&gadget->dev);
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 755e8bc..c2610c4 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct usb_ep;
 
@@ -537,6 +538,7 @@ struct usb_gadget_ops {
struct usb_ep *(*match_ep)(struct usb_gadget *,
struct usb_endpoint_descriptor *,
struct usb_ss_ep_comp_descriptor *);
+   enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
 };
 
 /**
@@ -611,6 +613,7 @@ struct usb_gadget {
struct usb_otg_caps *otg_caps;
struct raw_notifier_headnh;
struct mutexlock;
+   struct usb_charger  *charger;
 
unsignedsg_supported:1;
unsignedis_otg:1;
@@ -815,10 +818,16 @@ static inline int usb_gadget_vbus_connect(struct 
usb_gadget *gadget)
  * reporting how much power the device may consume.  For example, this
  * could affect how quickly batteries are recharged.
  *
+ * It will also notify the USB charger how much power the device may
+ * consume if there is a USB charger linking with the gadget.
+ *
  * Returns zero on success, else negative errno.
  */
 static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 {
+   if (gadget->charger)
+   usb_charger_set_cur_limit_by_type(gadget->charger, mA);
+
if (!gadget->ops->vbus_draw)
return -EOPNOTSUPP;
return gadget->ops->vbus_draw(gadget, mA);
-- 
1.7.9.5

--
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/5] Introduce usb charger framework to deal with the usb gadget power negotation

[Baolin Wang]

Currently the Linux kernel does not provide any standard integration of this
feature that integrates the USB subsystem with the system power regulation
provided by PMICs meaning that either vendors must add this in their kernels
or USB gadget devices based on Linux (such as mobile phones) may not behave
as they should. Thus provide a standard framework for doing this in kernel.

Now introduce one user with wm831x_power to support and test the usb charger,
which is pending testing. Moreover there may be other potential users will use
it in future.

Changes since v4:
 - Flesh out the port type detection which combines the USB negotiation and
 PMICs detection.
 - Supply the notification mechanism to userspace when charger state is changed.
 - Integrate with the vbus staff in the gadget API.

Baolin Wang (5):
  gadget: Introduce the notifier functions
  gadget: Introduce the usb charger framework
  gadget: Support for the usb charger framework
  gadget: Integrate with the usb gadget supporting for usb charger
  power: wm831x_power: Support USB charger current limit management

 drivers/power/wm831x_power.c  |   69 +
 drivers/usb/gadget/Kconfig|7 +
 drivers/usb/gadget/Makefile   |1 +
 drivers/usb/gadget/charger.c  |  620 +
 drivers/usb/gadget/udc/udc-core.c |   40 +++
 include/linux/mfd/wm831x/pdata.h  |3 +
 include/linux/usb/gadget.h|   27 ++
 include/linux/usb/usb_charger.h   |  152 +
 8 files changed, 919 insertions(+)
 create mode 100644 drivers/usb/gadget/charger.c
 create mode 100644 include/linux/usb/usb_charger.h

-- 
1.7.9.5

--
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 5/5] power: wm831x_power: Support USB charger current limit management

[Baolin Wang]

Integrate with the newly added USB charger interface to limit the current
we draw from the USB input based on the input device configuration
identified by the USB stack, allowing us to charge more quickly from high
current inputs without drawing more current than specified from others.

Signed-off-by: Mark Brown 
Signed-off-by: Baolin Wang 
Acked-by: Lee Jones 
Acked-by: Charles Keepax 
Acked-by: Peter Chen 
Acked-by: Sebastian Reichel 
---
 drivers/power/wm831x_power.c |   69 ++
 include/linux/mfd/wm831x/pdata.h |3 ++
 2 files changed, 72 insertions(+)

diff --git a/drivers/power/wm831x_power.c b/drivers/power/wm831x_power.c
index db11ae6..33ae27a 100644
--- a/drivers/power/wm831x_power.c
+++ b/drivers/power/wm831x_power.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -31,6 +32,8 @@ struct wm831x_power {
char usb_name[20];
char battery_name[20];
bool have_battery;
+   struct usb_charger *usb_charger;
+   struct notifier_block usb_notify;
 };
 
 static int wm831x_power_check_online(struct wm831x *wm831x, int supply,
@@ -125,6 +128,43 @@ static enum power_supply_property wm831x_usb_props[] = {
POWER_SUPPLY_PROP_VOLTAGE_NOW,
 };
 
+/* In milliamps */
+static unsigned int wm831x_usb_limits[] = {
+   0,
+   2,
+   100,
+   500,
+   900,
+   1500,
+   1800,
+   550,
+};
+
+static int wm831x_usb_limit_change(struct notifier_block *nb,
+  unsigned long limit, void *data)
+{
+   struct wm831x_power *wm831x_power = container_of(nb,
+struct wm831x_power,
+usb_notify);
+   int i, best;
+
+   /* Find the highest supported limit */
+   best = 0;
+   for (i = 0; i < ARRAY_SIZE(wm831x_usb_limits); i++) {
+   if (limit >= wm831x_usb_limits[i] &&
+   wm831x_usb_limits[best] < wm831x_usb_limits[i])
+   best = i;
+   }
+
+   dev_dbg(wm831x_power->wm831x->dev,
+   "Limiting USB current to %dmA", wm831x_usb_limits[best]);
+
+   wm831x_set_bits(wm831x_power->wm831x, WM831X_POWER_STATE,
+   WM831X_USB_ILIM_MASK, best);
+
+   return 0;
+}
+
 /*
  * Battery properties
  */
@@ -606,8 +646,31 @@ static int wm831x_power_probe(struct platform_device *pdev)
}
}
 
+   if (wm831x_pdata && wm831x_pdata->usb_gadget) {
+   power->usb_charger =
+   usb_charger_find_by_name(wm831x_pdata->usb_gadget);
+   if (IS_ERR(power->usb_charger)) {
+   ret = PTR_ERR(power->usb_charger);
+   dev_err(&pdev->dev,
+   "Failed to find USB gadget: %d\n", ret);
+   goto err_bat_irq;
+   }
+
+   power->usb_notify.notifier_call = wm831x_usb_limit_change;
+
+   ret = usb_charger_register_notify(power->usb_charger,
+ &power->usb_notify);
+   if (ret != 0) {
+   dev_err(&pdev->dev,
+   "Failed to register notifier: %d\n", ret);
+   goto err_usb_charger;
+   }
+   }
+
return ret;
 
+err_usb_charger:
+   /* put_device on charger */
 err_bat_irq:
--i;
for (; i >= 0; i--) {
@@ -637,6 +700,12 @@ static int wm831x_power_remove(struct platform_device 
*pdev)
struct wm831x *wm831x = wm831x_power->wm831x;
int irq, i;
 
+   if (wm831x_power->usb_charger) {
+   usb_charger_unregister_notify(wm831x_power->usb_charger,
+ &wm831x_power->usb_notify);
+   /* Free charger */
+   }
+
for (i = 0; i < ARRAY_SIZE(wm831x_bat_irqs); i++) {
irq = wm831x_irq(wm831x, 
 platform_get_irq_byname(pdev,
diff --git a/include/linux/mfd/wm831x/pdata.h b/include/linux/mfd/wm831x/pdata.h
index dcc9631..5af8399 100644
--- a/include/linux/mfd/wm831x/pdata.h
+++ b/include/linux/mfd/wm831x/pdata.h
@@ -126,6 +126,9 @@ struct wm831x_pdata {
/** The driver should initiate a power off sequence during shutdown */
bool soft_shutdown;
 
+   /** dev_name of USB charger gadget to integrate with */
+   const char *usb_gadget;
+
int irq_base;
int gpio_base;
int gpio_defaults[WM831X_GPIO_NUM];
-- 
1.7.9.5

--
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 2/5] gadget: Introduce the usb charger framework

[Baolin Wang]

This patch introduces the usb charger driver based on usb gadget that
makes an enhancement to a power driver. It works well in practice but
that requires a system with suitable hardware.

The basic conception of the usb charger is that, when one usb charger
is added or removed by reporting from the usb gadget state change or
the extcon device state change, the usb charger will report to power
user to set the current limitation.

The usb charger will register notifiees on the usb gadget or the extcon
device to get notified the usb charger state. It also supplies the
notification mechanism to userspace When the usb charger state is changed.

Power user will register a notifiee on the usb charger to get notified
by status changes from the usb charger. It will report to power user
to set the current limitation when detecting the usb charger is added
or removed from extcon device state or usb gadget state.

This patch doesn't yet integrate with the gadget code, so some functions
which rely on the 'gadget' are not completed, that will be implemented
in the following patches.

Signed-off-by: Baolin Wang 
---
 drivers/usb/gadget/Kconfig  |7 +
 drivers/usb/gadget/Makefile |1 +
 drivers/usb/gadget/charger.c|  579 +++
 include/linux/usb/usb_charger.h |  152 ++
 4 files changed, 739 insertions(+)
 create mode 100644 drivers/usb/gadget/charger.c
 create mode 100644 include/linux/usb/usb_charger.h

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index bcf83c0..3d2b959 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -127,6 +127,13 @@ config USB_GADGET_STORAGE_NUM_BUFFERS
   a module parameter as well.
   If unsure, say 2.
 
+config USB_CHARGER
+   bool "USB charger support"
+   help
+ The usb charger driver based on the usb gadget that makes an
+ enhancement to a power driver which can set the current limitation
+ when the usb charger is added or removed.
+
 source "drivers/usb/gadget/udc/Kconfig"
 
 #
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 598a67d..1e421c1 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -10,3 +10,4 @@ libcomposite-y:= usbstring.o config.o 
epautoconf.o
 libcomposite-y += composite.o functions.o configfs.o u_f.o
 
 obj-$(CONFIG_USB_GADGET)   += udc/ function/ legacy/
+obj-$(CONFIG_USB_CHARGER)  += charger.o
diff --git a/drivers/usb/gadget/charger.c b/drivers/usb/gadget/charger.c
new file mode 100644
index 000..8387820
--- /dev/null
+++ b/drivers/usb/gadget/charger.c
@@ -0,0 +1,579 @@
+/*
+ * charger.c -- USB charger driver
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DEFAULT_CUR_PROTECT(50)
+#define DEFAULT_SDP_CUR_LIMIT  (500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_DCP_CUR_LIMIT  (1500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_CDP_CUR_LIMIT  (1500 - DEFAULT_CUR_PROTECT)
+#define DEFAULT_ACA_CUR_LIMIT  (1500 - DEFAULT_CUR_PROTECT)
+#define UCHGER_STATE_LENGTH(50)
+
+static DEFINE_IDA(usb_charger_ida);
+static struct bus_type usb_charger_subsys = {
+   .name   = "usb-charger",
+   .dev_name   = "usb-charger",
+};
+
+static struct usb_charger *dev_to_uchger(struct device *udev)
+{
+   return container_of(udev, struct usb_charger, dev);
+}
+
+static ssize_t cur_limit_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+   struct usb_charger *uchger = dev_to_uchger(dev);
+
+   return scnprintf(buf, PAGE_SIZE, "%d %d %d %d\n",
+uchger->cur_limit.sdp_cur_limit,
+uchger->cur_limit.dcp_cur_limit,
+uchger->cur_limit.cdp_cur_limit,
+uchger->cur_limit.aca_cur_limit);
+}
+
+static ssize_t cur_limit_store(struct device *dev,
+  struct device_attribute *attr,
+  const char *buf, size_t count)
+{
+   struct usb_charger *uchger = dev_to_uchger(dev);
+   struct usb_charger_cur_limit cur;
+   int ret;
+
+   ret = sscanf(buf, "%d %d %d %d",
+&cur.sdp_cur_limit, &cur.dcp_cur_limit,
+&cur.cdp_cur_limit, &cur.aca_cur_limit);
+   if (ret == 0)
+   return -EINVAL;
+
+   ret = usb_charger_set_cur_limit(uchger, &cur);
+   if (ret < 0)
+   return ret;
+
+   return count;
+}
+static DEVICE_ATTR_RW(cur_limit);
+
+static struc

Re: [PATCH 1/2] cdc_acm: Ignore Infineon Flash Loader utility

[Johan Hovold]

On Thu, Nov 05, 2015 at 01:57:43PM +0100, Jonas Jonsson wrote:
> Some modems, such as the Telit UE910, are using an Infineon Flash Loader
> utility. It has two interfaces, 2/2/0 (Abstract Modem) and 10/0/0 (CDC
> Data). The latter can be used as a serial interface to upgrade the
> firmware of the modem. However, that isn't possible when the cdc-acm
> driver takes control of the device.

Why can't you just use the tty device that the cdc-acm driver provides?

Thanks,
Johan
--
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] USB: serial: Another Infineon flash loader USB ID

[Daniele Palmas]

Hi Jonas,

2015-11-05 13:57 GMT+01:00 Jonas Jonsson :
> This has been seen on a Telit UE910 modem.
>
> Signed-off-by: Jonas Jonsson 
> ---
>  drivers/usb/serial/usb-serial-simple.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/usb-serial-simple.c 
> b/drivers/usb/serial/usb-serial-simple.c
> index 3658662..93ab784 100644
> --- a/drivers/usb/serial/usb-serial-simple.c
> +++ b/drivers/usb/serial/usb-serial-simple.c
> @@ -53,7 +53,9 @@ DEVICE(funsoft, FUNSOFT_IDS);
>
>  /* Infineon Flashloader driver */
>  #define FLASHLOADER_IDS()  \
> -   { USB_DEVICE(0x8087, 0x0716) }
> +   { USB_DEVICE(0x8087, 0x0716) }, \
> +   { USB_DEVICE_INTERFACE_CLASS(0x058b, 0x0041, 0x0a) }
> +
>  DEVICE(flashloader, FLASHLOADER_IDS);
>
>  /* Google Serial USB SubClass */
> --
> 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


This and the previous patch are fine to me for all Telit devices based
on 3G IMC chipset with Telit proprietary flashing protocol.

I'm wondering if this is fine for all other non-Telit devices based on
the same chipset, but a different protocol...

Anyway, if you wish you can add my

Tested-by: Daniele Palmas 

Regards,
Daniele
--
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] usb: host: ehci-msm: Use posted data writes on AHB

[Georgi Djakov]

On 11/06/2015 08:04 AM, Andy Gross wrote:
> This patch sets the AHBMODE to allow for posted data writes.  This
> results in higher performance.
> 
> Signed-off-by: Andy Gross 

With these patches I see significant improvement in throughput
on my db410c board.

Tested-by: Georgi Djakov 

> ---
>  drivers/usb/host/ehci-msm.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
> index c4f84c8..c23e285 100644
> --- a/drivers/usb/host/ehci-msm.c
> +++ b/drivers/usb/host/ehci-msm.c
> @@ -57,8 +57,8 @@ static int ehci_msm_reset(struct usb_hcd *hcd)
>  
>   /* bursts of unspecified length. */
>   writel(0, USB_AHBBURST);
> - /* Use the AHB transactor */
> - writel(0, USB_AHBMODE);
> + /* Use the AHB transactor, allow posted data writes */
> + writel(0x8, USB_AHBMODE);
>   /* Disable streaming mode and select host mode */
>   writel(0x13, USB_USBMODE);
>  
> 

--
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: [RFC] usb: dwc2: hcd: fix split schedule issue

[Yunzhi Li]

hi John ,

  As we talked yesterday, I tried to fix the split schedule sequence. This 
patch will
avoid scheduling SSPLIT-IN packet for another device between SSPLIT-OUT-begin 
and
SSPLIT-OUT-end, now the keyboard and Jebra audio speaker could work together 
well, but
I'm not sure if this is exactly the right way to schedule split transfers and 
if there
is any dide effect with this patch. Please help review this patch. Thanks.


Fix dwc2 split schedule sequence issue. Not schedule a SSPLIT_IN
packet between SSPLIT-begin and SSPLIT-end.

Signed-off-by: Yunzhi Li 
---
  drivers/usb/dwc2/hcd.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index e79baf7..a32ed01 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1122,6 +1122,10 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
break;
}
  
+		if (qh->channel->xact_pos == DWC2_HCSPLT_XACTPOS_BEGIN ||

+   qh->channel->xact_pos == DWC2_HCSPLT_XACTPOS_MID)
+   break;
+
/*
 * In Slave mode, stay on the current transfer until there is
 * nothing more to do or the high-bandwidth request count is



On 11/5/2015 2:13 AM, l...@rock-chips.com wrote:

Hi John :

  We found some problem when we tested usb audio speaker on rk3288 platform
which use dwc2 IP v3.10a as usb controller

Steps to reproduce the problem:
1. Plug in USB2.0 hub to rk3288 platform board.
2. Plug in USB keyboard to the hub.
3. Plug in USB audio speaker speaker(Jabra 410 or 510) to the hub
(These audio speakers support full speed data packet length 192 byte and it will
be split into 2 SSPLIT-OUT packets (188B + 4B) in
high speed bus other usb audio devices which has FS data packets length smaller
then 188B not has this issue )
4. Play music via usb speaker then USB keyboard stop working

I do some debug work and try to figure out the root cause of this issue :
Use the usb protocol analyzer to catch usb traffic in high speed bus
I see something weired that dwc2 send SSPLIT IN for dev 5 between two SSPLIT OUT
transaction for dev 6
then hub respond a NYET for dev 5 CSPLIT and keyboard not working any more.
It seems  some problem with split scheduling sequence and it let the hub
confused, but I'm not sure which rule
in usb20 spec chapters 11 is broken and how to fix it.
DWC2 traffic

I alsocatch the usb traffic between an EHCI controller in pc and the hub
connected with audio speaker and keyboard
both keyboard and audio speaker work well with EHCI. EHCI schedules the SSPLIT
IN for keyboard in the next microframe
after OUT SSPLIT OUT for audio data packets and the hub can respond NAK .


EHCI traffic

I will keep on debugging for this issue and try to fix the scheduling sequence ,
does anyone have any  ideas could be help with this issue ?


l...@rock-chips.com


Thanks for this report. I'll try to reproduce and forward it
along to some of our experts.

Regards,
John





--
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 03/23] usb: gadget: f_sourcesink: free requests in sourcesink_disable()

[Krzysztof Opasiak]

On 11/06/2015 10:48 AM, Peter Chen wrote:

On Fri, Nov 06, 2015 at 09:50:11AM +0100, Robert Baldyga wrote:

On 11/06/2015 09:15 AM, Peter Chen wrote:

On Tue, Nov 03, 2015 at 01:53:42PM +0100, Robert Baldyga wrote:

USB requests in SourceSink function are allocated in sourcesink_get_alt()
function, so we prefer to free them rather in sourcesink_disable() than
in source_sink_complete() when request is completed with error. It provides
better symetry in resource management and improves code readability.

Signed-off-by: Robert Baldyga 
---
  drivers/usb/gadget/function/f_sourcesink.c | 33 +++---
  1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_sourcesink.c 
b/drivers/usb/gadget/function/f_sourcesink.c
index a8b68c6..d8f5f56 100644
--- a/drivers/usb/gadget/function/f_sourcesink.c
+++ b/drivers/usb/gadget/function/f_sourcesink.c
@@ -22,6 +22,8 @@
  #include "g_zero.h"
  #include "u_f.h"

+#define REQ_CNT8
+


It would be buffer if we can have module parameter for this like
qlen at f_loopback.

@@ -561,7 +568,6 @@ static void source_sink_complete(struct usb_ep *ep, struct 
usb_request *req)
req->actual, req->length);
if (ep == ss->out_ep)
check_read_data(ss, req);
-   free_ep_req(ep, req);


I find the current code may cause memory leak, since above code
is only called one time.



I don't see what you mean. Can you explain a bit more in which situation
can memory leak take place?


Oh, sorry, I have not seen there is a "break;" at the source_sink_start_ep for
none-iso endpoint, so, I am wrong, no memory leak here.

I have tried changing bulk for 8 requests, the performance improves
greatly, but still a little problem for OUT,  I will see what's the matter.
Besides, it will be better we can have a qlen parameters like f_loopback,
in that case, we can improve the performance for gadget, and get the best 
performance
when testing with usbtest, I will do it later.



Moreover, I would suggest to add qlen and iso_qlen params not only qlen 
as even now we are using different qlen for bulk and iso.


Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
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 03/23] usb: gadget: f_sourcesink: free requests in sourcesink_disable()

[Peter Chen]

On Fri, Nov 06, 2015 at 09:50:11AM +0100, Robert Baldyga wrote:
> On 11/06/2015 09:15 AM, Peter Chen wrote:
> > On Tue, Nov 03, 2015 at 01:53:42PM +0100, Robert Baldyga wrote:
> >> USB requests in SourceSink function are allocated in sourcesink_get_alt()
> >> function, so we prefer to free them rather in sourcesink_disable() than
> >> in source_sink_complete() when request is completed with error. It provides
> >> better symetry in resource management and improves code readability.
> >>
> >> Signed-off-by: Robert Baldyga 
> >> ---
> >>  drivers/usb/gadget/function/f_sourcesink.c | 33 
> >> +++---
> >>  1 file changed, 30 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/usb/gadget/function/f_sourcesink.c 
> >> b/drivers/usb/gadget/function/f_sourcesink.c
> >> index a8b68c6..d8f5f56 100644
> >> --- a/drivers/usb/gadget/function/f_sourcesink.c
> >> +++ b/drivers/usb/gadget/function/f_sourcesink.c
> >> @@ -22,6 +22,8 @@
> >>  #include "g_zero.h"
> >>  #include "u_f.h"
> >>  
> >> +#define REQ_CNT   8
> >> +
> > 
> > It would be buffer if we can have module parameter for this like
> > qlen at f_loopback.
> >> @@ -561,7 +568,6 @@ static void source_sink_complete(struct usb_ep *ep, 
> >> struct usb_request *req)
> >>req->actual, req->length);
> >>if (ep == ss->out_ep)
> >>check_read_data(ss, req);
> >> -  free_ep_req(ep, req);
> > 
> > I find the current code may cause memory leak, since above code
> > is only called one time.
> > 
> 
> I don't see what you mean. Can you explain a bit more in which situation
> can memory leak take place?

Oh, sorry, I have not seen there is a "break;" at the source_sink_start_ep for
none-iso endpoint, so, I am wrong, no memory leak here.

I have tried changing bulk for 8 requests, the performance improves
greatly, but still a little problem for OUT,  I will see what's the matter.
Besides, it will be better we can have a qlen parameters like f_loopback,
in that case, we can improve the performance for gadget, and get the best 
performance
when testing with usbtest, I will do it later.

Peter

> 
> I've only changed place where requests are freed. Now they are freed in
> sourcesink_disable(). The current code is even more memory leak
> resistant, because requests will be freed even in case of failure of
> usb_ep_queue() in source_sink_complete(), which was not provided so far.
> 
> >>return;
> >>  
> >>case -EOVERFLOW:/* buffer overrun on read means that
> >> @@ -613,7 +619,7 @@ static int source_sink_start_ep(struct f_sourcesink 
> >> *ss, bool is_in,
> >>ep = is_in ? ss->in_ep : ss->out_ep;
> >>}
> >>  
> >> -  for (i = 0; i < 8; i++) {
> >> +  for (i = 0; i < REQ_CNT; i++) {
> >>req = ss_alloc_ep_req(ep, size);
> >>if (!req)
> >>return -ENOMEM;
> >> @@ -635,8 +641,18 @@ static int source_sink_start_ep(struct f_sourcesink 
> >> *ss, bool is_in,
> >>free_ep_req(ep, req);
> >>}
> >>  
> >> -  if (!is_iso)
> >> +  if (is_iso) {
> >> +  if (is_in)
> >> +  ss->iso_in_req[i] = req;
> >> +  else
> >> +  ss->iso_out_req[i] = req;
> >> +  } else {
> >> +  if (is_in)
> >> +  ss->in_req = req;
> >> +  else
> >> +  ss->out_req = req;
> > 
> > The req will be different for both bulk and iso, why you only
> > have array for iso requests?
> 
> Because only for iso endpoints there is allocated more than one request.
> I don't know why, but I've decided to not change this.
> 
> > 
> >>break;
> >> +  }
> >>}
> >>  
> >>return status;
> >> @@ -764,8 +780,19 @@ static int sourcesink_get_alt(struct usb_function *f, 
> >> unsigned intf)
> >>  static void sourcesink_disable(struct usb_function *f)
> >>  {
> >>struct f_sourcesink *ss = func_to_ss(f);
> >> +  int i;
> >>  
> >>disable_source_sink(ss);
> >> +
> >> +  free_ep_req(ss->in_ep, ss->in_req);
> >> +  free_ep_req(ss->out_ep, ss->out_req);
> >> +
> >> +  if (ss->iso_in_ep) {
> >> +  for (i = 0; i < REQ_CNT; i++) {
> >> +  free_ep_req(ss->iso_in_ep, ss->iso_in_req[i]);
> >> +  free_ep_req(ss->iso_out_ep, ss->iso_out_req[i]);
> >> +  }
> >> +  }
> >>  }
> >>  
> >>  
> >> /*-*/
> >> -- 
> >> 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
> > 
> 

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More ma

[RFC] usb: dwc2: hcd: fix split schedule issue

[Yunzhi Li]

Fix dwc2 split schedule sequence issue. Not schedule a SSPLIT_IN
packet between SSPLIT-begin and SSPLIT-end.

Signed-off-by: Yunzhi Li 
---
 drivers/usb/dwc2/hcd.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index e79baf7..a32ed01 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1122,6 +1122,10 @@ static void dwc2_process_periodic_channels(struct 
dwc2_hsotg *hsotg)
break;
}
 
+   if (qh->channel->xact_pos == DWC2_HCSPLT_XACTPOS_BEGIN ||
+   qh->channel->xact_pos == DWC2_HCSPLT_XACTPOS_MID)
+   break;
+
/*
 * In Slave mode, stay on the current transfer until there is
 * nothing more to do or the high-bandwidth request count is
-- 
2.0.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: [PATCH v2 00/13] usb: dwc2: descriptor dma mode bug fixes

[Herrero, Gregory]

Hi Doug,

On Thu, Nov 05, 2015 at 04:32:46PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Thu, Nov 5, 2015 at 12:41 AM, Gregory Herrero
>  wrote:
> > Hi,
> >
> > This patchset contains bug fixes for host descriptor dma mode.
> >
> > Descriptor dma mode can't be used as the default mode since controller
> > does not support split transfers in this mode.
> > So we add a new configuration parameter which allows descriptor dma mode
> > to be enabled for full-speed devices only.
> >
> > All patches are verified on dwc2 v3.0a with dedicated fifos and our
> > main test target was usb audio devices.
> >
> > All patches are based on Felipe's testing/next branch.
> >
> > Regards,
> > Gregory
> >
> > History:
> > v2:
> >   - Use dma cache in "usb: dwc2: host: use kmem cache to allocate 
> > descriptors"
> > and replace usage of deprecated GFP_DMA32 flag.
> >   - Remove "usb: dwc2: host: free status_buf on hcd de-init".
> >
> > v1:
> >   - Fix compilation error introduced by:
> > "usb: dwc2: host: avoid usage of dma_alloc_coherent with irqs disabled"
> >   - Fix warning introduced by:
> > "usb: dwc2: host: fix descriptor list address masking"
> >
> > Gregory Herrero (11):
> >   usb: dwc2: host: ensure filling of isoc desc is correctly done
> >   usb: dwc2: host: set active bit in isochronous descriptors
> >   usb: dwc2: host: rework isochronous halt path
> >   usb: dwc2: host: fix use of qtd after free in desc dma mode
> >   usb: dwc2: host: spinlock release channel
> >   usb: dwc2: host: add function to compare frame index
> >   usb: dwc2: host: program descriptor for next frame
> >   usb: dwc2: host: always increment available host channel during
> > release
> >   usb: dwc2: host: process all completed urbs
> >   usb: dwc2: host: avoid usage of dma_alloc_coherent with irqs disabled
> >   usb: dwc2: host: use kmem cache to allocate descriptors
> >
> > Mian Yousaf Kaukab (2):
> >   usb: dwc2: host: enable descriptor dma for fs devices
> >   usb: dwc2: host: fix descriptor list address masking
> >
> >  drivers/usb/dwc2/core.c  |  37 +--
> >  drivers/usb/dwc2/core.h  |  26 +
> >  drivers/usb/dwc2/hcd.c   |  76 +-
> >  drivers/usb/dwc2/hcd.h   |  19 
> >  drivers/usb/dwc2/hcd_ddma.c  | 240 
> > +++
> >  drivers/usb/dwc2/hcd_intr.c  |  15 ++-
> >  drivers/usb/dwc2/hcd_queue.c |   2 +-
> >  drivers/usb/dwc2/hw.h|   4 -
> >  drivers/usb/dwc2/platform.c  |   4 +
> >  9 files changed, 367 insertions(+), 56 deletions(-)
> 
> With the previous version of this series, I could confirm that the
> series didn't break things terribly (after fixing the DMA flag) and it
> _sometimes_ made DMA Descriptor mode work for me (I believe it never
> worked before).  It was unclear why it didn't work sometimes and I
> haven't had a chance to debug it yet.
> 
> I'm not sure I can give a Tested-by for that, but I figured I'd at
> least let you know the current status on my rk3288 board.
> 

Thanks for trying. It would be interesting to see traces when you get
a chance to debug it again. I didn't notice any instability with core
version 3.00a.

Those patches will only impact descriptor DMA mode, so there is no
risk to break anything else now that DMA flag is fixed :).

BR,
Gregory
--
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 03/23] usb: gadget: f_sourcesink: free requests in sourcesink_disable()

[Robert Baldyga]

On 11/06/2015 09:15 AM, Peter Chen wrote:
> On Tue, Nov 03, 2015 at 01:53:42PM +0100, Robert Baldyga wrote:
>> USB requests in SourceSink function are allocated in sourcesink_get_alt()
>> function, so we prefer to free them rather in sourcesink_disable() than
>> in source_sink_complete() when request is completed with error. It provides
>> better symetry in resource management and improves code readability.
>>
>> Signed-off-by: Robert Baldyga 
>> ---
>>  drivers/usb/gadget/function/f_sourcesink.c | 33 
>> +++---
>>  1 file changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_sourcesink.c 
>> b/drivers/usb/gadget/function/f_sourcesink.c
>> index a8b68c6..d8f5f56 100644
>> --- a/drivers/usb/gadget/function/f_sourcesink.c
>> +++ b/drivers/usb/gadget/function/f_sourcesink.c
>> @@ -22,6 +22,8 @@
>>  #include "g_zero.h"
>>  #include "u_f.h"
>>  
>> +#define REQ_CNT 8
>> +
> 
> It would be buffer if we can have module parameter for this like
> qlen at f_loopback.
> 
>>  /*
>>   * SOURCE/SINK FUNCTION ... a primary testing vehicle for USB peripheral
>>   * controller drivers.
>> @@ -51,6 +53,11 @@ struct f_sourcesink {
>>  struct usb_ep   *iso_out_ep;
>>  int cur_alt;
>>  
>> +struct usb_request  *in_req;
>> +struct usb_request  *out_req;
>> +struct usb_request  *iso_in_req[REQ_CNT];
>> +struct usb_request  *iso_out_req[REQ_CNT];
>> +
>>  unsigned pattern;
>>  unsigned isoc_interval;
>>  unsigned isoc_maxpacket;
>> @@ -561,7 +568,6 @@ static void source_sink_complete(struct usb_ep *ep, 
>> struct usb_request *req)
>>  req->actual, req->length);
>>  if (ep == ss->out_ep)
>>  check_read_data(ss, req);
>> -free_ep_req(ep, req);
> 
> I find the current code may cause memory leak, since above code
> is only called one time.
> 

I don't see what you mean. Can you explain a bit more in which situation
can memory leak take place?

I've only changed place where requests are freed. Now they are freed in
sourcesink_disable(). The current code is even more memory leak
resistant, because requests will be freed even in case of failure of
usb_ep_queue() in source_sink_complete(), which was not provided so far.

>>  return;
>>  
>>  case -EOVERFLOW:/* buffer overrun on read means that
>> @@ -613,7 +619,7 @@ static int source_sink_start_ep(struct f_sourcesink *ss, 
>> bool is_in,
>>  ep = is_in ? ss->in_ep : ss->out_ep;
>>  }
>>  
>> -for (i = 0; i < 8; i++) {
>> +for (i = 0; i < REQ_CNT; i++) {
>>  req = ss_alloc_ep_req(ep, size);
>>  if (!req)
>>  return -ENOMEM;
>> @@ -635,8 +641,18 @@ static int source_sink_start_ep(struct f_sourcesink 
>> *ss, bool is_in,
>>  free_ep_req(ep, req);
>>  }
>>  
>> -if (!is_iso)
>> +if (is_iso) {
>> +if (is_in)
>> +ss->iso_in_req[i] = req;
>> +else
>> +ss->iso_out_req[i] = req;
>> +} else {
>> +if (is_in)
>> +ss->in_req = req;
>> +else
>> +ss->out_req = req;
> 
> The req will be different for both bulk and iso, why you only
> have array for iso requests?

Because only for iso endpoints there is allocated more than one request.
I don't know why, but I've decided to not change this.

> 
>>  break;
>> +}
>>  }
>>  
>>  return status;
>> @@ -764,8 +780,19 @@ static int sourcesink_get_alt(struct usb_function *f, 
>> unsigned intf)
>>  static void sourcesink_disable(struct usb_function *f)
>>  {
>>  struct f_sourcesink *ss = func_to_ss(f);
>> +int i;
>>  
>>  disable_source_sink(ss);
>> +
>> +free_ep_req(ss->in_ep, ss->in_req);
>> +free_ep_req(ss->out_ep, ss->out_req);
>> +
>> +if (ss->iso_in_ep) {
>> +for (i = 0; i < REQ_CNT; i++) {
>> +free_ep_req(ss->iso_in_ep, ss->iso_in_req[i]);
>> +free_ep_req(ss->iso_out_ep, ss->iso_out_req[i]);
>> +}
>> +}
>>  }
>>  
>>  
>> /*-*/
>> -- 
>> 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
> 

--
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 1/2] usb: chipidea: msm: Use posted data writes on AHB

[Peter Chen]

On Fri, Nov 06, 2015 at 12:04:06AM -0600, Andy Gross wrote:
> This patch sets the AHBMODE to allow for posted data writes.  This
> results in higher performance.
> 
> Signed-off-by: Andy Gross 
> ---
>  drivers/usb/chipidea/ci_hdrc_msm.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c 
> b/drivers/usb/chipidea/ci_hdrc_msm.c
> index d79ecc0..3889809 100644
> --- a/drivers/usb/chipidea/ci_hdrc_msm.c
> +++ b/drivers/usb/chipidea/ci_hdrc_msm.c
> @@ -25,7 +25,8 @@ static void ci_hdrc_msm_notify_event(struct ci_hdrc *ci, 
> unsigned event)
>   case CI_HDRC_CONTROLLER_RESET_EVENT:
>   dev_dbg(dev, "CI_HDRC_CONTROLLER_RESET_EVENT received\n");
>   writel(0, USB_AHBBURST);
> - writel(0, USB_AHBMODE);
> + /* use AHB transactor, allow posted data writes */
> + writel(0x8, USB_AHBMODE);
>   usb_phy_init(ci->usb_phy);
>   break;
>   case CI_HDRC_CONTROLLER_STOPPED_EVENT:
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 

Greg, another related changes is at host driver, pick it up
directly please, thanks.

Acked-by: Peter Chen 

-- 

Best Regards,
Peter Chen
--
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 03/23] usb: gadget: f_sourcesink: free requests in sourcesink_disable()

[Peter Chen]

On Tue, Nov 03, 2015 at 01:53:42PM +0100, Robert Baldyga wrote:
> USB requests in SourceSink function are allocated in sourcesink_get_alt()
> function, so we prefer to free them rather in sourcesink_disable() than
> in source_sink_complete() when request is completed with error. It provides
> better symetry in resource management and improves code readability.
> 
> Signed-off-by: Robert Baldyga 
> ---
>  drivers/usb/gadget/function/f_sourcesink.c | 33 
> +++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_sourcesink.c 
> b/drivers/usb/gadget/function/f_sourcesink.c
> index a8b68c6..d8f5f56 100644
> --- a/drivers/usb/gadget/function/f_sourcesink.c
> +++ b/drivers/usb/gadget/function/f_sourcesink.c
> @@ -22,6 +22,8 @@
>  #include "g_zero.h"
>  #include "u_f.h"
>  
> +#define REQ_CNT  8
> +

It would be buffer if we can have module parameter for this like
qlen at f_loopback.

>  /*
>   * SOURCE/SINK FUNCTION ... a primary testing vehicle for USB peripheral
>   * controller drivers.
> @@ -51,6 +53,11 @@ struct f_sourcesink {
>   struct usb_ep   *iso_out_ep;
>   int cur_alt;
>  
> + struct usb_request  *in_req;
> + struct usb_request  *out_req;
> + struct usb_request  *iso_in_req[REQ_CNT];
> + struct usb_request  *iso_out_req[REQ_CNT];
> +
>   unsigned pattern;
>   unsigned isoc_interval;
>   unsigned isoc_maxpacket;
> @@ -561,7 +568,6 @@ static void source_sink_complete(struct usb_ep *ep, 
> struct usb_request *req)
>   req->actual, req->length);
>   if (ep == ss->out_ep)
>   check_read_data(ss, req);
> - free_ep_req(ep, req);

I find the current code may cause memory leak, since above code
is only called one time.

>   return;
>  
>   case -EOVERFLOW:/* buffer overrun on read means that
> @@ -613,7 +619,7 @@ static int source_sink_start_ep(struct f_sourcesink *ss, 
> bool is_in,
>   ep = is_in ? ss->in_ep : ss->out_ep;
>   }
>  
> - for (i = 0; i < 8; i++) {
> + for (i = 0; i < REQ_CNT; i++) {
>   req = ss_alloc_ep_req(ep, size);
>   if (!req)
>   return -ENOMEM;
> @@ -635,8 +641,18 @@ static int source_sink_start_ep(struct f_sourcesink *ss, 
> bool is_in,
>   free_ep_req(ep, req);
>   }
>  
> - if (!is_iso)
> + if (is_iso) {
> + if (is_in)
> + ss->iso_in_req[i] = req;
> + else
> + ss->iso_out_req[i] = req;
> + } else {
> + if (is_in)
> + ss->in_req = req;
> + else
> + ss->out_req = req;

The req will be different for both bulk and iso, why you only
have array for iso requests?

>   break;
> + }
>   }
>  
>   return status;
> @@ -764,8 +780,19 @@ static int sourcesink_get_alt(struct usb_function *f, 
> unsigned intf)
>  static void sourcesink_disable(struct usb_function *f)
>  {
>   struct f_sourcesink *ss = func_to_ss(f);
> + int i;
>  
>   disable_source_sink(ss);
> +
> + free_ep_req(ss->in_ep, ss->in_req);
> + free_ep_req(ss->out_ep, ss->out_req);
> +
> + if (ss->iso_in_ep) {
> + for (i = 0; i < REQ_CNT; i++) {
> + free_ep_req(ss->iso_in_ep, ss->iso_in_req[i]);
> + free_ep_req(ss->iso_out_ep, ss->iso_out_req[i]);
> + }
> + }
>  }
>  
>  /*-*/
> -- 
> 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

-- 

Best Regards,
Peter Chen
--
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