Re: usb: chipidea: hdc: kernel panic during shutdown

[Peter Chen]

On Tue, Aug 23, 2016 at 09:17:02PM +0200, Stefan Wahren wrote:
> Hi,
> 
> i'm using a iMX233-OLinuXino board and the kernel panics during shutdown with
> 4.8.0-rc2-next-20160819:
> 
> [  420.04] ci_hdrc ci_hdrc.0: remove, state 1
> [  420.05] usb usb1: USB disconnect, device number 1
> [  420.06] usb 1-1: USB disconnect, device number 2
> [  420.06] usb 1-1.1: USB disconnect, device number 3
> [  420.09] smsc95xx 1-1.1:1.0 eth0: unregister 'smsc95xx' 
> usb-ci_hdrc.0-1.1,
> smsc95xx USB 2.0 Ethernet
> [  420.29] ci_hdrc ci_hdrc.0: USB bus 1 deregistered
> [  420.30] Unable to handle kernel NULL pointer dereference at virtual
> address 0118
> [  420.30] pgd = c2ea4000
> [  420.30] [0118] *pgd=
> [  420.30] Internal error: Oops: 5 [#1] ARM
> [  420.30] Modules linked in:
> [  420.30] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted
> 4.8.0-rc2-next-20160819 #1
> [  420.30] Hardware name: Freescale MXS (Device Tree)
> [  420.30] task: c349 task.stack: c348e000
> [  420.30] PC is at usb_hcd_irq+0x0/0x34
> [  420.30] LR is at ci_irq+0x58/0x12c
> [  420.30] pc : []lr : []psr: a093
> [  420.30] sp : c348fd20  ip : c348fcd8  fp : c3704624
> [  420.30] r10: c3708300  r9 : c3602010  r8 : 6013
> [  420.30] r7 : 0094  r6 : 2013  r5 : c3549600  r4 : c3602010
> [  420.30] r3 : 12a0  r2 : 0808  r1 :   r0 : 0094
> [  420.30] Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment
> none
> [  420.30] Control: 0005317f  Table: 42ea4000  DAC: 0051
> [  420.30] Process systemd-shutdow (pid: 1, stack limit = 0xc348e190)
> [  420.30] Stack: (0xc348fd20 to 0xc349)
> [  420.30] fd20: c3549610 c3549600 2013 c006bfac c3549600 
> 0094 c3602010
> [  420.30] fd40: c370e2e0 c36f4e80 c36f4e80 0005 c370e2e0 c006c098
> c348fd70 c3704410
> [  420.30] fd60: c370e2e0 c039cd68 0005 a013 c36f4e00 c370e2e0
> a013 c039ce0c
> [  420.30] fd80: c354b844 c348fe5c  c08d604c c3704624 c370e2e0
> 0001 c3704410
> [  420.30] fda0: c08bc194 c08ad460 c354b844 c348fe5c  c08d604c
> be829c58 c03997e8
> [  420.30] fdc0: c370 c3704410 c08ad460 c0399954 c3402a4c c3704410
> c08ad460 c0398d64
> [  420.30] fde0: c3704410 c354b810 c112ebf4 c0396e2c c349 0007
> 0006 c348fe5c
> [  420.30] fe00: c3704400 c3704410 c354b810 c354b844 c348fe5c c039bb90
> c3704400 c3708090
> [  420.30] fe20: c354b810 c039c118  c044941c c354b800 c044e6f8
> c354b810 c03a47fc
> [  420.30] fe40: c354b810 c354b410 c112ebf4 c039b578 c354b810 c0397410
> 0002 c08ad22c
> [  420.30] fe60: c342fa20 c354b81c 4321fedc c0866838 cdef0123 f3f82200
> c000a484 c348e000
> [  420.30] fe80:  c00427f4 4321fedc c0042a10  
> c348fec8 0029
> [  420.30] fea0: c2e2d488  c34b9c28 0002 c00690fc 
>  c0132e0c
> [  420.30] fec0:   be8291f8 0004 be829f8a 0010
> be829224 0005
> [  420.30] fee0: be8292c4 000f 7f5fb9f8 0001 be828d3c c08629b4
> c348ffb0 00053177
> [  420.30] ff00: 07ff 000f0f24 c2e2d480   
>  
> [  420.30] ff20:   0001  0029 c348fec8
> 0005 
> [  420.30] ff40:  0004001e 0005 be82924c 0005 be82924c
> c348e000 
> [  420.30] ff60: 7f60e854 c0132f24 c348ff88  c2e2d480 c2e2d480
>  c0132f70
> [  420.30] ff80:  c000a484  8102d190 8102c118 
> 8102d190 8102c118
> [  420.30] ffa0: 0058 c000a2e0  8102d190 fee1dead 28121969
> cdef0123 f3f82200
> [  420.30] ffc0:  8102d190 8102c118 0058 be829bc4 
>  be829c58
> [  420.30] ffe0: 7f60de4c be829b08 7f5ee114 b6e46088 6010 fee1dead
>  
> [  420.30] [] (usb_hcd_irq) from [] (0xc3549600)
> [  420.30] Code: c077a4d4 c08b802c c08b8040 c08b7fb0 (e5913118)
> [  420.30] ---[ end trace 5e83b1955a5bd084 ]---
> [  420.62] systemd-shutdow: 2 output lines suppressed due to ratelimiting
> [  420.62] Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x000b
> [  420.62]
> [  420.62] ---[ end Kernel panic - not syncing: Attempted to kill init!
> 

I am afraid the hcd is freed before the interrupt triggered. Would you
please try below changes:

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 96ae695..61237a9 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -103,7 +103,7 @@ static const struct ehci_driver_overrides ehci_ci_overrides 
= {
static irqreturn_t host_irq(struct ci_hdrc *ci)
{
-   return usb_hcd_irq(ci->irq, ci->hcd);
+   return ci->hcd ? usb_hcd_irq(ci->irq, ci->hcd) : IRQ_NONE;
}
  
static int host_start(struct ci_hdrc *ci)

[no subject]

[Jeffrey Chu]

unsubscribe 
--
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 03/22] usb: ulpi: Support device discovery via device properties

[Stephen Boyd]

On Tue, Aug 23, 2016 at 4:06 PM, Rob Herring  wrote:
> On Tue, Aug 23, 2016 at 3:00 PM, Stephen Boyd  wrote:
>> On Fri, Aug 5, 2016 at 2:40 PM, Stephen Boyd  wrote:
>>> Quoting Rob Herring (2016-07-17 19:23:55)
 On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote:
 > +---
 > +
 > +usb {
 > + compatible = "vendor,usb-controller";
 > +
 > + ulpi {
 > + phy {
 > + compatible = "vendor,phy";
 > + ulpi-vendor = /bits/ 16 <0x1d6b>;
 > + ulpi-product = /bits/ 16 <0x0002>;
 > + };
 > + };

 I'm still having concerns about describing both phys and devices. If I
 have a controller with 2 ports and 2 devices attached, I'd have
 something like this under the USB controller:

 ulpi {
 phy@1 {
 };
 phy@2 {
 };
 };
>>>
>>> My understanding is there would only be one status="ok" node on the ULPI
>>> bus for the single phy that a usb controller would have. At the least,
>>> the kernel's ULPI layer only seems to support one ULPI phy for a
>>> controller right now. So even if there are two ports, it doesn't mean
>>> there are two phys.
>>>

 dev@1 {
 ...
 };

 dev@2 {
 ...
 };


 That doesn't seem the best, but I don't have a better suggestion. Maybe
 the device nodes need to go under the phy nodes?

>>>
>>> What if we moved the dev@1 and dev@2 to another sub node like "ports" or
>>> "usb-devices"? Legacy code can support having those devices directly
>>> underneath the usb controller, but future users would always need to put
>>> them in a different sub-node so that we can easily differentiate the
>>> different busses that a usb controller node may support?
>>>
>>> I'm not sure I see any need to relate the phy to the ports that are on
>>> the controller, but if that is needed then perhaps you're right and we
>>> should move the ports underneath the phy. USB core could be modified to
>>> go through the legacy path or through the phy, if it even exists, to
>>> find ports.
>>>
>>> Do we typically do this for other phy designs like sata or pci? The phy
>>> always seemed like a parallel thing to the logical bus that the phy is
>>> used for.
>>
>> Rob does this sound ok to you?
>
> Well, if there's only ever 1 phy under the controller, then as you had
> it is fine.
>

Ok. For ULPI I believe that's the case, but in general usb controllers
can have more than one phy.
--
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 03/22] usb: ulpi: Support device discovery via device properties

[Rob Herring]

On Tue, Aug 23, 2016 at 3:00 PM, Stephen Boyd  wrote:
> On Fri, Aug 5, 2016 at 2:40 PM, Stephen Boyd  wrote:
>> Quoting Rob Herring (2016-07-17 19:23:55)
>>> On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote:
>>> > +---
>>> > +
>>> > +usb {
>>> > + compatible = "vendor,usb-controller";
>>> > +
>>> > + ulpi {
>>> > + phy {
>>> > + compatible = "vendor,phy";
>>> > + ulpi-vendor = /bits/ 16 <0x1d6b>;
>>> > + ulpi-product = /bits/ 16 <0x0002>;
>>> > + };
>>> > + };
>>>
>>> I'm still having concerns about describing both phys and devices. If I
>>> have a controller with 2 ports and 2 devices attached, I'd have
>>> something like this under the USB controller:
>>>
>>> ulpi {
>>> phy@1 {
>>> };
>>> phy@2 {
>>> };
>>> };
>>
>> My understanding is there would only be one status="ok" node on the ULPI
>> bus for the single phy that a usb controller would have. At the least,
>> the kernel's ULPI layer only seems to support one ULPI phy for a
>> controller right now. So even if there are two ports, it doesn't mean
>> there are two phys.
>>
>>>
>>> dev@1 {
>>> ...
>>> };
>>>
>>> dev@2 {
>>> ...
>>> };
>>>
>>>
>>> That doesn't seem the best, but I don't have a better suggestion. Maybe
>>> the device nodes need to go under the phy nodes?
>>>
>>
>> What if we moved the dev@1 and dev@2 to another sub node like "ports" or
>> "usb-devices"? Legacy code can support having those devices directly
>> underneath the usb controller, but future users would always need to put
>> them in a different sub-node so that we can easily differentiate the
>> different busses that a usb controller node may support?
>>
>> I'm not sure I see any need to relate the phy to the ports that are on
>> the controller, but if that is needed then perhaps you're right and we
>> should move the ports underneath the phy. USB core could be modified to
>> go through the legacy path or through the phy, if it even exists, to
>> find ports.
>>
>> Do we typically do this for other phy designs like sata or pci? The phy
>> always seemed like a parallel thing to the logical bus that the phy is
>> used for.
>
> Rob does this sound ok to you?

Well, if there's only ever 1 phy under the controller, then as you had
it is fine.

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

[PATCH V3] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

From: Rafał Miłecki 

This commit adds a new trigger responsible for turning on LED when USB
device gets connected to the specified USB port. This can can useful for
various home routers that have USB port(s) and a proper LED telling user
a device is connected.

The trigger gets its documentation file but basically it just requires
specifying USB port in a Linux format (e.g. echo 1-1 > new_port).

During work on this trigger there was a plan to add DT bindings for it,
but there wasn't an agreement on the format yet. This can be worked on
later, a sysfs interface is needed anyway for platforms not using DT.

Signed-off-by: Rafał Miłecki 
---
V2: Trying to add DT support, idea postponed as it will take more time
to discuss the bindings.
V3: Fix typos in commit and Documentation (thanks Jacek!)
Use "ports" sysfs file for adding and removing USB ports (thx Jacek)
Check if there is USB device connected after adding new USB port
Fix memory leak or two

Felipe: I'd like to ask for your Ack before having this patch pushed.
---
 Documentation/leds/ledtrig-usbport.txt |  49 +++
 drivers/leds/trigger/Kconfig   |   8 ++
 drivers/leds/trigger/Makefile  |   1 +
 drivers/leds/trigger/ledtrig-usbport.c | 253 +
 4 files changed, 311 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-usbport.txt
 create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

diff --git a/Documentation/leds/ledtrig-usbport.txt 
b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 000..fa42227
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,49 @@
+USB port LED trigger
+
+
+This LED trigger can be used for signalling to the user a presence of USB 
device
+in a given port. It simply turns on LED when device appears and turns it off
+when it disappears.
+
+It requires specifying a list of USB ports that should be observed. Used format
+matches Linux kernel format and consists of a root hub number and a hub port
+separated by a dash (e.g. 3-1).
+
+It is also possible to handle devices with internal hubs (that are always
+connected to the root hub). User can simply specify internal hub ports then
+(e.g. 1-1.1, 1-1.2, etc.).
+
+Please note that this trigger allows assigning multiple USB ports to a single
+LED. This can be useful in two cases:
+
+1) Device with single USB LED and few physical ports
+
+In such a case LED will be turned on as long as there is at least one connected
+USB device.
+
+2) Device with a physical port handled by few controllers
+
+Some devices have e.g. one controller per PHY standard. E.g. USB 3.0 physical
+port may be handled by ohci-platform, ehci-platform and xhci-hcd. If there is
+only one LED user will most likely want to assign ports from all 3 hubs.
+
+
+This trigger can be activated from user space on led class devices as shown
+below:
+
+  echo usbport > trigger
+
+This adds the following sysfs attributes to the LED:
+
+  ports - Reading it lists all USB ports assigned to the trigger. Writing USB
+ port number to it will make this driver start observing it. It's also
+ possible to remove USB port from observable list by writing it with a
+ "-" prefix.
+
+Example use-case:
+
+  echo usbport > trigger
+  echo 4-1 > ports
+  echo 2-1 > ports
+  echo -4-1 > ports
+  cat ports
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 3f9ddb9..bdd6fd2 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC
  a different trigger.
  If unsure, say Y.
 
+config LEDS_TRIGGER_USBPORT
+   tristate "USB port LED trigger"
+   depends on LEDS_TRIGGERS && USB
+   help
+ This allows LEDs to be controlled by USB events. Enabling this option
+ allows specifying list of USB ports that should turn on LED when some
+ USB device gets connected.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index a72c43c..56e1741 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)   += ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)  += ledtrig-camera.o
 obj-$(CONFIG_LEDS_TRIGGER_PANIC)   += ledtrig-panic.o
