[PATCH 1/3] usb: gadget: f_sourcesink: add queue depth

[Peter Chen]

Add queue depth for both iso and bulk transfer, with more queues, we
can do performance and stress test using sourcesink, and update g_zero
accordingly.

Signed-off-by: Peter Chen 
---
 drivers/usb/gadget/function/f_sourcesink.c | 144 ++---
 drivers/usb/gadget/function/g_zero.h   |   6 ++
 drivers/usb/gadget/legacy/zero.c   |  12 +++
 3 files changed, 129 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/gadget/function/f_sourcesink.c 
b/drivers/usb/gadget/function/f_sourcesink.c
index d7646d3..4ab603e8 100644
--- a/drivers/usb/gadget/function/f_sourcesink.c
+++ b/drivers/usb/gadget/function/f_sourcesink.c
@@ -34,13 +34,6 @@
  * plus two that support control-OUT tests.  If the optional "autoresume"
  * mode is enabled, it provides good functional coverage for the "USBCV"
  * test harness from USB-IF.
- *
- * Note that because this doesn't queue more than one request at a time,
- * some other function must be used to test queueing logic.  The network
- * link (g_ether) is the best overall option for that, since its TX and RX
- * queues are relatively independent, will receive a range of packet sizes,
- * and can often be made to run out completely.  Those issues are important
- * when stress testing peripheral controller drivers.
  */
 struct f_sourcesink {
struct usb_function function;
@@ -57,6 +50,8 @@ struct f_sourcesink {
unsigned isoc_mult;
unsigned isoc_maxburst;
unsigned buflen;
+   unsigned bulk_qlen;
+   unsigned iso_qlen;
 };
 
 static inline struct f_sourcesink *func_to_ss(struct usb_function *f)
@@ -595,31 +590,33 @@ static int source_sink_start_ep(struct f_sourcesink *ss, 
bool is_in,
 {
struct usb_ep   *ep;
struct usb_request  *req;
-   int i, size, status;
-
-   for (i = 0; i < 8; i++) {
-   if (is_iso) {
-   switch (speed) {
-   case USB_SPEED_SUPER:
-   size = ss->isoc_maxpacket *
-   (ss->isoc_mult + 1) *
-   (ss->isoc_maxburst + 1);
-   break;
-   case USB_SPEED_HIGH:
-   size = ss->isoc_maxpacket * (ss->isoc_mult + 1);
-   break;
-   default:
-   size = ss->isoc_maxpacket > 1023 ?
-   1023 : ss->isoc_maxpacket;
-   break;
-   }
-   ep = is_in ? ss->iso_in_ep : ss->iso_out_ep;
-   req = ss_alloc_ep_req(ep, size);
-   } else {
-   ep = is_in ? ss->in_ep : ss->out_ep;
-   req = ss_alloc_ep_req(ep, 0);
+   int i, size, qlen, status = 0;
+
+   if (is_iso) {
+   switch (speed) {
+   case USB_SPEED_SUPER:
+   size = ss->isoc_maxpacket *
+   (ss->isoc_mult + 1) *
+   (ss->isoc_maxburst + 1);
+   break;
+   case USB_SPEED_HIGH:
+   size = ss->isoc_maxpacket * (ss->isoc_mult + 1);
+   break;
+   default:
+   size = ss->isoc_maxpacket > 1023 ?
+   1023 : ss->isoc_maxpacket;
+   break;
}
+   ep = is_in ? ss->iso_in_ep : ss->iso_out_ep;
+   qlen = ss->iso_qlen;
+   } else {
+   ep = is_in ? ss->in_ep : ss->out_ep;
+   qlen = ss->bulk_qlen;
+   size = 0;
+   }
 
+   for (i = 0; i < qlen; i++) {
+   req = ss_alloc_ep_req(ep, size);
if (!req)
return -ENOMEM;
 
@@ -639,9 +636,6 @@ static int source_sink_start_ep(struct f_sourcesink *ss, 
bool is_in,
  ep->name, status);
free_ep_req(ep, req);
}
-
-   if (!is_iso)
-   break;
}
 
return status;
@@ -869,6 +863,8 @@ static struct usb_function *source_sink_alloc_func(
ss->isoc_mult = ss_opts->isoc_mult;
ss->isoc_maxburst = ss_opts->isoc_maxburst;
ss->buflen = ss_opts->bulk_buflen;
+   ss->bulk_qlen = ss_opts->bulk_qlen;
+   ss->iso_qlen = ss_opts->iso_qlen;
 
ss->function.name = "source/sink";
ss->function.bind = sourcesink_bind;
@@ -1164,6 +1160,84 @@ static struct f_ss_opts_attribute f_ss_opts_bulk_buflen =
f_ss_opts_bulk_buflen_show,
f_ss_opts_bulk_buflen_store);
 
+static ssize_t f_ss_opts_bulk_qlen_show(struct f_ss_opts *opts, char *page)
+{
+   

[PATCH 2/3] Documentation: usb: gadget-testing: add description for depth of queue

[Peter Chen]

Add both bulk and iso depth of queue for sourcesink.

Signed-off-by: Peter Chen 
---
 Documentation/usb/gadget-testing.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/usb/gadget-testing.txt 
b/Documentation/usb/gadget-testing.txt
index b24d3ef..84b3d10 100644
--- a/Documentation/usb/gadget-testing.txt
+++ b/Documentation/usb/gadget-testing.txt
@@ -579,6 +579,8 @@ The SOURCESINK function provides these attributes in its 
function directory:
isoc_mult   - 0..2 (hs/ss only)
isoc_maxburst   - 0..15 (ss only)
bulk_buflen - buffer length
+   bulk_qlen   - depth of queue for bulk
+   iso_qlen- depth of queue for iso
 
 Testing the SOURCESINK function
 ---
-- 
1.9.1

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

[PATCH 3/3] Doc: ABI: configfs-usb-gadget-sourcesink: add two entries for depth of queue

[Peter Chen]

Add both bulk and iso depth of queue entries.

Signed-off-by: Peter Chen 
---
 Documentation/ABI/testing/configfs-usb-gadget-sourcesink | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-sourcesink 
b/Documentation/ABI/testing/configfs-usb-gadget-sourcesink
index bc7ff73..f56335a 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-sourcesink
+++ b/Documentation/ABI/testing/configfs-usb-gadget-sourcesink
@@ -10,3 +10,5 @@ Description:
isoc_mult   - 0..2 (hs/ss only)
isoc_maxburst   - 0..15 (ss only)
buflen  - buffer length
+   bulk_qlen   - depth of queue for bulk
+   iso_qlen- depth of queue for iso
-- 
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 1/2] cdc_acm: Ignore Infineon Flash Loader utility

[Johan Hovold]

On Thu, Nov 12, 2015 at 02:55:04PM +0100, Bjørn Mork wrote:
> Daniele Palmas  writes:

> > But I see that Infineon flashing devices in newer chipsets have become
> > an only bulk serial link device (see device 0x8087/0x0716 in
> > usb-serial-simple), so maybe this has a meaning...
> 
> Yes.  I have a Sierra Wireless EM7345 modem which is based on the same
> chipset. I haven't really paid much attemtion to the flashloader
> functionality before, but this is how that modem looks before it loads
> its Sierra firmware:

> So this device looks very much like an ACM device, except that it is
> missing the necessary control interface and status endpoint.  And
> without a control interface there is of course no way to send a properly
> formatted CDC control request either.
> 
> I assume the different vendors also use the same Intel provided firmware
> toolkit, making the firmwares share basic functionality like bootloader
> and flashing.  But the USB descriptors are likely configurable by the
> vendor.  Telit might have tried to "improve" the above into a proper ACM
> device.
> 
> I believe this supports the assumption that Infineon Flash Loader
> devices have some ACM descriptors without actually being ACM class
> devices, and that it is best to just collect them all in the
> usb-serial-simple "flashloader" driver.

I'm still not convinced. The device at hand does look and apparently
behaves like a compliant ACM device, and I think we need to figure out
why it does not work with this particular proprietary user-space tool
before modifying the kernel.

For example, using usb-serial-simple means that no line-control requests
are sent. Perhaps the tool simply needs to set the termios settings to
something else than the default 9600N81 that the cdc-acm driver uses.

Modem control-requests would not be sent either (and HUPCL determines if
DTR/RTS is lowered at close).

Daniele, could you provide some more details about what happens when the
proprietary tool fails. Do you have access to the sources?

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: Page allocation failure

[Steinar H. Gunderson]

On Fri, Nov 13, 2015 at 12:00:53AM +0100, Steinar H. Gunderson wrote:
> Interesting. I hacked libusb to get the fd. Then I did USBDEVFS_ALLOC_MEMORY,
> which succeeded (well, as soon as I filled mem.size before calling),
> then mmap on it as described (which also succeeded), and everything _works_,
> but I don't think it's actually using zerocopy, because I still see
> copy_user_enhanced_fast_string() using a lot of CPU
> (with libusb_handle_events() in the call stack).

OK, so I did some more testing. It does actually seem to do _something_.
In particular, memset_erms() all but disappeared from my profile, which was
stated in the thread as one advantage of the patch.

I ran overnight for eight hours (one card, continuous stream at about
900 Mbit/sec, 128 kB buffers) and it didn't get any page allocation features.
Before the patch, it was unlikely but not impossible to run for that long.
I've kept it running, but it's weak evidence of solving the problem.

As for general performance, I'm unsure; from a quick glance at perf, I _may_
have more L3 misses when reading the data back in userspace, but I won't say
anything for sure here without a closer look.

So, in general I think it's good news, although I don't fully understand why
I still need the kernel-to-userspace copy for isochronous transfer.

/* Steinar */
-- 
Homepage: https://www.sesse.net/
--
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 3/7] usb: gadget: define free_ep_req as universal function

[Robert Baldyga]

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> This function is shared between gadget functions, so this avoid unnecessary
> duplicated code and potentially avoid memory leaks.
> 
> Signed-off-by: Felipe F. Tonello 

Reviewed-by: Robert Baldyga 

> ---
>  drivers/usb/gadget/function/f_midi.c   |  6 --
>  drivers/usb/gadget/function/f_sourcesink.c |  6 --
>  drivers/usb/gadget/function/g_zero.h   |  1 -
>  drivers/usb/gadget/u_f.c   |  1 -
>  drivers/usb/gadget/u_f.h   | 10 --
>  5 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 488111d..f36db2d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -201,12 +201,6 @@ static inline struct usb_request 
> *midi_alloc_ep_req(struct usb_ep *ep,
>   return alloc_ep_req(ep, length, length);
>  }
>  
> -static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
> -{
> - kfree(req->buf);
> - usb_ep_free_request(ep, req);
> -}
> -
>  static const uint8_t f_midi_cin_length[] = {
>   0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
>  };
> diff --git a/drivers/usb/gadget/function/f_sourcesink.c 
> b/drivers/usb/gadget/function/f_sourcesink.c
> index d7646d3..74f95b1 100644
> --- a/drivers/usb/gadget/function/f_sourcesink.c
> +++ b/drivers/usb/gadget/function/f_sourcesink.c
> @@ -303,12 +303,6 @@ static inline struct usb_request *ss_alloc_ep_req(struct 
> usb_ep *ep, int len)
>   return alloc_ep_req(ep, len, ss->buflen);
>  }
>  
> -void free_ep_req(struct usb_ep *ep, struct usb_request *req)
> -{
> - kfree(req->buf);
> - usb_ep_free_request(ep, req);
> -}
> -
>  static void disable_ep(struct usb_composite_dev *cdev, struct usb_ep *ep)
>  {
>   int value;
> diff --git a/drivers/usb/gadget/function/g_zero.h 
> b/drivers/usb/gadget/function/g_zero.h
> index 15f1809..5ed90b4 100644
> --- a/drivers/usb/gadget/function/g_zero.h
> +++ b/drivers/usb/gadget/function/g_zero.h
> @@ -59,7 +59,6 @@ void lb_modexit(void);
>  int lb_modinit(void);
>  
>  /* common utilities */
> -void free_ep_req(struct usb_ep *ep, struct usb_request *req);
>  void disable_endpoints(struct usb_composite_dev *cdev,
>   struct usb_ep *in, struct usb_ep *out,
>   struct usb_ep *iso_in, struct usb_ep *iso_out);
> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
> index c6276f0..4bc7eea 100644
> --- a/drivers/usb/gadget/u_f.c
> +++ b/drivers/usb/gadget/u_f.c
> @@ -11,7 +11,6 @@
>   * published by the Free Software Foundation.
>   */
>  
> -#include 
>  #include "u_f.h"
>  
>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
> diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
> index 1d5f0eb..4247cc0 100644
> --- a/drivers/usb/gadget/u_f.h
> +++ b/drivers/usb/gadget/u_f.h
> @@ -16,6 +16,8 @@
>  #ifndef __U_F_H__
>  #define __U_F_H__
>  
> +#include 
> +
>  /* Variable Length Array Macros 
> **/
>  #define vla_group(groupname) size_t groupname##__next = 0
>  #define vla_group_size(groupname) groupname##__next
> @@ -45,8 +47,12 @@
>  struct usb_ep;
>  struct usb_request;
>  
> +/* Requests allocated via alloc_ep_req() must be freed by free_ep_req(). */
>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int 
> default_len);
> +static inline void free_ep_req(struct usb_ep *ep, struct usb_request *req)
> +{
> + kfree(req->buf);
> + usb_ep_free_request(ep, req);
> +}
>  
>  #endif /* __U_F_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

Re: [PATCH v5 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
> On 10/11/15 18:43, Sergei Shtylyov wrote:
>> On 11/10/2015 08:52 PM, Felipe F. Tonello wrote:
>>> @@ -75,6 +75,7 @@ struct f_midi {
>>>   struct usb_ep*in_ep, *out_ep;
>>>   struct snd_card*card;
>>>   struct snd_rawmidi*rmidi;
>>> +u8ms_id;
>>
>>   Why 'ms_id' is not aligned with the above field names?
>
> It is actually aligned.

It's not in the original mail, which contains tab characters.

> Here is from my local git diff:
>
> @@ -75,6 +75,7 @@ struct f_midi {
> struct usb_ep   *in_ep, *out_ep;
> struct snd_card *card;
> struct snd_rawmidi  *rmidi;
> +   u8  ms_id;

Apparently, you're using four spaces per tab.  Linux uses eight.


Regards,
Clemens
--
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 0/4 V2] usb: dwc2: fix usb host support on Raspberry Pi

[Marek Szyprowski]

Hello,

On 2015-11-12 22:08, Stefan Wahren wrote:

This patch series fixes multiple issues on Raspberry Pi which
were reproducable since commit 09a75e857790
("usb: dwc2: refactor common low-level hw code to platform.c")

Changes in V2:
- add fix for kernel oops
- extend "usb: dwc2: Return errors from PHY" to handle kernel
   without PHY support
- take care of Sergei Shtylyov comments
- add patch to make otg clk optional again

John Youn (1):
   usb: dwc2: Make PHY optional

Stefan Wahren (3):
   usb: dwc2: fix kernel oops during driver probe
   usb: dwc2: Return errors from PHY
   usb: dwc2: make otg clk optional

  drivers/usb/dwc2/platform.c |   81 ---
  1 file changed, 53 insertions(+), 28 deletions(-)


Tested-by: Marek Szyprowski 

Works fine on both OdroidU3 and Trats2 boards (both Exynos4412 based).

Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

--
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 2/7] usb: gadget: f_midi: remove duplicated code

[Robert Baldyga]

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> This code is duplicated from f_midi_start_ep(midi, f, midi->out_ep).
> 
> Signed-off-by: Felipe F. Tonello 

Reviewed-by: Robert Baldyga 

> ---
>  drivers/usb/gadget/function/f_midi.c | 19 ---
>  1 file changed, 19 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index eeb989d..488111d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -324,7 +324,6 @@ static int f_midi_start_ep(struct f_midi *midi,
>  static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned 
> alt)
>  {
>   struct f_midi *midi = func_to_midi(f);
> - struct usb_composite_dev *cdev = f->config->cdev;
>   unsigned i;
>   int err;
>  
> @@ -340,24 +339,6 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>   if (err)
>   return err;
>  
> - usb_ep_disable(midi->out_ep);
> -
> - err = config_ep_by_speed(midi->gadget, f, midi->out_ep);
> - if (err) {
> - ERROR(cdev, "can't configure %s: %d\n",
> -   midi->out_ep->name, err);
> - return err;
> - }
> -
> - err = usb_ep_enable(midi->out_ep);
> - if (err) {
> - ERROR(cdev, "can't start %s: %d\n",
> -   midi->out_ep->name, err);
> - return err;
> - }
> -
> - midi->out_ep->driver_data = midi;
> -
>   /* allocate a bunch of read buffers and queue them all at once. */
>   for (i = 0; i < midi->qlen && err == 0; i++) {
>   struct usb_request *req =
> 

--
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/7] usb: gadget: f_midi: Transmit data only when IN ep is enabled

[Robert Baldyga]

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> This makes sure f_midi doesn't try to enqueue data when the IN endpoint is
> disabled, ie, USB cable is disconnected.
> 
> Signed-off-by: Felipe F. Tonello 

Reviewed-by: Robert Baldyga 

> ---
>  drivers/usb/gadget/function/f_midi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index ce3c8a6..eeb989d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -545,7 +545,7 @@ static void f_midi_transmit(struct f_midi *midi, struct 
> usb_request *req)
>   }
>   }
>  
> - if (req->length > 0) {
> + if (req->length > 0 && ep->enabled) {
>   int err;
>  
>   err = usb_ep_queue(ep, req, GFP_ATOMIC);
> 

--
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 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface

[Clemens Ladisch]

Felipe F. Tonello wrote:
> This avoids duplication of USB requests for OUT endpoint and
> re-enabling endpoints.

>  ...
>   /* For Control Device interface we do nothing */
> - if (intf == 0)
> + if (intf != midi->ms_id)
>   return 0;

The comment now is misleading.


Regards,
Clemens
--
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 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests

[Robert Baldyga]

Hi Felipe,

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> This patch fixes a memory leak that occurs when an endpoint fails to enqueue
> the request. If that happens the complete function will never be called, thus
> never freeing the request.
> 
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/gadget/function/f_midi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index f36db2d..76ea53c 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>   if (err) {
>   ERROR(midi, "%s queue req: %d\n",
>   midi->out_ep->name, err);
> + free_ep_req(midi->out_ep, req);
>   }
>   }
>  
> 

There is one more thing I haven't noticed before. We can have situation
when all requests were allocated successfully, but their allocation
failed. What we get then is set_alt() returning 0, while no request is
allocated, hence the function is, in fact, inactive.

Best regards,
Robert
--
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 7/7] usb: gadget: f_midi: pre-allocate IN requests

[Clemens Ladisch]

Felipe F. Tonello wrote:
> This patch introduces pre-allocation of IN endpoint USB requests. This
> improves on latency (requires no usb request allocation on transmit) and avoid
> several potential probles on allocating too many usb requests (which involves
> DMA pool allocation problems).
>
> This implementation also handles better multiple MIDI Gadget ports, always
> processing the last processed MIDI substream if the last USB request wasn't
> enought to handle the whole stream.

> ...
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -88,6 +89,9 @@ struct f_midi {
>   int index;
>   char *id;
>   unsigned int buflen, qlen;
> + DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
> + unsigned int in_req_num;
> + unsigned int in_last_port;

As far as I can see, in_req_num must always have the same value as qlen.

> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
> + while (!kfifo_is_empty(>in_req_fifo)) {
> + ...
> + len = kfifo_get(>in_req_fifo, );
> + if (len == 1)
> + free_ep_req(midi->in_ep, req);
> + else
> + ERROR(midi, "%s couldn't free usb request: something 
> went wrong with kfifo\n",
> +   midi->in_ep->name);
> + }

kfifo_get() already checks for the FIFO being empty, so you can just
drop kfifi_is_empty().

> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request 
> *req,
> ...
> +static void f_midi_transmit(struct f_midi *midi)
> +{
> +...
> + len = kfifo_peek(>in_req_fifo, );
> + ...
> + if (req->length > 0) {
> + WARNING(midi, "%s: All USB requests have been used. 
> Current queue size "
> + "is %u, consider increasing it.\n", __func__, 
> midi->in_req_num);
> + goto drop_out;
> + }

There are two cases where the in_req FIFO might overflow:
1) the gadget is trying to send too much data at once; or
2) the host does not bother to read any of the data.

In case 1), the appropriate action would be to do nothing, so that the
remaining data is sent after some currently queued packets have been
transmitted.  In case 2), the appropriate action would be to drop the
data (even better, the _oldest_ data), and spamming the log with error
messages would not help.