+obj-$(CONFIG_LEDS_TRIGGER_USBPORT) += ledtrig-usbport.o
diff --git a/drivers/leds/trigger/ledtrig-usbport.c 
b/drivers/leds/trigger/ledtrig-usbport.c
new file mode 100644
index 000..7f5237c
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -0,0 +1,253 @@
+/*
+ * USB port LED trigger
+ *
+ * Copyright (C) 2016 Rafał Miłecki 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ 

[RFC PATCH v3 1/2] usb: typec: USB Type-C Port Manager (tcpm)

[Guenter Roeck]

This driver implements the USB Type-C Power Delivery state machine
for both source and sink ports. Alternate mode support is not
fully implemented.

The driver attaches to the USB Type-C class code implemented in
the following patches.

usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY
usb: USB Type-C connector class

This driver only implements the state machine. Lower level drivers are
responsible for
- Reporting VBUS status and activating VBUS
- Setting CC lines and providing CC line status
- Setting line polarity
- Activating and deactivating VCONN
- Setting the current limit
- Activating and deactivating PD message transfers
- Sending and receiving PD messages

The driver provides both a functional API as well as callbacks for
lower level drivers.

Signed-off-by: Guenter Roeck 
---
v3:
- Improve TCPM state machine resiliency if there are spurious CC line changes
  while the state machine is in a transient change (waiting for a timeout)
- Update current limit after CC voltage level changes on a port which is not
  PD capable.

v2:
- Only update polarity if setting it was successful
  If setting the CC line polarity in the driver was not successful,
  don't update the internal polarity state.
- All PD messages are little endian; convert to and from CPU endianness.
- Avoid comparisons against NULL.
- Use u8/u16/u32 instead of uint8_t/uint16_t/uint32_t consistently.
- Callbacks into tcpm need to be lockless to avoid timing problems
  in low level drivers.
- Simplify callbacks; tcpm can request the current state of cc/vbus
  when it is ready to use it.

 drivers/usb/typec/Kconfig  |7 +
 drivers/usb/typec/Makefile |1 +
 drivers/usb/typec/tcpm.c   | 3163 
 drivers/usb/typec/tcpm.h   |  137 ++
 include/linux/usb/pd.h |  282 
 include/linux/usb/pd_bdo.h |   31 +
 include/linux/usb/pd_vdo.h |  412 ++
 7 files changed, 4033 insertions(+)
 create mode 100644 drivers/usb/typec/tcpm.c
 create mode 100644 drivers/usb/typec/tcpm.h
 create mode 100644 include/linux/usb/pd.h
 create mode 100644 include/linux/usb/pd_bdo.h
 create mode 100644 include/linux/usb/pd_vdo.h

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 7a345a4b1e7a..113bb1b3589c 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -18,4 +18,11 @@ config TYPEC_WCOVE
  To compile this driver as module, choose M here: the module will be
  called typec_wcove
 
+config TYPEC_TCPM
+   tristate "USB Type-C Port Controller Manager"
+   select TYPEC
+   help
+ The Type-C Port Controller Manager provides a USB PD and USB Type-C
+ state machine for use with Type-C Port Controllers.
+
 endmenu
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index b9cb862221af..bbe45721cf52 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_TYPEC)+= typec.o
 obj-$(CONFIG_TYPEC_WCOVE)  += typec_wcove.o
+obj-$(CONFIG_TYPEC_TCPM)   += tcpm.o
diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
new file mode 100644
index ..bd98a2dda36f
--- /dev/null
+++ b/drivers/usb/typec/tcpm.c
@@ -0,0 +1,3163 @@
+/*
+ * Copyright 2015-2016 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * USB Power Delivery protocol stack.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "tcpm.h"
+
+#define FOREACH_STATE(S)   \
+   S(INVALID_STATE),   \
+   S(SRC_UNATTACHED),  \
+   S(SRC_ATTACH_WAIT), \
+   S(SRC_ATTACHED),\
+   S(SRC_STARTUP), \
+   S(SRC_SEND_CAPABILITIES),   \
+   S(SRC_NEGOTIATE_CAPABILITIES),  \
+   S(SRC_TRANSITION_SUPPLY),   \
+   S(SRC_READY),   \
+   S(SRC_WAIT_NEW_CAPABILITIES),   \
+   \
+   S(SNK_UNATTACHED),  \
+   S(SNK_ATTACH_WAIT), \
+   S(SNK_DEBOUNCED),   \
+   S(SNK_ATTACHED),\
+   S(SNK_STARTUP), \
+   S(SNK_DISCOVERY),   \
+  

[RFC PATCH v3 0/2] Type-C Port Manager

[Guenter Roeck]

The following series of patches implements a USB Type-C Port Manager
using the pending USB Type-C class code as basis. The code is still WIP,
but I think it is important to get feedback from the community at this point.

There are two patches in the series. The first patch implements the Type-C
Port Manager state machine. The forth patch is an interface between the
Type-C Port Manager and a TCPCI (Type-C Port Controller Interface) compliant
USB Type-C Port Controller.

Patch 2/2 (the interface to a TCPCI compliant chip) is currently untested
since I don't have the necessary hardware available. The port manager code
was tested connecting to an Embedded Controller on a Chromebook, bypassing
the Port Manager implementation in the EC.

Both Source and Sink operation was tested with various Type-C chargers, docks,
and connectors. Alternate mode support is partially implemented (Alternate mode
support is requested from the partner), but alternate modes are not actually
selected. Implementing this will require more thought, since the actual
alternate mode support has to be implemented elsewhere, such as in a dedicated
Phy driver. It should be possible to implement the interface between phy driver
and Type-C Port Controller driver using extcon, but I have not further explored
the possibilities, and other options might be possible and/or better.

v3:
- Improve TCPM state machine resiliency if there are spurious CC line changes
  while the state machine is in a transient change (waiting for a timeout)
- Update current limit after CC voltage level changes on a port which is not
  PD capable.
- Applies to v6 of "USB Type-C Connector class" patch series.

v2:
- Class code no longer uses locking, so the patch to remove it is no longer
  necessary.
- tcpm: Only update polarity if setting it was successful
  If setting the CC line polarity in the driver was not successful,
  don't update the internal polarity state.
- tcpm: All PD messages are little endian; convert to and from CPU endianness.
- tcpm: Avoid comparisons against NULL.
- tcpm: Use u8/u16/u32 instead of uint8_t/uint16_t/uint32_t consistently.
- tcpm/tcpc: Callbacks into tcpm need to be lockless to avoid timing problems
  in low level drivers.
- tcpm/tcpc: Simplify callbacks; tcpm can request the current state of cc/vbus
  when it is ready to use it.
- Applies to v5 of "USB Type-C Connector class" patch series.
--
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

[RFC PATCH v3 2/2] usb: typec: Type-C Port Controller Interface driver (tcpci)

[Guenter Roeck]

The port controller interface driver interconnects the Type-C Port
Manager with a Type-C Port Controller Interface (TCPCI) compliant
port controller.

Signed-off-by: Guenter Roeck 
---
v3:
- No change

v2:
- Adjust to modified callbacks into tcpm code
 
 drivers/usb/typec/Kconfig  |   9 +
 drivers/usb/typec/Makefile |   1 +
 drivers/usb/typec/tcpci.c  | 487 +
 drivers/usb/typec/tcpci.h  | 133 +
 4 files changed, 630 insertions(+)
 create mode 100644 drivers/usb/typec/tcpci.c
 create mode 100644 drivers/usb/typec/tcpci.h

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 113bb1b3589c..a92c9d1a3e00 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -25,4 +25,13 @@ config TYPEC_TCPM
  The Type-C Port Controller Manager provides a USB PD and USB Type-C
  state machine for use with Type-C Port Controllers.
 
+if TYPEC_TCPM
+
+config TYPEC_TCPCI
+   tristate "Type-C Port Controller Interface driver"
+   help
+ Type-C Port Controller driver for TCPCI-compliant controller.
+
+endif
+
 endmenu
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index bbe45721cf52..7dbaf8c3911d 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_TYPEC)+= typec.o
 obj-$(CONFIG_TYPEC_WCOVE)  += typec_wcove.o
 obj-$(CONFIG_TYPEC_TCPM)   += tcpm.o
+obj-$(CONFIG_TYPEC_TCPCI)  += tcpci.o
diff --git a/drivers/usb/typec/tcpci.c b/drivers/usb/typec/tcpci.c
new file mode 100644
index ..af338218a1f3
--- /dev/null
+++ b/drivers/usb/typec/tcpci.c
@@ -0,0 +1,487 @@
+/*
+ * Copyright 2015-2016 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * USB Type-C Port Controller Interface.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "tcpci.h"
+#include "tcpm.h"
+
+#define PD_RETRY_COUNT 3
+
+struct tcpci {
+   struct device *dev;
+   struct i2c_client *client;
+
+   struct tcpm_port *port;
+
+   struct regmap *regmap;
+
+   bool controls_vbus;
+
+   struct tcpc_dev tcpc;
+};
+
+static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
+{
+   return container_of(tcpc, struct tcpci, tcpc);
+}
+
+static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
+   unsigned int *val)
+{
+   return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
+}
+
+static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
+{
+   return regmap_raw_write(tcpci->regmap, reg, , sizeof(u16));
+}
+
+static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
+{
+   struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
+   unsigned int reg;
+   int ret;
+
+   switch (cc) {
+   case TYPEC_CC_RA:
+   reg = (TCPC_ROLE_CTRL_CC_RA << TCPC_ROLE_CTRL_CC1_SHIFT) |
+   (TCPC_ROLE_CTRL_CC_RA << TCPC_ROLE_CTRL_CC2_SHIFT);
+   break;
+   case TYPEC_CC_RD:
+   reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
+   (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
+   break;
+   case TYPEC_CC_RP_DEF:
+   reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
+   (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) |
+   (TCPC_ROLE_CTRL_RP_VAL_DEF <<
+TCPC_ROLE_CTRL_RP_VAL_SHIFT);
+   break;
+   case TYPEC_CC_RP_1_5:
+   reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
+   (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) |
+   (TCPC_ROLE_CTRL_RP_VAL_1_5 <<
+TCPC_ROLE_CTRL_RP_VAL_SHIFT);
+   break;
+   case TYPEC_CC_RP_3_0:
+   reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
+   (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT) |
+   (TCPC_ROLE_CTRL_RP_VAL_1_5 <<
+TCPC_ROLE_CTRL_RP_VAL_SHIFT);
+   break;
+   case TYPEC_CC_OPEN:
+   default:
+   reg = (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC1_SHIFT) |
+   (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC2_SHIFT);
+   break;
+   }
+
+   ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
+   if 

Re: Gadgetfs - adding support for delegation of setup requests

[Alan Stern]

On Thu, 18 Aug 2016, Felipe Balbi wrote:

> >> You mean a ep0 ioctl? Makes sense to me. Then we can add more features
> >> later. Perhaps just add a "Get supported features" IOCTL which returns
> >> a struct with 256 bits (4x uint64_t). I doubt we will ever need that
> >> many bits, but better safe than sorry, I guess.
> >>
> > Don't we need some "set feature" IOCTL as well?
> 
> Why? Features are enabled at kernel compile time. Userspace issues ioctl
> to ask the kernel "which features do you have enabled for
> GadgetFS?". Note that we have a backwards compatibility problem here. If
> "which features do you have enabled for GadgetFS" ioctl fails, we will
> assume that kernel is older than the first kernel where this ioctl was
> added.
> 
> To help userland a little more, we can make sure that, from now on, the
> ioctl itself is always enabled and bit0 out of the 256 bits we have MUST
> always be set. This means that "Forward all requests to userspace"
> feature has to be, at least, bit1.
> 
> This mimics what USB descriptors where bit7 of config descriptor's
> bmAttributes must always be set. We're just using bit0 instead.

Binyamin's question refers to existing userspace drivers for gadgetfs.  
They don't expect all requests to be forwarded and will fail if that 
happens.  This is undesirable; we would like old programs to continue 
to work even under kernels built with the EXPERT feature configured in.

Therefore the proposal is to have an "enable-feature" ioctl.  It would 
be issued only by userspace drivers that are aware of the 
forward-all-requests feature.  Drivers that aren't aware of it would 
never issue the ioctl, and so gadgetfs would revert to its original 
behavior for them.

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: [RESEND PATCH 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform

[John Youn]

On 8/21/2016 12:32 PM, Randy Li wrote:
> On the rk3288 USB host-only port (the one that's not the OTG-enabled
> port) the PHY can get into a bad state when a wakeup is asserted (not
> just a wakeup from full system suspend but also a wakeup from
> autosuspend).
> 
> We can get the PHY out of its bad state by asserting its "port reset",
> but unfortunately that seems to assert a reset onto the USB bus so it
> could confuse things if we don't actually deenumerate / reenumerate the
> device.
> 
> We can also get the PHY out of its bad state by fully resetting it using
> the reset from the CRU (clock reset unit) in chip, which does a more full
> reset.  The CRU-based reset appears to actually cause devices on the bus
> to be removed and reinserted, which fixes the problem (albeit in a hacky
> way).
> 
> It's unfortunate that we need to do a full re-enumeration of devices at
> wakeup time, but this is better than alternative of letting the bus get
> wedged.
> 
> Signed-off-by: Randy Li 
> ---
>  drivers/usb/dwc2/core_intr.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index d85c5c9..f57c48a 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct 
> dwc2_hsotg *hsotg)
>  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  {
>   int ret;
> + struct device_node *np = hsotg->dev->of_node;
>  
>   /* Clear interrupt */
>   dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> @@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct 
> dwc2_hsotg *hsotg)
>   /* Restart the Phy Clock */
>   pcgcctl &= ~PCGCTL_STOPPCLK;
>   dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
> +
> + /* 
> +  * It is a quirk in Rockchip RK3288, causing by
> +  * a hardware bug. This will propagate out and
> +  * eventually we'll re-enumerate the device. 
> +  * Not great but the best we can do 

Remove the trailing whitespaces in this comment.

Run scripts/checkpatch.pl to catch these.

> +  */
> + if (of_device_is_compatible(np, "rockchip,rk3288-usb"))
> + hsotg->phy->ops->reset(hsotg->phy);
> +

You should probably check for NULL before calling the reset()
callback.

Also, I'd rather see a generic quirk property that you set for your
platform.

Something like "phy_reset_on_wakeup_quirk".

Also, try to preserve the version tag in your subject for all the
patches so that we can easily identify the latest version of the
series, like:

[PATCH v5 3/4] ...

And, typically "RESEND" means there are no code change.

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 v2 03/22] usb: ulpi: Support device discovery via device properties

[Stephen Boyd]

On Fri, Aug 5, 2016 at 2:40 PM, Stephen Boyd  wrote:
> Quoting Rob Herring (2016-07-17 19:23:55)
>> On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote:
>> > +---
>> > +
>> > +usb {
>> > + compatible = "vendor,usb-controller";
>> > +
>> > + ulpi {
>> > + phy {
>> > + compatible = "vendor,phy";
>> > + ulpi-vendor = /bits/ 16 <0x1d6b>;
>> > + ulpi-product = /bits/ 16 <0x0002>;
>> > + };
>> > + };
>>
>> I'm still having concerns about describing both phys and devices. If I
>> have a controller with 2 ports and 2 devices attached, I'd have
>> something like this under the USB controller:
>>
>> ulpi {
>> phy@1 {
>> };
>> phy@2 {
>> };
>> };
>
> My understanding is there would only be one status="ok" node on the ULPI
> bus for the single phy that a usb controller would have. At the least,
> the kernel's ULPI layer only seems to support one ULPI phy for a
> controller right now. So even if there are two ports, it doesn't mean
> there are two phys.
>
>>
>> dev@1 {
>> ...
>> };
>>
>> dev@2 {
>> ...
>> };
>>
>>
>> That doesn't seem the best, but I don't have a better suggestion. Maybe
>> the device nodes need to go under the phy nodes?
>>
>
> What if we moved the dev@1 and dev@2 to another sub node like "ports" or
> "usb-devices"? Legacy code can support having those devices directly
> underneath the usb controller, but future users would always need to put
> them in a different sub-node so that we can easily differentiate the
> different busses that a usb controller node may support?
>
> I'm not sure I see any need to relate the phy to the ports that are on
> the controller, but if that is needed then perhaps you're right and we
> should move the ports underneath the phy. USB core could be modified to
> go through the legacy path or through the phy, if it even exists, to
> find ports.
>
> Do we typically do this for other phy designs like sata or pci? The phy
> always seemed like a parallel thing to the logical bus that the phy is
> used for.

Rob does this sound ok to 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 v2 03/22] usb: ulpi: Support device discovery via device properties

[Stephen Boyd]

On Fri, Aug 5, 2016 at 2:27 PM, Stephen Boyd  wrote:
> Quoting Peter Chen (2016-07-08 02:04:58)
>> On Thu, Jul 07, 2016 at 03:20:54PM -0700, Stephen Boyd wrote:
>> > @@ -39,6 +42,10 @@ static int ulpi_match(struct device *dev, struct 
>> > device_driver *driver)
>> >   struct ulpi *ulpi = to_ulpi_dev(dev);
>> >   const struct ulpi_device_id *id;
>> >
>> > + /* Some ULPI devices don't have a product id so rely on OF match */
>> > + if (ulpi->id.product == 0)
>> > + return of_driver_match_device(dev, driver);
>> > +
>>
>> How about using vendor id? It can't be 0, but pid may be 0.
>> See: http://www.linux-usb.org/usb.ids
>
> Heikki suggested a product id of 0 would mean we need to use DT
> matching. Should it be changed to vendor id instead?

Any comments here?
--
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: avoid left shift by -1

[Alan Stern]

UBSAN complains about a left shift by -1 in proc_do_submiturb().  This
can occur when an URB is submitted for a bulk or control endpoint on
a high-speed device, since the code doesn't bother to check the
endpoint type; normally only interrupt or isochronous endpoints have
a nonzero bInterval value.

Aside from the fact that the operation is illegal, it shouldn't matter
because the result isn't used.  Still, in theory it could cause a
hardware exception or other problem, so we should work around it.
This patch avoids doing the left shift unless the shift amount is >= 0.

The same piece of code has another problem.  When checking the device
speed (the exponential encoding for interrupt endpoints is used only
by high-speed or faster devices), we need to look for speed >=
USB_SPEED_SUPER as well as speed == USB_SPEED HIGH.  The patch adds
this check.

Signed-off-by: Alan Stern 
Reported-by: Vittorio Zecca 
Tested-by: Vittorio Zecca 
Suggested-by: Bjørn Mork 
CC: 

---


[as1810]


 drivers/usb/core/devio.c |   16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

Index: usb-4.x/drivers/usb/core/devio.c
===
--- usb-4.x.orig/drivers/usb/core/devio.c
+++ usb-4.x/drivers/usb/core/devio.c
@@ -1709,11 +1709,17 @@ static int proc_do_submiturb(struct usb_
as->urb->start_frame = uurb->start_frame;
as->urb->number_of_packets = number_of_packets;
as->urb->stream_id = stream_id;
-   if (uurb->type == USBDEVFS_URB_TYPE_ISO ||
-   ps->dev->speed == USB_SPEED_HIGH)
-   as->urb->interval = 1 << min(15, ep->desc.bInterval - 1);
-   else
-   as->urb->interval = ep->desc.bInterval;
+
+   if (ep->desc.bInterval) {
+   if (uurb->type == USBDEVFS_URB_TYPE_ISO ||
+   ps->dev->speed == USB_SPEED_HIGH ||
+   ps->dev->speed >= USB_SPEED_SUPER)
+   as->urb->interval = 1 <<
+   min(15, ep->desc.bInterval - 1);
+   else
+   as->urb->interval = ep->desc.bInterval;
+   }
+
as->urb->context = as;
as->urb->complete = async_completed;
for (totlen = u = 0; u < number_of_packets; u++) {

--
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: chipidea: hdc: kernel panic during shutdown

[Stefan Wahren]

Hi,

i'm using a iMX233-OLinuXino board and the kernel panics during shutdown with
4.8.0-rc2-next-20160819:

[  420.04] ci_hdrc ci_hdrc.0: remove, state 1
[  420.05] usb usb1: USB disconnect, device number 1
[  420.06] usb 1-1: USB disconnect, device number 2
[  420.06] usb 1-1.1: USB disconnect, device number 3
[  420.09] smsc95xx 1-1.1:1.0 eth0: unregister 'smsc95xx' usb-ci_hdrc.0-1.1,
smsc95xx USB 2.0 Ethernet
[  420.29] ci_hdrc ci_hdrc.0: USB bus 1 deregistered
[  420.30] Unable to handle kernel NULL pointer dereference at virtual
address 0118
[  420.30] pgd = c2ea4000
[  420.30] [0118] *pgd=
[  420.30] Internal error: Oops: 5 [#1] ARM
[  420.30] Modules linked in:
[  420.30] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted
4.8.0-rc2-next-20160819 #1
[  420.30] Hardware name: Freescale MXS (Device Tree)
[  420.30] task: c349 task.stack: c348e000
[  420.30] PC is at usb_hcd_irq+0x0/0x34
[  420.30] LR is at ci_irq+0x58/0x12c
[  420.30] pc : []lr : []psr: a093
[  420.30] sp : c348fd20  ip : c348fcd8  fp : c3704624
[  420.30] r10: c3708300  r9 : c3602010  r8 : 6013
[  420.30] r7 : 0094  r6 : 2013  r5 : c3549600  r4 : c3602010
[  420.30] r3 : 12a0  r2 : 0808  r1 :   r0 : 0094
[  420.30] Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment
none
[  420.30] Control: 0005317f  Table: 42ea4000  DAC: 0051
[  420.30] Process systemd-shutdow (pid: 1, stack limit = 0xc348e190)
[  420.30] Stack: (0xc348fd20 to 0xc349)
[  420.30] fd20: c3549610 c3549600 2013 c006bfac c3549600 
0094 c3602010
[  420.30] fd40: c370e2e0 c36f4e80 c36f4e80 0005 c370e2e0 c006c098
c348fd70 c3704410
[  420.30] fd60: c370e2e0 c039cd68 0005 a013 c36f4e00 c370e2e0
a013 c039ce0c
[  420.30] fd80: c354b844 c348fe5c  c08d604c c3704624 c370e2e0
0001 c3704410
[  420.30] fda0: c08bc194 c08ad460 c354b844 c348fe5c  c08d604c
be829c58 c03997e8
[  420.30] fdc0: c370 c3704410 c08ad460 c0399954 c3402a4c c3704410
c08ad460 c0398d64
[  420.30] fde0: c3704410 c354b810 c112ebf4 c0396e2c c349 0007
0006 c348fe5c
[  420.30] fe00: c3704400 c3704410 c354b810 c354b844 c348fe5c c039bb90
c3704400 c3708090
[  420.30] fe20: c354b810 c039c118  c044941c c354b800 c044e6f8
c354b810 c03a47fc
[  420.30] fe40: c354b810 c354b410 c112ebf4 c039b578 c354b810 c0397410
0002 c08ad22c
[  420.30] fe60: c342fa20 c354b81c 4321fedc c0866838 cdef0123 f3f82200
c000a484 c348e000
[  420.30] fe80:  c00427f4 4321fedc c0042a10  
c348fec8 0029
[  420.30] fea0: c2e2d488  c34b9c28 0002 c00690fc 
 c0132e0c
[  420.30] fec0:   be8291f8 0004 be829f8a 0010
be829224 0005
[  420.30] fee0: be8292c4 000f 7f5fb9f8 0001 be828d3c c08629b4
c348ffb0 00053177
[  420.30] ff00: 07ff 000f0f24 c2e2d480   
 
[  420.30] ff20:   0001  0029 c348fec8
0005 
[  420.30] ff40:  0004001e 0005 be82924c 0005 be82924c
c348e000 
[  420.30] ff60: 7f60e854 c0132f24 c348ff88  c2e2d480 c2e2d480
 c0132f70
[  420.30] ff80:  c000a484  8102d190 8102c118 
8102d190 8102c118
[  420.30] ffa0: 0058 c000a2e0  8102d190 fee1dead 28121969
cdef0123 f3f82200
[  420.30] ffc0:  8102d190 8102c118 0058 be829bc4 
 be829c58
[  420.30] ffe0: 7f60de4c be829b08 7f5ee114 b6e46088 6010 fee1dead
 
[  420.30] [] (usb_hcd_irq) from [] (0xc3549600)
[  420.30] Code: c077a4d4 c08b802c c08b8040 c08b7fb0 (e5913118)
[  420.30] ---[ end trace 5e83b1955a5bd084 ]---
[  420.62] systemd-shutdow: 2 output lines suppressed due to ratelimiting
[  420.62] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x000b
[  420.62]
[  420.62] ---[ end Kernel panic - not syncing: Attempted to kill init!

I will try to bisect this issue.

Regards
Stefan
--
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: xHCI problem? [was Re: Erratic USB device behavior and device loss]

[Alan Stern]

On Tue, 23 Aug 2016, Ritesh Raj Sarraf wrote:

> This one is after a fresh boot.
> 
> T:  Bus=02 Lev=01 Prnt=01 Port=03 Cnt=01 Dev#=  9 Spd=480  MxCh= 0
> D:  Ver= 2.00 Cls=ff(vend.) Sub=ff Prot=ff MxPS=64 #Cfgs=  1
> P:  Vendor=0bda ProdID=0129 Rev=39.60
> S:  Manufacturer=Generic
> S:  Product=USB2.0-CRW
> S:  SerialNumber=2010020139600
> C:* #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=500mA
> I:* If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=06 Prot=50 Driver=rtsx_usb
> E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=83(I) Atr=03(Int.) MxPS=   3 Ivl=64ms
> 
> 
> Here it got enumerated as Bus 02.

Okay, good.  The "Driver=rtsx_usb" is what I wanted to see.  Something
funny is going on with that driver -- it claims to support autosuspend 
but it doesn't ever call usb_mark_last_busy() or 
usb_autopm_get_interface().

In the meantime, can you post the output from "lsmod"?

Also, I'd like to see the output from the following (do this before 
the device disappears):

cd /sys/bus/pci/devices/:00:14.0/usb?/usb?-4/
ls -lR

Finally, you can try applying this patch.  It should prevent the 
constant runtime suspend & resume cycling, but it is not the proper 
solution to your problem.  And it may not prevent the occasional 
spontaneous disconnect.

Alan Stern



Index: usb-4.x/drivers/mfd/rtsx_usb.c
===
--- usb-4.x.orig/drivers/mfd/rtsx_usb.c
+++ usb-4.x/drivers/mfd/rtsx_usb.c
@@ -780,7 +780,6 @@ static struct usb_driver rtsx_usb_driver
.pre_reset  = rtsx_usb_pre_reset,
.post_reset = rtsx_usb_post_reset,
.id_table   = rtsx_usb_usb_ids,
-   .supports_autosuspend   = 1,
.soft_unbind= 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: UBSAN: Undefined behaviour in linux-4.7.2/drivers/usb/core/devio.c:1713:25

[Alan Stern]

On Tue, 23 Aug 2016, Vittorio Zecca wrote:

> After applying the patch above the UBSAN issue in devio.c disappeared.
> 
> However, I got the following messages in dmesg, probably due to a 
> NetworkManager
> malfunctioning, and if I click on the NetworkManager icon I get
> "NetworkManager is not running"
> but maybe this is another issue and your patch did solve the devio.c problem.
> Unfortunately I cannot use the 4.7.2 kernel with ubsan and asan because
> I need the networkmanager running, as it is doing now on kernel version 4.6.6

The NetworkManager problem does look like something completely 
separate.

So thanks for testing the patch, and I will submit it soon.

Alan Stern

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

Re: [Bug] usb: dwc2: host function broken in OTG mode on bcm283x

[Stefan Wahren]

Hi John,

> John Youn  hat am 23. August 2016 um 20:37
> geschrieben:
> 
> 
> 
> How about we fall back to smaller defaults if the original values
> fail? And don't configure more than the amount of endpoints.

this sounds good. I would prefer to calculate the estimated gadget fifo size
before configuring the hardware, so we can bail out without breaking the host
function.

Regards
Stefan

> 
> That should take care of most of the cases. If an irregular
> configuration shows up, require it to use the DT properties.
> 
> Regards,
> John
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug] usb: dwc2: host function broken in OTG mode on bcm283x

[John Youn]

On 8/22/2016 9:41 PM, Stefan Wahren wrote:
> Hi John,
> 
>> John Youn  hat am 23. August 2016 um 00:08
>> geschrieben:
>>
>>
>> On 8/22/2016 1:47 PM, Stefan Wahren wrote:
>>> Hi John,
>>>
 John Youn  hat am 22. August 2016 um 22:06
 geschrieben:


 On 8/20/2016 6:03 AM, Stefan Wahren wrote:
> Hi John,
>
>

 Hi Stefan,

 Why doesn't DT work? I think all the properties are there to set
 these.
>>>
>>> it only works for the future releases, not for existing DT blobs. The DT is
>>> part
>>> of the ABI.
>>>
>>> Yes, i already send a patch to fix the DT [1], but it would be better to fix
>>> the
>>> issue in deep.
>>
>> I don't know much about DT issues. So I'm not sure what the issue is
>> with this.The properties for the gadget fifo sizes already exist so
>> there's no change to the ABI, correct?
> 
> It's correct the ABI doesn't change, but it also means that newer kernel must
> work with older DTB files. So the fix should be better in the driver and not 
> in
> the DT sources.
> 
>>
>> Also, the patch you linked doesn't seem to have settings for the FIFO
>> sizes.
> 
> It wouldn't make sense to provide gadget fifo sizes for a host-only setup. The
> mentioned patch doesn't really fix this issue, that's why i mentioned that we
> should fix this issue first.
> 
>>
>>>

 I hesitate to change the legacy/default settings in case it breaks any
 existing drivers that depend on them.
>>>
>>> Platforms? I didn't expected that other drivers use these settings.
>>
>> Yes, I mean platforms.
>>
>> I believe these values were hard-coded since before the unified "dwc2"
>> existed. So for sure there are platforms using these settings and to
>> lower them will negatively impact those platforms. The DT properties
>> were introduced to override these.
> 
> Unfortunately the gadget fifo sizes should have been marked as required for 
> OTG
> and gadget mode. But now it's too late. We only can improve DT binding here or
> at least add a warning during probe.
> 
>>
>> I think it is best to override them in the broken platforms. Either
>> that or fix it so that the defaults are more intelligently determined
>> in a way that maximizes the fifo sizes and cannot fail, as opposed to
>> some other arbitrary values.
>>
> 
> I don't have the knowledge for the second solution and i wouldn't prefer much
> more detection intelligence.
> 

How about we fall back to smaller defaults if the original values
fail? And don't configure more than the amount of endpoints.

That should take care of most of the cases. If an irregular
configuration shows up, require it to use the DT properties.

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 v3 6/7] phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" dt property

[Hans de Goede]

Hi,

On 08/23/2016 08:00 PM, Rob Herring wrote:

On Sun, Aug 21, 2016 at 02:16:38PM +0200, Hans de Goede wrote:

On some devices the musb otg controller can be used in both device and
host mode, but requires software mode switching since there is no id pin
connected. The usb0 phy code will default to device mode in this case.

On some systems usb0 is connected to a female USB-A port. It can still
be used in device mode when using software mode switching and a USB
A to A cable. To configure the controller to support both modes we must
set its "dr_mode" dt property to "otg" (*). For these setups the device
mode default is wrong, for a female USB-A port the default should be
host mode.

This commit adds support to the sun4i phy code for a new
"allwinner,usb0-usb-a-connector" dt property which can be used in dt
files to indicate this scenario and when present it changes the default
mode to host mode.

*) dr_mode = "host" is used when device mode is _never_ used. E.g.
a wifi module soldered onto the PCB is connected to the musb controller.