This code shows the error message for case 1), but does the action for
case 2).

I'm not quite sure if trying to detect which of these cases we have is
possible, or worthwhile.  Anyway, with a packet size of 64, the queue
size would be 32*64 = 2KB, which should be enough for everyone.  So I
propose to ignore case 1), and to drop the error message.

> @@ -1130,6 +1222,12 @@ static struct usb_function *f_midi_alloc(struct 
> usb_function_instance *fi)
> + if (kfifo_alloc(>in_req_fifo, midi->qlen, GFP_KERNEL))
> + goto setup_fail;

The setup_fail code expects an error code in the status variable.


Regards,
Clemens
--
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/7] usb: gadget: gmidi: Cleanup legacy code

[Robert Baldyga]

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> Remove unnecessary headers and variables.
> 
> Signed-off-by: Felipe F. Tonello 

Reviewed-by: Robert Baldyga 

> ---
>  drivers/usb/gadget/legacy/gmidi.c | 8 
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/usb/gadget/legacy/gmidi.c 
> b/drivers/usb/gadget/legacy/gmidi.c
> index 8a18348..be8e91d 100644
> --- a/drivers/usb/gadget/legacy/gmidi.c
> +++ b/drivers/usb/gadget/legacy/gmidi.c
> @@ -21,19 +21,12 @@
>  /* #define VERBOSE_DEBUG */
>  
>  #include 
> -#include 
>  #include 
> -#include 
>  
> -#include 
>  #include 
> -#include 
>  
> -#include 
>  #include 
>  #include 
> -#include 
> -#include 
>  
>  #include "u_midi.h"
>  
> @@ -42,7 +35,6 @@
>  MODULE_AUTHOR("Ben Williamson");
>  MODULE_LICENSE("GPL v2");
>  
> -static const char shortname[] = "g_midi";
>  static const char longname[] = "MIDI Gadget";
>  
>  USB_GADGET_COMPOSITE_OPTIONS();
> 

--
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 1/1] usb: misc: usbtest: improve the description fo error message

[Peter Chen]

Now the function of complicated_callback is not only used for iso
transfer, improve the error message to reflect it.

Signed-off-by: Peter Chen 
---
 drivers/usb/misc/usbtest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 637f3f7..c1bd1eb 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -1849,7 +1849,7 @@ static void complicated_callback(struct urb *urb)
goto done;
default:
dev_err(>dev->intf->dev,
-   "iso resubmit err %d\n",
+   "resubmit err %d\n",
status);
/* FALLTHROUGH */
case -ENODEV:   /* disconnected */
@@ -1863,7 +1863,7 @@ static void complicated_callback(struct urb *urb)
if (ctx->pending == 0) {
if (ctx->errors)
dev_err(>dev->intf->dev,
-   "iso test, %lu errors out of %lu\n",
+   "during the test, %lu errors out of %lu\n",
ctx->errors, ctx->packet_count);
complete(>done);
}
-- 
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 v3 5/5] Add ioctls to enable and disable local controls on an instrument

[Dave Penkler]

Hi Andy,
On Wed, Nov 11, 2015 at 09:36:41PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 11, 2015 at 1:21 PM, Dave Penkler  wrote:
> > These ioctls provide support for the USBTMC-USB488 control requests
> > for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT

snip

> > +   goto exit;
> > +   }
> > +   wValue = (val) ? 1 : 0;
> 
> !!val;
I checked by compiling both variants - the compiler emits identical code
for both so I prefer to keep the original as it is more obvious for
maintainers reading the code.

regards,
-Dave

--
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] cdc_acm: Ignore Infineon Flash Loader utility

[Johan Hovold]

On Fri, Nov 13, 2015 at 03:13:41PM +0100, Daniele Palmas wrote:
> 2015-11-13 11:58 GMT+01:00 Johan Hovold :
> > On Thu, Nov 12, 2015 at 02:55:04PM +0100, Bjørn Mork wrote:

> >> I believe this supports the assumption that Infineon Flash Loader
> >> devices have some ACM descriptors without actually being ACM class
> >> devices, and that it is best to just collect them all in the
> >> usb-serial-simple "flashloader" driver.
> >
> > I'm still not convinced. The device at hand does look and apparently
> > behaves like a compliant ACM device, and I think we need to figure out
> > why it does not work with this particular proprietary user-space tool
> > before modifying the kernel.
> >
> > For example, using usb-serial-simple means that no line-control requests
> > are sent. Perhaps the tool simply needs to set the termios settings to
> > something else than the default 9600N81 that the cdc-acm driver uses.
> >
> > Modem control-requests would not be sent either (and HUPCL determines if
> > DTR/RTS is lowered at close).
> >
> > Daniele, could you provide some more details about what happens when the
> > proprietary tool fails. Do you have access to the sources?
> 
> I'll try to explain how the whole thing should work.

> Using the cdc-acm driver, the string, though being sent in the same
> way than using the usb-serial-simple driver (I can confirm that the
> data is passing properly since I used an hw usb sniffer), does not
> make the device to stay in flashing mode.
> 
> I thought that the problem was something done by the acm driver when
> opening the port that is not accepted well by the flashing device on
> the firmware side, so I tried also in the past to debug the driver,
> but without success.
> 
> I can start again to understand which is really the problem with
> cdc-acm, but my fear is that to have a working solution with cdc-acm
> will be more complicated (e.g. adding a new quirk) than adding support
> in the usb-serial-simple driver. Consider also that this is not really
> a communication device with subclass modem, as wrongly stated by the
> usb descriptors, since it is simply a flashing device.
> 
> Another interesting piece of the puzzle is that in Windows this device
> requires a specific flashloader driver and does not use the acm driver
> suggested by the vendor.

Thanks for the details. I agree with you now, moving the device to
usb-serial-simple makes sense.

I'll take a look at these patches when 4.4-rc1 is out.

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

[PATCH 2/4 V2 RESEND] usb: dwc2: Make PHY optional

[Stefan Wahren]

From: John Youn 

Fixes commit 09a75e857790
("usb: dwc2: refactor common low-level hw code to platform.c")

The above commit consolidated the low-level phy access into a common
location. This change introduced a check from the gadget requiring
that a PHY is specified. This requirement never existed on the host
side and broke some platforms when it was moved into platform.c.