Signed-off-by: Hans de Goede 
---
Changes in v2:
-New patch in v2 of this patchset
Changes in v3:
-No changes
---
 Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt | 2 ++
 drivers/phy/phy-sun4i-usb.c | 9 -
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt 
b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
index 287150d..8646b53 100644
--- a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
+++ b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
@@ -35,6 +35,8 @@ Optional properties:
 - usb0_vbus-supply : regulator phandle for controller usb0 vbus
 - usb1_vbus-supply : regulator phandle for controller usb1 vbus
 - usb2_vbus-supply : regulator phandle for controller usb2 vbus
+- allwinner,usb0-usb-a-connector: usb0 is connected to an USB-A connector,
+ rather then an USB-B connector as one would expect (bool)


This seems like a pretty generic problem: a board has an A connector
wired to an OTG controller.  So I think we should have a generic
property.


OK, any suggestions for a name ? And which bindings .txt file should
this go in ?


The only part is here that is not is you have 3 USB buses for
the PHY. That probably should have been 3 child nodes for each PHY port.


Only one is an otg phy / bus though, so we can just use the generic
property and apply it to bus0 / the otg-bus if present.

Regards,

Hans
--
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] phy: rockchip-usb: use rockchip_usb_phy_reset to reset phy during wakeup

[Rob Herring]

On Sun, Aug 21, 2016 at 03:56:43PM +0800, Randy Li wrote:
> It is a hardware bug in RK3288, the only way to solve it is to
> reset the phy.
> 
> Signed-off-by: Randy Li 
> ---
>  .../devicetree/bindings/phy/rockchip-usb-phy.txt |  3 +++
>  drivers/phy/phy-rockchip-usb.c   | 20 
> 
>  2 files changed, 23 insertions(+)

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

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Mathias Nyman]

On 23.08.2016 19:14, Jose Marino wrote:


On 08/23/2016 06:36 AM, Mathias Nyman wrote:

On 23.08.2016 14:26, Mathias Nyman wrote:

On 23.08.2016 13:54, Mathias Nyman wrote:

On 23.08.2016 02:21, Jose Marino wrote:

I'm using my phone (Nexus 5X running Android) to tether a USB
connection to my laptop (XPS 15 9550). I plug the phone through the
USB-C connection and in the phone I  select USB tethering. Initially
things look normal: a usb0 network interface appears in the laptop
and it tries to get an IP with dhcp. However, I observe two
different behaviors depending on whether it's a fresh boot, or I
have suspend/resumed the laptop. In a fresh boot everything works
fine, I get an IP and the connection works as expected. If I unplug
the phone, everything also works as expected.

However, after a suspend/resume cycle, I plug the phone in but the
laptop never connects to it. The usb0 interface still appears, but
the dhcp daemon is unable to get any response and finally times out.
The fun part happens when I unplug the phone. I consistently get a
kernel panic.


...

Anyways, I'll look at that panic in more detail as well





The patch did not apply on top of 4.7.2. I applied this patch instead, which I 
hope is equivalent:

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d7d5025..20b1b18 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -840,6 +840,10 @@ void xhci_stop_endpoint_command_watchdog(unsigned long arg)
  spin_lock_irqsave(>lock, flags);

  ep->stop_cmds_pending--;
+if (xhci->xhc_state & XHCI_STATE_REMOVING) {
+spin_unlock_irqrestore(>lock, flags);
+return;
+}
  if (xhci->xhc_state & XHCI_STATE_DYING) {
  xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
  "Stop EP timer ran, but another timer marked "
@@ -893,7 +897,7 @@ void xhci_stop_endpoint_command_watchdog(unsigned long arg)
  spin_unlock_irqrestore(>lock, flags);
  xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
  "Calling usb_hc_died()");
-usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
+usb_hc_died(xhci_to_hcd(xhci));
  xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
  "xHCI host controller is dead.");
  }


So, I apply the patch, reboot, suspend/resume, plug in phone and tell it to 
tether. The dhcp client is still unable to communicate and times out. However, 
the patch seems to have avoided the NULL dereference. The computer did not 
panic although my X session stopped responding. I went to virtual console and 
recorded a dmesg (find attached).


Patch looks correct, It only solves the NULL pointer issue
Thanks for testing.
The new log again points to a locking issue, I'll take a look.

-Mathias
  




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

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Mathias Nyman]

On 23.08.2016 18:10, Alan Stern wrote:

On Tue, 23 Aug 2016, Mathias Nyman wrote:


The Dell XPS 9550 has an additional xhci controller for handling the type-C 
port.
This controller is hotplug removed from the PCI bus when the last USB type-c
device is disconnected.

xhci driver, and usb core it seems is not really designed with this in mind.
xhci driver will suddenly start reading  from PCI.

I've been looking at issues related to this. Currently there is at least one 
similar case with mass storage
where we see the device release function being called for the mass storage 
interface device _after_ we
freed all memory related to both xhci hcd's. bug for that is here:

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

usb devices with their children should be synchronously removed before hcd's 
are freed, but seems
that is not the case, at least not for the device release function for the 
interface device.


Be careful what words you use.  USB devices and their children are
indeed synchronously _removed_ before hcd's are freed.  However, they
may not be _released_ until later.

What you encountered in that bug report was probably usb_release_dev()
calling bus_to_hcd() and usb_put_hcd().  This action is part of a
release, and it's allowed to happen long after everything has been
removed (i.e., unregistered).



Yes, it's the release function which is called later,
I did mix up the concepts of remove and release.


Reference counting is supposed to keep everything important from being
deallocated until all the releases are finished.  In particular, if the
hcd structure was already deallocated when usb_put_hcd() was called
then there is a refcounting bug somewhere.


That seems like a possible cause, I'll look into the refcouting.

This helps a lot. I was getting stuck with that bug.
Thanks

-Mathias

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

[PATCH 3/3] usb: gadget: f_hid: use alloc_ep_req()

[Felipe F. Tonello]

Use gadget's framework allocation function instead of directly calling
usb_ep_alloc_request().

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/gadget/function/f_hid.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c 
b/drivers/usb/gadget/function/f_hid.c
index aa1c19946d10..e2966f87c860 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -617,14 +617,10 @@ static int hidg_bind(struct usb_configuration *c, struct 
usb_function *f)
 
/* preallocate request and buffer */
status = -ENOMEM;
-   hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
+   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
if (!hidg->req)
goto fail;
 
-   hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
-   if (!hidg->req->buf)
-   goto fail;
-
/* set descriptor dynamic values */
hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
-- 
2.9.3

--
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/3] usb: gadget: f_hid: use free_ep_req()

[Felipe F. Tonello]

We should always use free_ep_req() when allocating requests with
alloc_ep_req().

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/gadget/function/f_hid.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c 
b/drivers/usb/gadget/function/f_hid.c
index fa807ce7fc0e..aa1c19946d10 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -677,11 +677,8 @@ fail_free_descs:
usb_free_all_descriptors(f);
 fail:
ERROR(f->config->cdev, "hidg_bind FAILED\n");
-   if (hidg->req != NULL) {
-   kfree(hidg->req->buf);
-   if (hidg->in_ep != NULL)
-   usb_ep_free_request(hidg->in_ep, hidg->req);
-   }
+   if (hidg->req != NULL)
+   free_ep_req(hidg->in_ep, hidg->req);
 
return status;
 }
@@ -920,8 +917,7 @@ static void hidg_unbind(struct usb_configuration *c, struct 
usb_function *f)
 
/* disable/free request and end point */
usb_ep_disable(hidg->in_ep);
-   kfree(hidg->req->buf);
-   usb_ep_free_request(hidg->in_ep, hidg->req);
+   free_ep_req(hidg->in_ep, hidg->req);
 
usb_free_all_descriptors(f);
 }
-- 
2.9.3

--
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/3] Gadget endpoint allocation request

[Felipe F. Tonello]

Patch #1 removes a unnecessary and confusing parameter from alloc_ep_req().
Also, this series updates f_hid function to use alloc/free_ep_req() instead of
allocating/freeing requests directly.

Felipe F. Tonello (3):
  usb: gadget: remove useless parameter in alloc_ep_req()
  usb: gadget: f_hid: use free_ep_req()
  usb: gadget: f_hid: use alloc_ep_req()

 drivers/usb/gadget/function/f_hid.c| 18 +-
 drivers/usb/gadget/function/f_loopback.c   |  6 ++
 drivers/usb/gadget/function/f_midi.c   |  2 +-
 drivers/usb/gadget/function/f_sourcesink.c |  6 ++
 drivers/usb/gadget/u_f.c   |  7 +++
 drivers/usb/gadget/u_f.h   |  3 +--
 6 files changed, 14 insertions(+), 28 deletions(-)

-- 
2.9.3

--
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/3] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe F. Tonello]

The default_length parameter of alloc_ep_req was not really necessary
and gadget drivers would almost always create an inline function to pass
the same value to len and default_len.

This patch removes that parameter and updates all calls to alloc_ep_req() to
use the new API.

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/gadget/function/f_hid.c| 2 +-
 drivers/usb/gadget/function/f_loopback.c   | 6 ++
 drivers/usb/gadget/function/f_midi.c   | 2 +-
 drivers/usb/gadget/function/f_sourcesink.c | 6 ++
 drivers/usb/gadget/u_f.c   | 7 +++
 drivers/usb/gadget/u_f.h   | 3 +--
 6 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c 
b/drivers/usb/gadget/function/f_hid.c
index 4cd486600a9b..fa807ce7fc0e 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -365,7 +365,7 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
 static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