The gadget doesn't require the PHY either so remove the check.

Reported-by: Stefan Wahren 
Cc: Marek Szyprowski 
Signed-off-by: John Youn 
Tested-by: Marek Szyprowski 
Fixes: 09a75e857790 ("usb: dwc2: refactor common low-level hw code to 
platform.c")
---
 drivers/usb/dwc2/platform.c |5 -
 1 file changed, 5 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 0e80087..702012a 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -228,11 +228,6 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
hsotg->phyif = GUSBCFG_PHYIF8;
}
 
-   if (!hsotg->phy && !hsotg->uphy && !hsotg->plat) {
-   dev_err(hsotg->dev, "no platform data or transceiver 
defined\n");
-   return -EPROBE_DEFER;
-   }
-
/* Clock */
hsotg->clk = devm_clk_get(hsotg->dev, "otg");
if (IS_ERR(hsotg->clk)) {
-- 
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

Re: Page allocation failure

[Steinar H. Gunderson]

On Fri, Nov 13, 2015 at 10:38:54AM -0500, Alan Stern wrote:
>> So, in general I think it's good news, although I don't fully understand why
>> I still need the kernel-to-userspace copy for isochronous transfer.
> Maybe you can add some debugging to copy_user_enhanced_fast_string().  
> Add a flag, and call dump_stack() in that routine if the flag is set.  
> Then set and clear this flag at the appropriate spots in devio.c.

What exactly am I looking for, beyond the stack trace the kernel already
gives me?

/* Steinar */
-- 
Homepage: https://www.sesse.net/
--
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 04/12] usb: xhci: dbc: add support for Intel xHCI dbc quirk

[Dmitry Malkin]

On Mon, 9 Nov 2015 15:38:33 +0800, Lu Baolu wrote:
> On Intel platform, if the debug target is connected with debug
> host, enabling DCE bit in command register leads to a hung bus
> state. In the hung state, the host system will not see a port
> connected status bit set. Hence debug target fails to be probed.
>
> The state could be resolved by performing a port reset to the
> debug port from the host xHCI. This patch introduces this work
> around.

Is it correct to call this a "hung bus state"?  Wouldn't calling it
"hung port state" more appropriate?

I have observed this DCE-enable-related hung port state,
but the reason seemed to be different in my case:

Citing a note (sic!) from The Holy XHCI Spec, section 7.6.4.1:
> If a Debug Host attempts to attach to a Debug Target before the DCE flag is 
> set,
> both ends of the link shall transition to the Inactive state.
> So a Debug Host should periodically issue a Warm Reset
> to ports that are Inactive to enable a connection to the DbC of the Debug 
> Target.

Indeed, the inactive state is what I have observed (PLS field of port register 
PORTSC)
when the DCE bit is set to 1 with the cable already plugged in.

Now, according to my interpretation of The Hole USB 3.1 spec, section 7.5.2,
which says:
> eSS.Inactive is a state where a link has failed Enhanced SuperSpeed operation.
> A downstream port can only exit from this state when directed, or upon 
> detection of
> an absence of a far-end receiver termination (R RX-DC ) specified in Table 
> 6-21,
> or upon a Warm Reset.

It follows, that since hosts without DBC cannot listen to upstream requests,
the debug target-originated port reset requests (both hot and warm) will be 
ignored
by the debug host.

This is the essence of the "hung port state" that I was able to observe.

Note, that this roadblock doesn't appear if you attach the cable /after/ 
enabling the DCE bit,
or, alternatively, if the host has DBC.

And indeed, your quirk will work in the latter case, since the debug host hub
will be able to see the upstream reset request.

--
with best regards,
Dmitry Malkin
--
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 1/3] usb: core: lpm: fix usb3_hardware_lpm sysfs node

[Alan Stern]

On Fri, 13 Nov 2015, Lu, Baolu wrote:

> On 11/13/2015 12:20 AM, Alan Stern wrote:
> > On Thu, 12 Nov 2015, Lu Baolu wrote:
> >
> >> Commit 655fe4effe0f ("usbcore: add sysfs support to xHCI usb3
> >> hardware LPM") introduced usb3_hardware_lpm sysfs node. This
> >> doesn't show the correct status of USB3 U1 and U2 LPM status.
> >>
> >> This patch fixes this by replacing usb3_hardware_lpm with two
> >> nodes, usb3_hardware_lpm_u1 (for U1) and usb3_hardware_lpm_u2
> >> (for U2), and recording the U1/U2 LPM status in right places.
> >>
> >> This patch should be back-ported to kernels as old as 4.3,
> >> that contains Commit 655fe4effe0f ("usbcore: add sysfs support
> >> to xHCI usb3 hardware LPM").
> >>
> >> Cc: sta...@vger.kernel.org
> >> Signed-off-by: Lu Baolu 
> > ...
> >
> >> --- a/drivers/usb/core/hub.c
> >> +++ b/drivers/usb/core/hub.c
> >> @@ -3875,17 +3875,23 @@ static void usb_enable_link_state(struct usb_hcd 
> >> *hcd, struct usb_device *udev,
> >>return;
> >>}
> >>   
> >> -  if (usb_set_lpm_timeout(udev, state, timeout))
> >> +  ret = usb_set_lpm_timeout(udev, state, timeout);
> >> +  if (ret)
> >>/* If we can't set the parent hub U1/U2 timeout,
> >> * device-initiated LPM won't be allowed either, so let the xHCI
> >> * host know that this link state won't be enabled.
> >> */
> >>hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state);
> >> -
> >>/* Only a configured device will accept the Set Feature U1/U2_ENABLE */
> >>else if (udev->actconfig)
> >>usb_set_device_initiated_lpm(udev, state, true);
> >>   
> >> +  if (!ret) {
> >> +  if (state == USB3_LPM_U1)
> >> +  udev->usb3_lpm_u1_enabled = 1;
> >> +  else if (state == USB3_LPM_U2)
> >> +  udev->usb3_lpm_u2_enabled = 1;
> >> +  }
> > This doesn't look right at all.  What happens if ret is 0 but the
> > device isn't configured?  You'll set the usb3_lpm_u*_enabled flag even
> > though LPM isn't really enabled.
> >
> > Don't you want to set these flags inside the
> > usb_set_device_initiated_lpm() function, where you know whether the
> > action succeeded?  And leave this routine unchanged?
> 
> My understand is that both hub and device can initiate LPM.
> As soon as usb_set_lpm_timeout(valid_timeout_value)
> returns 0, the hub-initiated LPM is enabled. Thus, LPM is
> enabled no matter the result of usb_set_device_initiated_lpm().
> The only difference is whether device is able to initiate LPM.
> 
> On disable side, as soon as usb_set_lpm_timeout(0) return 0,
> hub initiated LPM is disabled. Hub will disallows link to enter
> U1/U2 as well, even device is initiating LPM. Hence LPM
> is disabled as soon as hub LPM timeout set to 0, no matter
> device-initiated LPM is disabled or not.

Then maybe you can add a comment explaining this.

The patch still looks strange, though.  Your new code does this:

ret = usb_set_lpm_timeout(...);
if (ret)
...
else if (udev->actconfig)
...
if (!ret) {
if (state == USB3_LPM_U1)
...
}

It would be better to do this:

if (usb_set_lpm_timeout(...)) {
...
} else {
if (udev->actconfig)
...
if (state == USB3_LPM_U1)
...
}

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 v4 1/4] fs: configfs: Drop unused parameter from configfs_undepend_item()

[Andrzej Pietrasiewicz]

Hello,

W dniu 26.10.2015 o 17:43, Krzysztof Opasiak pisze:

subsys parameter is never used by configfs_undepend_item()
so there is no point in passing it to this function.

Signed-off-by: Krzysztof Opasiak 
---
Changes since v3:
- fix build break in ocfs2



Any comments on the series adding unlocked versions of
configfs_(un)depend_item()?

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

Re: [PATCH 2/4 V2] usb: dwc2: Make PHY optional

[Stefan Wahren]

Am 13.11.2015 um 13:25 schrieb Sergei Shtylyov:
> Hello.
>
> On 11/13/2015 12:08 AM, Stefan Wahren wrote:
>
>> From: John Youn 
>>
>> Fixes commit 09a75e857790
>> ("usb: dwc2: refactor common low-level hw code to platform.c")
>>
>> The above commit consolidated the low-level phy access into a common
>> location. This change introduced a check from the gadget requiring
>> that a PHY is specified. This requirement never existed on the host
>> side and broke some platforms when it was moved into platform.c.
>>
>> The gadget doesn't require the PHY either so remove the check.
>>
>> Reported-by: Stefan Wahren 
>> Cc: Marek Szyprowski 
>> Signed-off-by: John Youn 
>> Fixes: 09a75e8577901489f77a14a3b305a9a1f67bf25b
>
>This tag has standardized commit citing format (the same one you've
> used in the first sentence of the change log).
>

Thanks for pointing out, i will fix the affected patches and resend.

Stefan

> [...]
>
> 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: Page allocation failure

[Alan Stern]

On Fri, 13 Nov 2015, Steinar H. Gunderson wrote:

> On Fri, Nov 13, 2015 at 12:00:53AM +0100, Steinar H. Gunderson wrote:
> > Interesting. I hacked libusb to get the fd. Then I did 
> > USBDEVFS_ALLOC_MEMORY,
> > which succeeded (well, as soon as I filled mem.size before calling),
> > then mmap on it as described (which also succeeded), and everything _works_,
> > but I don't think it's actually using zerocopy, because I still see
> > copy_user_enhanced_fast_string() using a lot of CPU
> > (with libusb_handle_events() in the call stack).
> 
> OK, so I did some more testing. It does actually seem to do _something_.
> In particular, memset_erms() all but disappeared from my profile, which was
> stated in the thread as one advantage of the patch.
> 
> I ran overnight for eight hours (one card, continuous stream at about
> 900 Mbit/sec, 128 kB buffers) and it didn't get any page allocation features.
> Before the patch, it was unlikely but not impossible to run for that long.
> I've kept it running, but it's weak evidence of solving the problem.
> 
> As for general performance, I'm unsure; from a quick glance at perf, I _may_
> have more L3 misses when reading the data back in userspace, but I won't say
> anything for sure here without a closer look.
> 
> So, in general I think it's good news, although I don't fully understand why
> I still need the kernel-to-userspace copy for isochronous transfer.

Maybe you can add some debugging to copy_user_enhanced_fast_string().  
Add a flag, and call dump_stack() in that routine if the flag is set.  
Then set and clear this flag at the appropriate spots in devio.c.

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: Page allocation failure

[Alan Stern]

On Fri, 13 Nov 2015, Steinar H. Gunderson wrote:

> On Fri, Nov 13, 2015 at 10:38:54AM -0500, Alan Stern wrote:
> >> So, in general I think it's good news, although I don't fully understand 
> >> why
> >> I still need the kernel-to-userspace copy for isochronous transfer.
> > Maybe you can add some debugging to copy_user_enhanced_fast_string().  
> > Add a flag, and call dump_stack() in that routine if the flag is set.  
> > Then set and clear this flag at the appropriate spots in devio.c.
> 
> What exactly am I looking for, beyond the stack trace the kernel already
> gives me?

Find out where copy_user_enhanced_fast_string is being called from, and 
using that information, figure out why it was called.  That will tell 
you why this approach isn't yielding true zerocopy performance.

Alan Stern

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

[PATCH 1/1] net: usb: cdc_ether: add Dell DW5580 as a mobile broadband adapter

[Daniele Palmas]

Since Dell DW5580 is a 3G modem, this patch adds the device as a
mobile broadband adapter

Signed-off-by: Daniele Palmas 
---
 drivers/net/usb/cdc_ether.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 35a2bff..5e92076 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -764,6 +764,11 @@ static const struct usb_device_id  products[] = {
USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
.driver_info = (kernel_ulong_t) _info,
 }, {
+   /* Dell DW5580 modules */
+   USB_DEVICE_AND_INTERFACE_INFO(DELL_VENDOR_ID, 0x81ba, USB_CLASS_COMM,
+   USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
+   .driver_info = (kernel_ulong_t)_info,
+}, {
USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_ETHERNET,
USB_CDC_PROTO_NONE),
.driver_info = (unsigned long) _info,
-- 
1.9.1

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

[PATCH 3/4 V2 RESEND] usb: dwc2: Return errors from PHY

[Stefan Wahren]

When searching for PHYs, any error was treated as if the PHY did not
exist or was not specified. Thus the probe function did not
correctly return error conditions such as -EPROBE_DEFER.

Fixed so that only a non-existing PHY is ignored and any other error
is returned.

Reported-by: Alexander Aring 
Signed-off-by: John Youn 
Signed-off-by: Stefan Wahren 
Tested-by: Marek Szyprowski 
---
 drivers/usb/dwc2/platform.c |   37 -
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 702012a..5c3704d 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -211,14 +211,41 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
 */
hsotg->phy = devm_phy_get(hsotg->dev, "usb2-phy");
if (IS_ERR(hsotg->phy)) {
-   hsotg->phy = NULL;
+   ret = PTR_ERR(hsotg->phy);
+   switch (ret) {
+   case -ENODEV:
+   case -ENOSYS:
+   hsotg->phy = NULL;
+   break;
+   case -EPROBE_DEFER:
+   return ret;
+   default:
+   dev_err(hsotg->dev, "error getting phy %d\n", ret);
+   return ret;
+   }
+   }
+
+   if (!hsotg->phy) {
hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2);
-   if (IS_ERR(hsotg->uphy))
-   hsotg->uphy = NULL;
-   else
-   hsotg->plat = dev_get_platdata(hsotg->dev);
+   if (IS_ERR(hsotg->uphy)) {
+   ret = PTR_ERR(hsotg->uphy);
+   switch (ret) {
+   case -ENODEV:
+   case -ENXIO:
+   hsotg->uphy = NULL;
+   break;
+   case -EPROBE_DEFER:
+   return ret;
+   default:
+   dev_err(hsotg->dev, "error getting usb phy 
%d\n",
+   ret);
+   return ret;
+   }
+   }
}
 