unsigned length)
 {
-   return alloc_ep_req(ep, length, length);
+   return alloc_ep_req(ep, length);
 }
 
 static void hidg_set_report_complete(struct usb_ep *ep, struct usb_request 
*req)
diff --git a/drivers/usb/gadget/function/f_loopback.c 
b/drivers/usb/gadget/function/f_loopback.c
index 3a9f8f9c77bd..ddb8c896eaa3 100644
--- a/drivers/usb/gadget/function/f_loopback.c
+++ b/drivers/usb/gadget/function/f_loopback.c
@@ -308,9 +308,7 @@ static void disable_loopback(struct f_loopback *loop)
 
 static inline struct usb_request *lb_alloc_ep_req(struct usb_ep *ep, int len)
 {
-   struct f_loopback   *loop = ep->driver_data;
-
-   return alloc_ep_req(ep, len, loop->buflen);
+   return alloc_ep_req(ep, len);
 }
 
 static int alloc_requests(struct usb_composite_dev *cdev,
@@ -333,7 +331,7 @@ static int alloc_requests(struct usb_composite_dev *cdev,
if (!in_req)
goto fail;
 
-   out_req = lb_alloc_ep_req(loop->out_ep, 0);
+   out_req = lb_alloc_ep_req(loop->out_ep, loop->buflen);
if (!out_req)
goto fail_in;
 
diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index 3a47596afcab..a5719f271bf0 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -211,7 +211,7 @@ static struct usb_gadget_strings *midi_strings[] = {
 static inline struct usb_request *midi_alloc_ep_req(struct usb_ep *ep,
unsigned length)
 {
-   return alloc_ep_req(ep, length, length);
+   return alloc_ep_req(ep, length);
 }
 
 static const uint8_t f_midi_cin_length[] = {
diff --git a/drivers/usb/gadget/function/f_sourcesink.c 
b/drivers/usb/gadget/function/f_sourcesink.c
index df0189ddfdd5..8784fa12ea2c 100644
--- a/drivers/usb/gadget/function/f_sourcesink.c
+++ b/drivers/usb/gadget/function/f_sourcesink.c
@@ -293,9 +293,7 @@ static struct usb_gadget_strings *sourcesink_strings[] = {
 
 static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
 {
-   struct f_sourcesink *ss = ep->driver_data;
-
-   return alloc_ep_req(ep, len, ss->buflen);
+   return alloc_ep_req(ep, len);
 }
 
 static void disable_ep(struct usb_composite_dev *cdev, struct usb_ep *ep)
@@ -606,7 +604,7 @@ static int source_sink_start_ep(struct f_sourcesink *ss, 
bool is_in,
} else {
ep = is_in ? ss->in_ep : ss->out_ep;
qlen = ss->bulk_qlen;
-   size = 0;
+   size = ss->buflen;
}
 
for (i = 0; i < qlen; i++) {
diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
index 907f8144813c..18839732c840 100644
--- a/drivers/usb/gadget/u_f.c
+++ b/drivers/usb/gadget/u_f.c
@@ -14,15 +14,14 @@
 #include "u_f.h"
 #include 
 
-struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int 
default_len)
+struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len)
 {
struct usb_request  *req;
 
req = usb_ep_alloc_request(ep, GFP_ATOMIC);
if (req) {
-   req->length = len ?: default_len;
-   if (usb_endpoint_dir_out(ep->desc))
-   req->length = usb_ep_align(ep, req->length);
+   req->length = usb_endpoint_dir_out(ep->desc) ?
+   usb_ep_align(ep, len) : len;
req->buf = kmalloc(req->length, GFP_ATOMIC);
if (!req->buf) {
usb_ep_free_request(ep, req);
diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
index 69a1d10df04f..7d53a4773d1a 100644
--- a/drivers/usb/gadget/u_f.h
+++ b/drivers/usb/gadget/u_f.h
@@ -53,14 +53,13 @@ struct usb_request;
  *
  * @ep: the endpoint to allocate a usb_request
  * @len: usb_requests's 

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Mathias Nyman]

On 23.08.2016 17:53, Alan Stern wrote:

On Tue, 23 Aug 2016, Mathias Nyman wrote:


Or then this happens:
(I'll call the hcds usb2_hcd and usb3_hcd to keep track of them, usb2_hcd is 
the primary_hcd)

to begin with:


Actually, to begin with neither usb2_hcd nor usb3_hcd exists.  Then
usb2_hcd is registered, at which point we have:

usb2_hcd->primary_hcd = NULL
usb2_hcd->shared_hcd = NULL


True



Then usb3_hcd is registered, at which point we have:


usb2_hcd->primary_hcd = usb2_hcd
usb2_hcd->shared_hcd  = usb3_hcd

usb3_hcd->primary_hcd = usb2_hcd
usb3_hcd->shared_hcd  = usb2_hcd


usb3_host is removed first:
xhci_pci_remove(struct pci_dev *dev)
usb_remove_hcd(xhci->shared_hcd);  // remove usb3_hcd
usb_put_hcd(xhci->shared_hcd)
  hcd_release(..)
if (hcd->shared_hcd) {   //true
  struct usb_hcd *peer = hcd->shared_hcd;   //peer is 
now usb2_hcd
  peer->shared_hcd = NULL; //sets usb2_hcd->shared_hcd to 
NULL
  peer->primary_hcd = NULL;// sets usb2_hcd->primary_hcd to 
NULL.  Why do we do this??


We do this because then the state is exactly the same as it was after
usb2_hcd was registered but before usb3_hcd was registered.

So what happened here is very much like what would happen if something
went wrong during probing, after the primary hcd was registered and
before the secondary hcd was registered.


Ok, thanks, that makes sense.

xhci driver shouldn't assume usb2_hcd->primary_hcd exists.
It was unnecessary anyway as xhci_to_hcd() returns the primary hcd

-Mathias

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

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Mathias Nyman]

On 23.08.2016 16:13, Greg KH wrote:

On Tue, Aug 23, 2016 at 01:54:05PM +0300, Mathias Nyman wrote:

On 23.08.2016 02:21, Jose Marino wrote:

I'm using my phone (Nexus 5X running Android) to tether a USB connection to my 
laptop (XPS 15 9550). I plug the phone through the USB-C connection and in the 
phone I  select USB tethering. Initially things look normal: a usb0 network 
interface appears in the laptop and it tries to get an IP with dhcp. However, I 
observe two different behaviors depending on whether it's a fresh boot, or I 
have suspend/resumed the laptop. In a fresh boot everything works fine, I get 
an IP and the connection works as expected. If I unplug the phone, everything 
also works as expected.

However, after a suspend/resume cycle, I plug the phone in but the laptop never 
connects to it. The usb0 interface still appears, but the dhcp daemon is unable 
to get any response and finally times out. The fun part happens when I unplug 
the phone. I consistently get a kernel panic.

I managed to get some logs of the oops+panic from pstore. Find them attached. 
In this particular situation this is what I did:
- Boot laptop (archlinux with kernel 4.7.2)
- Suspend/resume
- Plug Nexus 5X
- After a few seconds unplug Nexus 5X

I filed a bug report about this: 
https://bugzilla.kernel.org/show_bug.cgi?id=153551



The Dell XPS 9550 has an additional xhci controller for handling the type-C 
port.
This controller is hotplug removed from the PCI bus when the last USB type-c
device is disconnected.

xhci driver, and usb core it seems is not really designed with this in mind.


The USB core can handle this just fine.


xhci driver will suddenly start reading  from PCI.


Which means the device is gone, and you need to handle it properly.  We
fixed up ehci and ohci for this years ago (they were on hotplug busses).

For every PCI read, you need to verify that the data is correct, that's
the way that any PCI driver needs to work in a "modern" system.

This is an XHCI issue, not a USB core issue :)


Yes, reading  and reacting properly to it is purely xhci.



I've been looking at issues related to this. Currently there is at least one 
similar case with mass storage
where we see the device release function being called for the mass storage 
interface device _after_ we
freed all memory related to both xhci hcd's. bug for that is here:

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

usb devices with their children should be synchronously removed before hcd's 
are freed, but seems
that is not the case, at least not for the device release function for the 
interface device.


Once you notice that your PCI device is gone, you need to start tearing
things down as soon as possible.  Or just stop things and wait for the
PCI core to come around and remove you from the system.  That's probably
much more simple and I think is what was done for EHCI.


That part should be doable, but the part where the interface device release is 
called after hcd
is freed still puzzles me. As Alan suggested I need to check if the reference 
counting is correct.




A horrible workaround to hide this issue was to sleep for a second or two 
before freeing the hcd memory,
this lets some pending work finish before hcds disappear. (more info in that 
bug report)


Yeah, that's not a good idea :)



Just a intermediate step to find which way to continue debugging, not a 
solution.

-Mathias


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

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Jose Marino]

On 08/23/2016 06:36 AM, Mathias Nyman wrote:

On 23.08.2016 14:26, Mathias Nyman wrote:

On 23.08.2016 13:54, Mathias Nyman wrote:

On 23.08.2016 02:21, Jose Marino wrote:

I'm using my phone (Nexus 5X running Android) to tether a USB
connection to my laptop (XPS 15 9550). I plug the phone through the
USB-C connection and in the phone I  select USB tethering. Initially
things look normal: a usb0 network interface appears in the laptop
and it tries to get an IP with dhcp. However, I observe two
different behaviors depending on whether it's a fresh boot, or I
have suspend/resumed the laptop. In a fresh boot everything works
fine, I get an IP and the connection works as expected. If I unplug
the phone, everything also works as expected.

However, after a suspend/resume cycle, I plug the phone in but the
laptop never connects to it. The usb0 interface still appears, but
the dhcp daemon is unable to get any response and finally times out.
The fun part happens when I unplug the phone. I consistently get a
kernel panic.


...

Anyways, I'll look at that panic in more detail as well



<6>[  178.693631] xhci_hcd :3e:00.0: USB bus 4 deregistered
<6>[  178.693642] xhci_hcd :3e:00.0: remove, state 1
<6>[  178.693648] usb usb3: USB disconnect, device number 1
<4>[  183.634994] xhci_hcd :3e:00.0: xHCI host not responding to
stop endpoint command.
<4>[  183.635001] xhci_hcd :3e:00.0: Assuming host is dying,
halting host.
<4>[  183.635019] xhci_hcd :3e:00.0: Host not halted after 16000
microseconds.
<4>[  183.635022] xhci_hcd :3e:00.0: Non-responsive xHCI host is
not halting.
<4>[  183.635025] xhci_hcd :3e:00.0: Completing active URBs anyway.
<1>[  183.635116] BUG: unable to handle kernel NULL pointer
dereference at   (null)
<1>[  183.635402] IP: [] usb_hc_died+0x16/0xc0
[usbcore]


Looks like the 5 second command timeout timer for stop endpoint
commands causes this.
the timer (stop_cmd_timer) will call
xhci_stop_endpoint_command_watchdog()  which calls
   usb_hc_died(xhci_to_hcd(xhci)->primary_hcd)

but hcd are probably freed and pointers set to null already  -> NULL
pointer dereference.

The timer should be synchronously deleted when the device is freed,
unless xhci_free_dev()
returns early.

So either hub_free_dev() is not called for this device at hcd removal,
or xhci_free_dev returns early.



Or then this happens:
(I'll call the hcds usb2_hcd and usb3_hcd to keep track of them,
usb2_hcd is the primary_hcd)

to begin with:
usb2_hcd->primary_hcd = usb2_hcd
usb2_hcd->shared_hcd  = usb3_hcd

usb3_hcd->primary_hcd = usb2_hcd
usb3_hcd->shared_hcd  = usb2_hcd


usb3_host is removed first:
xhci_pci_remove(struct pci_dev *dev)
  usb_remove_hcd(xhci->shared_hcd);  // remove usb3_hcd
  usb_put_hcd(xhci->shared_hcd)
hcd_release(..)
  if (hcd->shared_hcd) {//true
struct usb_hcd *peer = hcd->shared_hcd;   //peer
is now usb2_hcd
peer->shared_hcd = NULL; //sets usb2_hcd->shared_hcd
to NULL
peer->primary_hcd = NULL;// sets
usb2_hcd->primary_hcd to NULL.  Why do we do this??
stop_cmd_timer triggers before the usb2_hcd is removed:
-> xhci_stop_endpoint_command_watchdog()
 usb_hc_died(xhci_to_hcd(xhci)->primary_hcd)  // xhci_to_hcd will
get usb2_hcd, usb2_hcd->primary_hcd is set to NULL here.


does something like this help?

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fd9fd12..797137e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -850,6 +850,10 @@ void xhci_stop_endpoint_command_watchdog(unsigned
long arg)
spin_lock_irqsave(>lock, flags);

ep->stop_cmds_pending--;
+   if (xhci->xhc_state & XHCI_STATE_REMOVING) {
+   spin_unlock_irqrestore(>lock, flags);
+   return;
+   }
if (xhci->xhc_state & XHCI_STATE_DYING) {
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"Stop EP timer ran, but another timer
marked "
@@ -903,7 +907,7 @@ void xhci_stop_endpoint_command_watchdog(unsigned
long arg)
spin_unlock_irqrestore(>lock, flags);
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"Calling usb_hc_died()");
-   usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
+   usb_hc_died(xhci_to_hcd(xhci));
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"xHCI host controller is dead.");
 }



The patch did not apply on top of 4.7.2. I applied this patch instead, 
which I hope is equivalent:


diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d7d5025..20b1b18 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -840,6 +840,10 @@ void xhci_stop_endpoint_command_watchdog(unsigned 
long arg)

spin_lock_irqsave(>lock, flags);

ep->stop_cmds_pending--;
+   if (xhci->xhc_state & XHCI_STATE_REMOVING) {
+   

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Jose Marino]

On 08/23/2016 01:57 AM, Oliver Neukum wrote:

On Mon, 2016-08-22 at 17:21 -0600, Jose Marino wrote:

I'm using my phone (Nexus 5X running Android) to tether a USB connection
to my laptop (XPS 15 9550). I plug the phone through the USB-C
connection and in the phone I  select USB tethering. Initially things
look normal: a usb0 network interface appears in the laptop and it tries
to get an IP with dhcp. However, I observe two different behaviors
depending on whether it's a fresh boot, or I have suspend/resumed the
laptop. In a fresh boot everything works fine, I get an IP and the
connection works as expected. If I unplug the phone, everything also
works as expected.

However, after a suspend/resume cycle, I plug the phone in but the
laptop never connects to it. The usb0 interface still appears, but the
dhcp daemon is unable to get any response and finally times out. The fun
part happens when I unplug the phone. I consistently get a kernel panic.


The HC has crashed thoroughly. Do other devices work after S3?


Everything works fine after S3. I regularly use a usb-c ethernet adapter 
and even though it's a bit flaky, I suspend/resume regularly without any 
kernel panics or oopses. About the phone, the kernel panic only happens 
when I enable tethering on the phone. Anything else seems to work fine: 
plug in to charge, download pictures or access its file system.





Regards
Oliver



--
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: UBSAN: Undefined behaviour in linux-4.7.2/drivers/usb/core/devio.c:1713:25

[Vittorio Zecca]

After applying the patch above the UBSAN issue in devio.c disappeared.

However, I got the following messages in dmesg, probably due to a NetworkManager
malfunctioning, and if I click on the NetworkManager icon I get
"NetworkManager is not running"
but maybe this is another issue and your patch did solve the devio.c problem.
Unfortunately I cannot use the 4.7.2 kernel with ubsan and asan because
I need the networkmanager running, as it is doing now on kernel version 4.6.6

[  +0.648205] BUG: unable to handle kernel paging request at 8272d6400205
[  +0.000118] IP: [] strcmp+0x48/0x8b
[  +0.75] PGD 0
[  +0.33] Oops:  [#1] SMP KASAN
[  +0.51] Modules linked in: ebtable_nat ebtable_broute bridge stp
llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6
nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security
ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4
nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle
iptable_security iptable_raw intel_rapl x86_pkg_temp_thermal coretemp
kvm_intel kvm snd_hda_codec_realtek snd_hda_codec_hdmi
snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hda_core
huawei_cdc_ncm snd_hwdep snd_seq irqbypass cdc_wdm ppdev
crct10dif_pclmul cdc_ncm snd_seq_device iTCO_wdt option
iTCO_vendor_support crc32_pclmul snd_pcm crc32c_intel usb_wwan usbnet
ghash_clmulni_intel snd_timer parport_pc i2c_i801 snd nuvoton_cir
pcspkr mei_me parport soc_button_array mei rc_core
[  +0.001125]  shpchp lpc_ich soundcore tpm_tis tpm nfsd auth_rpcgss
nfs_acl lockd grace sunrpc i915 i2c_algo_bit drm_kms_helper r8169 drm
serio_raw mii video fjes uas usb_storage
[  +0.000265] CPU: 1 PID: 856 Comm: NetworkManager Not tainted 4.7.2sanitized #4
[  +0.94] Hardware name: To Be Filled By O.E.M. To Be Filled By
O.E.M./H81M-DGS R2.0, BIOS P1.30 07/02/2014
[  +0.000127] task: 8800346f9e00 ti: 8800d5df8000 task.ti:
8800d5df8000
[  +0.96] RIP: 0010:[]  []
strcmp+0x48/0x8b
[  +0.000104] RSP: 0018:8800d5dfed28  EFLAGS: 00010246
[  +0.71] RAX:  RBX: c0263c20 RCX: 818e11d4
[  +0.93] RDX: 104e5ac80040 RSI: 8272d6400205 RDI: 8272d6400205
[  +0.93] RBP: 8800d5dfed50 R08: 880389669150 R09: ed00712cd3d2
[  +0.92] R10: 8800346f9e00 R11: 0010 R12: 8272d6400205
[  +0.92] R13: 0072 R14: c0263c21 R15: 8272d6400206
[  +0.93] FS:  7fce6294e880() GS:88038ed0()
knlGS:
[  +0.000103] CS:  0010 DS:  ES:  CR0: 80050033
[  +0.002818] CR2: 8272d6400205 CR3: 350b6000 CR4: 000406e0
[  +0.002903] Stack:
[  +0.002833]  82aa6a78 11001abbfdaf 880384ba0b80
c0263c20
[  +0.47]  880389669e70
[  +0.01]  8800d5dfee00
[  +0.01]  81bf33dd
[  +0.00]  88038b1c8000

[  +0.01]  026040c0
[  +0.01]  88038c7fc560
[  +0.00]  41b58ab3
[  +0.01]  82a70a84

[  +0.01] Call Trace:
[  +0.07]  [] _request_firmware+0x156/0x274
[  +0.04]  [] ? request_firmware_nowait+0x2ad/0x2ad
[  +0.04]  [] ? kasan_kmalloc+0x5e/0x64
[  +0.04]  [] ? kmem_cache_alloc_trace+0x124/0x390
[  +0.04]  [] request_firmware+0x31/0x43
[  +0.09]  [] rtl_open+0x1133/0x1b3a [r8169]
[  +0.09]  [] ? rtl_remove_one+0x35d/0x35d [r8169]
[  +0.04]  [] ? packet_notifier+0xdb/0x593
[  +0.04]  [] ? register_prot_hook.part.17+0x6e/0x6e
[  +0.03]  [] ? ip6mr_device_event+0xa6/0x276
[  +0.03]  [] ? mif6_delete+0x41c/0x41c
[  +0.14]  [] ? br_device_event+0x41/0x360 [bridge]
[  +0.05]  [] ? raw_notifier_call_chain+0x85/0xec
[  +0.05]  [] __dev_open+0x161/0x24c
[  +0.02]  [] ? dev_set_rx_mode+0x33/0x33
[  +0.03]  [] ? __dev_set_rx_mode+0x3d/0x1bc
[  +0.03]  [] __dev_change_flags+0xe8/0x229
[  +0.03]  [] dev_change_flags+0x5d/0xbd
[  +0.04]  [] do_setlink+0x628/0x1c26
[  +0.03]  [] ? __alloc_pages_nodemask+0x26a/0x1ebe
[  +0.03]  [] ? rtnl_unregister+0x201/0x201
[  +0.04]  [] ? alloc_debug_processing+0x56/0x36e
[  +0.03]  [] ? __alloc_pages_nodemask+0x26a/0x1ebe
[  +0.03]  [] ? warn_alloc_failed+0x266/0x266
[  +0.03]  [] ? __init_rwsem+0xfd/0x17c
[  +0.04]  [] ? rt_mutex_finish_proxy_lock+0x1bd/0x1bd
[  +0.05]  [] ? is_ftrace_trampoline+0x62/0xaf
[  +0.03]  [] ? __kernel_text_address+0x63/0x82
[  +0.04]  [] ? print_context_stack+0x7e/0x177
[  +0.02]  [] ? memset+0x31/0x38
[  +0.04]  [] ? nla_parse+0xa1/0x2d9
[  +0.03]  [] ? validate_linkmsg+0x140/0x3df
[  +0.03]  [] ? rtnl_link_get_net+0xf3/0xf3
[  +0.03]  [] ? is_ftrace_trampoline+0x62/0xaf
[  +0.03]  [] rtnl_newlink+0x9c8/0xf17
[  +0.03]  [] ? rtnl_newlink+0x69e/0xf17
[  +0.03]  [] ? rtnetlink_put_metrics+0x454/0x454
[  +0.04]  [] ? __kernel_text_address+0x63/0x82
[  +0.03]  [] ? print_context_stack+0x7e/0x177
[  +0.04]  [] ? 

Re: [PATCH v7] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Alan Stern]

On Tue, 23 Aug 2016, Wenyou Yang wrote:

> The usb controller does not manage correctly the suspend mode for
> the ehci. In echi mode, there is no way to suspend without any
> device connected to it. This is why this specific control is added
> to fix this issue. Since the suspend mode works in ohci mode, this
> specific control works by suspend the usb controller in ohci mode.
> 
> This specific control is by setting the SUSPEND_A/B/C fields of
> SFR_OHCIICR(OHCI Interrupt Configuration Register) in the SFR
> while the OHCI USB suspend.
> 
> This set operation must be done before the USB clock disabled,
> clear operation after the USB clock enabled.
> 
> Signed-off-by: Wenyou Yang 
> Reviewed-by: Alexandre Belloni 
> Acked-by: Nicolas Ferre 
> ---
> 
> Changes in v7:
>  - Rename ohci_at91_port_ctrl() to ohci_at91_port_suspend().
>  - Add check valid_port(wIndex)  before invoke
>ohci_at91_port_suspend().
>  - Call ohci_at91_port_suspend() directly on suspend/resume
>operations.

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: musb: am3358: having problem with high-speed on usb1 at peripheral

[Bin Liu]

Hi,

On Tue, Aug 23, 2016 at 09:57:17AM +0800, Ayaka wrote:
> 
> 
> 從我的 iPad 傳送
> 
> > Bin Liu  於 2016年8月19日 上午12:08 寫道：
> > 
> > Hi,
> > 
> >> On Fri, Aug 12, 2016 at 10:23:15AM +0800, ayaka wrote:
> >> Hello all:
> >>  I recently add a support for customize am3358 board using the branch
> >> processor-sdk-linux-03.00.00 from Ti git. But I meet a problem with musb
> >> at the peripheral mode.
> >>  I have force usb1 in peripheral mode in dts as it only have USB_DP,
> >> USB_DM and USBDRV connected. I start up a ether gadget using the configfs,
> > 
> > Do you mean USB1_DRVVBUS pin? If so, you have wrong hw design. For
> > peripheral mode, you should float this pin, but connect USB1_VBUS pin
> > instead.
> > 
> Thank you very much. After hardware modification, it could work on hi-speed 
> mode now.
> But I wonder it have effect? As the vbus is not used to transmit data in USB?

>From am3358 perspective, USB1_VBUS is an input pin, which is one of the
conditions for musb role switching. More detail is in the am335x TRM.

Regards,
-Bin.

> > Regards,
> > -Bin.
> > 
> >> But I found it would always work on the USB 2.0 full speed mode,
> >> even the testmode can't force it in high speed mode or it won't appear
> >> at host side. If I want it work at high speed mode, what should I do?
> >>  I also meet the DMA problem, I have to apply the "ARM: dts: am33xx: 
> >> fix DMA support for the second USB interface", but does patches really
> >> work? Although it make the error message disapear, but the mail list
> >> said the just make the usb back to PIO mode.
> >> 
> >> -- 
> >> 2.7.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
> 
--
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: usbip: vudc: fix left shift overflow

[Nicolas Iooss]

In v_recv_cmd_submit(), urb_p->urb->pipe has the type unsigned int
(which is 32-bit long on x86_64) but 11<<30 results in a 34-bit integer.
Therefore the 2 leading bits are truncated and

urb_p->urb->pipe &= ~(11 << 30);

has the same meaning as

urb_p->urb->pipe &= ~(3 << 30);

This second statement seems to be how the code was intended to be
written, as PIPE_ constants have values between 0 and 3.

The overflow has been detected with a clang warning:

drivers/usb/usbip/vudc_rx.c:145:27: warning: signed shift result
(0x2C000) requires 35 bits to represent, but 'int' only has 32
bits [-Wshift-overflow]
urb_p->urb->pipe &= ~(11 << 30);
  ~~ ^  ~~

Fixes: 79c02cb1fd5c ("usbip: vudc: Add vudc_rx")
Signed-off-by: Nicolas Iooss 
---
 drivers/usb/usbip/vudc_rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/vudc_rx.c b/drivers/usb/usbip/vudc_rx.c
index 344bd9473475..e429b59f6f8a 100644
--- a/drivers/usb/usbip/vudc_rx.c
+++ b/drivers/usb/usbip/vudc_rx.c
@@ -142,7 +142,7 @@ static int v_recv_cmd_submit(struct vudc *udc,
urb_p->urb->status = -EINPROGRESS;
 
/* FIXME: more pipe setup to please usbip_common */
-   urb_p->urb->pipe &= ~(11 << 30);
+   urb_p->urb->pipe &= ~(3 << 30);
switch (urb_p->ep->type) {
case USB_ENDPOINT_XFER_BULK:
urb_p->urb->pipe |= (PIPE_BULK << 30);
-- 
2.9.3

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

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Alan Stern]

On Tue, 23 Aug 2016, Mathias Nyman wrote:

> The Dell XPS 9550 has an additional xhci controller for handling the type-C 
> port.
> This controller is hotplug removed from the PCI bus when the last USB type-c
> device is disconnected.
> 
> xhci driver, and usb core it seems is not really designed with this in mind.
> xhci driver will suddenly start reading  from PCI.
> 
> I've been looking at issues related to this. Currently there is at least one 
> similar case with mass storage
> where we see the device release function being called for the mass storage 
> interface device _after_ we
> freed all memory related to both xhci hcd's. bug for that is here:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=120241
> 
> usb devices with their children should be synchronously removed before hcd's 
> are freed, but seems
> that is not the case, at least not for the device release function for the 
> interface device.

Be careful what words you use.  USB devices and their children are
indeed synchronously _removed_ before hcd's are freed.  However, they
may not be _released_ until later.

What you encountered in that bug report was probably usb_release_dev()  
calling bus_to_hcd() and usb_put_hcd().  This action is part of a
release, and it's allowed to happen long after everything has been
removed (i.e., unregistered).

Reference counting is supposed to keep everything important from being
deallocated until all the releases are finished.  In particular, if the 
hcd structure was already deallocated when usb_put_hcd() was called 
then there is a refcounting bug somewhere.

> A horrible workaround to hide this issue was to sleep for a second or two 
> before freeing the hcd memory,
> this lets some pending work finish before hcds disappear. (more info in that 
> bug report)

The driver should synchronously cancel the work instead of sleeping.

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: UBSAN: Undefined behaviour in linux-4.7.2/drivers/usb/core/devio.c:1713:25

[Alan Stern]

On Mon, 22 Aug 2016, Vittorio Zecca wrote:

> I can reproduce the UBSAN report by inserting in the USB receptacle a
> HUAWEI Mobile Broadband E353 HSPA+ USB Stick.

> If there is any patch I'll be happy to try it.

Thank you.  Please let us know what happens with the patch below.

Alan Stern



Index: usb-4.x/drivers/usb/core/devio.c
===
--- usb-4.x.orig/drivers/usb/core/devio.c
+++ usb-4.x/drivers/usb/core/devio.c
@@ -1709,11 +1709,17 @@ static int proc_do_submiturb(struct usb_
as->urb->start_frame = uurb->start_frame;
as->urb->number_of_packets = number_of_packets;
as->urb->stream_id = stream_id;
-   if (uurb->type == USBDEVFS_URB_TYPE_ISO ||
-   ps->dev->speed == USB_SPEED_HIGH)
-   as->urb->interval = 1 << min(15, ep->desc.bInterval - 1);
-   else
-   as->urb->interval = ep->desc.bInterval;
+
+   if (ep->desc.bInterval) {
+   if (uurb->type == USBDEVFS_URB_TYPE_ISO ||
+   ps->dev->speed == USB_SPEED_HIGH ||
+   ps->dev->speed >= USB_SPEED_SUPER)
+   as->urb->interval = 1 <<
+   min(15, ep->desc.bInterval - 1);
+   else
+   as->urb->interval = ep->desc.bInterval;
+   }
+
as->urb->context = as;
as->urb->complete = async_completed;
for (totlen = u = 0; u < number_of_packets; u++) {

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

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Alan Stern]

On Tue, 23 Aug 2016, Mathias Nyman wrote:

> Or then this happens:
> (I'll call the hcds usb2_hcd and usb3_hcd to keep track of them, usb2_hcd is 
> the primary_hcd)
> 
> to begin with:

Actually, to begin with neither usb2_hcd nor usb3_hcd exists.  Then 
usb2_hcd is registered, at which point we have:

usb2_hcd->primary_hcd = NULL
usb2_hcd->shared_hcd = NULL

Then usb3_hcd is registered, at which point we have:

> usb2_hcd->primary_hcd = usb2_hcd
> usb2_hcd->shared_hcd  = usb3_hcd
> 
> usb3_hcd->primary_hcd = usb2_hcd
> usb3_hcd->shared_hcd  = usb2_hcd
> 
> 
> usb3_host is removed first:
> xhci_pci_remove(struct pci_dev *dev)
>usb_remove_hcd(xhci->shared_hcd);  // remove usb3_hcd
>usb_put_hcd(xhci->shared_hcd)
>  hcd_release(..)
>if (hcd->shared_hcd) { //true
>  struct usb_hcd *peer = hcd->shared_hcd;   //peer is 
> now usb2_hcd
>  peer->shared_hcd = NULL; //sets usb2_hcd->shared_hcd to 
> NULL
>  peer->primary_hcd = NULL;// sets usb2_hcd->primary_hcd 
> to NULL.  Why do we do this??

We do this because then the state is exactly the same as it was after 
usb2_hcd was registered but before usb3_hcd was registered.

So what happened here is very much like what would happen if something 
went wrong during probing, after the primary hcd was registered and 
before the secondary hcd was registered.

Alan Stern

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

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Greg KH]

On Tue, Aug 23, 2016 at 01:54:05PM +0300, Mathias Nyman wrote:
> On 23.08.2016 02:21, Jose Marino wrote:
> > I'm using my phone (Nexus 5X running Android) to tether a USB connection to 
> > my laptop (XPS 15 9550). I plug the phone through the USB-C connection and 
> > in the phone I  select USB tethering. Initially things look normal: a usb0 
> > network interface appears in the laptop and it tries to get an IP with 
> > dhcp. However, I observe two different behaviors depending on whether it's 
> > a fresh boot, or I have suspend/resumed the laptop. In a fresh boot 
> > everything works fine, I get an IP and the connection works as expected. If 
> > I unplug the phone, everything also works as expected.
> > 
> > However, after a suspend/resume cycle, I plug the phone in but the laptop 
> > never connects to it. The usb0 interface still appears, but the dhcp daemon 
> > is unable to get any response and finally times out. The fun part happens 
> > when I unplug the phone. I consistently get a kernel panic.
> > 
> > I managed to get some logs of the oops+panic from pstore. Find them 
> > attached. In this particular situation this is what I did:
> > - Boot laptop (archlinux with kernel 4.7.2)
> > - Suspend/resume
> > - Plug Nexus 5X
> > - After a few seconds unplug Nexus 5X
> > 
> > I filed a bug report about this: 
> > https://bugzilla.kernel.org/show_bug.cgi?id=153551
> 
> 
> The Dell XPS 9550 has an additional xhci controller for handling the type-C 
> port.
> This controller is hotplug removed from the PCI bus when the last USB type-c
> device is disconnected.
> 
> xhci driver, and usb core it seems is not really designed with this in mind.

The USB core can handle this just fine.

> xhci driver will suddenly start reading  from PCI.

Which means the device is gone, and you need to handle it properly.  We
fixed up ehci and ohci for this years ago (they were on hotplug busses).

For every PCI read, you need to verify that the data is correct, that's
the way that any PCI driver needs to work in a "modern" system.

This is an XHCI issue, not a USB core issue :)

> I've been looking at issues related to this. Currently there is at least one 
> similar case with mass storage
> where we see the device release function being called for the mass storage 
> interface device _after_ we
> freed all memory related to both xhci hcd's. bug for that is here:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=120241
> 
> usb devices with their children should be synchronously removed before hcd's 
> are freed, but seems
> that is not the case, at least not for the device release function for the 
> interface device.

Once you notice that your PCI device is gone, you need to start tearing
things down as soon as possible.  Or just stop things and wait for the
PCI core to come around and remove you from the system.  That's probably
much more simple and I think is what was done for EHCI.

> A horrible workaround to hide this issue was to sleep for a second or two 
> before freeing the hcd memory,
> this lets some pending work finish before hcds disappear. (more info in that 
> bug report)

Yeah, that's not a good idea :)

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

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Mathias Nyman]

On 23.08.2016 14:26, Mathias Nyman wrote:

On 23.08.2016 13:54, Mathias Nyman wrote:

On 23.08.2016 02:21, Jose Marino wrote:

I'm using my phone (Nexus 5X running Android) to tether a USB connection to my 
laptop (XPS 15 9550). I plug the phone through the USB-C connection and in the 
phone I  select USB tethering. Initially things look normal: a usb0 network 
interface appears in the laptop and it tries to get an IP with dhcp. However, I 
observe two different behaviors depending on whether it's a fresh boot, or I 
have suspend/resumed the laptop. In a fresh boot everything works fine, I get 
an IP and the connection works as expected. If I unplug the phone, everything 
also works as expected.

However, after a suspend/resume cycle, I plug the phone in but the laptop never 
connects to it. The usb0 interface still appears, but the dhcp daemon is unable 
to get any response and finally times out. The fun part happens when I unplug 
the phone. I consistently get a kernel panic.


...

Anyways, I'll look at that panic in more detail as well



<6>[  178.693631] xhci_hcd :3e:00.0: USB bus 4 deregistered
<6>[  178.693642] xhci_hcd :3e:00.0: remove, state 1
<6>[  178.693648] usb usb3: USB disconnect, device number 1
<4>[  183.634994] xhci_hcd :3e:00.0: xHCI host not responding to stop 
endpoint command.
<4>[  183.635001] xhci_hcd :3e:00.0: Assuming host is dying, halting host.
<4>[  183.635019] xhci_hcd :3e:00.0: Host not halted after 16000 
microseconds.
<4>[  183.635022] xhci_hcd :3e:00.0: Non-responsive xHCI host is not 
halting.
<4>[  183.635025] xhci_hcd :3e:00.0: Completing active URBs anyway.
<1>[  183.635116] BUG: unable to handle kernel NULL pointer dereference at  
 (null)
<1>[  183.635402] IP: [] usb_hc_died+0x16/0xc0 [usbcore]


Looks like the 5 second command timeout timer for stop endpoint commands causes 
this.
the timer (stop_cmd_timer) will call
xhci_stop_endpoint_command_watchdog()  which calls
   usb_hc_died(xhci_to_hcd(xhci)->primary_hcd)

but hcd are probably freed and pointers set to null already  -> NULL pointer 
dereference.

The timer should be synchronously deleted when the device is freed, unless 
xhci_free_dev()
returns early.

So either hub_free_dev() is not called for this device at hcd removal, or 
xhci_free_dev returns early.



Or then this happens:
(I'll call the hcds usb2_hcd and usb3_hcd to keep track of them, usb2_hcd is 
the primary_hcd)

to begin with:
usb2_hcd->primary_hcd = usb2_hcd
usb2_hcd->shared_hcd  = usb3_hcd

usb3_hcd->primary_hcd = usb2_hcd
usb3_hcd->shared_hcd  = usb2_hcd


usb3_host is removed first:
xhci_pci_remove(struct pci_dev *dev)
  usb_remove_hcd(xhci->shared_hcd);  // remove usb3_hcd
  usb_put_hcd(xhci->shared_hcd)
hcd_release(..)
  if (hcd->shared_hcd) { //true
struct usb_hcd *peer = hcd->shared_hcd;   //peer is now 
usb2_hcd
peer->shared_hcd = NULL; //sets usb2_hcd->shared_hcd to NULL
peer->primary_hcd = NULL;// sets usb2_hcd->primary_hcd to 
NULL.  Why do we do this??
stop_cmd_timer triggers before the usb2_hcd is removed:
-> xhci_stop_endpoint_command_watchdog()
 usb_hc_died(xhci_to_hcd(xhci)->primary_hcd)  // xhci_to_hcd will get 
usb2_hcd, usb2_hcd->primary_hcd is set to NULL here.
 


does something like this help?

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fd9fd12..797137e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -850,6 +850,10 @@ void xhci_stop_endpoint_command_watchdog(unsigned long arg)
spin_lock_irqsave(>lock, flags);
 
ep->stop_cmds_pending--;

+   if (xhci->xhc_state & XHCI_STATE_REMOVING) {
+   spin_unlock_irqrestore(>lock, flags);
+   return;
+   }
if (xhci->xhc_state & XHCI_STATE_DYING) {
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"Stop EP timer ran, but another timer marked "
@@ -903,7 +907,7 @@ void xhci_stop_endpoint_command_watchdog(unsigned long arg)
spin_unlock_irqrestore(>lock, flags);
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"Calling usb_hc_died()");
-   usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
+   usb_hc_died(xhci_to_hcd(xhci));
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"xHCI host controller is dead.");
 }

--
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: gadget: udc: renesas-usb3: clear VBOUT bit in DRD_CON

[Yoshihiro Shimoda]

This driver should clear the bit. Otherwise, the VBUS will output
wrongly if the usb port on a board has VBUS output capability.

Fixes: 746bfe63bba3 ("usb: gadget: renesas_usb3: add support for
  Renesas USB3.0 peripheral controller")
Cc:  # v4.5+
Signed-off-by: Yoshihiro Shimoda 
---
 drivers/usb/gadget/udc/renesas_usb3.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/udc/renesas_usb3.c 
b/drivers/usb/gadget/udc/renesas_usb3.c
index 93a3bec..fb8fc34 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -106,6 +106,7 @@
 
 /* DRD_CON */
 #define DRD_CON_PERI_CON   BIT(24)
+#define DRD_CON_VBOUT  BIT(0)
 
 /* USB_INT_ENA_1 and USB_INT_STA_1 */
 #define USB_INT_1_B3_PLLWKUP   BIT(31)
@@ -363,6 +364,7 @@ static void usb3_init_epc_registers(struct renesas_usb3 
*usb3)
 {
/* FIXME: How to change host / peripheral mode as well? */
usb3_set_bit(usb3, DRD_CON_PERI_CON, USB3_DRD_CON);
+   usb3_clear_bit(usb3, DRD_CON_VBOUT, USB3_DRD_CON);
 
usb3_write(usb3, ~0, USB3_USB_INT_STA_1);
usb3_enable_irq_1(usb3, USB_INT_1_VBUS_CNG);
-- 
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 v2] usb: dwc3: gadget: don't rely on jiffies while holding spinlock

[Nicolas Saenz Julienne]

__dwc3_gadget_wakeup() is called while holding a spinlock, then depends on
jiffies in order to timeout while polling the USB core for a link state
update. In the case the wakeup failed, the timeout will never happen and
will also cause the cpu to stall until rcu_preempt kicks in.

This switches to a "decrement variable and wait" timeout scheme.

Signed-off-by: Nicolas Saenz Julienne 
---
v2. removed udelay() from while loop
note: resubmitting since earlier patch had errata on e-mail address

 drivers/usb/dwc3/gadget.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1f5597e..122e64d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1433,7 +1433,7 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
 
 static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
 {
-   unsigned long   timeout;
+   int retries;
 
int ret;
u32 reg;
@@ -1484,9 +1484,9 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
}
 
/* poll until Link State changes to ON */
-   timeout = jiffies + msecs_to_jiffies(100);
+   retries = 2;
 
-   while (!time_after(jiffies, timeout)) {
+   while (retries--) {
reg = dwc3_readl(dwc->regs, DWC3_DSTS);
 
/* in HS, means ON */
-- 
2.7.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 v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Balbi,

On 23/08/16 12:03, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>>> John Youn  writes:
 On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
> Use gadget's framework allocation function instead of directly calling
> usb_ep_alloc_request().
>
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/gadget/function/f_hid.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_hid.c 
> b/drivers/usb/gadget/function/f_hid.c
> index a010496e4e05..89d2e9a5a04f 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, 
> struct usb_function *f)
>  
>   /* preallocate request and buffer */
>   status = -ENOMEM;
> - hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
> + hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>   if (!hidg->req)
>   goto fail;
>  
> - hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
> - if (!hidg->req->buf)
> - goto fail;
> -
>   /* set descriptor dynamic values */
>   hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>   hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
>

 Hi Felipe,

 This commit on your testing/next breaks compilation.

 ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
 ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to 
 function ‘alloc_ep_req’
   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
   ^
 In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
 ../drivers/usb/gadget/u_f.h:63:21: note: declared here
  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int 
 default_len);
>>>
>>> true that :-) Dropping from my queue.
>>>
>>
>> Are you applying the previous patches? Specially that this is the last
>> patch in the series, how can it break with you if it doesn't break here?
>> What should I do then?
> 
> Can you rebase your series on top of my testing/next? My HEAD is
> at commit 95bbb3474f1e87c9ec7ebe2acf25006e5e94a824.

It was at that time. I will rebease it then on v5. Just waiting for your
comments on the other thread.

Thanks

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Balbi,

On 23/08/16 12:01, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>>> "Felipe F. Tonello"  writes:
 The default_length parameter of alloc_ep_req was not really necessary
 and gadget drivers would almost always create an inline function to pass
 the same value to len and default_len.

 So this patch also removes duplicate code from few drivers.

 Signed-off-by: Felipe F. Tonello 
 ---
  drivers/usb/gadget/function/f_hid.c| 10 ++
  drivers/usb/gadget/function/f_loopback.c   |  9 +
  drivers/usb/gadget/function/f_midi.c   | 10 ++
  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
  drivers/usb/gadget/u_f.c   |  7 +++
  drivers/usb/gadget/u_f.h   |  3 +--
  6 files changed, 11 insertions(+), 39 deletions(-)

 diff --git a/drivers/usb/gadget/function/f_hid.c 
 b/drivers/usb/gadget/function/f_hid.c
 index 51980c50546d..e82a7468252e 100644
 --- a/drivers/usb/gadget/function/f_hid.c
 +++ b/drivers/usb/gadget/function/f_hid.c
 @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct 
 file *fd)
  
 /*-*/
  /*usb_function
  */
  
 -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
 -  unsigned length)
 -{
 -  return alloc_ep_req(ep, length, length);
 -}