+   hsotg->plat = dev_get_platdata(hsotg->dev);
+
if (hsotg->phy) {
/*
 * If using the generic PHY framework, check if the PHY bus
-- 
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 1/4 V2 RESEND] usb: dwc2: fix kernel oops during driver probe

[Stefan Wahren]

This patch make sure that all necessary members of dwc2_hsotg
are initialized before the irq handler is requested. So
the kernel oops triggered by dwc2_handle_common_intr has
been fixed.

  dwc2 2098.usb: Configuration mismatch. Forcing host mode
  dwc2 2098.usb: no platform data or transceiver defined
  Unable to handle kernel paging request at virtual address cc860040
  pgd = c0004000
  [cc860040] *pgd=0b41e811, *pte=, *ppte=
  Internal error: Oops: 7 [#1] ARM
  CPU: 0 PID: 1 Comm: swapper Not tainted 4.3.0-rc3+ #19
  Hardware name: BCM2835
  task: cb494000 ti: cb4d task.ti: cb4d
  PC is at dwc2_is_controller_alive+0x18/0x34
  LR is at dwc2_handle_common_intr+0x24/0xb60

Signed-off-by: Stefan Wahren 
Acked-by: John Youn 
Tested-by: Marek Szyprowski 

---
 drivers/usb/dwc2/platform.c |   28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 5859b0f..0e80087 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -341,20 +341,6 @@ static int dwc2_driver_probe(struct platform_device *dev)
if (retval)
return retval;
 
-   irq = platform_get_irq(dev, 0);
-   if (irq < 0) {
-   dev_err(>dev, "missing IRQ resource\n");
-   return irq;
-   }
-
-   dev_dbg(hsotg->dev, "registering common handler for irq%d\n",
-   irq);
-   retval = devm_request_irq(hsotg->dev, irq,
- dwc2_handle_common_intr, IRQF_SHARED,
- dev_name(hsotg->dev), hsotg);
-   if (retval)
-   return retval;
-
res = platform_get_resource(dev, IORESOURCE_MEM, 0);
hsotg->regs = devm_ioremap_resource(>dev, res);
if (IS_ERR(hsotg->regs))
@@ -389,6 +375,20 @@ static int dwc2_driver_probe(struct platform_device *dev)
 
dwc2_set_all_params(hsotg->core_params, -1);
 
+   irq = platform_get_irq(dev, 0);
+   if (irq < 0) {
+   dev_err(>dev, "missing IRQ resource\n");
+   return irq;
+   }
+
+   dev_dbg(hsotg->dev, "registering common handler for irq%d\n",
+   irq);
+   retval = devm_request_irq(hsotg->dev, irq,
+ dwc2_handle_common_intr, IRQF_SHARED,
+ dev_name(hsotg->dev), hsotg);
+   if (retval)
+   return retval;
+
retval = dwc2_lowlevel_hw_enable(hsotg);
if (retval)
return retval;
-- 
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 0/4 V2 RESEND] usb: dwc2: fix usb host support on Raspberry Pi

[Stefan Wahren]

This patch series fixes multiple issues on Raspberry Pi which 
were reproducable since commit 09a75e857790
("usb: dwc2: refactor common low-level hw code to platform.c")

Changes in V2:
- add fix for kernel oops
- extend "usb: dwc2: Return errors from PHY" to handle kernel
  without PHY support
- take care of Sergei Shtylyov comments
- add patch to make otg clk optional again

John Youn (1):
  usb: dwc2: Make PHY optional

Stefan Wahren (3):
  usb: dwc2: fix kernel oops during driver probe
  usb: dwc2: Return errors from PHY
  usb: dwc2: make otg clk optional

 drivers/usb/dwc2/platform.c |   81 ---
 1 file changed, 53 insertions(+), 28 deletions(-)

-- 
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 4/4 V2 RESEND] usb: dwc2: make otg clk optional

[Stefan Wahren]

Fixes commit 09a75e857790
("usb: dwc2: refactor common low-level hw code to platform.c")

The above commit consolidated the low-level phy access into a common
location. This change made the otg clk a requirement and broke some
platforms when it was moved into platform.c.

So make clk handling optional again.

Signed-off-by: Stefan Wahren 
Cc: Marek Szyprowski 
Acked-by: John Youn 
Tested-by: Marek Szyprowski 
Fixes: 09a75e857790 ("usb: dwc2: refactor common low-level hw code to 
platform.c")
---
 drivers/usb/dwc2/platform.c |   11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 5c3704d..947c2e8 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -124,9 +124,11 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg 
*hsotg)
if (ret)
return ret;
 
-   ret = clk_prepare_enable(hsotg->clk);
-   if (ret)
-   return ret;
+   if (hsotg->clk) {
+   ret = clk_prepare_enable(hsotg->clk);
+   if (ret)
+   return ret;
+   }
 
if (hsotg->uphy)
ret = usb_phy_init(hsotg->uphy);
@@ -174,7 +176,8 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg 
*hsotg)
if (ret)
return ret;
 
-   clk_disable_unprepare(hsotg->clk);
+   if (hsotg->clk)
+   clk_disable_unprepare(hsotg->clk);
 
ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
 hsotg->supplies);
-- 
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

Re: Page allocation failure

[Alan Stern]

On Fri, 13 Nov 2015, Steinar H. Gunderson wrote:

> On Fri, Nov 13, 2015 at 11:24:39AM -0500, Alan Stern wrote:
> >> What exactly am I looking for, beyond the stack trace the kernel already
> >> gives me?
> > Find out where copy_user_enhanced_fast_string is being called from, and 
> > using that information, figure out why it was called.  That will tell 
> > you why this approach isn't yielding true zerocopy performance.
> 
> The stack trace is simple enough, although I fear there's some inlining going
> on:
> 
> -9,50% 9,50%  app   [kernel.kallsyms]  [k] 
> copy_user_enhanced_fast_string
>- copy_user_enhanced_fast_string
>   - 9,45% __GI___ioctl
>  - 9,30% op_handle_events
>   handle_events
>   libusb_handle_events_timeout_completed
>   libusb_handle_events
>   BMUSBCapture::usb_thread_func
>   execute_native_thread_routine
> 
> I'm not honestly sure if the patch even tries to do zerocopy for isochronous
> (it might be bulk only), but I'll try to understand what's going on.

The routine to look at is copy_urb_data_to_user() in devio.c.  As far
as I can see, the patch doesn't touch it.  That's undoubtedly where all 
the waste is coming from.  The routine shouldn't do anything if the 
data buffer was mmap'ed.

> For the better news: My program ran for eight hours more without hitting the
> page allocation failures. So I suppose the patch (with cooperation from user
> space, of course) really solves the immediate problem.

That's good news.

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: Page allocation failure

[Steinar H. Gunderson]

On Fri, Nov 13, 2015 at 11:24:39AM -0500, Alan Stern wrote:
>> What exactly am I looking for, beyond the stack trace the kernel already
>> gives me?
> Find out where copy_user_enhanced_fast_string is being called from, and 
> using that information, figure out why it was called.  That will tell 
> you why this approach isn't yielding true zerocopy performance.

The stack trace is simple enough, although I fear there's some inlining going
on:

-9,50% 9,50%  app   [kernel.kallsyms]  [k] 
copy_user_enhanced_fast_string
   - copy_user_enhanced_fast_string
  - 9,45% __GI___ioctl
 - 9,30% op_handle_events
  handle_events
  libusb_handle_events_timeout_completed
  libusb_handle_events
  BMUSBCapture::usb_thread_func
  execute_native_thread_routine

I'm not honestly sure if the patch even tries to do zerocopy for isochronous
(it might be bulk only), but I'll try to understand what's going on.

For the better news: My program ran for eight hours more without hitting the
page allocation failures. So I suppose the patch (with cooperation from user
space, of course) really solves the immediate problem.

/* Steinar */
-- 
Homepage: https://www.sesse.net/
--
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: Page allocation failure

[Steinar H. Gunderson]

On Fri, Nov 13, 2015 at 06:50:27PM +0100, Steinar H. Gunderson wrote:
> The stack trace is simple enough, although I fear there's some inlining going
> on:

OK, I guess since copy_user_enhanced_fast_string is an assembler function,
the unwinding doesn't work properly. I added a dump_stack() in
copy_urb_data_to_user() instead, which is probably a better place:

[   38.535633] Call Trace:
[   38.535640]  [] ? dump_stack+0x40/0x5d
[   38.535652]  [] ? copy_urb_data_to_user+0x15/0x110 
[usbcore]
[   38.535654]  [] ? processcompl+0x8d/0x130 [usbcore]
[   38.535656]  [] ? usbdev_do_ioctl+0xa3/0x1310 [usbcore]
[   38.535659]  [] ? usbdev_ioctl+0x5/0x10 [usbcore]
[   38.535663]  [] ? do_vfs_ioctl+0x2be/0x490
[   38.535666]  [] ? ktime_get_ts64+0x3a/0xe0
[   38.535668]  [] ? SyS_ioctl+0x71/0x80
[   38.535672]  [] ? entry_SYSCALL_64_fastpath+0x12/0x71

There's still the jump from processcompl to copy_urb_data_to_user, but I
guess this is the more relevant one. (The dump_stack() triggers hundreds of
times per second.)

I tried just not setting as->userbuffer if usbm == NULL, and lo and behold,
it actually seems to help. I'm wondering if the copy is just from the buffer
to itself? copy_user_enhanced_fast_string is now down at 0.26%, ie.
effectively nothing.

/* Steinar */
-- 
Homepage: https://www.sesse.net/
--
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: Page allocation failure

[Steinar H. Gunderson]

On Fri, Nov 13, 2015 at 03:20:56PM -0500, Alan Stern wrote:
>> I tried just not setting as->userbuffer if usbm == NULL, and lo and behold,
>> it actually seems to help.
> In fact, there's no need to call copy_urb_data_to_user at all if the 
> buffer lies in the mmap'ed area.

usbm != NULL is meant to signal this. It only checks for exact match, though,
not containment. (Well, matching start; the size can be smaller.)

>> I'm wondering if the copy is just from the buffer
>> to itself?
> Yes, it is.  I can't imagine why this wasn't handled in the original 
> patch.  Simple oversight?

Or perhaps something changed between patch submission and now? Or maybe it
was never properly tested. I don't know.

>> copy_user_enhanced_fast_string is now down at 0.26%, ie.
>> effectively nothing.
> Very good.

Of course, not all if this gets translated to actual savings. Previously,
the data copy faulted data in from main memory (lots of L3 misses), and that
cost now gets moved into my own userspace code. But that's fine; it's got to
be somewhere.

So what is the road from here? I guess the original questions about cache
coherency still apply, and that this is what I'm seeing in dmesg.

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

[PATCH] usb: musb: enable usb_dma parameter

[Bin Liu]

Change the permission of usb_dma parameter so it can be used for runtime
debug without reboot.

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

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 0fcf01f..6aa01b8 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1655,7 +1655,7 @@ EXPORT_SYMBOL_GPL(musb_interrupt);
 static bool use_dma = 1;
 
 /* "modprobe ... use_dma=0" etc */
-module_param(use_dma, bool, 0);
+module_param(use_dma, bool, 0644);
 MODULE_PARM_DESC(use_dma, "enable/disable use of DMA");
 
 void musb_dma_completion(struct musb *musb, u8 epnum, u8 transmit)
-- 
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: Page allocation failure

[Steinar H. Gunderson]

On Fri, Nov 13, 2015 at 04:02:48PM -0500, Alan Stern wrote:
>> So what is the road from here? I guess the original questions about cache
>> coherency still apply, and that this is what I'm seeing in dmesg.
> What questions?  It should be obvious that the user program should not 
> touch the buffer contents while the transfer is taking place.

The subthread I'm thinking of starts at 

  http://marc.info/?l=linux-usb=138091207413756=2

I can't claim to have gone deeply into the details, though.

> What are you seeing in dmesg?

Several copies of

[ 1175.838536] x86/PAT: app:2838 map pfn RAM range req uncached-minus for [mem 
0x9fa4c000-0x9fa4], got write-back

> The next step would be to massage the patch and get it into a form
> suitable for applying.  This may well include changing the way the API
> works; for example, I'm not sure that allocating memory should be a
> separate step from mmap.

Yes, it sounds a bit odd to me, too.

I suppose there's no way to let userspace allocate this memory? Again,
for me personally it would be ideal to be able to give it in from a PBO
(ie., GPU memory).

/* Steinar */
-- 
Homepage: https://www.sesse.net/
--
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: Page allocation failure

[Alan Stern]

On Fri, 13 Nov 2015, Steinar H. Gunderson wrote:

> On Fri, Nov 13, 2015 at 06:50:27PM +0100, Steinar H. Gunderson wrote:
> > The stack trace is simple enough, although I fear there's some inlining 
> > going
> > on:
> 
> OK, I guess since copy_user_enhanced_fast_string is an assembler function,
> the unwinding doesn't work properly. I added a dump_stack() in
> copy_urb_data_to_user() instead, which is probably a better place:
> 
> [   38.535633] Call Trace:
> [   38.535640]  [] ? dump_stack+0x40/0x5d
> [   38.535652]  [] ? copy_urb_data_to_user+0x15/0x110 
> [usbcore]
> [   38.535654]  [] ? processcompl+0x8d/0x130 [usbcore]
> [   38.535656]  [] ? usbdev_do_ioctl+0xa3/0x1310 [usbcore]
> [   38.535659]  [] ? usbdev_ioctl+0x5/0x10 [usbcore]
> [   38.535663]  [] ? do_vfs_ioctl+0x2be/0x490
> [   38.535666]  [] ? ktime_get_ts64+0x3a/0xe0
> [   38.535668]  [] ? SyS_ioctl+0x71/0x80
> [   38.535672]  [] ? entry_SYSCALL_64_fastpath+0x12/0x71
> 
> There's still the jump from processcompl to copy_urb_data_to_user, but I
> guess this is the more relevant one. (The dump_stack() triggers hundreds of
> times per second.)
> 
> I tried just not setting as->userbuffer if usbm == NULL, and lo and behold,
> it actually seems to help.

In fact, there's no need to call copy_urb_data_to_user at all if the 
buffer lies in the mmap'ed area.

> I'm wondering if the copy is just from the buffer
> to itself?

Yes, it is.  I can't imagine why this wasn't handled in the original 
patch.  Simple oversight?

> copy_user_enhanced_fast_string is now down at 0.26%, ie.
> effectively nothing.

Very good.

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: Page allocation failure

[Alan Stern]

On Fri, 13 Nov 2015, Steinar H. Gunderson wrote:

> So what is the road from here? I guess the original questions about cache
> coherency still apply, and that this is what I'm seeing in dmesg.

What questions?  It should be obvious that the user program should not 
touch the buffer contents while the transfer is taking place.  What are 
you seeing in dmesg?

The next step would be to massage the patch and get it into a form
suitable for applying.  This may well include changing the way the API
works; for example, I'm not sure that allocating memory should be a
separate step from mmap.

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

[Doug Anderson]

John,

On Thu, Nov 12, 2015 at 9:05 PM, John Youn  wrote:
> It seems to be an issue with single TT hubs. I've tried several
> multi-TT hubs with no issues.

Agreed.


> With a single TT hub I do see a problem though not the exact one
> described. I see corrupted and dropped packets on the FS side of
> the hub. In a microframe with SSPLIT.begin, taking up the max
> bandwidth for the microframe, when another SSPLIT for a different
> device is issued, the data gets corrupted on the other side of
> the TT. Probably due to exceeding the bandwidth in the microframe
> since a single TT hub's ports all share the bandwidth.

Seems like different single TT hubs react differently.  I got one
where the mouse kept working but the audio was just static...


> With this fix, the next SSPLIT goes out in the same microframe as
> the SSPLIT.end and the data goes through fine.
>
> However I don't think this will work as a general fix. Since it
> is just skipping things without rescheduling. For example SSPLIT
> now happens a microframe later but the CSPLIT is not adjusted so
> it comes a microframe too early.
>
> I think the correct fix is to create a proper schedule based on
> all the active endpoints to make sure we don't go over the
> bandwidth for a single TT hub. Or to make the adjustments earlier
> like in dwc2_sched_periodic_split().

I've started working on this and just before I needed to leave my desk
I got something that seemed to work.  I'll keep at it on Monday.

At the moment I'm making the assumption that we never got a multi_tt
hub attached to us.  My code will always just schedule one split per
microframe.  Would that be OK for now until we make the scheduler
better?

To handle things smarter, I think I need to research how to deal with
hubs attached to hubs attached to hubs.  For instance:

dwc2
-> multi_tt hub
-> single_tt hub
-> device 1
-> device 2
-> single_tt hub
-> device 3
-> device 4
vs.

dwc2
-> single_tt hub
-> multi_tt hub
-> device 1
-> device 2
-> multi_tt hub
-> device 3
-> device 4

In the first case I presume I could schedule device 1 and device 3 at
the same time, but not device 2 and device 4.  In the 2nd case I
presume I could schedule all 4 devices independently.  ...but I
haven't dug through the spec to confirm that, yet.
--
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: musb: enable usb_dma parameter

[Greg KH]

On Fri, Nov 13, 2015 at 03:45:24PM -0600, Bin Liu wrote:
> Change the permission of usb_dma parameter so it can be used for runtime
> debug without reboot.

Why would you want to do that?  This is only used at init time, so if
you change it while the driver is running, I don't think you will
actually affect anything.

Have you tested this out?

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

Test. Please ignore

[team]

Test message. Please ignore.
--
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 1/3] usb: core: lpm: fix usb3_hardware_lpm sysfs node

[Lu Baolu]

On 11/13/2015 11:28 PM, Alan Stern wrote:

On Fri, 13 Nov 2015, Lu, Baolu wrote:


On 11/13/2015 12:20 AM, Alan Stern wrote:

On Thu, 12 Nov 2015, Lu Baolu wrote:


Commit 655fe4effe0f ("usbcore: add sysfs support to xHCI usb3
hardware LPM") introduced usb3_hardware_lpm sysfs node. This
doesn't show the correct status of USB3 U1 and U2 LPM status.

This patch fixes this by replacing usb3_hardware_lpm with two
nodes, usb3_hardware_lpm_u1 (for U1) and usb3_hardware_lpm_u2
(for U2), and recording the U1/U2 LPM status in right places.

This patch should be back-ported to kernels as old as 4.3,
that contains Commit 655fe4effe0f ("usbcore: add sysfs support
to xHCI usb3 hardware LPM").

Cc: sta...@vger.kernel.org
Signed-off-by: Lu Baolu 

...


--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3875,17 +3875,23 @@ static void usb_enable_link_state(struct usb_hcd *hcd, 
struct usb_device *udev,
return;
}
   
-	if (usb_set_lpm_timeout(udev, state, timeout))

+   ret = usb_set_lpm_timeout(udev, state, timeout);
+   if (ret)
/* If we can't set the parent hub U1/U2 timeout,
 * device-initiated LPM won't be allowed either, so let the xHCI
 * host know that this link state won't be enabled.
 */
hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state);
-
/* Only a configured device will accept the Set Feature U1/U2_ENABLE */
else if (udev->actconfig)
usb_set_device_initiated_lpm(udev, state, true);
   
+	if (!ret) {

+   if (state == USB3_LPM_U1)
+   udev->usb3_lpm_u1_enabled = 1;
+   else if (state == USB3_LPM_U2)
+   udev->usb3_lpm_u2_enabled = 1;
+   }

This doesn't look right at all.  What happens if ret is 0 but the
device isn't configured?  You'll set the usb3_lpm_u*_enabled flag even
though LPM isn't really enabled.

Don't you want to set these flags inside the
usb_set_device_initiated_lpm() function, where you know whether the
action succeeded?  And leave this routine unchanged?

My understand is that both hub and device can initiate LPM.
As soon as usb_set_lpm_timeout(valid_timeout_value)
returns 0, the hub-initiated LPM is enabled. Thus, LPM is
enabled no matter the result of usb_set_device_initiated_lpm().
The only difference is whether device is able to initiate LPM.

On disable side, as soon as usb_set_lpm_timeout(0) return 0,
hub initiated LPM is disabled. Hub will disallows link to enter
U1/U2 as well, even device is initiating LPM. Hence LPM
is disabled as soon as hub LPM timeout set to 0, no matter
device-initiated LPM is disabled or not.

Then maybe you can add a comment explaining this.


Yes, I will add comments for this.



The patch still looks strange, though.  Your new code does this:

ret = usb_set_lpm_timeout(...);
if (ret)
...
else if (udev->actconfig)
...
if (!ret) {
if (state == USB3_LPM_U1)
...
}

It would be better to do this:

if (usb_set_lpm_timeout(...)) {
...
} else {
if (udev->actconfig)
...
if (state == USB3_LPM_U1)
...
}


Yes, this looks better. I will refactor this part of code.



Alan Stern



Thank you.
-Baolu


--
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/4 V2] usb: dwc2: Make PHY optional

[Sergei Shtylyov]

Hello.

On 11/13/2015 12:08 AM, Stefan Wahren wrote:


From: John Youn 

Fixes commit 09a75e857790
("usb: dwc2: refactor common low-level hw code to platform.c")

The above commit consolidated the low-level phy access into a common
location. This change introduced a check from the gadget requiring
that a PHY is specified. This requirement never existed on the host
side and broke some platforms when it was moved into platform.c.

The gadget doesn't require the PHY either so remove the check.

Reported-by: Stefan Wahren 
Cc: Marek Szyprowski 
Signed-off-by: John Youn 
Fixes: 09a75e8577901489f77a14a3b305a9a1f67bf25b


   This tag has standardized commit citing format (the same one you've used 
in the first sentence of the change log).


[...]

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 v5 7/7] usb: gadget: f_midi: pre-allocate IN requests