>>>
>>> actually, I prefer to keep these little helpers. I was recently playing
>>> with adding SG list support to g_zero (I should have patches soon) and
>>> it was actually very nice to have the sourcesink helper as I could just
>>> ditch alloc_ep_req(). The change to the driver was local to
>>> ss_alloc_ep_req() and nothing else changed :-)
>>>
>>
>> Right, but then it is worth to have the helper function. In this
>> particular case, I am removing a useless helper functions, especially
>> that the previous patch removes the need for the optional parameter in
>> alloc_ep_req.
> 
> it's a static inline :-) It won't do any bad to keep it. And, as I said,
> if we want to ditch aloc_ep_req() eventually, then we have just one
> place to patch ;-)

Yes, sure. But why drop alloc_ep_req()?

So should I keep all these helper functions? If so, I actually still
need to fix them to use the newer alloc_ep_req() API.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [patch] USB: serial: option: add WeTelecom WM-D200

[Aleksandr Makarov]

22.08.2016 22:20, Dan Williams пишет:
> On Mon, 2016-08-22 at 20:07 +0300, Aleksandr Makarov wrote:
>> 22.08.2016 18:03, Dan Williams пишет:
>>>
>>> On Sat, 2016-08-20 at 14:50 +0300, Aleksandr Makarov wrote:

 USB: serial: option: add WeTelecom WM-D200

 Add support for WeTelecom WM-D200.

 T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#=  4 Spd=12  MxCh= 0
 D:  Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
 P:  Vendor=22de ProdID=6801 Rev=00.00
 S:  Manufacturer=WeTelecom Incorporated
 S:  Product=WeTelecom Mobile Products
 C:  #Ifs= 4 Cfg#= 1 Atr=80 MxPwr=500mA
 I:  If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff
 Driver=(none)
 I:  If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff
 Driver=(none)
 I:  If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff
 Driver=(none)
 I:  If#= 3 Alt= 0 #EPs= 2 Cls=08(stor.) Sub=06 Prot=50
 Driver=usb-
 storage
>>> While you're at it, why not add PID 0x6802 and 0x6803?  These are
>>> also
>>> listed here:
>>>
>>> http://www.mts.ua/upload/files/we/te/wetelecom_windows_drivers.zip
>>>
>>> with the same USB layout as the WM-D200.
>>>
>>> Dan
>>>
>> Does it mean that if 0x6803 (WeTelecom WM-D300) has the same
>> USB layout, it does not need testing?
> 
> Yes, since the Windows drivers use the exact same layout for all three
> devices, we should do the same for Linux.
> 
>> If it does not, why don't I copy the whole usb_modeswitch data
>> into the option driver?
> 
> What specifically would you copy over?  All I see there are two data
> files, one for 6801 and one for 6803, but they don't have any
> interesting data in them...
> 
> Dan
> 
I am talking about using usb_modeswitch as a database for 
"composite" devices. Like if I knew WM-D200 is a 3g modem and
its layout is deduced from a Windows driver, then I copy-paste
its pid:vid to the Linux driver.
This would add support for all the devices that were reported
to usb_modeswitch.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Mathias Nyman]

On 23.08.2016 13:54, Mathias Nyman wrote:

On 23.08.2016 02:21, Jose Marino wrote:

I'm using my phone (Nexus 5X running Android) to tether a USB connection to my 
laptop (XPS 15 9550). I plug the phone through the USB-C connection and in the 
phone I  select USB tethering. Initially things look normal: a usb0 network 
interface appears in the laptop and it tries to get an IP with dhcp. However, I 
observe two different behaviors depending on whether it's a fresh boot, or I 
have suspend/resumed the laptop. In a fresh boot everything works fine, I get 
an IP and the connection works as expected. If I unplug the phone, everything 
also works as expected.

However, after a suspend/resume cycle, I plug the phone in but the laptop never 
connects to it. The usb0 interface still appears, but the dhcp daemon is unable 
to get any response and finally times out. The fun part happens when I unplug 
the phone. I consistently get a kernel panic.


...

Anyways, I'll look at that panic in more detail as well



<6>[  178.693631] xhci_hcd :3e:00.0: USB bus 4 deregistered
<6>[  178.693642] xhci_hcd :3e:00.0: remove, state 1
<6>[  178.693648] usb usb3: USB disconnect, device number 1
<4>[  183.634994] xhci_hcd :3e:00.0: xHCI host not responding to stop 
endpoint command.
<4>[  183.635001] xhci_hcd :3e:00.0: Assuming host is dying, halting host.
<4>[  183.635019] xhci_hcd :3e:00.0: Host not halted after 16000 
microseconds.
<4>[  183.635022] xhci_hcd :3e:00.0: Non-responsive xHCI host is not 
halting.
<4>[  183.635025] xhci_hcd :3e:00.0: Completing active URBs anyway.
<1>[  183.635116] BUG: unable to handle kernel NULL pointer dereference at  
 (null)
<1>[  183.635402] IP: [] usb_hc_died+0x16/0xc0 [usbcore]


Looks like the 5 second command timeout timer for stop endpoint commands causes 
this.
the timer (stop_cmd_timer) will call
xhci_stop_endpoint_command_watchdog()  which calls
  usb_hc_died(xhci_to_hcd(xhci)->primary_hcd)

but hcd are probably freed and pointers set to null already  -> NULL pointer 
dereference.

The timer should be synchronously deleted when the device is freed, unless 
xhci_free_dev()
returns early.

So either hub_free_dev() is not called for this device at hcd removal, or 
xhci_free_dev returns early.

hub_free_dev()
  hcd->driver->free_dev(hcd, udev);
xhci_free_dev()
  (possible early return here)
  for (i = 0; i < 31; ++i) {
virt_dev->eps[i].ep_state &= ~EP_HALT_PENDING;
del_timer_sync(_dev->eps[i].stop_cmd_timer);

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

Re: xHCI problem? [was Re: Erratic USB device behavior and device loss]

[Ritesh Raj Sarraf]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

On Tue, 2016-08-23 at 14:21 +0530, Ritesh Raj Sarraf wrote:
> In one of yesterday's email, I have shared the output of
> /sys/kernel/debug/usb/devices before the bug is triggered. The same I've
> pasted
> below.
> 
> T:  Bus=01 Lev=01 Prnt=01 Port=03 Cnt=01 Dev#=  2 Spd=480  MxCh= 0
> D:  Ver= 2.00 Cls=ff(vend.) Sub=ff Prot=ff MxPS=64 #Cfgs=  1
> P:  Vendor=0bda ProdID=0129 Rev=39.60
> S:  Manufacturer=Generic
> S:  Product=USB2.0-CRW
> S:  SerialNumber=2010020139600
> C:* #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=500mA
> I:* If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=06 Prot=50 Driver=rtsx_usb
> E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=83(I) Atr=03(Int.) MxPS=   3 Ivl=64ms
> 
> 
> I'll pick the same again when I reboot.

This one is after a fresh boot.

T:  Bus=02 Lev=01 Prnt=01 Port=03 Cnt=01 Dev#=  9 Spd=480  MxCh= 0
D:  Ver= 2.00 Cls=ff(vend.) Sub=ff Prot=ff MxPS=64 #Cfgs=  1
P:  Vendor=0bda ProdID=0129 Rev=39.60
S:  Manufacturer=Generic
S:  Product=USB2.0-CRW
S:  SerialNumber=2010020139600
C:* #Ifs= 1 Cfg#= 1 Atr=a0 MxPwr=500mA
I:* If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=06 Prot=50 Driver=rtsx_usb
E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E:  Ad=83(I) Atr=03(Int.) MxPS=   3 Ivl=64ms


Here it got enumerated as Bus 02.

- -- 
Ritesh Raj Sarraf
RESEARCHUT - http://www.researchut.com
"Necessity is the mother of invention."
-BEGIN PGP SIGNATURE-

iQIcBAEBCgAGBQJXvC8YAAoJEKY6WKPy4XVpWqsP/2RSUu/gB7ntEtK8K4J0fJJ9
dngnktH8ijM3qSuzOn8Brl28fBp6jOlyEjNcD+hjq1UrZKz02EG3yDp7nWDjHqqG
PTcK/i2lPu73TsRiBpCSF4kiupnCYyUClnyPbVRX/+4sPqzNWBzIk/wzF0ebn9/i
m5NP9OkMWuEK2c/L+Doe99FrFfImoJweqBqzZAgoK0L+UCQ4u2NYdGOzUudUBZGO
/xRwNwWebQ9fPKYLn3armXKVo5NH1bf96+ti6+AQDfPr0O2CcWg8JyiHOqZzUHEg
VNNyNdw1T0E7QxBnNtaoVP2+NvZ8n4eiXpDm/Mu+rwMEg+KtN9W3qKMsgTyjBqJN
92eDp/HiS2rIZOnrmUOhziAIrdV00cAmt6ihjEwirU0hiULllHoBZ468wa3rf7Ju
2xUCtW/KpW1e/Emw5CngOfXUQyDb2jgxAf2u/m7axhN3TOt1RFKumpJKf/EyhB5E
qFZlTfozShQP9Txu+0/lnc2zGS2Rh8E/7i1tj3QuYAZV8xaQJQ96MO6X+YPdnUkr
nbOpFRAjf18E2rTasShA3d7WrMtpZzTY2/OpEUzAM/R5wieLuZ4DY4kgNs5ofWYj
bK0vmlhqKPDjeCv+9YBttOZBkoRkxfk5QehMls/wBTWxCawgroYhilp1koc8oEs/
p8usFkJFKSCBfCo3jCOC
=4CGy
-END PGP SIGNATURE-

--
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 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Balbi]

Hi,

Felipe Ferreri Tonello  writes:
>> "Felipe F. Tonello"  writes:
>>> The default_length parameter of alloc_ep_req was not really necessary
>>> and gadget drivers would almost always create an inline function to pass
>>> the same value to len and default_len.
>>>
>>> So this patch also removes duplicate code from few drivers.
>>>
>>> Signed-off-by: Felipe F. Tonello 
>>> ---
>>>  drivers/usb/gadget/function/f_hid.c| 10 ++
>>>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>>>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>>>  drivers/usb/gadget/u_f.c   |  7 +++
>>>  drivers/usb/gadget/u_f.h   |  3 +--
>>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>>> b/drivers/usb/gadget/function/f_hid.c
>>> index 51980c50546d..e82a7468252e 100644
>>> --- a/drivers/usb/gadget/function/f_hid.c
>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct 
>>> file *fd)
>>>  
>>> /*-*/
>>>  /*usb_function 
>>> */
>>>  
>>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>>> -   unsigned length)
>>> -{
>>> -   return alloc_ep_req(ep, length, length);
>>> -}
>> 
>> actually, I prefer to keep these little helpers. I was recently playing
>> with adding SG list support to g_zero (I should have patches soon) and
>> it was actually very nice to have the sourcesink helper as I could just
>> ditch alloc_ep_req(). The change to the driver was local to
>> ss_alloc_ep_req() and nothing else changed :-)
>> 
>
> Right, but then it is worth to have the helper function. In this
> particular case, I am removing a useless helper functions, especially
> that the previous patch removes the need for the optional parameter in
> alloc_ep_req.

it's a static inline :-) It won't do any bad to keep it. And, as I said,
if we want to ditch aloc_ep_req() eventually, then we have just one
place to patch ;-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Balbi]

Hi,

Felipe Ferreri Tonello  writes:
>> John Youn  writes:
>>> On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
 Use gadget's framework allocation function instead of directly calling
 usb_ep_alloc_request().

 Signed-off-by: Felipe F. Tonello 
 ---
  drivers/usb/gadget/function/f_hid.c | 6 +-
  1 file changed, 1 insertion(+), 5 deletions(-)

 diff --git a/drivers/usb/gadget/function/f_hid.c 
 b/drivers/usb/gadget/function/f_hid.c
 index a010496e4e05..89d2e9a5a04f 100644
 --- a/drivers/usb/gadget/function/f_hid.c
 +++ b/drivers/usb/gadget/function/f_hid.c
 @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, 
 struct usb_function *f)
  
/* preallocate request and buffer */
status = -ENOMEM;
 -  hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
 +  hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
if (!hidg->req)
goto fail;
  
 -  hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
 -  if (!hidg->req->buf)
 -  goto fail;
 -
/* set descriptor dynamic values */
hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;

>>>
>>> Hi Felipe,
>>>
>>> This commit on your testing/next breaks compilation.
>>>
>>> ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
>>> ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to 
>>> function ‘alloc_ep_req’
>>>   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>>   ^
>>> In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
>>> ../drivers/usb/gadget/u_f.h:63:21: note: declared here
>>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int 
>>> default_len);
>> 
>> true that :-) Dropping from my queue.
>> 
>
> Are you applying the previous patches? Specially that this is the last
> patch in the series, how can it break with you if it doesn't break here?
> What should I do then?

Can you rebase your series on top of my testing/next? My HEAD is
at commit 95bbb3474f1e87c9ec7ebe2acf25006e5e94a824.

-- 
balbi


signature.asc
Description: PGP signature

Re: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Mathias Nyman]

On 23.08.2016 02:21, Jose Marino wrote:

I'm using my phone (Nexus 5X running Android) to tether a USB connection to my 
laptop (XPS 15 9550). I plug the phone through the USB-C connection and in the 
phone I  select USB tethering. Initially things look normal: a usb0 network 
interface appears in the laptop and it tries to get an IP with dhcp. However, I 
observe two different behaviors depending on whether it's a fresh boot, or I 
have suspend/resumed the laptop. In a fresh boot everything works fine, I get 
an IP and the connection works as expected. If I unplug the phone, everything 
also works as expected.

However, after a suspend/resume cycle, I plug the phone in but the laptop never 
connects to it. The usb0 interface still appears, but the dhcp daemon is unable 
to get any response and finally times out. The fun part happens when I unplug 
the phone. I consistently get a kernel panic.

I managed to get some logs of the oops+panic from pstore. Find them attached. 
In this particular situation this is what I did:
- Boot laptop (archlinux with kernel 4.7.2)
- Suspend/resume
- Plug Nexus 5X
- After a few seconds unplug Nexus 5X

I filed a bug report about this: 
https://bugzilla.kernel.org/show_bug.cgi?id=153551



The Dell XPS 9550 has an additional xhci controller for handling the type-C 
port.
This controller is hotplug removed from the PCI bus when the last USB type-c
device is disconnected.

xhci driver, and usb core it seems is not really designed with this in mind.
xhci driver will suddenly start reading  from PCI.

I've been looking at issues related to this. Currently there is at least one 
similar case with mass storage
where we see the device release function being called for the mass storage 
interface device _after_ we
freed all memory related to both xhci hcd's. bug for that is here:

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

usb devices with their children should be synchronously removed before hcd's 
are freed, but seems
that is not the case, at least not for the device release function for the 
interface device.

A horrible workaround to hide this issue was to sleep for a second or two 
before freeing the hcd memory,
this lets some pending work finish before hcds disappear. (more info in that 
bug report)

Also this commit in 4.8-rc3 solves a related xhci PCI hotplug issue:
f1f6d9a xhci: don't dereference a xhci member after removing xhci

Anyways, I'll look at that panic in more detail as well

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

Re: [PATCH v2 0/6] power: add power sequence library

[Vaibhav Hiremath]

On Tuesday 23 August 2016 03:52 PM, Vaibhav Hiremath wrote:



On Wednesday 13 July 2016 07:36 AM, Peter Chen wrote:

Hi all,

This is a follow-up for my last power sequence framework patch set [1].
According to Rob Herring and Ulf Hansson's comments[2], I use a generic
power sequence library for parsing the power sequence elements on DT,
and implement generic power sequence on library. The host driver
can allocate power sequence instance, and calls pwrseq APIs accordingly.

In future, if there are special power sequence requirements, the special
power sequence library can be created.

This patch set is tested on i.mx6 sabresx evk using a dts change, I use
two hot-plug devices to simulate this use case, the related binding
change is updated at patch[1/6], The udoo board changes were tested
using my last power sequence patch set.[3]

Except for hard-wired MMC and USB devices, I find the USB ULPI PHY also
need to power on itself before it can be found by ULPI bus.


Sorry being so late in the discussion...



Mistakenly responded to v2. Please ignore this, I will paste
same content on V6, which is the latest revision.

Thanks,
Vaibhav

If I am not missing anything, then I am afraid to say that the generic 
library
implementation in this patch series is not going to solve many of the 
custom

requirement of power on, off, etc...
I know you mentioned about adding another library when we come
across such platforms, but should we not keep provision (or easy 
hooks/path)

to enable that ?

Let me bring in the use case I am dealing with,


  Host
 ||
 V
   USB port

 ||
 V
  USB HUB device (May need custom on/off seq)
 ||
 V
  ==
 ||  ||
 V  V
Device-1Device-2
(Needs special power   (Needs special power
 on/off sequence.   on/off sequence.
Also may need custom  Also, may need custom
sequence for sequence for
suspend/resume)  suspend/resume).


Note: Both Devices are connected to HUB via HSIC and may differ
  in terms of functionality, features they support.

In the above case, both Device-1 and Device-2, need separate
power on/off sequence. So generic library currently we have in this
patch series is not going to satisfy the need here.

I looked at all 6 revisions of this patch-series, went through the
review comments, and looked at MMC power sequence code;
what I can say here is, we need something similar to
MMC power sequence here, where every device can have its own
power sequence (if needed).

I know Rob is not in favor of creating platform device for
this, and I understand his comment.
If not platform device, but atleast we need mechanism to
connect each device back to its of_node and its respective
driver/library fns. For example, the Devices may support different
boot modes, and platform driver needs to make sure that
the right sequence is followed for booting.

Peter, My apologies for taking you back again on this series.
I am OK, if you wish to address this in incremental addition,
but my point is, we know that the current generic way is not
enough for us, so I think we should try to fix it in initial phase only.


Thanks,
Vaibhav


[1] http://www.spinics.net/lists/linux-usb/msg142755.html
[2] http://www.spinics.net/lists/linux-usb/msg143106.html
[3] http://www.spinics.net/lists/linux-usb/msg142815.html

Changes for v2:
- Delete "pwrseq" prefix and clock-names for properties at dt binding
- Should use structure not but its pointer for kzalloc
- Since chipidea core has no of_node, let core's of_node equals glue
   layer's at core's probe


Peter Chen (6):
   binding-doc: power: pwrseq-generic: add binding doc for generic power
 sequence library
   power: add power sequence library
   binding-doc: usb: usb-device: add optional properties for power
 sequence
   usb: core: add power sequence handling for USB devices
   usb: chipidea: let chipidea core device of_node equal's glue layer
 device of_node
   ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

  .../bindings/power/pwrseq/pwrseq-generic.txt   |  53 
  .../devicetree/bindings/usb/usb-device.txt |   9 ++
  arch/arm/boot/dts/imx6qdl-udoo.dtsi|  27 +++--
  drivers/power/Kconfig  |   1 +
  drivers/power/Makefile |   1 +
  drivers/power/pwrseq/Kconfig   |  20 +++
  drivers/power/pwrseq/Makefile  |   2 +
  

Re: [PATCH v6 0/8] power: add power sequence library

[Vaibhav Hiremath]

On Monday 15 August 2016 02:43 PM, Peter Chen wrote:

Hi all,

This is a follow-up for my last power sequence framework patch set [1].
According to Rob Herring and Ulf Hansson's comments[2], I use a generic
power sequence library for parsing the power sequence elements on DT,
and implement generic power sequence on library. The host driver
can allocate power sequence instance, and calls pwrseq APIs accordingly.