[Robert Baldyga]

Hi Felipe,

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> This patch introduces pre-allocation of IN endpoint USB requests. This
> improves on latency (requires no usb request allocation on transmit) and avoid
> several potential probles on allocating too many usb requests (which involves
> DMA pool allocation problems).
> 
> This implementation also handles better multiple MIDI Gadget ports, always
> processing the last processed MIDI substream if the last USB request wasn't
> enought to handle the whole stream.
> 
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/gadget/function/f_midi.c | 174 
> +++
>  drivers/usb/gadget/legacy/gmidi.c|   2 +-
>  2 files changed, 137 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 615d632..6ac39f6 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -88,6 +89,9 @@ struct f_midi {
>   int index;
>   char *id;
>   unsigned int buflen, qlen;
> + DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
> + unsigned int in_req_num;
> + unsigned int in_last_port;
>  };
>  
>  static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -95,7 +99,7 @@ static inline struct f_midi *func_to_midi(struct 
> usb_function *f)
>   return container_of(f, struct f_midi, func);
>  }
>  
> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req);
> +static void f_midi_transmit(struct f_midi *midi);
>  
>  DECLARE_UAC_AC_HEADER_DESCRIPTOR(1);
>  DECLARE_USB_MIDI_OUT_JACK_DESCRIPTOR(1);
> @@ -253,7 +257,8 @@ f_midi_complete(struct usb_ep *ep, struct usb_request 
> *req)
>   } else if (ep == midi->in_ep) {
>   /* Our transmit completed. See if there's more to go.
>* f_midi_transmit eats req, don't queue it again. */
> - f_midi_transmit(midi, req);
> + req->length = 0;
> + f_midi_transmit(midi);
>   return;
>   }
>   break;
> @@ -264,10 +269,12 @@ f_midi_complete(struct usb_ep *ep, struct usb_request 
> *req)
>   case -ESHUTDOWN:/* disconnect from host */
>   VDBG(cdev, "%s gone (%d), %d/%d\n", ep->name, status,
>   req->actual, req->length);
> - if (ep == midi->out_ep)
> + if (ep == midi->out_ep) {
>   f_midi_handle_out_data(ep, req);
> -
> - free_ep_req(ep, req);
> + /* We don't need to free IN requests because it's 
> handled
> +  * by the midi->in_req_fifo. */
> + free_ep_req(ep, req);
> + }
>   return;
>  
>   case -EOVERFLOW:/* buffer overrun on read means that
> @@ -334,6 +341,21 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>   if (err)
>   return err;
>  
> + /* pre-allocate write usb requests to use on f_midi_transmit. */
> + while (kfifo_avail(>in_req_fifo)) {
> + struct usb_request *req =
> + midi_alloc_ep_req(midi->in_ep, midi->buflen);
> +
> + if (req == NULL)
> + return -ENOMEM;

We need to free previously allocated requests in case of failure.

> +
> + req->length = 0;
> + req->complete = f_midi_complete;
> +
> + kfifo_put(>in_req_fifo, req);
> + midi->in_req_num++;
> + }
> +
>   /* allocate a bunch of read buffers and queue them all at once. */
>   for (i = 0; i < midi->qlen && err == 0; i++) {
>   struct usb_request *req =
> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
>*/
>   usb_ep_disable(midi->in_ep);
>   usb_ep_disable(midi->out_ep);
> +
> + /* release IN requests */
> + while (!kfifo_is_empty(>in_req_fifo)) {
> + struct usb_request *req = NULL;
> + unsigned int len;
> +
> + len = kfifo_get(>in_req_fifo, );
> + if (len == 1)
> + free_ep_req(midi->in_ep, req);
> + else
> + ERROR(midi, "%s couldn't free usb request: something 
> went wrong with kfifo\n",
> +   midi->in_ep->name);
> + }
> + midi->in_req_num = 0;
>  }
>  
>  static int f_midi_snd_free(struct snd_device *device)
> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request 
> *req,
>   }
>  }
>  
> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
> +static void f_midi_drop_out_substreams(struct f_midi *midi)
>  {
> - struct usb_ep *ep = midi->in_ep;
> - int i;
> -
> 

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

[Daniele Palmas]

Hi Johan,

2015-11-13 11:58 GMT+01:00 Johan Hovold :
> On Thu, Nov 12, 2015 at 02:55:04PM +0100, Bjørn Mork wrote:
>> Daniele Palmas  writes:
>
>> > But I see that Infineon flashing devices in newer chipsets have become
>> > an only bulk serial link device (see device 0x8087/0x0716 in
>> > usb-serial-simple), so maybe this has a meaning...
>>
>> Yes.  I have a Sierra Wireless EM7345 modem which is based on the same
>> chipset. I haven't really paid much attemtion to the flashloader
>> functionality before, but this is how that modem looks before it loads
>> its Sierra firmware:
>
>> So this device looks very much like an ACM device, except that it is
>> missing the necessary control interface and status endpoint.  And
>> without a control interface there is of course no way to send a properly
>> formatted CDC control request either.
>>
>> I assume the different vendors also use the same Intel provided firmware
>> toolkit, making the firmwares share basic functionality like bootloader
>> and flashing.  But the USB descriptors are likely configurable by the
>> vendor.  Telit might have tried to "improve" the above into a proper ACM
>> device.
>>
>> I believe this supports the assumption that Infineon Flash Loader
>> devices have some ACM descriptors without actually being ACM class
>> devices, and that it is best to just collect them all in the
>> usb-serial-simple "flashloader" driver.
>
> I'm still not convinced. The device at hand does look and apparently
> behaves like a compliant ACM device, and I think we need to figure out
> why it does not work with this particular proprietary user-space tool
> before modifying the kernel.
>
> For example, using usb-serial-simple means that no line-control requests
> are sent. Perhaps the tool simply needs to set the termios settings to
> something else than the default 9600N81 that the cdc-acm driver uses.
>
> Modem control-requests would not be sent either (and HUPCL determines if
> DTR/RTS is lowered at close).
>
> Daniele, could you provide some more details about what happens when the
> proprietary tool fails. Do you have access to the sources?

I'll try to explain how the whole thing should work.

This is what happens when the device is turned on (without modifying
the drivers):

[155492.352031] usb 1-3: new high-speed USB device number 27 using ehci-pci
[155492.485429] usb 1-3: config 1 interface 0 altsetting 0 endpoint
0x81 has an invalid bInterval 255, changing to 11
[155492.485436] usb 1-3: New USB device found, idVendor=058b, idProduct=0041
[155492.485439] usb 1-3: New USB device strings: Mfr=0, Product=0,
SerialNumber=0
[155492.485952] cdc_acm 1-3:1.0: ttyACM0: USB ACM device

This is the flashing device that is caught by the cdc-acm driver. Once
the ttyACM appears, the application starts sending a magic string
(simple write on the file descriptor) to keep the device in flashing
mode. If this magic string is not properly received in a certain time
interval, the modem goes on in normal operative mode:

[155493.748094] usb 1-3: USB disconnect, device number 27
[155494.916025] usb 1-3: new high-speed USB device number 28 using ehci-pci
[155495.059978] usb 1-3: New USB device found, idVendor=1bc7, idProduct=0021
[155495.059983] usb 1-3: New USB device strings: Mfr=1, Product=2,
SerialNumber=3
[155495.059986] usb 1-3: Product: 6 CDC-ACM + 1 CDC-ECM
[155495.059989] usb 1-3: Manufacturer: Telit
[155495.059992] usb 1-3: SerialNumber: 359658044004697
[155495.138958] cdc_acm 1-3:1.0: ttyACM0: USB ACM device
[155495.140832] cdc_acm 1-3:1.2: ttyACM1: USB ACM device
[155495.142827] cdc_acm 1-3:1.4: ttyACM2: USB ACM device
[155495.144462] cdc_acm 1-3:1.6: ttyACM3: USB ACM device
[155495.145967] cdc_acm 1-3:1.8: ttyACM4: USB ACM device
[155495.147588] cdc_acm 1-3:1.10: ttyACM5: USB ACM device
[155495.154322] cdc_ether 1-3:1.12 wwan0: register 'cdc_ether' at
usb-:00:1a.7-3, Mobile Broadband Network Device, 00:00:11:12:13:14

Using the cdc-acm driver, the string, though being sent in the same
way than using the usb-serial-simple driver (I can confirm that the
data is passing properly since I used an hw usb sniffer), does not
make the device to stay in flashing mode.

I thought that the problem was something done by the acm driver when
opening the port that is not accepted well by the flashing device on
the firmware side, so I tried also in the past to debug the driver,
but without success.

I can start again to understand which is really the problem with
cdc-acm, but my fear is that to have a working solution with cdc-acm
will be more complicated (e.g. adding a new quirk) than adding support
in the usb-serial-simple driver. Consider also that this is not really
a communication device with subclass modem, as wrongly stated by the
usb descriptors, since it is simply a flashing device.

Another interesting piece of the puzzle is that in Windows this device
requires a specific flashloader driver and does not use the acm driver