In future, if there are special power sequence requirements, the special
power sequence library can be created.

This patch set is tested on i.mx6 sabresx evk using a dts change, I use
two hot-plug devices to simulate this use case, the related binding
change is updated at patch [1/6], The udoo board changes were tested
using my last power sequence patch set.[3]

Except for hard-wired MMC and USB devices, I find the USB ULPI PHY also
need to power on itself before it can be found by ULPI bus.

[1] http://www.spinics.net/lists/linux-usb/msg142755.html
[2] http://www.spinics.net/lists/linux-usb/msg143106.html
[3] http://www.spinics.net/lists/linux-usb/msg142815.html

(Please ignore my response on V2)

Sorry being so late in the discussion...

If I am not missing anything, then I am afraid to say that the generic 
library
implementation in this patch series is not going to solve many of the 
custom

requirement of power on, off, etc...
I know you mentioned about adding another library when we come
across such platforms, but should we not keep provision (or easy 
hooks/path)

to enable that ?

Let me bring in the use case I am dealing with,


  Host
   |
   V
   USB port

   |
   V
  USB HUB device (May need custom on/off seq)
   |
   V
  =
 | |
 V V
 Device-1   Device-2
(Needs special power   (Needs special power
 on/off sequence.   on/off sequence.
 Also may need custom   Also, may need custom
 sequence for   sequence for
 suspend/resume)suspend/resume)


Note: Both Devices are connected to HUB via HSIC and may differ
  in terms of functionality, features they support.

In the above case, both Device-1 and Device-2, need separate
power on/off sequence. So generic library currently we have in this
patch series is not going to satisfy the need here.

I looked at all 6 revisions of this patch-series, went through the
review comments, and looked at MMC power sequence code;
what I can say here is, we need something similar to
MMC power sequence here, where every device can have its own
power sequence (if needed).

I know Rob is not in favor of creating platform device for
this, and I understand his comment.
If not platform device, but atleast we need mechanism to
connect each device back to its of_node and its respective
driver/library fns. For example, the Devices may support different
boot modes, and platform driver needs to make sure that
the right sequence is followed for booting.

Peter, My apologies for taking you back again on this series.
I am OK, if you wish to address this in incremental addition,
but my point is, we know that the current generic way is not
enough for us, so I think we should try to fix it in initial phase only.


Thanks,
Vaibhav




Changes for v6:
- Add Matthias Kaehlcke's Reviewed-by and Tested-by. (patch [2/6])
- Change chipidea core of_node assignment for coming user. (patch [5/6])
- Applies Joshua Clayton's three dts changes for two boards,
   the USB device's reg has only #address-cells, but without #size-cells.

Changes for v5:
- Delete pwrseq_register/pwrseq_unregister, which is useless currently
- Fix the linker error when the pwrseq user is compiled as module

Changes for v4:
- Create the patch on next-20160722
- Fix the of_node is not NULL after chipidea driver is unbinded [Patch 5/6]
- Using more friendly wait method for reset gpio [Patch 2/6]
- Support multiple input clocks [Patch 2/6]
- Add Rob Herring's ack for DT changes
- Add Joshua Clayton's Tested-by

Changes for v3:
- Delete "power-sequence" property at binding-doc, and change related code
   at both library and user code.
- Change binding-doc example node name with Rob's comments
- of_get_named_gpio_flags only gets the gpio, but without setting gpio flags,
   add additional code request gpio with proper gpio flags
- Add Philipp Zabel's Ack and MAINTAINER's entry

Changes for v2:
- Delete "pwrseq" prefix and clock-names for properties at dt binding
- Should use structure not but its pointer for kzalloc
- Since chipidea core has no of_node, let core's of_node equals glue
   

Re: [PATCH v2 0/6] power: add power sequence library

[Vaibhav Hiremath]

On Wednesday 13 July 2016 07:36 AM, Peter Chen wrote:

Hi all,

This is a follow-up for my last power sequence framework patch set [1].
According to Rob Herring and Ulf Hansson's comments[2], I use a generic
power sequence library for parsing the power sequence elements on DT,
and implement generic power sequence on library. The host driver
can allocate power sequence instance, and calls pwrseq APIs accordingly.

In future, if there are special power sequence requirements, the special
power sequence library can be created.

This patch set is tested on i.mx6 sabresx evk using a dts change, I use
two hot-plug devices to simulate this use case, the related binding
change is updated at patch[1/6], The udoo board changes were tested
using my last power sequence patch set.[3]

Except for hard-wired MMC and USB devices, I find the USB ULPI PHY also
need to power on itself before it can be found by ULPI bus.


Sorry being so late in the discussion...

If I am not missing anything, then I am afraid to say that the generic 
library

implementation in this patch series is not going to solve many of the custom
requirement of power on, off, etc...
I know you mentioned about adding another library when we come
across such platforms, but should we not keep provision (or easy hooks/path)
to enable that ?

Let me bring in the use case I am dealing with,


  Host
 ||
 V
   USB port

 ||
 V
  USB HUB device (May need custom on/off seq)
 ||
 V
  ==
 ||  ||
 V  V
Device-1Device-2
(Needs special power   (Needs special power
 on/off sequence.   on/off sequence.
Also may need custom  Also, may need custom
sequence for sequence for
suspend/resume)  suspend/resume).


Note: Both Devices are connected to HUB via HSIC and may differ
  in terms of functionality, features they support.

In the above case, both Device-1 and Device-2, need separate
power on/off sequence. So generic library currently we have in this
patch series is not going to satisfy the need here.

I looked at all 6 revisions of this patch-series, went through the
review comments, and looked at MMC power sequence code;
what I can say here is, we need something similar to
MMC power sequence here, where every device can have its own
power sequence (if needed).

I know Rob is not in favor of creating platform device for
this, and I understand his comment.
If not platform device, but atleast we need mechanism to
connect each device back to its of_node and its respective
driver/library fns. For example, the Devices may support different
boot modes, and platform driver needs to make sure that
the right sequence is followed for booting.

Peter, My apologies for taking you back again on this series.
I am OK, if you wish to address this in incremental addition,
but my point is, we know that the current generic way is not
enough for us, so I think we should try to fix it in initial phase only.


Thanks,
Vaibhav


[1] http://www.spinics.net/lists/linux-usb/msg142755.html
[2] http://www.spinics.net/lists/linux-usb/msg143106.html
[3] http://www.spinics.net/lists/linux-usb/msg142815.html

Changes for v2:
- Delete "pwrseq" prefix and clock-names for properties at dt binding
- Should use structure not but its pointer for kzalloc
- Since chipidea core has no of_node, let core's of_node equals glue
   layer's at core's probe


Peter Chen (6):
   binding-doc: power: pwrseq-generic: add binding doc for generic power
 sequence library
   power: add power sequence library
   binding-doc: usb: usb-device: add optional properties for power
 sequence
   usb: core: add power sequence handling for USB devices
   usb: chipidea: let chipidea core device of_node equal's glue layer
 device of_node
   ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

  .../bindings/power/pwrseq/pwrseq-generic.txt   |  53 
  .../devicetree/bindings/usb/usb-device.txt |   9 ++
  arch/arm/boot/dts/imx6qdl-udoo.dtsi|  27 +++--
  drivers/power/Kconfig  |   1 +
  drivers/power/Makefile |   1 +
  drivers/power/pwrseq/Kconfig   |  20 +++
  drivers/power/pwrseq/Makefile  |   2 +
  drivers/power/pwrseq/core.c|  79 
  drivers/power/pwrseq/pwrseq_generic.c  | 134 +
  drivers/usb/chipidea/core.c   

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Ferreri Tonello]

Hi,

On 22/08/16 08:45, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>> On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
>>> Use gadget's framework allocation function instead of directly calling
>>> usb_ep_alloc_request().
>>>
>>> Signed-off-by: Felipe F. Tonello 
>>> ---
>>>  drivers/usb/gadget/function/f_hid.c | 6 +-
>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>>> b/drivers/usb/gadget/function/f_hid.c
>>> index a010496e4e05..89d2e9a5a04f 100644
>>> --- a/drivers/usb/gadget/function/f_hid.c
>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, 
>>> struct usb_function *f)
>>>  
>>> /* preallocate request and buffer */
>>> status = -ENOMEM;
>>> -   hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
>>> +   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>> if (!hidg->req)
>>> goto fail;
>>>  
>>> -   hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
>>> -   if (!hidg->req->buf)
>>> -   goto fail;
>>> -
>>> /* set descriptor dynamic values */
>>> hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>>> hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
>>>
>>
>> Hi Felipe,
>>
>> This commit on your testing/next breaks compilation.
>>
>> ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
>> ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to 
>> function ‘alloc_ep_req’
>>   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>   ^
>> In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
>> ../drivers/usb/gadget/u_f.h:63:21: note: declared here
>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int 
>> default_len);
> 
> true that :-) Dropping from my queue.
> 

Are you applying the previous patches? Specially that this is the last
patch in the series, how can it break with you if it doesn't break here?
What should I do then?

Thanks

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Blabi,

On 18/08/16 08:12, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello"  writes:
>> The default_length parameter of alloc_ep_req was not really necessary
>> and gadget drivers would almost always create an inline function to pass
>> the same value to len and default_len.
>>
>> So this patch also removes duplicate code from few drivers.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_hid.c| 10 ++
>>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>>  drivers/usb/gadget/u_f.c   |  7 +++
>>  drivers/usb/gadget/u_f.h   |  3 +--
>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>> b/drivers/usb/gadget/function/f_hid.c
>> index 51980c50546d..e82a7468252e 100644
>> --- a/drivers/usb/gadget/function/f_hid.c
>> +++ b/drivers/usb/gadget/function/f_hid.c
>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file 
>> *fd)
>>  
>> /*-*/
>>  /*usb_function 
>> */
>>  
>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>> -unsigned length)
>> -{
>> -return alloc_ep_req(ep, length, length);
>> -}
> 
> actually, I prefer to keep these little helpers. I was recently playing
> with adding SG list support to g_zero (I should have patches soon) and
> it was actually very nice to have the sourcesink helper as I could just
> ditch alloc_ep_req(). The change to the driver was local to
> ss_alloc_ep_req() and nothing else changed :-)
> 

Right, but then it is worth to have the helper function. In this
particular case, I am removing a useless helper functions, especially
that the previous patch removes the need for the optional parameter in
alloc_ep_req.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH V9 1/1] usb:serial: Add Fintek F81532/534 driver

[Johan Hovold]

On Tue, Aug 23, 2016 at 04:23:44PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
> 
> Johan Hovold 於 2016/8/22 下午 09:14 寫道:

> >> +{
> >> +  size_t count = F81534_USB_MAX_RETRY;
> >> +  int status;
> >> +  u8 *tmp;
> >> +
> >> +  tmp = kmalloc(sizeof(u8), GFP_KERNEL);
> >> +  if (!tmp)
> >> +  return -ENOMEM;
> >> +
> >> +  *tmp = data;
> >> +
> >> +  /*
> >> +   * Our device maybe not reply when heavily loading, We'll retry for
> >> +   * F81534_USB_MAX_RETRY times.
> >> +   */
> >> +  while (count--) {
> >> +  status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> >> +   F81534_SET_GET_REGISTER,
> >> +   USB_TYPE_VENDOR | USB_DIR_OUT,
> >> +   reg, 0, tmp, sizeof(u8),
> >> +   F81534_USB_TIMEOUT);
> >> +  if (status > 0)
> >> +  break;
> >> +
> >> +  if (status == 0)
> >> +  status = -EIO;
> >> +  }
> >> +
> >> +  if (status < 0) {
> >> +  dev_err(>dev, "%s: reg: %x data: %x failed: %d\n",
> >> +  __func__, reg, data, status);
> >> +  kfree(tmp);
> >> +  return status;
> >
> > I'd use a common exit path to free tmp, and just print an error here.
> 
> I'll change this with next patch. BTW, Alan had suggested me to re-write
> set/get register function to avoid kmalloc(), but I found some issue
> to re-write.
> 
> We need to read the internal storage to determinate the port counts in
> f81534_calc_num_ports(), but in this moment the usb_serial had no
> private data, it still need to use kmalloc() to get memory.
> 
> The following source code is my current modification. I'll kmalloc
> the buffer if it can't get serial_private, otherwise I'll use
> serial_private buffer and protected by a mutex. Should I do something
> to improve it?

I'd say it's not worth trying to avoid that extra allocation, and there
will be several further allocations done in the usb_control_msg path
anyway. What you have today (i.e. in v9) is fine.

> >> +static int f81534_calc_num_ports(struct usb_serial *serial)
> >> +{
> >> +  unsigned char setting[F81534_CUSTOM_DATA_SIZE];
> >> +  uintptr_t setting_idx;
> >> +  u8 num_port = 0;
> >> +  int status;
> >> +  size_t i;
> >> +
> >> +  /* Check had custom setting */
> >> +  status = f81534_find_config_idx(serial, _idx);
> >> +  if (status) {
> >> +  dev_err(>dev->dev, "%s: find idx failed: %d\n",
> >> +  __func__, status);
> >> +  return 0;
> >> +  }
> >> +
> >> +  /* Save the configuration area idx as private data for attach() */
> >> +  usb_set_serial_data(serial, (void *)setting_idx);
> >> +
> >> +  /* Read default board setting */
> >> +  status = f81534_read_data(serial, F81534_DEF_CONF_ADDRESS_START,
> >> +F81534_NUM_PORT, setting);
> >> +  if (status) {
> >> +  dev_err(>dev->dev, "%s: read failed: %d\n", __func__,
> >> +  status);
> >> +  return 0;
> >> +  }
> >> +
> >> +  /*
> >> +   * If had custom setting, override it. 1st byte is a indicator, 0xff
> >> +   * is empty, F81534_CUSTOM_VALID_TOKEN is had data, then skip with 1st
> >> +   * data
> >> +   */
> >> +  if (setting_idx != F81534_CUSTOM_NO_CUSTOM_DATA) {
> >> +  status = f81534_read_data(serial, F81534_CUSTOM_ADDRESS_START +
> >> +  F81534_CONF_OFFSET,
> >> +  sizeof(setting), setting);
> >> +  if (status) {
> >> +  dev_err(>dev->dev,
> >> +  "%s: get custom data failed: %d\n",
> >> +  __func__, status);
> >> +  return 0;
> >> +  }
> >> +
> >> +  dev_dbg(>dev->dev,
> >> +  "%s: read configure from block: %d\n",
> >> +  __func__, (unsigned int)setting_idx);
> >> +  } else {
> >> +  dev_dbg(>dev->dev, "%s: read configure default\n",
> >> +  __func__);
> >> +  }
> >> +
> >> +  /* New style, find all possible ports */
> >> +  num_port = 0;
> >> +  for (i = 0; i < F81534_NUM_PORT; ++i) {
> >> +  if (setting[i] & F81534_PORT_UNAVAILABLE)
> >> +  continue;
> >
> > Looks like setting[] could be uninitialised here.
> 
> Our IC will preserve 2 section for configuration data. One is
> F81534_DEF_CONF_ADDRESS_START, another is F81534_CUSTOM_ADDRESS_START.
> 
> We'll read F81534_DEF_CONF_ADDRESS_START first for default value and
> read F81534_CUSTOM_ADDRESS_START for customer value.

My bad, I missed the first read above, sorry.

> >> +  tty_port_num = f81534_phy_to_logic_port(serial, phy_port_num);
> >> +  port = serial->port[tty_port_num];
> >> +
> >> +  /*
> >> +   * The device will send back all information when we submitted
> >> +   * 

Re: [PATCH v3] usb: serial: mos7840.c Support TIOCGRS485 and TIOCSRS485 ioctls()

[Johan Hovold]

On Mon, Apr 11, 2016 at 02:27:20PM -0700, Aaron Marburg wrote:
> 
>   Add callbacks to handle TIOCGRS485 and TIOCSRS485 ioctl
>   calls in mos7840.c, allowing configuration of the chip's
>   "scratchpad" register to strobe the DTR line while transmitting.
> 
>   This functionality is required for RS485 mode on the B Electronics
>   USOPTL4-4P and USOPTL4-2P USB-to-RS485/422 hubs based on this chip.
> 
> Signed-off-by: Aaron Marburg 
> ---
> Changes relative to v2:
> -- As suggested, implemented handlers for TIOC?RS485 ioctl callbacks
>rather than command-line options to enable RS485 configurations.

Thanks for the update, and sorry for not getting back to you on this
one.

Always remember to run checkpatch.pl on patches before submitting, it
would have let know about a few minor issues.

>   drivers/usb/serial/mos7840.c | 78 
> 
>   1 file changed, 78 insertions(+)
> 
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index ed378fb..8a78143 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -1977,6 +1977,74 @@ static int mos7840_get_serial_info(struct moschip_port 
> *mos7840_port,
>   }
> 
>   
> /*
> + * mos7840_get_rs485_info
> + *  function to handle RS485 "get" ioctl
> + 
> */
> +

No need to add such function headers, just drop them.

> +static int mos7840_get_rs485_info(struct usb_serial_port *port,
> + struct serial_rs485 __user *retinfo)
> +{
> + struct serial_rs485 tmp;
> + int status;
> + __u16 scratchpad;

Use u16.

> +
> + if (port == NULL)
> + return -EFAULT;
> +
> + if (!retinfo)
> + return -EFAULT;

Neither NULL check is needed.

> +
> + memset(, 0, sizeof(tmp));
> +
> + status = mos7840_get_uart_reg(port, SCRATCH_PAD_REGISTER, );
> + if (status < 0) {
> + dev_dbg(>dev, "Reading ScratchPad (RS485 config) register 
> failed status=0x%x\n", status);

This should be dev_err, and please use a more compact error such as

"failed to read scratch-pad register: %d\n"

> + return -EFAULT;

Return usb_translate_errors(status) here.

> + }
> +
> + tmp.flags |= (scratchpad & 0x80) ? SER_RS485_ENABLED : 0x00;
> + tmp.flags |= (scratchpad & 0x40) ? SER_RS485_RTS_ON_SEND : 0x00;
> +
> + if (copy_to_user(retinfo, , sizeof(*retinfo)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +/*
> + * mos7840_set_rs485_info
> + *  function to handle RS485 "set" ioctl
> + 
> */
> +
> +static int mos7840_set_rs485_info(struct usb_serial_port *port,
> + struct serial_rs485 __user *retinfo)
> +{
> + struct serial_rs485 tmp;
> + int status;
> + __u16 scratchpad = 0;
> +
> + if (port == NULL)
> + return -EFAULT;
> +
> + if (!retinfo)
> + return -EFAULT;

Same comments as above.

> + if (copy_from_user(, retinfo, sizeof(*retinfo)))
> + return -EFAULT;
> +
> + if (tmp.flags & SER_RS485_ENABLED)
> + scratchpad = 0x80 | ((tmp.flags & SER_RS485_RTS_ON_SEND) ? 0x40 
> : 0x00);

Please us an inner conditional for rts-on-send.

> +
> + status = mos7840_set_uart_reg(port, SCRATCH_PAD_REGISTER, scratchpad);
> + if (status < 0) {
> + dev_dbg(>dev, "Writing ScratchPad (RS485 config) register 
> failed status=0x%x\n", status);
> + return -EFAULT;

Same comments as above apply.

> + }

You also need to clear any unsupported fields in retinfo, and copy the
accepted flags back before returning.

> +
> + return 0;
> +}
> +
> +/*
>* SerialIoctl
>*  this function handles any ioctl calls to the driver
>
> */
> @@ -2010,6 +2078,16 @@ static int mos7840_ioctl(struct tty_struct *tty,
>   case TIOCSSERIAL:
>   dev_dbg(>dev, "%s TIOCSSERIAL\n", __func__);
>   break;
> +
> + case TIOCGRS485:
> + dev_dbg(>dev, "%s TIOCGRS485\n", __func__);
> + return mos7840_get_rs485_info(port, argp);
> +
> + case TIOCSRS485:
> + dev_dbg(>dev, "%s TIOCSRS485\n", __func__);
> + return mos7840_set_rs485_info(port, argp);
> +
> +

Drop the dev_dbgs along whit the empty lines.

>   default:
>   break;
>   }

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  

Re: [PATCH v2 06/22] usb: serial: ti_usb_3410_5052: Remove unused variables

[Johan Hovold]

On Tue, Jul 26, 2016 at 07:59:46PM +0200, Mathieu OTHACEHE wrote:
> Remove variables affected but never read.

This commit message is also incomplete. The tp_flags variable was indeed
read, even if it was always ANDed with 0.

I applied it anyway this time, and also dropped the now unused
TI_SET_SERIAL_FLAGS define that you did remove in v1.

> Signed-off-by: Mathieu OTHACEHE 

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: xHCI problem? [was Re: Erratic USB device behavior and device loss]

[Ritesh Raj Sarraf]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

Thank you Alan for confirming the bug.

On Mon, 2016-08-22 at 15:09 -0400, Alan Stern wrote:
> > https://people.debian.org/~rrs/tmp/usb_1u-usbmon.txt.gz
> > 
> > https://people.debian.org/~rrs/tmp/dmesg-usbmon.txt
> 
> The usbmon trace shows two separate issues, one in software and one in 
> hardware.  The two issues may or may not be related.
> 
> The software bug is in the driver for this device.  Every 3 seconds it 
> does a runtime suspend followed immediately by a resume, for no 
> apparent reason.  Even worse, it tries to communicate with the device 
> during the time that it is suspended!
> 
> The hardware bug is a series of spontaneous disconnections.  Again, 
> there's no obvious reason for this, although it's possible that the 
> device doesn't like going into and out of suspend very often.  (But 
> there are a lot more suspends than disconnects.)  It's also possible 
> that the USB cable isn't making a firm connection.
> 
> The final blow came when the driver tried to communicate with the
> device _as_ it was being suspended.  That seemed to mess up the
> hardware so badly that it was unable to recover, and from that point it
> stopped working entirely.
> 
> The timing may explain why it takes so long for the device to stop 
> working.  That doesn't happen until, by chance, the driver tries to 
> communicate with the device as it is being suspended.
> 
> Try doing this: Reboot.  Then before the problem occurs, mount a 
> debugfs filesystem on /sys/kernel/debug and make a copy of 
> /sys/kernel/debug/usb/devices.  Let's see what it says about the faulty 
> card reader.

Upon resume today morning, surprisingly, I get the lsusb output reporting the
device. Earlier, it used to knockoff the device from the list.

rrs@learner:/var/tmp$ lsusb 
Bus 003 Device 002: ID 8087:8000 Intel Corp. 
Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 001 Device 006: ID 048d:8350 Integrated Technology Express, Inc. 
Bus 001 Device 005: ID 0bda:b728 Realtek Semiconductor Corp. 
Bus 001 Device 004: ID 04f2:b40f Chicony Electronics Co., Ltd 
Bus 001 Device 003: ID 04f3:0303 Elan Microelectronics Corp. 
Bus 001 Device 013: ID 0bda:0129 Realtek Semiconductor Corp. RTS5129 Card Reader
Controller
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
2016-08-23 / 14:08:05 ♒♒♒  ☺  

And dmesg reported it detected and then, like you've analyzed so far,
disconnected.

[ 1904.885366] usb 1-4: USB disconnect, device number 50
[ 1906.123534] usb 1-4: new high-speed USB device number 51 using xhci_hcd
[ 1912.870405] usb 1-4: new high-speed USB device number 57 using xhci_hcd
[ 1920.370643] usb 1-4: new high-speed USB device number 66 using xhci_hcd
[ 1987.822768] usb 1-4: new high-speed USB device number 71 using xhci_hcd
[ 1994.799738] usb 1-4: new high-speed USB device number 78 using xhci_hcd
[ 2002.373323] usb 1-4: new high-speed USB device number 88 using xhci_hcd
[ 2002.547056] usb 1-4: New USB device found, idVendor=0bda, idProduct=0129
[ 2002.547059] usb 1-4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[ 2002.547061] usb 1-4: Product: USB2.0-CRW
[ 2002.547062] usb 1-4: Manufacturer: Generic
[ 2002.547064] usb 1-4: SerialNumber: 2010020139600
[ 2005.804225] usb 1-4: reset high-speed USB device number 88 using xhci_hcd
[ 3052.573655] usb 1-4: USB disconnect, device number 88
[ 3053.854647] usb 1-4: new high-speed USB device number 89 using xhci_hcd
[ 3054.028343] usb 1-4: New USB device found, idVendor=0bda, idProduct=0129
[ 3054.028347] usb 1-4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[ 3054.028349] usb 1-4: Product: USB2.0-CRW
[ 3054.028351] usb 1-4: Manufacturer: Generic
[ 3054.028352] usb 1-4: SerialNumber: 2010020139600
[ 6376.220631] usb 1-4: USB disconnect, device number 89
[ 6377.469613] usb 1-4: new high-speed USB device number 90 using xhci_hcd
[ 6377.643178] usb 1-4: New USB device found, idVendor=0bda, idProduct=0129
[ 6377.643182] usb 1-4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[ 6377.643184] usb 1-4: Product: USB2.0-CRW
[ 6377.643186] usb 1-4: Manufacturer: Generic
[ 6377.643188] usb 1-4: SerialNumber: 2010020139600
[ 7166.914656] usb 1-4: reset high-speed USB device number 90 using xhci_hcd
[ 7167.074275] usb 1-4: Device not responding to setup address.
[ 7167.277738] usb 1-4: Device not responding to setup address.
[ 7167.480937] usb 1-4: device not accepting address 90, error -71
[ 7167.641011] usb 1-4: reset high-speed USB device number 90 using xhci_hcd


Trying to read the /sys/kernel/debug/usb/devices led to a hung process. The
process is in uninterruptible sleep. I'm seeing this for the very first time for
a USB (Non Storage) device. Perhaps it has to do with the frequent resets.

rrs@learner:/var/tmp$ ps aux | grep "cat devices"
root 18453  0.0  0.0   6004   772 pts/0D+   13:56   0:00 cat devices

Re: [PATCH V9 1/1] usb:serial: Add Fintek F81532/534 driver

[Ji-Ze Hong (Peter Hong)]

Hi Johan,

Johan Hovold 於 2016/8/22 下午 09:14 寫道:


+static const struct reg_value f81534_pin_control[4][3] = {
+   /* M0_SDM1  M2 */
+   {{0x2ae8, 7}, {0x2a90, 5}, {0x2a90, 4}, },  /* port 0 pins */
+   {{0x2ae8, 6}, {0x2ae8, 0}, {0x2ae8, 3}, },  /* port 1 pins */
+   {{0x2a90, 0}, {0x2ae8, 2}, {0x2a80, 6}, },  /* port 2 pins */
+   {{0x2a90, 3}, {0x2a90, 2}, {0x2a90, 1}, },  /* port 3 pins */
+};


I thought we agreed to drop the transceiver configuration from the
driver in favour of a user-space tool?


OK, I'll remove all transceiver setting next version.



+{
+   size_t count = F81534_USB_MAX_RETRY;
+   int status;
+   u8 *tmp;
+
+   tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+   if (!tmp)
+   return -ENOMEM;
+
+   *tmp = data;
+
+   /*
+* Our device maybe not reply when heavily loading, We'll retry for
+* F81534_USB_MAX_RETRY times.
+*/
+   while (count--) {
+   status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+F81534_SET_GET_REGISTER,
+USB_TYPE_VENDOR | USB_DIR_OUT,
+reg, 0, tmp, sizeof(u8),
+F81534_USB_TIMEOUT);
+   if (status > 0)
+   break;
+
+   if (status == 0)
+   status = -EIO;
+   }
+
+   if (status < 0) {
+   dev_err(>dev, "%s: reg: %x data: %x failed: %d\n",
+   __func__, reg, data, status);
+   kfree(tmp);
+   return status;


I'd use a common exit path to free tmp, and just print an error here.


I'll change this with next patch. BTW, Alan had suggested me to re-write
set/get register function to avoid kmalloc(), but I found some issue
to re-write.

We need to read the internal storage to determinate the port counts in
f81534_calc_num_ports(), but in this moment the usb_serial had no
private data, it still need to use kmalloc() to get memory.

The following source code is my current modification. I'll kmalloc
the buffer if it can't get serial_private, otherwise I'll use
serial_private buffer and protected by a mutex. Should I do something
to improve it?

static int f81534_set_normal_register(struct usb_serial *serial, u16 reg,
u8 data)
{
struct f81534_serial_private *serial_priv =
usb_get_serial_data(serial);
struct usb_interface *interface = serial->interface;
struct usb_device *dev = serial->dev;
size_t count = F81534_USB_MAX_RETRY;
int status;
u8 *tmp;

/*
 * The f81534_set_normal_register can be called by
 * f81534_calc_num_ports(). It'll run before allocate private data of
 * usb_serial. So we should treat as a special case.
 */
if (serial_priv) {
mutex_lock(_priv->register_mutex);
tmp = _priv->reg_value;
} else {
tmp = kmalloc(sizeof(u8), GFP_KERNEL);
if (!tmp)
return -ENOMEM;
}

*tmp = data;

/*
 * Our device maybe not reply when heavily loading, We'll retry for
 * F81534_USB_MAX_RETRY times.
 */
while (count--) {
status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
 F81534_SET_GET_REGISTER,
 USB_TYPE_VENDOR | USB_DIR_OUT,
 reg, 0, tmp, sizeof(u8),
 F81534_USB_TIMEOUT);
if (status > 0) {
status = 0;
break;
} else if (status == 0) {
status = -EIO;
}
}

if (status < 0) {
dev_err(>dev, "%s: reg: %x data: %x failed: %d\n",
__func__, reg, data, status);
}

if (serial_priv)
mutex_unlock(_priv->register_mutex);
else
kfree(tmp);

return status;
}





+static int f81534_command_delay(struct usb_serial *usbserial)


Please explain why and when you need to use this "delay" function, and
how the BUS_REG_STATUS register works.

Please use "serial" consistently throughout for usb_serial pointers
(instead of "usbserial").


OK, I'll add comment above f81534_command_delay() and change all
usbserial to serial.



+static int f81534_calc_num_ports(struct usb_serial *serial)
+{
+   unsigned char setting[F81534_CUSTOM_DATA_SIZE];
+   uintptr_t setting_idx;
+   u8 num_port = 0;
+   int status;
+   size_t i;
+
+   /* Check had custom setting */
+   status = f81534_find_config_idx(serial, _idx);
+   if (status) {
+  

Re: [PATCH v2 07/22] usb: serial: ti_usb_3410_5052: Use macros instead of magic values

[Johan Hovold]

On Tue, Jul 26, 2016 at 07:59:47PM +0200, Mathieu OTHACEHE wrote:
> Use macros to define 3410 and 5052 baud bases.
> Use macro to define usb download timeout.
> 
> Signed-off-by: Mathieu OTHACEHE 
> ---
>  drivers/usb/serial/ti_usb_3410_5052.c | 22 +++---
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c 
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index 2b7fe89..b5f3328 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -270,10 +270,12 @@ struct ti_firmware_header {
>  #define TI_DRIVER_AUTHOR "Al Borchers "
>  #define TI_DRIVER_DESC   "TI USB 3410/5052 Serial Driver"
>  
> -#define TI_FIRMWARE_BUF_SIZE 16284
> +#define TI_3410_BAUD_BASE   923077
> +#define TI_5052_BAUD_BASE   461538
>  
> +#define TI_FIRMWARE_BUF_SIZE 16284
>  #define TI_TRANSFER_TIMEOUT  2
> -
> +#define TI_DOWNLOAD_TIMEOUT  1000
>  #define TI_DEFAULT_CLOSING_WAIT  4000/* in .01 secs */
>  
>  /* supported setserial flags */
> @@ -1016,9 +1018,9 @@ static void ti_set_termios(struct tty_struct *tty,
>   if (!baud)
>   baud = 9600;
>   if (tport->tp_tdev->td_is_3410)
> - wbaudrate = (923077 + baud/2) / baud;
> + wbaudrate = (TI_3410_BAUD_BASE + baud / 2) / baud;
>   else
> - wbaudrate = (461538 + baud/2) / baud;
> + wbaudrate = (TI_5052_BAUD_BASE + baud / 2) / baud;
>  
>   /* FIXME: Should calculate resulting baud here and report it back */
>   if ((C_BAUD(tty)) != B0)
> @@ -1434,6 +1436,7 @@ static int ti_get_serial_info(struct ti_port *tport,
>   struct usb_serial_port *port = tport->tp_port;
>   struct serial_struct ret_serial;
>   unsigned cwait;
> + int baud_base;
>  
>   if (!ret_arg)
>   return -EFAULT;
> @@ -1444,11 +1447,16 @@ static int ti_get_serial_info(struct ti_port *tport,
>  
>   memset(_serial, 0, sizeof(ret_serial));
>  
> + if (tport->tp_tdev->td_is_3410)
> + baud_base = TI_3410_BAUD_BASE;
> + else
> + baud_base = TI_5052_BAUD_BASE;
> +
>   ret_serial.type = PORT_16550A;
>   ret_serial.line = port->minor;
>   ret_serial.port = port->port_number;
>   ret_serial.xmit_fifo_size = kfifo_size(>write_fifo);
> - ret_serial.baud_base = tport->tp_tdev->td_is_3410 ? 921600 : 460800;
> + ret_serial.baud_base = baud_base;

The above is not functionally equivalent, since your now returning a
different baud_base.

This may be fine, but again you're hiding changes in the code without
describing them properly in the commit messages.

Please go through the rest of the series, and make sure that the commit
messages are correct. I'm not gonna look at the rest of the series.

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 v2 04/22] usb: serial: ti_usb_3410_5052: Remove useless NULL-testing

[Johan Hovold]

On Tue, Jul 26, 2016 at 07:59:44PM +0200, Mathieu OTHACEHE wrote:
> It is useless to check the return of usb_get_serial_port_data.

Please be more specific in your commit messages in general. In this case
it should mention that there's no need to check for NULL private data in
the tty or tty-port callbacks.

> Signed-off-by: Mathieu OTHACEHE 
> ---
>  drivers/usb/serial/ti_usb_3410_5052.c | 34 +-
>  1 file changed, 1 insertion(+), 33 deletions(-)
> 
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c 
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index b694d69..c8ed3f9 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -653,9 +653,6 @@ static int ti_open(struct tty_struct *tty, struct 
> usb_serial_port *port)
>TI_PIPE_TIMEOUT_ENABLE |
>(TI_TRANSFER_TIMEOUT << 2));
>  
> - if (tport == NULL)
> - return -ENODEV;
> -
>   dev = port->serial->dev;
>   tdev = tport->tp_tdev;
>  
> @@ -784,8 +781,6 @@ static void ti_close(struct usb_serial_port *port)
>  
>   tdev = usb_get_serial_data(port->serial);
>   tport = usb_get_serial_port_data(port);
> - if (tdev == NULL || tport == NULL)
> - return;

You also forgot to mention that you are removing a check for interface
private data in close().

I fixes this up before applying this time.

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: Bug 153551: Kernel panic on Nexus 5X USB unplug while tethering

[Oliver Neukum]

On Mon, 2016-08-22 at 17:21 -0600, Jose Marino wrote:
> I'm using my phone (Nexus 5X running Android) to tether a USB connection 
> to my laptop (XPS 15 9550). I plug the phone through the USB-C 
> connection and in the phone I  select USB tethering. Initially things 
> look normal: a usb0 network interface appears in the laptop and it tries 
> to get an IP with dhcp. However, I observe two different behaviors 
> depending on whether it's a fresh boot, or I have suspend/resumed the 
> laptop. In a fresh boot everything works fine, I get an IP and the 
> connection works as expected. If I unplug the phone, everything also 
> works as expected.
> 
> However, after a suspend/resume cycle, I plug the phone in but the 
> laptop never connects to it. The usb0 interface still appears, but the 
> dhcp daemon is unable to get any response and finally times out. The fun 
> part happens when I unplug the phone. I consistently get a kernel panic.

The HC has crashed thoroughly. Do other devices work after S3?

Regards
Oliver


--
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 03/22] usb: serial: ti_usb_3410_5052: Use kzalloc instead of kmalloc

[Johan Hovold]

On Wed, Jul 27, 2016 at 02:46:32PM +0200, Mathieu OTHACEHE wrote:
> 
> > in that case, where is the initialisation to 0 you avoid and hence
> > can remove from the code?
> 
> Hi,
> 
> In v1, kzalloc was useful to avoid wFlags initialisation to 0 :
> 
> https://lkml.org/lkml/2016/5/12/139
> 
> In v2 wFlags initialisation has be removed so this patch has no purpose
> anymore, my bad.

Actually, the kzalloc is now making sure that the cXon and cXoff fields
are always initialised. So I think the change is good, but please update
commit message.

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 v2 02/22] usb: serial: ti_usb_3410_5052: Remove useless dev_dbg messages

[Johan Hovold]

On Tue, Jul 26, 2016 at 07:59:42PM +0200, Mathieu OTHACEHE wrote:
> Remove useless or redundant dev_dbg messages.
> Fix debug-message typos.

You never fix any typos, and forgot to mention the added NULL-check for
oldtermios, which is currently not needed and really should go in its
own patch.

> Signed-off-by: Mathieu OTHACEHE 
> ---
> Changelog:
> v2:
> * Keep some debug messages
> 
>  drivers/usb/serial/ti_usb_3410_5052.c | 23 +--
>  1 file changed, 9 insertions(+), 14 deletions(-)

> @@ -967,9 +956,15 @@ static void ti_set_termios(struct tty_struct *tty,
>   cflag = tty->termios.c_cflag;
>   iflag = tty->termios.c_iflag;
>  
> - dev_dbg(>dev, "%s - cflag %08x, iflag %08x\n", __func__, cflag, 
> iflag);
> - dev_dbg(>dev, "%s - old clfag %08x, old iflag %08x\n", __func__,
> - old_termios->c_cflag, old_termios->c_iflag);
> + dev_dbg(>dev,
> + "%s - cflag 0x%08x, iflag 0x%08x\n", __func__, cflag, iflag);
> +
> + if (old_termios) {
> + dev_dbg(>dev, "%s - old clfag 0x%08x, old iflag 0x%08x\n",

Notice that "clfag" is still misspelled.

> + __func__,
> + old_termios->c_cflag,
> + old_termios->c_iflag);
> + }
>  
>   if (tport == NULL)
>   return;

So, I dropped this chunk, and applied the rest.

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 v2 01/22] usb: serial: ti_usb_3410_5052: Do not use __uX types

[Johan Hovold]

On Tue, Jul 26, 2016 at 07:59:41PM +0200, Mathieu OTHACEHE wrote:
> __uX types should only be used for user-space interactions.
> 
> Signed-off-by: Mathieu OTHACEHE 
> ---
> 
> Changelog:
> v2:
> * Replace cpu_to_be16s calls by cpu_to_be16
> * Remove other useless casts

You should have mentioned this in the commit message as well (i.e. that
you're doing more than just replacing __uX types).

>  drivers/usb/serial/ti_usb_3410_5052.c | 101 
> +-
>  1 file changed, 51 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c 
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index 07b4bf0..ebeea51 100644
 
>  static int ti_do_download(struct usb_device *dev, int pipe,
> - u8 *buffer, int size)
> +   u8 *buffer, int size)
>  {
>   int pos;
>   u8 cs = 0;

I dropped this unrelated change, and amended the commit message before
applying.

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