Re: High CPU load produced by USB (DW2)

[Minas Harutyunyan]

Hi,

On 3/5/2018 11:14 PM, Marek Vasut wrote:
> On 02/20/2018 06:51 AM, Minas Harutyunyan wrote:
> [...]
> Is there a way to reduce that or is that the absolute minimum in HS mode?
>
 We already discussed, in this email thread earlier, why SOF interrupts
 required and unmasked.
 Only in case when connected device with CTRL+BLK EP's only (like flash
 drive) and directly connected to cores root HUB, SOF's will be masked.
>>>
>>> That's the setup I have on Altera SoCFPGA, yet the SOFs are still present.
>>>
>> Could you please send verbose lsusb on connected to dwc2 device
> 
> See attached
> 
>> and driver debug log.
> 
> What exactly do you mean by this one ?
Enable debugging messages and verbose debugging messages for dwc2 from 
make menuconfig. Provide dmesg starting from dwc2 load till HS device 
connected to dwc2 port and enumerated.

Thanks,
Minas

> 

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

Donation For Charity Work

[Friedrich And Ann Mayrhofer]

--
Good Day,

My wife and I have awarded you with a donation of $ 1,000,000.00 Dollars from
part of our Jackpot Lottery of 50 Million Dollars, respond with your details
for claims.

We await your earliest response and God Bless you.

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

Re: [PATCH v4 1/2] dt-bindings: usb: ehci: add optional external vbus supply property

[Rob Herring]

On Thu, Mar 01, 2018 at 10:51:38AM +0100, Amelie Delaunay wrote:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> per port may be used.
> Add portN_vbus-supply property to usb-ehci bindings. As the number of ports
> depends on the ehci controller, and the port on which an external vbus
> supply depends on the platform,  is used to make it generic.
> 
> Signed-off-by: Amelie Delaunay 
> ---
>  Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt 
> b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..cd576db 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
>   - phys : phandle + phy specifier pair
>   - phy-names : "usb"
>   - resets : phandle + reset specifier pair
> + - portN_vbus-supply : phandle of regulator supplying vbus for port N

Just make this an array with the index being the port (and drop 
"portN_").

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

Re: [PATCHv2] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

[Rob Herring]

On Wed, Feb 28, 2018 at 07:50:57PM -0800, Tony Lindgren wrote:
> Let's add support for the GPIO controlled USB PHY on the MDM6600 modem.
> It is used on some Motorola Mapphone series of phones and tablets such
> as Droid 4.
> 
> The MDM6600 is hardwired to the first OHCI port in the Droid 4 case, and
> is controlled by several GPIOs. The USB PHY is integrated into the MDM6600
> device it seems. We know this as we get L3 errors from omap-usb-host if
> trying to use the PHY before MDM6600 is configured.
> 
> The GPIOs controlling MDM6600 are used to power device on and off, to
> configure the USB start-up mode (normal mode versus USB flashing), and
> they also tell the state of the MDM6600 device.
> 
> The two start-up mode GPIOs are dual-purposed and used for out of band
> (OOB) wake-up for USB and TS 27.010 serial mux. But we need to configure
> the USB start-up mode first to get MDM6600 booted in the right mode to
> be usable in the first place.
> 
> Note that the Motorola Mapphone Linux kernel tree has a "radio-ctrl"
> driver for modems. But it really does not control the radio at all, it
> just controls the modem power and start-up mode for USB. So I came to
> the conclusion that we're better off having this done in the USB PHY
> driver. For adding support for USB flashing mode, we can later on add
> a kernel module option for flash_mode=1 or something similar.
> 
> Also note that currently there is no PM runtime support for the OHCI
> on omap variant SoCs. So for low(er) power idle states, currenty both
> ohci-platform and phy-mapphone-mdm6600 must be unloaded or unbound.
> 
> For reference here is what I measured for total power consumption on
> an idle Droid 4 with and without USB related MDM6600 modules:
> 
> idle lcd off  phy-mapphone-mdm6600ohci-platform
> 153mW 284mW   344mW
> 
> So it seems that MDM6600 is currently not yet idling even with it's
> radio turned off, but that's something that is beyond the control of
> this USB PHY driver. This patch does get us to the point where modem
> data and GPS are usable with libqmi and ModemManager for example.
> Voice calls need more audio driver work.
> 
> Cc: devicet...@vger.kernel.org
> Cc: Mark Rutland 
> Cc: Marcel Partap 
> Cc: Michael Scott 
> Cc: Rob Herring 
> Cc: Sebastian Reichel 
> Signed-off-by: Tony Lindgren 
> ---
> 
> Changes since v1:
> - Fixed up issues noticed by Rob and Sebastian
> - Implemented wake irq handler (for debug use only for now)
> - Improved error handling based on more testing
> 
> ---
>  .../bindings/phy/phy-mapphone-mdm6600.txt  |  30 ++

Reviewed-by: Rob Herring 

>  drivers/phy/motorola/Kconfig   |   9 +
>  drivers/phy/motorola/Makefile  |   1 +
>  drivers/phy/motorola/phy-mapphone-mdm6600.c| 556 
> +
>  4 files changed, 596 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
>  create mode 100644 drivers/phy/motorola/phy-mapphone-mdm6600.c
--
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 v7] staging: typec: handle vendor defined part and modify drp toggling flow

[ShuFan Lee]

From: ShuFan Lee 

Handle vendor defined behavior in tcpci_init, tcpci_set_vconn, 
tcpci_start_drp_toggling
and export tcpci_irq. More operations can be extended in tcpci_data if needed.
According to TCPCI specification, 4.4.5.2 ROLE_CONTROL,
TCPC shall not start DRP toggling until subsequently the TCPM
writes to the COMMAND register to start DRP toggling.
DRP toggling flow is changed as following:
  - Write DRP = 1, Rp level and RC.CCx to Rd/Rd or Rp/Rp
  - Set LOOK4CONNECTION command

Signed-off-by: ShuFan Lee 
---
 drivers/staging/typec/tcpci.c | 127 +-
 drivers/staging/typec/tcpci.h |  15 +
 2 files changed, 116 insertions(+), 26 deletions(-)

 patch changelogs between v1 & v2
 - Remove unnecessary i2c_client in the structure of tcpci.
 - Rename structure of tcpci_vendor_data to tcpci_data.
 - Not exporting tcpci read/write wrappers but register i2c regmap in glue 
driver.
 - Add set_vconn ops in tcpci_data.
   (It is necessary for RT1711H to enable/disable idle mode before 
disabling/enabling vconn)
 - Export tcpci_irq so that vendor can call it in their own IRQ handler.

 patch changelogs between v2 & v3
 - Change the return type of tcpci_irq from int to irqreturn_t.

 patch changelogs between v3 & v4
 - Directly return regmap_write at the end of drp_toggling function.
 - Because tcpci_irq is called in _tcpci_irq, move _tcpci_irq to the place 
after tcpci_irq.

 patch changelogs between v4 & v5
 - Add a space between my first & last name, from ShuFanLee to ShuFan Lee.

 patch changelogs between v5 & v6
 - Add start_drp_toggling in tcpci_data and call it at the beginning of 
tcpci_start_drp_toggling.
 - Modify the flow of tcpci_start_drp_toggling as following:
- Set Rp level, RC.CCx to Rd/Rd or Rp/Rp and DRP = 1
- Set LOOK4CONNECTION command

 patch changelogs between v6 & v7
 - Remove checking whether tcpci->data is NULL because it is guaranteed to be 
available.

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 9bd4412..8043740 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -21,7 +21,6 @@
 
 struct tcpci {
struct device *dev;
-   struct i2c_client *client;
 
struct tcpm_port *port;
 
@@ -30,6 +29,12 @@ struct tcpci {
bool controls_vbus;
 
struct tcpc_dev tcpc;
+   struct tcpci_data *data;
+};
+
+struct tcpci_chip {
+   struct tcpci *tcpci;
+   struct tcpci_data data;
 };
 
 static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
@@ -37,8 +42,7 @@ 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,
-   u16 *val)
+static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
 {
return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
 }
@@ -98,9 +102,17 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum 
typec_cc_status cc)
 static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
enum typec_cc_status cc)
 {
+   int ret;
struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
unsigned int reg = TCPC_ROLE_CTRL_DRP;
 
+   /* Handle vendor drp toggling */
+   if (tcpci->data->start_drp_toggling) {
+   ret = tcpci->data->start_drp_toggling(tcpci, tcpci->data, cc);
+   if (ret < 0)
+   return ret;
+   }
+
switch (cc) {
default:
case TYPEC_CC_RP_DEF:
@@ -117,7 +129,17 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
break;
}
 
-   return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
+   if (cc == TYPEC_CC_RD)
+   reg |= (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
+  (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
+   else
+   reg |= (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
+  (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT);
+   ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
+   if (ret < 0)
+   return ret;
+   return regmap_write(tcpci->regmap, TCPC_COMMAND,
+   TCPC_CMD_LOOK4CONNECTION);
 }
 
 static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
@@ -178,6 +200,13 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool 
enable)
struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
int ret;
 
+   /* Handle vendor set vconn */
+   if (tcpci->data->set_vconn) {
+   ret = tcpci->data->set_vconn(tcpci, tcpci->data, enable);
+   if (ret < 0)
+   return ret;
+   }
+
ret = regmap_write(tcpci->regmap, TCPC_POWER_CTRL,
   enable ? TCPC_POWER_CTRL_VCONN_ENABLE : 0);
   

[PATCH 1/1] usb: host: xhci: do not return error status for URB

[Peter Chen]

Current XHCI implementation does not consider completion interrupt
for SETUP packet standalone, so it will show warning message
and return error status for URB. In fact, it can support it. In
this commit, we change warning message as debug message and set
status as zero for URB.

Support completion interrupt for SETUP packet is needed for USB EH2.0
SINGLE_STEP_SET_FEATURE Test.

Signed-off-by: Peter Chen 
---
 drivers/usb/host/xhci-ring.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index daa94c3..0729e46 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2010,12 +2010,9 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
 
switch (trb_comp_code) {
case COMP_SUCCESS:
-   if (trb_type != TRB_STATUS) {
-   xhci_warn(xhci, "WARN: Success on ctrl %s TRB without 
IOC set?\n",
+   if (trb_type != TRB_STATUS)
+   xhci_dbg(xhci, "Success on ctrl %s TRB without IOC 
set?\n",
  (trb_type == TRB_DATA) ? "data" : "setup");
-   *status = -ESHUTDOWN;
-   break;
-   }
*status = 0;
break;
case COMP_SHORT_PACKET:
-- 
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: [PATH 0/4] usbip: make vhci_hcd.* objects independent of vhci_hcd.0

[Shuah Khan]

On 03/05/2018 02:00 AM, Salvador Fandiño wrote:
> On 02/21/2018 01:35 AM, Shuah Khan wrote:
>> Hi Salvador,
>>
>> On 01/30/2018 01:36 AM, Salvador Fandino wrote:
>>> Let me start by explaining the problem that have motivated me to write
>>> this patches:
>>>
>>> I work on the QVD, a virtual desktop platform for Linux. This software
>>> runs Linux desktops (i.e. XFCE, KDE) and their applications inside LXC
>>> containers, and makes then available through the network to remote
>>> users.
>>>
>>> Supporting USB devices is a common feature customers have been
>>> requesting us for a long time (in order to use, for instance, remote
>>> signature pads, bar-code scanners, fingerprint readers, etc.). So, we
>>> have been working on that feature using the USB/IP layer on the
>>> kernel.
>>>
>>> Connecting and disconnecting devices and transferring data works
>>> seamless for the devices listed above. But we also want to make the
>>> usbip operations private to the container where they are run.  For
>>> instance, it is unacceptable for our product, that one user could list
>>> the devices connected by other users or access them.
>>>
>>> We can control how can access every device using cgroups once those
>>> are attached, but the usbip layer is not providing any mechanism for
>>> controlling who can attach, detach or list the devices.

In this use-case:

- does a container act as usbip client and attach devices from their
  host?
- do containers attach remote devices from other systems?

Is the core of the problem really that any remote system can import without
a provision for being able to restrict export to a set of remote machines?
If so, this is a generic problem even without containers and I would like
to solve this with a generic solution that works in all cases, not just for
containers.

The approach in this patch series appears to solve the problem just for
containers.

>>
>> Did you explore a solution to add a mechanism for access control to
>> usbip?
> 
> Could you elaborate on that?
> 
> For "usbip", do you mean the user space tools? 
> If that is the case, I don't think it would be enough.
> My aim is to limit vhci usage from containers and I have no control about 
> what runs inside the containers. So, a mangled usbip tool-set could > > be 
> used by a malicious user to circumvent any access control set there.> 

I mean the driver. There might be changes necessary in the user-space
as well depending on how the access controls are implemented. I am not
proposing implementing access controls in the user-space.


> IMO, there is no other choice but to control access to VHCI at the kernel 
> level.
> 

Probably. Please give as many details as possible on your environment
for me to make a call on if this problem can be solved in a different
way.

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

Re: [PATCH 1/2] usb: renesas_usbhs: add binding for r8a77965

[Rob Herring]

On Tue, Feb 27, 2018 at 05:16:02PM +0900, Yoshihiro Shimoda wrote:
> This patch adds binding for r8a77965 (R-Car M3-N).
> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  Documentation/devicetree/bindings/usb/renesas_usbhs.txt | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-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: [PATCH 2/2] usb: gadget: udc: renesas_usb3: add binging for r8a77965

[Rob Herring]

On Tue, Feb 27, 2018 at 05:16:03PM +0900, Yoshihiro Shimoda wrote:
> This patch adds binding for r8a77965 (R-Car M3-N).
> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  Documentation/devicetree/bindings/usb/renesas_usb3.txt | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-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: [PATCH] usb: host: xhci-rcar: add support for r8a77965

[Rob Herring]

On Tue, Feb 27, 2018 at 05:15:20PM +0900, Yoshihiro Shimoda wrote:
> This patch adds support for r8a77965 (R-Car M3-N).
> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
>  drivers/usb/host/xhci-rcar.c   | 4 
>  2 files changed, 5 insertions(+)

Reviewed-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: [PATCH v5 1/6] dt-bindings: add bindings for USB physical connector

[Rob Herring]

On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
> These bindings allow to describe most known standard USB connectors
> and it should be possible to extend it if necessary.
> USB connectors, beside USB can be used to route other protocols,
> for example UART, Audio, MHL. In such case every device passing data
> through the connector should have appropriate graph bindings.
> 
> Signed-off-by: Andrzej Hajda 
> ---
> v4:
> - improved 'type' description (Rob),
> - improved description of 2nd example (Rob).
> v3:
> - removed MHL port (samsung connector will have separate bindings),
> - added 2nd example for USB-C,
> - improved formatting.
> v2:
> - moved connector type(A,B,C) to compatible string (Rob),
> - renamed size property to type (Rob),
> - changed type description to be less confusing (Laurent),
> - removed vendor specific compatibles (implied by graph port number),
> - added requirement of connector being a child of IC (Rob),
> - removed max-mode (subtly suggested by Rob, it should be detected anyway
>   by USB Controller in runtime, downside is that device is not able to
>   report its real capabilities, maybe better would be to make it optional(?)),
> - assigned port numbers to data buses (Rob).
> 
> Regards
> Andrzej
> ---
>  .../bindings/connector/usb-connector.txt   | 75 
> ++
>  1 file changed, 75 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/connector/usb-connector.txt

Reviewed-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: usb: musb: "(null)" in sysfs mode file after disabling a gadget (and at other times, system hangs)

[Merlijn Wajer]

Hi Bin,

On 05/03/18 20:28, Bin Liu wrote:

> The musb udc driver sets the state to b_idle without checking a
> gadget driver, this should be cleaned up. I have add this in my backlog.
> But if this issue doesn't bother you much right now, I will make the
> action low priority and address it later whenever I got time. (likely
> not very soon, I have a hand full of musb driver bugs to fix...)

I can try to fix it this (or next) week. Do you have a pointer for me?

Cheers,
Merlijn



signature.asc
Description: OpenPGP digital signature

[PATCH net] net: usbnet: fix potential deadlock on 32bit hosts

[Eric Dumazet]

From: Eric Dumazet 

Marek reported a LOCKDEP issue occurring on 32bit host,
that we tracked down to the fact that usbnet could either
run from soft or hard irqs.

This patch adds u64_stats_update_begin_irqsave() and
u64_stats_update_end_irqrestore() helpers to solve this case.

[   17.768040] 
[   17.772239] WARNING: inconsistent lock state
[   17.776511] 4.16.0-rc3-next-20180227-7-g876c53a7493c #453 Not tainted
[   17.783329] 
[   17.787580] inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
[   17.793607] swapper/0/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
[   17.798751]  (>seq#5){?.-.}, at: [<9b22e5f0>] 
asix_rx_fixup_internal+0x188/0x288
[   17.806790] {IN-HARDIRQ-W} state was registered at:
[   17.811677]   tx_complete+0x100/0x208
[   17.815319]   __usb_hcd_giveback_urb+0x60/0xf0
[   17.819770]   xhci_giveback_urb_in_irq+0xa8/0x240
[   17.824469]   xhci_td_cleanup+0xf4/0x16c
[   17.828367]   xhci_irq+0xe74/0x2240
[   17.831827]   usb_hcd_irq+0x24/0x38
[   17.835343]   __handle_irq_event_percpu+0x98/0x510
[   17.840111]   handle_irq_event_percpu+0x1c/0x58
[   17.844623]   handle_irq_event+0x38/0x5c
[   17.848519]   handle_fasteoi_irq+0xa4/0x138
[   17.852681]   generic_handle_irq+0x18/0x28
[   17.856760]   __handle_domain_irq+0x6c/0xe4
[   17.860941]   gic_handle_irq+0x54/0xa0
[   17.864666]   __irq_svc+0x70/0xb0
[   17.867964]   arch_cpu_idle+0x20/0x3c
[   17.871578]   arch_cpu_idle+0x20/0x3c
[   17.875190]   do_idle+0x144/0x218
[   17.878468]   cpu_startup_entry+0x18/0x1c
[   17.882454]   start_kernel+0x394/0x400
[   17.886177] irq event stamp: 161912
[   17.889616] hardirqs last  enabled at (161912): [<7bedfacf>] 
__netdev_alloc_skb+0xcc/0x140
[   17.897893] hardirqs last disabled at (161911): [] 
__netdev_alloc_skb+0x94/0x140
[   17.904903] exynos5-hsi2c 12ca.i2c: tx timeout
[   17.906116] softirqs last  enabled at (161904): [<387102ff>] 
irq_enter+0x78/0x80
[   17.906123] softirqs last disabled at (161905): [] 
irq_exit+0x134/0x158
[   17.925722].
[   17.925722] other info that might help us debug this:
[   17.933435]  Possible unsafe locking scenario:
[   17.933435].
[   17.940331]CPU0
[   17.942488]
[   17.944894]   lock(>seq#5);
[   17.948274]   
[   17.950847] lock(>seq#5);
[   17.954386].
[   17.954386]  *** DEADLOCK ***
[   17.954386].
[   17.962422] no locks held by swapper/0/0.

Fixes: c8b5d129ee29 ("net: usbnet: support 64bit stats")
Signed-off-by: Eric Dumazet 
Reported-by: Marek Szyprowski 
Cc: Greg Ungerer 
---
 drivers/net/usb/usbnet.c   |   10 ++
 include/linux/u64_stats_sync.h |   22 ++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 
8a22ff67b0268a588428c61c6a6211e3c6c2a02a..d9eea8cfe6cb9a3bf8d0d4ce9198af9bccf9c757
 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -315,6 +315,7 @@ static void __usbnet_status_stop_force(struct usbnet *dev)
 void usbnet_skb_return (struct usbnet *dev, struct sk_buff *skb)
 {
struct pcpu_sw_netstats *stats64 = this_cpu_ptr(dev->stats64);
+   unsigned long flags;
int status;
 
if (test_bit(EVENT_RX_PAUSED, >flags)) {
@@ -326,10 +327,10 @@ void usbnet_skb_return (struct usbnet *dev, struct 
sk_buff *skb)
if (skb->protocol == 0)
skb->protocol = eth_type_trans (skb, dev->net);
 
-   u64_stats_update_begin(>syncp);
+   flags = u64_stats_update_begin_irqsave(>syncp);
stats64->rx_packets++;
stats64->rx_bytes += skb->len;
-   u64_stats_update_end(>syncp);
+   u64_stats_update_end_irqrestore(>syncp, flags);
 
netif_dbg(dev, rx_status, dev->net, "< rx, len %zu, type 0x%x\n",
  skb->len + sizeof (struct ethhdr), skb->protocol);
@@ -1248,11 +1249,12 @@ static void tx_complete (struct urb *urb)
 
if (urb->status == 0) {
struct pcpu_sw_netstats *stats64 = this_cpu_ptr(dev->stats64);
+   unsigned long flags;
 
-   u64_stats_update_begin(>syncp);
+   flags = u64_stats_update_begin_irqsave(>syncp);
stats64->tx_packets += entry->packets;
stats64->tx_bytes += entry->length;
-   u64_stats_update_end(>syncp);
+   u64_stats_update_end_irqrestore(>syncp, flags);
} else {
dev->net->stats.tx_errors++;
 
diff --git a/include/linux/u64_stats_sync.h b/include/linux/u64_stats_sync.h
index 
5bdbd9f49395f883ca2dc5aa0d7bbde11f379063..07ee0f84a46caa9e2b1c446f96009f63b3b99f50
 100644
--- a/include/linux/u64_stats_sync.h
+++ b/include/linux/u64_stats_sync.h
@@ -90,6 +90,28 @@ static inline void u64_stats_update_end(struct 
u64_stats_sync *syncp)
 #endif
 }
 
+static inline unsigned long
+u64_stats_update_begin_irqsave(struct 

Re: usb: musb: "(null)" in sysfs mode file after disabling a gadget (and at other times, system hangs)

[Bin Liu]

Merlijn,

On Fri, Mar 02, 2018 at 05:54:39PM +0100, Pali Rohár wrote:
> On Friday 02 March 2018 17:47:52 Merlijn Wajer wrote:
> > >> I would expect it to state "b_idle" instead of "(null)".
> > > 
> > > Actually, I'd like to see (null) whenever a gadget driver is not loaded,
> > > which indicates a gadget is not bound to the udc.
> > 
> > Hm... Sounds fine to me. I'm using this mode in combination with the usb
> > phy (vbus property) to detect if the phone is detect to a 'dumb' charger
> > of a PC, but I can just always have a gadget loaded -- same as before,
> > really.
> 
> For detection if wallcharger or pc usb charger is connected, there is
> isp1704_charger driver. It uses some standard ULPI interface. It reports
> current_max and type (DCP - dedicated, CDP or just usb).

The musb udc driver sets the state to b_idle without checking a
gadget driver, this should be cleaned up. I have add this in my backlog.
But if this issue doesn't bother you much right now, I will make the
action low priority and address it later whenever I got time. (likely
not very soon, I have a hand full of musb driver bugs to fix...)

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

Re: howto debug xhci driver?

[Bin Liu]

On Mon, Mar 05, 2018 at 10:16:49AM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Liu  writes:
> > I am relatively new to xhci and its driver. I am trying to get a xhci
> > driver runtime log to understand how it handles usb transactions, but I
> > don't get much information with dynamic debug (module xhci_hcd) or
> > enabling xhci trace events. Is there any other methods you guys use to
> > debug xhci driver?
> 
> tracepoints, the best thing since sliced bread ;-)

I know, but I didn't get any trace log in bulk IN transfers :(
It turns out - I was on v4.9, which doesn't have much tracepoints. Now I
see you added more since v4.11. I will try to move to the latest kernel.

> 
> > BTY, the issue I am trying to debug is when reading bulk IN data from a
> > USB2.0 device, if the device doesn't have data to transmit and NAKs the
> > IN packet, after 4 pairs of IN-NAK transactions, xhci stops sending
> > further IN tokens until the next SOF, which leaves ~90us gape on the
> > bus.
> >
> > But when reading data from a USB2.0 thumb drive, this issue doesn't
> > happen, even if the device NAKs the IN tokens, xhci still keeps sending
> > IN tokens, which is way more than 4 pairs of IN-NAK transactions.
> 
> Thumb drive has Bulk endpoints, what is the other device's transfer type?

It is bulk too. I asked for device descriptors. This is a remote debug
effort for me, I don't have that device...

> 
> > Any one has a clue on what causes xhci to stop sending IN tokens after
> > the device NAK'd 4 times?
> 
> tracepoints, please :-)

I will :)
I checked the xhci spec, but didn't find any info on what controls
xhci's behavor on IN-NAK...

Regards,
-Bin.

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

Re: High CPU load produced by USB (DW2)

[Marek Vasut]

On 02/20/2018 06:51 AM, Minas Harutyunyan wrote:
[...]
 Is there a way to reduce that or is that the absolute minimum in HS mode?

>>> We already discussed, in this email thread earlier, why SOF interrupts
>>> required and unmasked.
>>> Only in case when connected device with CTRL+BLK EP's only (like flash
>>> drive) and directly connected to cores root HUB, SOF's will be masked.
>>
>> That's the setup I have on Altera SoCFPGA, yet the SOFs are still present.
>>
> Could you please send verbose lsusb on connected to dwc2 device

See attached

> and driver debug log.

What exactly do you mean by this one ?

-- 
Best regards,
Marek Vasut

Bus 001 Device 003: ID 0781:5581 SanDisk Corp. Ultra
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.10
  bDeviceClass0 (Defined at Interface level)
  bDeviceSubClass 0 
  bDeviceProtocol 0 
  bMaxPacketSize064
  idVendor   0x0781 SanDisk Corp.
  idProduct  0x5581 Ultra
  bcdDevice1.00
  iManufacturer   1 SanDisk
  iProduct2 Ultra
  iSerial 3 4C530001110620109113
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength   32
bNumInterfaces  1
bConfigurationValue 1
iConfiguration  0 
bmAttributes 0x80
  (Bus Powered)
MaxPower  224mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass 8 Mass Storage
  bInterfaceSubClass  6 SCSI
  bInterfaceProtocol 80 Bulk-Only
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81  EP 1 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0200  1x 512 bytes
bInterval   0
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02  EP 2 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0200  1x 512 bytes
bInterval   0
Binary Object Store Descriptor:
  bLength 5
  bDescriptorType15
  wTotalLength   22
  bNumDeviceCaps  2
  USB 2.0 Extension Device Capability:
bLength 7
bDescriptorType16
bDevCapabilityType  2
bmAttributes   0x0002
  Link Power Management (LPM) Supported
  SuperSpeed USB Device Capability:
bLength10
bDescriptorType16
bDevCapabilityType  3
bmAttributes 0x00
wSpeedsSupported   0x000e
  Device can operate at Full Speed (12Mbps)
  Device can operate at High Speed (480Mbps)
  Device can operate at SuperSpeed (5Gbps)
bFunctionalitySupport   1
  Lowest fully-functional device speed is Full Speed (12Mbps)
bU1DevExitLat  10 micro seconds
bU2DevExitLat 256 micro seconds
Device Status: 0x
  (Bus Powered)

Bus 001 Device 002: ID 0424:2514 Standard Microsystems Corp. USB 2.0 Hub
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass9 Hub
  bDeviceSubClass 0 Unused
  bDeviceProtocol 2 TT per port
  bMaxPacketSize064
  idVendor   0x0424 Standard Microsystems Corp.
  idProduct  0x2514 USB 2.0 Hub
  bcdDeviceb.b3
  iManufacturer   0 
  iProduct0 
  iSerial 0 
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength   41
bNumInterfaces  1
bConfigurationValue 1
iConfiguration  0 
bmAttributes 0xe0
  Self Powered
  Remote Wakeup
MaxPower2mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   1
  bInterfaceClass 9 Hub
  bInterfaceSubClass  0 Unused
  bInterfaceProtocol  1 Single TT
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81  EP 1 IN
bmAttributes3
  Transfer TypeInterrupt
  Synch Type   None
  Usage Type   Data

Re: inconsistent lock state with usbnet/asix usb ethernet and xhci

[Eric Dumazet]

On Mon, 2018-03-05 at 12:46 +0100, Oliver Neukum wrote:
> On Mon, 2018-03-05 at 08:45 +0100, Marek Szyprowski wrote:
> > Hi Oliver,
> > 
> > On 2018-02-27 17:07, Oliver Neukum wrote:
> > > Am Dienstag, den 27.02.2018, 07:13 -0800 schrieb Eric Dumazet:
> > > > On Tue, 2018-02-27 at 07:09 -0800, Eric Dumazet wrote:
> > > > > 
> > > > > Note that for this one, it seems we also could perform stats
> > > > > updates in
> > > > > BH context, since skb is queued via defer_bh()
> > > > > 
> > > > > But simplicity wins I guess.
> > > > 
> > > > Thinking more about this, I am not sure we have any guarantee
> > > > that TX
> > > > and RX can not run on multiple cpus.
> > > > 
> > > > Using an unique syncp is not going to be safe, even if we make
> > > > lockdep
> > > > happy enough with the local_irq save/restore.
> > > 
> > > Unfortunately you are right. It is not guaranteed for some
> > > hardware.
> > 
> > Does it mean that the fix proposed by Eric is not the proper
> > solution?
> 
> For asix it should work, but asix is unlikely to be the only driver
> with that issue. 32 bit recieves less testing nowadays.

Yes, although the lockdep part could be enforced in 64bit if we really
care.

I will send a patch using two different sync (one for RX, one for TX)


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

Re: [PATCH v2 1/1] usb: musb: call pm_runtime_{get,put}_sync before reading vbus registers

[Bin Liu]

On Mon, Mar 05, 2018 at 08:35:10PM +0200, Ivaylo Dimitrov wrote:
> Hi,
> 
> On  5.03.2018 19:38, Bin Liu wrote:
> >On Wed, Feb 28, 2018 at 01:59:43PM -0800, Tony Lindgren wrote:
> >>* Merlijn Wajer  [180227 22:29]:
> >>>Without pm_runtime_{get,put}_sync calls in place, reading
> >>>vbus status via /sys causes the following error:
> >>>
> >>>Unhandled fault: external abort on non-linefetch (0x1028) at 0xfa0ab060
> >>>pgd = b333e822
> >>>[fa0ab060] *pgd=48011452(bad)
> >>>
> >>>[] (musb_default_readb) from [] 
> >>>(musb_vbus_show+0x58/0xe4)
> >>>[] (musb_vbus_show) from [] (dev_attr_show+0x20/0x44)
> >>>[] (dev_attr_show) from [] 
> >>>(sysfs_kf_seq_show+0x80/0xdc)
> >>>[] (sysfs_kf_seq_show) from [] (seq_read+0x250/0x448)
> >>>[] (seq_read) from [] (__vfs_read+0x1c/0x118)
> >>>[] (__vfs_read) from [] (vfs_read+0x90/0x144)
> >>>[] (vfs_read) from [] (SyS_read+0x3c/0x74)
> >>>[] (SyS_read) from [] (ret_fast_syscall+0x0/0x54)
> >>>
> >>>Solution was suggested by Tony Lindgren .
> >>>
> >>>Signed-off-by: Merlijn Wajer 
> >>
> >>Thanks for fixing this. Assuming it passes Bin's tests:
> >>
> >>Acked-by: Tony Lindgren 
> >
> >Applied and sent out. Thanks.
> >
> 
> What about stable kernels? Shouldn't this be fixed there as well?

Yes, it should, but I need to figure out which stables to send to and I
am busy in something at the moment. So I will send it @stable some time
later.

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

Re: [PATCH v2 1/1] usb: musb: call pm_runtime_{get,put}_sync before reading vbus registers

[Ivaylo Dimitrov]

Hi,

On  5.03.2018 19:38, Bin Liu wrote:

On Wed, Feb 28, 2018 at 01:59:43PM -0800, Tony Lindgren wrote:

* Merlijn Wajer  [180227 22:29]:

Without pm_runtime_{get,put}_sync calls in place, reading
vbus status via /sys causes the following error:

Unhandled fault: external abort on non-linefetch (0x1028) at 0xfa0ab060
pgd = b333e822
[fa0ab060] *pgd=48011452(bad)

[] (musb_default_readb) from [] (musb_vbus_show+0x58/0xe4)
[] (musb_vbus_show) from [] (dev_attr_show+0x20/0x44)
[] (dev_attr_show) from [] (sysfs_kf_seq_show+0x80/0xdc)
[] (sysfs_kf_seq_show) from [] (seq_read+0x250/0x448)
[] (seq_read) from [] (__vfs_read+0x1c/0x118)
[] (__vfs_read) from [] (vfs_read+0x90/0x144)
[] (vfs_read) from [] (SyS_read+0x3c/0x74)
[] (SyS_read) from [] (ret_fast_syscall+0x0/0x54)

Solution was suggested by Tony Lindgren .

Signed-off-by: Merlijn Wajer 


Thanks for fixing this. Assuming it passes Bin's tests:

Acked-by: Tony Lindgren 


Applied and sent out. Thanks.



What about stable kernels? Shouldn't this be fixed there as well?

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

Re: [PATCH v2 1/1] usb: musb: call pm_runtime_{get,put}_sync before reading vbus registers

[Bin Liu]

On Wed, Feb 28, 2018 at 01:59:43PM -0800, Tony Lindgren wrote:
> * Merlijn Wajer  [180227 22:29]:
> > Without pm_runtime_{get,put}_sync calls in place, reading
> > vbus status via /sys causes the following error:
> > 
> > Unhandled fault: external abort on non-linefetch (0x1028) at 0xfa0ab060
> > pgd = b333e822
> > [fa0ab060] *pgd=48011452(bad)
> > 
> > [] (musb_default_readb) from [] 
> > (musb_vbus_show+0x58/0xe4)
> > [] (musb_vbus_show) from [] (dev_attr_show+0x20/0x44)
> > [] (dev_attr_show) from [] (sysfs_kf_seq_show+0x80/0xdc)
> > [] (sysfs_kf_seq_show) from [] (seq_read+0x250/0x448)
> > [] (seq_read) from [] (__vfs_read+0x1c/0x118)
> > [] (__vfs_read) from [] (vfs_read+0x90/0x144)
> > [] (vfs_read) from [] (SyS_read+0x3c/0x74)
> > [] (SyS_read) from [] (ret_fast_syscall+0x0/0x54)
> > 
> > Solution was suggested by Tony Lindgren .
> > 
> > Signed-off-by: Merlijn Wajer 
> 
> Thanks for fixing this. Assuming it passes Bin's tests:
> 
> Acked-by: Tony Lindgren 

Applied and sent out. Thanks.

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

[PATCH] usb: musb: call pm_runtime_{get,put}_sync before reading vbus registers

[Bin Liu]

From: Merlijn Wajer 

Without pm_runtime_{get,put}_sync calls in place, reading
vbus status via /sys causes the following error:

Unhandled fault: external abort on non-linefetch (0x1028) at 0xfa0ab060
pgd = b333e822
[fa0ab060] *pgd=48011452(bad)

[] (musb_default_readb) from [] (musb_vbus_show+0x58/0xe4)
[] (musb_vbus_show) from [] (dev_attr_show+0x20/0x44)
[] (dev_attr_show) from [] (sysfs_kf_seq_show+0x80/0xdc)
[] (sysfs_kf_seq_show) from [] (seq_read+0x250/0x448)
[] (seq_read) from [] (__vfs_read+0x1c/0x118)
[] (__vfs_read) from [] (vfs_read+0x90/0x144)
[] (vfs_read) from [] (SyS_read+0x3c/0x74)
[] (SyS_read) from [] (ret_fast_syscall+0x0/0x54)

Solution was suggested by Tony Lindgren .

Signed-off-by: Merlijn Wajer 
Acked-by: Tony Lindgren 
Signed-off-by: Bin Liu 
---
 drivers/usb/musb/musb_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index eef4ad578b31..c344ef4e5355 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1756,6 +1756,7 @@ int musb_mailbox(enum musb_vbus_id_status status)
int vbus;
u8  devctl;
 
+   pm_runtime_get_sync(dev);
spin_lock_irqsave(>lock, flags);
val = musb->a_wait_bcon;
vbus = musb_platform_get_vbus_status(musb);
@@ -1769,6 +1770,7 @@ int musb_mailbox(enum musb_vbus_id_status status)
vbus = 0;
}
spin_unlock_irqrestore(>lock, flags);
+   pm_runtime_put_sync(dev);
 
return sprintf(buf, "Vbus %s, timeout %lu msec\n",
vbus ? "on" : "off", val);
-- 
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] musb fixes for v4.16-rc5

[Bin Liu]

Hi Greg,

Here is only musb patch for v4.16-rc5, which fixes the clk gating problem when
read a sysfs entry. Please let me know if any change is needed.

Regards,
-Bin.
---

Merlijn Wajer (1):
  usb: musb: call pm_runtime_{get,put}_sync before reading vbus registers

 drivers/usb/musb/musb_core.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
1.9.1

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

Re: [RFC PATCH] cdc-acm: do not drop data from fast devices

[Oliver Neukum]

On Mon, 2018-03-05 at 10:55 +0100, Romain Izard wrote:

> The TTY buffer is 4096 bytes large, throttling when there are only 128
> free bytes left, and unthrottling when there are only 128 bytes available.
> But the TTY buffer is filled from an intermediate flip buffer that
> contains up to 64 KiB of data, and each time unthrottle() is called 16
> URBs are scheduled by the driver, sending up to 16 KiB to the flip buffer.
> As the result of tty_insert_flip_string() is not checked in the URB
> reception callback, data can be lost when the flip buffer is filled faster
> than the TTY is emptied.

It seems to me that the problem is in the concept here. If you cannot
take all data, you should tell the lower layer how much data you can
take.

> Moreover, as the URB callbacks are called from a tasklet context, whereas
> throttling is called from the system workqueue, it is possible for the
> throttling to be delayed by high traffic in the tasklet. As a result,
> completed URBs can be resubmitted even if the flip buffer is full, and we
> request more data from the device only to drop it immediately.

That points to a deficiency with how we throttle. Maybe we should poison
URBs upon throttle() being called?

> To prevent this problem, the URBs whose data did not reach the flip buffer
> are placed in a waiting list, which is only processed when the serial port
> is unthrottled.

So we introduce yet another buffer? That does look like we are papering
over a design problem.


> This is working when using the normal line discipline on ttyACM. But
> there is a big hole in this: other line disciplines do not use throttling
> and thus unthrottle is never called. The URBs will never get resubmitted,

Now that is a real problem. This introduces a regression.

> and the port is dead. Unfortunately, there is no notification when the
> flip buffer is ready to receive new data, so the only alternative would
> be to schedule a timer polling the flip buffer. But with an additional
> asynchronous process in the mix, the code starts to be very brittle.

Well, no. This tells us something is broken in the tty layer.

> I believe this issue is not limited to ttyACM. As the TTY layer is not
> performance-oriented, it can be easy to overwhelm devices with a low
> available processing power. In my case, a modem sending a sustained 2 MB/s
> on a debug port (exported as a CDC-ACM port) was enough to trigger the
> issue. The code handling the CDC-ACM class and the generic USB serial port
> is very similar when it comes to URB handling, so all drivers that rely on
> that code have the same issue.
> 
> But in general, it seems to me that there is no code in the kernel that
> checks the return value of tty_insert_flip_string(). This means that we
> are working with the assumption that the kernel will consume the data
> faster than the source can send it, or that the upper layer will be
> able or willing to throttle it fast enough to avoid losing data.

Yes. And if the assumption is false, you need to go for the tty layer.
I am sorry, but NACK.

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 v6] staging: typec: handle vendor defined part and modify drp toggling flow

[李書帆]

Hi,

2018-03-05 22:49 GMT+08:00 Guenter Roeck :
> On 03/04/2018 08:16 PM, ShuFan Lee wrote:
>>
>> From: ShuFan Lee 
>>
>> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn,
>> tcpci_start_drp_toggling
>> and export tcpci_irq. More operations can be extended in tcpci_data if
>> needed.
>> According to TCPCI specification, 4.4.5.2 ROLE_CONTROL,
>> TCPC shall not start DRP toggling until subsequently the TCPM
>> writes to the COMMAND register to start DRP toggling.
>> DRP toggling flow is chagned as following:
>
>
> s/chagned/changed/
Sorry for the type error, I'll correct it in v7.
>
>
>>- Write DRP = 1, Rp level and RC.CCx to Rd/Rd or Rp/Rp
>>- Set LOOK4CONNECTION command
>>
>> Signed-off-by: ShuFan Lee 
>> ---
>>   drivers/staging/typec/tcpci.c | 134
>> ++
>>   drivers/staging/typec/tcpci.h |  15 +
>>   2 files changed, 123 insertions(+), 26 deletions(-)
>>
>>   patch changelogs between v1 & v2
>>   - Remove unnecessary i2c_client in the structure of tcpci
>>   - Rename structure of tcpci_vendor_data to tcpci_data
>>   - Not exporting tcpci read/write wrappers but register i2c regmap in
>> glue driver
>>   - Add set_vconn ops in tcpci_data
>> (It is necessary for RT1711H to enable/disable idle mode before
>> disabling/enabling vconn)
>>   - Export tcpci_irq so that vendor can call it in their own IRQ handler
>>
>>   patch changelogs between v2 & v3
>>   - Change the return type of tcpci_irq from int to irqreturn_t
>>
>>   patch changelogs between v3 & v4
>>   - Directly return regmap_write at the end of drp_toggling function
>>   - Because tcpci_irq is called in _tcpci_irq, move _tcpci_irq to the
>> place after tcpci_irq
>>
>>   patch changelogs between v4 & v5
>>   - Add a space between my first & last name, from ShuFanLee to ShuFan
>> Lee.
>>
>>   patch changelogs between v5 & v6
>>   - Add start_drp_toggling in tcpci_data and call it at the beginning of
>> tcpci_start_drp_toggling
>>   - Modify the flow of tcpci_start_drp_toggling as following
>>  - Set Rp level, RC.CCx to Rd/Rd or Rp/Rp and DRP = 1
>>  - Set LOOK4CONNECTION command
>>
>> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
>> index 9bd4412..9e600f7 100644
>> --- a/drivers/staging/typec/tcpci.c
>> +++ b/drivers/staging/typec/tcpci.c
>> @@ -21,7 +21,6 @@
>> struct tcpci {
>> struct device *dev;
>> -   struct i2c_client *client;
>> struct tcpm_port *port;
>>   @@ -30,6 +29,12 @@ struct tcpci {
>> bool controls_vbus;
>> struct tcpc_dev tcpc;
>> +   struct tcpci_data *data;
>> +};
>> +
>> +struct tcpci_chip {
>> +   struct tcpci *tcpci;
>> +   struct tcpci_data data;
>>   };
>> static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
>> @@ -37,8 +42,7 @@ 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,
>> -   u16 *val)
>> +static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
>>   {
>> return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
>>   }
>> @@ -98,9 +102,19 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum
>> typec_cc_status cc)
>>   static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>> enum typec_cc_status cc)
>>   {
>> +   int ret;
>> struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>> unsigned int reg = TCPC_ROLE_CTRL_DRP;
>>   + if (tcpci->data) {
>> +   if (tcpci->data->start_drp_toggling) {
>
>
> From the code flow it is guaranteed that ->data is set. It should therefore
> be unnecessary to check for it (we don't check if ->reg is set either).
Ok, I'll modify it in v7, thank you.
>
>
>> +   ret = tcpci->data->start_drp_toggling(tcpci,
>> + tcpci->data,
>> cc);
>> +   if (ret < 0)
>> +   return ret;
>> +   }
>> +   }
>> +
>> switch (cc) {
>> default:
>> case TYPEC_CC_RP_DEF:
>> @@ -117,7 +131,17 @@ static int tcpci_start_drp_toggling(struct tcpc_dev
>> *tcpc,
>> break;
>> }
>>   - return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
>> +   if (cc == TYPEC_CC_RD)
>> +   reg |= (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT)
>> |
>> +  (TCPC_ROLE_CTRL_CC_RD <<
>> TCPC_ROLE_CTRL_CC2_SHIFT);
>> +   else
>> +   reg |= (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT)
>> |
>> +  (TCPC_ROLE_CTRL_CC_RP <<
>> TCPC_ROLE_CTRL_CC2_SHIFT);
>> +   ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
>> +   if (ret < 0)
>> +   return 

Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

[Christoph Hellwig]

On Sat, Mar 03, 2018 at 07:19:06PM +0100, Fredrik Noring wrote:
> Christoph, Alan,
> 
> > If it is allocating / freeing this memory all the time in the hot path
> > it should really use a dma pool (see include/ilinux/dmapool.h).
> > The dma coherent APIs aren't really built for being called in the
> > hot path.
> 
> hcd_buffer_free uses a combination of dma pools and dma coherent APIs:
> 
>   ...
>   for (i = 0; i < HCD_BUFFER_POOLS; i++) {
>   if (size <= pool_max[i]) {
>   dma_pool_free(hcd->pool[i], addr, dma);
>   return;
>   }
>   }
>   dma_free_coherent(hcd->self.sysdev, size, addr, dma);
> 
> Alan, can dma_free_coherent be delayed to a point when IRQs are enabled?

The point is that you should always use a pool, period.
dma_alloc*/dma_free* are fundamentally expensive operations on my
architectures, so if you call them from a fast path you are doing
something wrong.
--
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 v6] staging: typec: handle vendor defined part and modify drp toggling flow

[Guenter Roeck]

On 03/04/2018 08:16 PM, ShuFan Lee wrote:

From: ShuFan Lee 

Handle vendor defined behavior in tcpci_init, tcpci_set_vconn, 
tcpci_start_drp_toggling
and export tcpci_irq. More operations can be extended in tcpci_data if needed.
According to TCPCI specification, 4.4.5.2 ROLE_CONTROL,
TCPC shall not start DRP toggling until subsequently the TCPM
writes to the COMMAND register to start DRP toggling.
DRP toggling flow is chagned as following:


s/chagned/changed/


   - Write DRP = 1, Rp level and RC.CCx to Rd/Rd or Rp/Rp
   - Set LOOK4CONNECTION command

Signed-off-by: ShuFan Lee 
---
  drivers/staging/typec/tcpci.c | 134 ++
  drivers/staging/typec/tcpci.h |  15 +
  2 files changed, 123 insertions(+), 26 deletions(-)

  patch changelogs between v1 & v2
  - Remove unnecessary i2c_client in the structure of tcpci
  - Rename structure of tcpci_vendor_data to tcpci_data
  - Not exporting tcpci read/write wrappers but register i2c regmap in glue 
driver
  - Add set_vconn ops in tcpci_data
(It is necessary for RT1711H to enable/disable idle mode before 
disabling/enabling vconn)
  - Export tcpci_irq so that vendor can call it in their own IRQ handler

  patch changelogs between v2 & v3
  - Change the return type of tcpci_irq from int to irqreturn_t

  patch changelogs between v3 & v4
  - Directly return regmap_write at the end of drp_toggling function
  - Because tcpci_irq is called in _tcpci_irq, move _tcpci_irq to the place 
after tcpci_irq

  patch changelogs between v4 & v5
  - Add a space between my first & last name, from ShuFanLee to ShuFan Lee.

  patch changelogs between v5 & v6
  - Add start_drp_toggling in tcpci_data and call it at the beginning of 
tcpci_start_drp_toggling
  - Modify the flow of tcpci_start_drp_toggling as following
 - Set Rp level, RC.CCx to Rd/Rd or Rp/Rp and DRP = 1
 - Set LOOK4CONNECTION command

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 9bd4412..9e600f7 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -21,7 +21,6 @@
  
  struct tcpci {

struct device *dev;
-   struct i2c_client *client;
  
  	struct tcpm_port *port;
  
@@ -30,6 +29,12 @@ struct tcpci {

bool controls_vbus;
  
  	struct tcpc_dev tcpc;

+   struct tcpci_data *data;
+};
+
+struct tcpci_chip {
+   struct tcpci *tcpci;
+   struct tcpci_data data;
  };
  
  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)

@@ -37,8 +42,7 @@ 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,

-   u16 *val)
+static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
  {
return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
  }
@@ -98,9 +102,19 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum 
typec_cc_status cc)
  static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
enum typec_cc_status cc)
  {
+   int ret;
struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
unsigned int reg = TCPC_ROLE_CTRL_DRP;
  
+	if (tcpci->data) {

+   if (tcpci->data->start_drp_toggling) {


From the code flow it is guaranteed that ->data is set. It should therefore
be unnecessary to check for it (we don't check if ->reg is set either).


+   ret = tcpci->data->start_drp_toggling(tcpci,
+ tcpci->data, cc);
+   if (ret < 0)
+   return ret;
+   }
+   }
+
switch (cc) {
default:
case TYPEC_CC_RP_DEF:
@@ -117,7 +131,17 @@ static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
break;
}
  
-	return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);

+   if (cc == TYPEC_CC_RD)
+   reg |= (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
+  (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
+   else
+   reg |= (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
+  (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT);
+   ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
+   if (ret < 0)
+   return ret;
+   return regmap_write(tcpci->regmap, TCPC_COMMAND,
+   TCPC_CMD_LOOK4CONNECTION);
  }
  
  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)

@@ -178,6 +202,16 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool 
enable)
struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
int ret;
  
+	/* Handle vendor set vconn */

+   if (tcpci->data) {
+   if (tcpci->data->set_vconn) {
+   ret = 

Re: Fwd: Re: Bug 198731 "USB devices not seen with newest kernel"

[Mathias Nyman]

On 26.02.2018 16:05, Oliver Neukum wrote:

Am Dienstag, den 13.02.2018, 13:56 -0800 schrieb The Real Bev:

I went back to the distribution kernel for 64-bit 14.2 slackware --
4.14.18. During boot, dmesg displays the following errors:

[   15.850711] xhci_hcd :09:00.0: not ready 4195ms after FLR; waiting
[   20.458401] xhci_hcd :09:00.0: not ready 8291ms after FLR; waiting
[   29.161245] xhci_hcd :09:00.0: not ready 16483ms after FLR; waiting
[   46.058397] xhci_hcd :09:00.0: not ready 32867ms after FLR; waiting
[   82.922254] xhci_hcd :09:00.0: not ready 65635ms after FLR; giving up
[   83.261301] xhci_hcd :09:00.0: Refused to change power state,
currently in D3
[   83.352431] xhci_hcd :09:00.0: xHCI Host Controller
[   83.387291] xhci_hcd :09:00.0: new USB bus registered, assigned
bus number 11
[   83.699853] xhci_hcd :09:00.0: Host halt failed, -19
[   83.731151] xhci_hcd :09:00.0: can't setup: -19
[   83.764138] xhci_hcd :09:00.0: USB bus 11 deregistered
[   83.821909] xhci_hcd :09:00.0: init :09:00.0 fail, -19

This is the same problem I've had for 4.9.79, 4.15.2 and now 4.14.18.
There is clearly a problem.  I give up.  I'm just going to live with it.


Hi Mathias,

it looks like a relatively old issue with D3 on some instances of XHCI.



Yep, The "Host halt failed, -19" (ENODEV) is seen when xhci driver tries to
halt the host during setup, polling a mmio register but reading only 0x.

Which I guess is expected if controller is still on D3.

No idea why this controller fails to change from D3 to D0.
Any PCI changes betewen 4.8 and 4.9?

-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 03/12] staging: typec: tcpci: support port config passed via dt

[Jun Li]

> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: 2018年3月5日 19:30
> To: Jun Li 
> Cc: gre...@linuxfoundation.org; robh...@kernel.org; li...@roeck-us.net;
> a.ha...@samsung.com; mark.rutl...@arm.com; yue...@google.com;
> Peter Chen ; garsi...@embeddedor.com;
> o_leve...@orange.fr; shufan_...@richtek.com; linux-usb@vger.kernel.org;
> devicet...@vger.kernel.org; dl-linux-imx 
> Subject: Re: [PATCH v2 03/12] staging: typec: tcpci: support port config
> passed via dt
> 
> Hi,
> 
> On Mon, Mar 05, 2018 at 10:35:07AM +, Jun Li wrote:
> > > So it actually does make sense to define those properties for the
> > > "connector" node instead of TCPC parent. They are generic "Type-C"
> > > properties (right?), so we may want to use them with multiport
> > > devices as well.
> > >
> >
> > Yes, that's the idea of my v2, I will keep this but via fwnode_property*.
> 
> Cool. While at it, can you also add a patch to this series where the fwnode is
> bind to the port? Something like this:

OK, I will add a patch for this.

> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412356c9..ac4e7605f9d5 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -452,6 +452,8 @@ static int tcpci_probe(struct i2c_client *client,
> if (IS_ERR(tcpci->regmap))
> return PTR_ERR(tcpci->regmap);
> 
> +   tcpci->tcpc.fwnode =
> device_get_named_child_node(>dev,
> + "connector");
> +
> tcpci->tcpc.init = tcpci_init;
> tcpci->tcpc.get_vbus = tcpci_get_vbus;
> tcpci->tcpc.set_vbus = tcpci_set_vbus; diff --git
> a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index
> f4d563ee7690..68a0ead400c0 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -3729,6 +3729,7 @@ struct tcpm_port *tcpm_register_port(struct
> device *dev, struct tcpc_dev *tcpc)
> else
> port->try_role = TYPEC_NO_PREFERRED_ROLE;
> 
> +   port->typec_caps.fwnode = tcpc->fwnode;
> port->typec_caps.prefer_role = tcpc->config->default_role;
> port->typec_caps.type = tcpc->config->type;
> port->typec_caps.revision = 0x0120; /* Type-C spec release
> 1.2 */
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
> ca1c0b57f03f..a25ebfea054d 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -127,6 +127,7 @@ struct tcpc_mux_dev {
>  /**
>   * struct tcpc_dev - Port configuration and callback functions
>   * @config:Pointer to port configuration
> + * @fwnode:Pointer to port fwnode
>   * @get_vbus:  Called to read current VBUS state
>   * @get_current_limit:
>   * Optional; called by the tcpm core when configured as a
> snk
> @@ -155,6 +156,7 @@ struct tcpc_mux_dev {
>   */
>  struct tcpc_dev {
> const struct tcpc_config *config;
> +   struct fwnode_handle *fwnode;
> 
> int (*init)(struct tcpc_dev *dev);
> int (*get_vbus)(struct tcpc_dev *dev);
> 
> 
> Thanks,
> 
> --
> heikki

RE: [PATCH v2 10/12] dt-bindings: connector: add properties for typec power delivery

[Jun Li]

> -Original Message-
> From: Jun Li
> Sent: 2018年3月5日 15:52
> To: Rob Herring ; Andrzej Hajda 
> Cc: gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com;
> li...@roeck-us.net; mark.rutl...@arm.com; yue...@google.com; Peter
> Chen ; garsi...@embeddedor.com;
> o_leve...@orange.fr; shufan_...@richtek.com; linux-usb@vger.kernel.org;
> devicet...@vger.kernel.org; dl-linux-imx 
> Subject: RE: [PATCH v2 10/12] dt-bindings: connector: add properties for
> typec power delivery
> 
> 
> > -Original Message-
> > From: Rob Herring [mailto:r...@kernel.org]
> > Sent: 2018年3月3日 6:39
> > To: Andrzej Hajda 
> > Cc: Jun Li ; gre...@linuxfoundation.org;
> > heikki.kroge...@linux.intel.com; li...@roeck-us.net;
> > mark.rutl...@arm.com; yue...@google.com; Peter Chen
> > ; garsi...@embeddedor.com;
> o_leve...@orange.fr;
> > shufan_...@richtek.com; linux-usb@vger.kernel.org;
> > devicet...@vger.kernel.org; dl-linux-imx 
> > Subject: Re: [PATCH v2 10/12] dt-bindings: connector: add properties
> > for typec power delivery
> >
> > On Tue, Feb 27, 2018 at 09:41:19AM +0100, Andrzej Hajda wrote:
> > > On 26.02.2018 12:49, Li Jun wrote:
> > > > In case of usb-c-connector with power delivery support, add
> > > > bingdings supported by current typec driver, so user can pass all
> > > > those properties via dt.
> > > >
> > > > Signed-off-by: Li Jun 
> > > > ---
> > > > Changes for v2:
> > > > - Added typec properties are based on general usb connector
> bindings[1]
> > > >   proposed by Andrzej Hajda.
> > > > - Use the standard unit suffixes as defined in property-units.txt.
> > > >
> > > > [1]
> > > >
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fp
> > > >
> >
> atchwork.kernel.org%2Fpatch%2F10231447%2F=02%7C01%7Cjun.li%
> > 40nx
> > > >
> >
> p.com%7Ccea32589f3314488e18f08d5808e5aae%7C686ea1d3bc2b4c6fa92
> > cd99c5
> > > >
> >
> c301635%7C0%7C1%7C636556271266469012=ANr1jeW8x8cy6CG6tz
> > ACgj%2B
> > > > FgNKl9DJbRpervFwaQKM%3D=0
> > > >
> > > >  .../bindings/connector/usb-connector.txt   | 43
> > ++
> > > >  1 file changed, 43 insertions(+)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > > b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > > index e1463f1..242f6df 100644
> > > > ---
> > > > a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > > +++
> b/Documentation/devicetree/bindings/connector/usb-connector.tx
> > > > +++ t
> > > > @@ -15,6 +15,30 @@ Optional properties:
> > > >  - type: size of the connector, should be specified in case of
> > > > USB-A,
> > USB-B
> > > >non-fullsize connectors: "mini", "micro".
> > > >
> > > > +Required properties for usb-c-connector with power delivery support:
> > > > +- port-type: should be one of "source", "sink" or "dual".
> > > > +- default-role: preferred power role if port-type is "dual"(drp),
> > > > +should be
> > > > +  "sink" or "source".
> > >
> > > Since port carries data and power, it would be better to explicitly
> > > mention it in names of properties which can be ambiguous.
> > > Maybe instead of 'port-type' you can use 'power-role', for example.
> > > Other thing is that default-role is required only in case of DRP, so
> > > maybe better would be to squash it in power-role, for example:
> > >     power-role: should be on of "source", "sink", "dual-source",
> > > "dual-sink", in case of dual roles suffix determines preferred role.
> > >
> > >
> > > > +- src-pdos: An array of u32 with each entry providing supported
> > > > +power
> > > > +  source data object(PDO), the detailed bit definitions of PDO
> > > > +can be found
> > > > +  in "Universal Serial Bus Power Delivery Specification" chapter
> > > > +6.4.1.2
> > > > +  Source_Capabilities Message, the order of each entry(PDO)
> > > > +should follow
> > > > +  the PD spec chapter 6.4.1. Required for power source and power
> > > > +dual
> > role.
> > > > +- snk-pdos: An array of u32 with each entry providing supported
> > > > +power
> > > > +  sink data object(PDO), the detailed bit definitions of PDO can
> > > > +be found in
> > > > +  "Universal Serial Bus Power Delivery Specification" chapter
> > > > +6.4.1.3 Sink
> > > > +  Capabilities Message, the order of each entry(PDO) should
> > > > +follow the PD
> > > > +  spec chapter 6.4.1. Required for power sink and power dual role.
> > >
> > > We should avoid magic numbers, magic numbers are bad :)
> >
> > I don't mind if there's a defined format for the data.
> >
> 
> Yes, there is defined format in spec for this data.
> 
> > > If we really need to use PDOs here, I think we can re-use PDO_*
> > > macros [1] - DTC should be able to parse it, maybe some lifting will be
> necessary.
> > >
> > > [1]:
> > >
> >
> 

Re: inconsistent lock state with usbnet/asix usb ethernet and xhci

[Oliver Neukum]

On Mon, 2018-03-05 at 08:45 +0100, Marek Szyprowski wrote:
> Hi Oliver,
> 
> On 2018-02-27 17:07, Oliver Neukum wrote:
> > Am Dienstag, den 27.02.2018, 07:13 -0800 schrieb Eric Dumazet:
> >> On Tue, 2018-02-27 at 07:09 -0800, Eric Dumazet wrote:
> >>>
> >>> Note that for this one, it seems we also could perform stats updates in
> >>> BH context, since skb is queued via defer_bh()
> >>>
> >>> But simplicity wins I guess.
> >> Thinking more about this, I am not sure we have any guarantee that TX
> >> and RX can not run on multiple cpus.
> >>
> >> Using an unique syncp is not going to be safe, even if we make lockdep
> >> happy enough with the local_irq save/restore.
> > Unfortunately you are right. It is not guaranteed for some hardware.
> 
> Does it mean that the fix proposed by Eric is not the proper solution?

For asix it should work, but asix is unlikely to be the only driver
with that issue. 32 bit recieves less testing nowadays.

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/12] staging: typec: tcpci: support port config passed via dt

[Heikki Krogerus]

Hi,

On Mon, Mar 05, 2018 at 10:35:07AM +, Jun Li wrote:
> > So it actually does make sense to define those properties for the
> > "connector" node instead of TCPC parent. They are generic "Type-C"
> > properties (right?), so we may want to use them with multiport devices as
> > well.
> > 
> 
> Yes, that's the idea of my v2, I will keep this but via fwnode_property*.

Cool. While at it, can you also add a patch to this series where the
fwnode is bind to the port? Something like this:

diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
index 9bd4412356c9..ac4e7605f9d5 100644
--- a/drivers/staging/typec/tcpci.c
+++ b/drivers/staging/typec/tcpci.c
@@ -452,6 +452,8 @@ static int tcpci_probe(struct i2c_client *client,
if (IS_ERR(tcpci->regmap))
return PTR_ERR(tcpci->regmap);

+   tcpci->tcpc.fwnode = device_get_named_child_node(>dev, 
"connector");
+
tcpci->tcpc.init = tcpci_init;
tcpci->tcpc.get_vbus = tcpci_get_vbus;
tcpci->tcpc.set_vbus = tcpci_set_vbus;
diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index f4d563ee7690..68a0ead400c0 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -3729,6 +3729,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, 
struct tcpc_dev *tcpc)
else
port->try_role = TYPEC_NO_PREFERRED_ROLE;

+   port->typec_caps.fwnode = tcpc->fwnode;
port->typec_caps.prefer_role = tcpc->config->default_role;
port->typec_caps.type = tcpc->config->type;
port->typec_caps.revision = 0x0120; /* Type-C spec release 1.2 */
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index ca1c0b57f03f..a25ebfea054d 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -127,6 +127,7 @@ struct tcpc_mux_dev {
 /**
  * struct tcpc_dev - Port configuration and callback functions
  * @config:Pointer to port configuration
+ * @fwnode:Pointer to port fwnode
  * @get_vbus:  Called to read current VBUS state
  * @get_current_limit:
  * Optional; called by the tcpm core when configured as a snk
@@ -155,6 +156,7 @@ struct tcpc_mux_dev {
  */
 struct tcpc_dev {
const struct tcpc_config *config;
+   struct fwnode_handle *fwnode;

int (*init)(struct tcpc_dev *dev);
int (*get_vbus)(struct tcpc_dev *dev);


Thanks,

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

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>  void dwc3_gadget_exit(struct dwc3 *dwc)
>  {
> +  int epnum;
> +  unsigned long flags;
> +
> +  spin_lock_irqsave(>lock, flags);
> +  for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
> +  struct dwc3_ep  *dep = dwc->eps[epnum];
> +
> +  if (!dep)
> +  continue;
> +
> +  dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
> +  }
> +  spin_unlock_irqrestore(>lock, flags);
> +
>usb_del_gadget_udc(>gadget);
>dwc3_gadget_free_endpoints(dwc);

 free endpoints is a better place for this. It's already going to free
 the memory anyway. Might as well clear all flags to 0 there.

>>>
>>> But it won't solve the deadlock issue. Since 
>>> dwc3_gadget_free_endpoints()
>>> is called after usb_del_gadget_udc() and the deadlock happens when
>>>
>>> usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()
>>>
>>> and DWC3_EP_END_TRANSFER_PENDING flag is set.
>>
>> indeed. Iterating twice over the entire endpoint list seems
>> wasteful. Perhaps we just shouldn't wait when removing the UDC since
>> that's essentially what this patch will do, right? If you clear the flag
>> before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
>> will do nothing. Might as well remove it.
>>
>
> This means that we will never wait for DWC3_EP_END_TRANSFER_PENDING to 
> clear
> in dwc3_gadget_stop() like we used to. This is perfectly fine, right?
>
> It makes sense to me as dwc3_gadget_stop() calls __dwc3_gadget_stop() 
> which
> masks all interrupts and nobody will ever clear that flag if it was set.

 I don't think so. It can not mask the endpoint events, please check
 the events which will be masked in DEVTEN register. The reason why we
 should wait for DWC3_EP_END_TRANSFER_PENDING to clear is that,
 sometimes the DWC3_DEPEVT_EPCMDCMPLT event will be triggered later
 than 100us, but now we may have freed the gadget irq which will cause
 crash.
>>>
>>> We could mask command complete events as soon as ->udc_stop() is called,
>>> right? Hmm, actually, __dwc3_gadget_stop() already clears DEVTEN
>>> completely.
>>
>> But which bit in DEVTEN says Endpoint events are disabled?
>
> When we set up the DWC3_DEPCMD_ENDTRANSFER command in
> dwc3_stop_active_transfer(), we can do not set DWC3_DEPCMD_CMDIOC,
> then there will no endpoint command complete interrupts I think.
>
> cmd |= DWC3_DEPCMD_CMDIOC;

I remember some part of the databook mandating CMDIOC to be set. We
could test it out without and see if anything blows up. I would,
however, require a lengthy comment explaining that we're deviating from
databook revision x.yya, section foobar because $reasons. :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> On 05/03/18 13:06, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Baolin Wang  writes:
> Roger Quadros  writes:
>>> Roger Quadros  writes:
 In the following test we get stuck by sleeping forever in 
 _dwc3_set_mode()
 after which dual-role switching doesn't work.

 On dra7-evm's dual-role port,
 - Load g_zero gadget driver and enumerate to host
 - suspend to mem
 - disconnect USB cable to host and connect otg cable with Pen drive in 
 it.
 - resume system
 - we sleep indefinitely in _dwc3_set_mode due to.
   dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()->
dwc3_gadget_stop()->wait_event_lock_irq()

 Let's clear the DWC3_EP_END_TRANSFER_PENDING flag on all endpoints
 so we don't wait in dwc3_gadget_stop().

 Signed-off-by: Roger Quadros 
 ---
  drivers/usb/dwc3/gadget.c | 14 ++
  1 file changed, 14 insertions(+)

 diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
 index 2bda4eb..0a360da 100644
 --- a/drivers/usb/dwc3/gadget.c
 +++ b/drivers/usb/dwc3/gadget.c
 @@ -3273,6 +3273,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)

  void dwc3_gadget_exit(struct dwc3 *dwc)
  {
 +  int epnum;
 +  unsigned long flags;
 +
 +  spin_lock_irqsave(>lock, flags);
 +  for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
 +  struct dwc3_ep  *dep = dwc->eps[epnum];
 +
 +  if (!dep)
 +  continue;
 +
 +  dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 +  }
 +  spin_unlock_irqrestore(>lock, flags);
 +
usb_del_gadget_udc(>gadget);
dwc3_gadget_free_endpoints(dwc);
>>>
>>> free endpoints is a better place for this. It's already going to free
>>> the memory anyway. Might as well clear all flags to 0 there.
>>>
>>
>> But it won't solve the deadlock issue. Since dwc3_gadget_free_endpoints()
>> is called after usb_del_gadget_udc() and the deadlock happens when
>>
>> usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()
>>
>> and DWC3_EP_END_TRANSFER_PENDING flag is set.
>
> indeed. Iterating twice over the entire endpoint list seems
> wasteful. Perhaps we just shouldn't wait when removing the UDC since
> that's essentially what this patch will do, right? If you clear the flag
> before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
> will do nothing. Might as well remove it.
>

 This means that we will never wait for DWC3_EP_END_TRANSFER_PENDING to 
 clear
 in dwc3_gadget_stop() like we used to. This is perfectly fine, right?

 It makes sense to me as dwc3_gadget_stop() calls __dwc3_gadget_stop() which
 masks all interrupts and nobody will ever clear that flag if it was set.
>>>
>>> I don't think so. It can not mask the endpoint events, please check
>>> the events which will be masked in DEVTEN register. The reason why we
>>> should wait for DWC3_EP_END_TRANSFER_PENDING to clear is that,
>>> sometimes the DWC3_DEPEVT_EPCMDCMPLT event will be triggered later
>>> than 100us, but now we may have freed the gadget irq which will cause
>>> crash.
>> 
>> We could mask command complete events as soon as ->udc_stop() is called,
>> right? Hmm, actually, __dwc3_gadget_stop() already clears DEVTEN
>> completely.
>
> But which bit in DEVTEN says Endpoint events are disabled?
>
>> 
>> /me goes check databook
>> 
>> At least on revision 2.60a of the databook, bit 10 is reserved. I wonder
>> if that's the start of all the problems. Anybody has access to older and
>> newer databook revisions so we can cross-check?
>> 
>
> I can access v2.40 and v3.10 books.
>
> bit 10 is reserved on both
>
> Differences in v2.4 vs v3.10 are:
>
> bit 8 reservedvs  L1SUSPEN
> bit 13reservedvs  StopOnDisconnectEn
> bit 14reservedvs  L1WKUPEVTEN

odd, at some point we lost command complete interrupt :-(

That line exists since first commit (see below), so that would mean it
existed in 1.73a (the revision the original was written against), but
vanished on 2.40a. Perhaps 2.00a still had it.

Hey John, do you know, off the top of your head, when we lost DEVTEN[10]
as mask/unmask bit for EP Command Completion IRQs?


commit 72246da40f3719af3bfd104a2365b32537c27d83
Author: Felipe Balbi 
Date:   Fri Aug 19 18:10:58 2011 +0300

usb: Introduce DesignWare USB3 DRD Driver

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Baolin Wang]

On 5 March 2018 at 19:14, Roger Quadros  wrote:
> On 05/03/18 13:06, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Baolin Wang  writes:
> Roger Quadros  writes:
>>> Roger Quadros  writes:
 In the following test we get stuck by sleeping forever in 
 _dwc3_set_mode()
 after which dual-role switching doesn't work.

 On dra7-evm's dual-role port,
 - Load g_zero gadget driver and enumerate to host
 - suspend to mem
 - disconnect USB cable to host and connect otg cable with Pen drive in 
 it.
 - resume system
 - we sleep indefinitely in _dwc3_set_mode due to.
   dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()->
dwc3_gadget_stop()->wait_event_lock_irq()

 Let's clear the DWC3_EP_END_TRANSFER_PENDING flag on all endpoints
 so we don't wait in dwc3_gadget_stop().

 Signed-off-by: Roger Quadros 
 ---
  drivers/usb/dwc3/gadget.c | 14 ++
  1 file changed, 14 insertions(+)

 diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
 index 2bda4eb..0a360da 100644
 --- a/drivers/usb/dwc3/gadget.c
 +++ b/drivers/usb/dwc3/gadget.c
 @@ -3273,6 +3273,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)

  void dwc3_gadget_exit(struct dwc3 *dwc)
  {
 +  int epnum;
 +  unsigned long flags;
 +
 +  spin_lock_irqsave(>lock, flags);
 +  for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
 +  struct dwc3_ep  *dep = dwc->eps[epnum];
 +
 +  if (!dep)
 +  continue;
 +
 +  dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 +  }
 +  spin_unlock_irqrestore(>lock, flags);
 +
usb_del_gadget_udc(>gadget);
dwc3_gadget_free_endpoints(dwc);
>>>
>>> free endpoints is a better place for this. It's already going to free
>>> the memory anyway. Might as well clear all flags to 0 there.
>>>
>>
>> But it won't solve the deadlock issue. Since dwc3_gadget_free_endpoints()
>> is called after usb_del_gadget_udc() and the deadlock happens when
>>
>> usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()
>>
>> and DWC3_EP_END_TRANSFER_PENDING flag is set.
>
> indeed. Iterating twice over the entire endpoint list seems
> wasteful. Perhaps we just shouldn't wait when removing the UDC since
> that's essentially what this patch will do, right? If you clear the flag
> before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
> will do nothing. Might as well remove it.
>

 This means that we will never wait for DWC3_EP_END_TRANSFER_PENDING to 
 clear
 in dwc3_gadget_stop() like we used to. This is perfectly fine, right?

 It makes sense to me as dwc3_gadget_stop() calls __dwc3_gadget_stop() which
 masks all interrupts and nobody will ever clear that flag if it was set.
>>>
>>> I don't think so. It can not mask the endpoint events, please check
>>> the events which will be masked in DEVTEN register. The reason why we
>>> should wait for DWC3_EP_END_TRANSFER_PENDING to clear is that,
>>> sometimes the DWC3_DEPEVT_EPCMDCMPLT event will be triggered later
>>> than 100us, but now we may have freed the gadget irq which will cause
>>> crash.
>>
>> We could mask command complete events as soon as ->udc_stop() is called,
>> right? Hmm, actually, __dwc3_gadget_stop() already clears DEVTEN
>> completely.
>
> But which bit in DEVTEN says Endpoint events are disabled?

When we set up the DWC3_DEPCMD_ENDTRANSFER command in
dwc3_stop_active_transfer(), we can do not set DWC3_DEPCMD_CMDIOC,
then there will no endpoint command complete interrupts I think.

cmd |= DWC3_DEPCMD_CMDIOC;

>
>>
>> /me goes check databook
>>
>> At least on revision 2.60a of the databook, bit 10 is reserved. I wonder
>> if that's the start of all the problems. Anybody has access to older and
>> newer databook revisions so we can cross-check?
>>
>
> I can access v2.40 and v3.10 books.
>
> bit 10 is reserved on both
>
> Differences in v2.4 vs v3.10 are:
>
> bit 8   reservedvs  L1SUSPEN
> bit 13  reservedvs  StopOnDisconnectEn
> bit 14  reservedvs  L1WKUPEVTEN
>
> --
> cheers,
> -roger
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki



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

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Roger Quadros]

On 05/03/18 13:06, Felipe Balbi wrote:
> 
> Hi,
> 
> Baolin Wang  writes:
 Roger Quadros  writes:
>> Roger Quadros  writes:
>>> In the following test we get stuck by sleeping forever in 
>>> _dwc3_set_mode()
>>> after which dual-role switching doesn't work.
>>>
>>> On dra7-evm's dual-role port,
>>> - Load g_zero gadget driver and enumerate to host
>>> - suspend to mem
>>> - disconnect USB cable to host and connect otg cable with Pen drive in 
>>> it.
>>> - resume system
>>> - we sleep indefinitely in _dwc3_set_mode due to.
>>>   dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()->
>>>dwc3_gadget_stop()->wait_event_lock_irq()
>>>
>>> Let's clear the DWC3_EP_END_TRANSFER_PENDING flag on all endpoints
>>> so we don't wait in dwc3_gadget_stop().
>>>
>>> Signed-off-by: Roger Quadros 
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 14 ++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 2bda4eb..0a360da 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -3273,6 +3273,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>>
>>>  void dwc3_gadget_exit(struct dwc3 *dwc)
>>>  {
>>> +  int epnum;
>>> +  unsigned long flags;
>>> +
>>> +  spin_lock_irqsave(>lock, flags);
>>> +  for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>>> +  struct dwc3_ep  *dep = dwc->eps[epnum];
>>> +
>>> +  if (!dep)
>>> +  continue;
>>> +
>>> +  dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
>>> +  }
>>> +  spin_unlock_irqrestore(>lock, flags);
>>> +
>>>usb_del_gadget_udc(>gadget);
>>>dwc3_gadget_free_endpoints(dwc);
>>
>> free endpoints is a better place for this. It's already going to free
>> the memory anyway. Might as well clear all flags to 0 there.
>>
>
> But it won't solve the deadlock issue. Since dwc3_gadget_free_endpoints()
> is called after usb_del_gadget_udc() and the deadlock happens when
>
> usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()
>
> and DWC3_EP_END_TRANSFER_PENDING flag is set.

 indeed. Iterating twice over the entire endpoint list seems
 wasteful. Perhaps we just shouldn't wait when removing the UDC since
 that's essentially what this patch will do, right? If you clear the flag
 before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
 will do nothing. Might as well remove it.

>>>
>>> This means that we will never wait for DWC3_EP_END_TRANSFER_PENDING to clear
>>> in dwc3_gadget_stop() like we used to. This is perfectly fine, right?
>>>
>>> It makes sense to me as dwc3_gadget_stop() calls __dwc3_gadget_stop() which
>>> masks all interrupts and nobody will ever clear that flag if it was set.
>>
>> I don't think so. It can not mask the endpoint events, please check
>> the events which will be masked in DEVTEN register. The reason why we
>> should wait for DWC3_EP_END_TRANSFER_PENDING to clear is that,
>> sometimes the DWC3_DEPEVT_EPCMDCMPLT event will be triggered later
>> than 100us, but now we may have freed the gadget irq which will cause
>> crash.
> 
> We could mask command complete events as soon as ->udc_stop() is called,
> right? Hmm, actually, __dwc3_gadget_stop() already clears DEVTEN
> completely.

But which bit in DEVTEN says Endpoint events are disabled?

> 
> /me goes check databook
> 
> At least on revision 2.60a of the databook, bit 10 is reserved. I wonder
> if that's the start of all the problems. Anybody has access to older and
> newer databook revisions so we can cross-check?
> 

I can access v2.40 and v3.10 books.

bit 10 is reserved on both

Differences in v2.4 vs v3.10 are:

bit 8   reservedvs  L1SUSPEN
bit 13  reservedvs  StopOnDisconnectEn
bit 14  reservedvs  L1WKUPEVTEN

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>>> Roger Quadros  writes:
> Roger Quadros  writes:
>> In the following test we get stuck by sleeping forever in 
>> _dwc3_set_mode()
>> after which dual-role switching doesn't work.
>>
>> On dra7-evm's dual-role port,
>> - Load g_zero gadget driver and enumerate to host
>> - suspend to mem
>> - disconnect USB cable to host and connect otg cable with Pen drive in 
>> it.
>> - resume system
>> - we sleep indefinitely in _dwc3_set_mode due to.
>>   dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()->
>>dwc3_gadget_stop()->wait_event_lock_irq()
>>
>> Let's clear the DWC3_EP_END_TRANSFER_PENDING flag on all endpoints
>> so we don't wait in dwc3_gadget_stop().
>>
>> Signed-off-by: Roger Quadros 
>> ---
>>  drivers/usb/dwc3/gadget.c | 14 ++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 2bda4eb..0a360da 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -3273,6 +3273,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>
>>  void dwc3_gadget_exit(struct dwc3 *dwc)
>>  {
>> +  int epnum;
>> +  unsigned long flags;
>> +
>> +  spin_lock_irqsave(>lock, flags);
>> +  for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>> +  struct dwc3_ep  *dep = dwc->eps[epnum];
>> +
>> +  if (!dep)
>> +  continue;
>> +
>> +  dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
>> +  }
>> +  spin_unlock_irqrestore(>lock, flags);
>> +
>>usb_del_gadget_udc(>gadget);
>>dwc3_gadget_free_endpoints(dwc);
>
> free endpoints is a better place for this. It's already going to free
> the memory anyway. Might as well clear all flags to 0 there.
>

 But it won't solve the deadlock issue. Since dwc3_gadget_free_endpoints()
 is called after usb_del_gadget_udc() and the deadlock happens when

 usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()

 and DWC3_EP_END_TRANSFER_PENDING flag is set.
>>>
>>> indeed. Iterating twice over the entire endpoint list seems
>>> wasteful. Perhaps we just shouldn't wait when removing the UDC since
>>> that's essentially what this patch will do, right? If you clear the flag
>>> before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
>>> will do nothing. Might as well remove it.
>>>
>>
>> This means that we will never wait for DWC3_EP_END_TRANSFER_PENDING to clear
>> in dwc3_gadget_stop() like we used to. This is perfectly fine, right?
>>
>> It makes sense to me as dwc3_gadget_stop() calls __dwc3_gadget_stop() which
>> masks all interrupts and nobody will ever clear that flag if it was set.
>
> I don't think so. It can not mask the endpoint events, please check
> the events which will be masked in DEVTEN register. The reason why we
> should wait for DWC3_EP_END_TRANSFER_PENDING to clear is that,
> sometimes the DWC3_DEPEVT_EPCMDCMPLT event will be triggered later
> than 100us, but now we may have freed the gadget irq which will cause
> crash.

We could mask command complete events as soon as ->udc_stop() is called,
right? Hmm, actually, __dwc3_gadget_stop() already clears DEVTEN
completely.

/me goes check databook

At least on revision 2.60a of the databook, bit 10 is reserved. I wonder
if that's the start of all the problems. Anybody has access to older and
newer databook revisions so we can cross-check?

best

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Roger Quadros]

On 05/03/18 12:41, Baolin Wang wrote:
> Hi Roger,
> 
> On 5 March 2018 at 17:45, Roger Quadros  wrote:
>> Felipe,
>>
>> On 05/03/18 10:49, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Roger Quadros  writes:
> Roger Quadros  writes:
>> In the following test we get stuck by sleeping forever in 
>> _dwc3_set_mode()
>> after which dual-role switching doesn't work.
>>
>> On dra7-evm's dual-role port,
>> - Load g_zero gadget driver and enumerate to host
>> - suspend to mem
>> - disconnect USB cable to host and connect otg cable with Pen drive in 
>> it.
>> - resume system
>> - we sleep indefinitely in _dwc3_set_mode due to.
>>   dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()->
>>dwc3_gadget_stop()->wait_event_lock_irq()
>>
>> Let's clear the DWC3_EP_END_TRANSFER_PENDING flag on all endpoints
>> so we don't wait in dwc3_gadget_stop().
>>
>> Signed-off-by: Roger Quadros 
>> ---
>>  drivers/usb/dwc3/gadget.c | 14 ++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 2bda4eb..0a360da 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -3273,6 +3273,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>
>>  void dwc3_gadget_exit(struct dwc3 *dwc)
>>  {
>> +  int epnum;
>> +  unsigned long flags;
>> +
>> +  spin_lock_irqsave(>lock, flags);
>> +  for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>> +  struct dwc3_ep  *dep = dwc->eps[epnum];
>> +
>> +  if (!dep)
>> +  continue;
>> +
>> +  dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
>> +  }
>> +  spin_unlock_irqrestore(>lock, flags);
>> +
>>usb_del_gadget_udc(>gadget);
>>dwc3_gadget_free_endpoints(dwc);
>
> free endpoints is a better place for this. It's already going to free
> the memory anyway. Might as well clear all flags to 0 there.
>

 But it won't solve the deadlock issue. Since dwc3_gadget_free_endpoints()
 is called after usb_del_gadget_udc() and the deadlock happens when

 usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()

 and DWC3_EP_END_TRANSFER_PENDING flag is set.
>>>
>>> indeed. Iterating twice over the entire endpoint list seems
>>> wasteful. Perhaps we just shouldn't wait when removing the UDC since
>>> that's essentially what this patch will do, right? If you clear the flag
>>> before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
>>> will do nothing. Might as well remove it.
>>>
>>
>> This means that we will never wait for DWC3_EP_END_TRANSFER_PENDING to clear
>> in dwc3_gadget_stop() like we used to. This is perfectly fine, right?
>>
>> It makes sense to me as dwc3_gadget_stop() calls __dwc3_gadget_stop() which
>> masks all interrupts and nobody will ever clear that flag if it was set.
> 
> I don't think so. It can not mask the endpoint events, please check
> the events which will be masked in DEVTEN register. The reason why we

Correct, endpoint events are not managed by DEVTEN.

> should wait for DWC3_EP_END_TRANSFER_PENDING to clear is that,
> sometimes the DWC3_DEPEVT_EPCMDCMPLT event will be triggered later
> than 100us, but now we may have freed the gadget irq which will cause
> crash.
> 

OK. So what is the right approach here?
In the test case I mentioned in the commit log the endpoint interrupt never
happens and it waits forever in dwc3_gadget_stop().

Since we know we're winding up, can we explicitly disable the endpoint events
here?

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Baolin Wang]

Hi Roger,

On 5 March 2018 at 17:45, Roger Quadros  wrote:
> Felipe,
>
> On 05/03/18 10:49, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Roger Quadros  writes:
 Roger Quadros  writes:
> In the following test we get stuck by sleeping forever in _dwc3_set_mode()
> after which dual-role switching doesn't work.
>
> On dra7-evm's dual-role port,
> - Load g_zero gadget driver and enumerate to host
> - suspend to mem
> - disconnect USB cable to host and connect otg cable with Pen drive in it.
> - resume system
> - we sleep indefinitely in _dwc3_set_mode due to.
>   dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()->
>dwc3_gadget_stop()->wait_event_lock_irq()
>
> Let's clear the DWC3_EP_END_TRANSFER_PENDING flag on all endpoints
> so we don't wait in dwc3_gadget_stop().
>
> Signed-off-by: Roger Quadros 
> ---
>  drivers/usb/dwc3/gadget.c | 14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 2bda4eb..0a360da 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3273,6 +3273,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>
>  void dwc3_gadget_exit(struct dwc3 *dwc)
>  {
> +  int epnum;
> +  unsigned long flags;
> +
> +  spin_lock_irqsave(>lock, flags);
> +  for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
> +  struct dwc3_ep  *dep = dwc->eps[epnum];
> +
> +  if (!dep)
> +  continue;
> +
> +  dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
> +  }
> +  spin_unlock_irqrestore(>lock, flags);
> +
>usb_del_gadget_udc(>gadget);
>dwc3_gadget_free_endpoints(dwc);

 free endpoints is a better place for this. It's already going to free
 the memory anyway. Might as well clear all flags to 0 there.

>>>
>>> But it won't solve the deadlock issue. Since dwc3_gadget_free_endpoints()
>>> is called after usb_del_gadget_udc() and the deadlock happens when
>>>
>>> usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()
>>>
>>> and DWC3_EP_END_TRANSFER_PENDING flag is set.
>>
>> indeed. Iterating twice over the entire endpoint list seems
>> wasteful. Perhaps we just shouldn't wait when removing the UDC since
>> that's essentially what this patch will do, right? If you clear the flag
>> before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
>> will do nothing. Might as well remove it.
>>
>
> This means that we will never wait for DWC3_EP_END_TRANSFER_PENDING to clear
> in dwc3_gadget_stop() like we used to. This is perfectly fine, right?
>
> It makes sense to me as dwc3_gadget_stop() calls __dwc3_gadget_stop() which
> masks all interrupts and nobody will ever clear that flag if it was set.

I don't think so. It can not mask the endpoint events, please check
the events which will be masked in DEVTEN register. The reason why we
should wait for DWC3_EP_END_TRANSFER_PENDING to clear is that,
sometimes the DWC3_DEPEVT_EPCMDCMPLT event will be triggered later
than 100us, but now we may have freed the gadget irq which will cause
crash.

-- 
Baolin.wang
Best Regards
--
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/12] staging: typec: tcpci: support port config passed via dt

[Jun Li]

> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: 2018年3月5日 17:54
> To: Jun Li 
> Cc: gre...@linuxfoundation.org; robh...@kernel.org; li...@roeck-us.net;
> a.ha...@samsung.com; mark.rutl...@arm.com; yue...@google.com;
> Peter Chen ; garsi...@embeddedor.com;
> o_leve...@orange.fr; shufan_...@richtek.com; linux-usb@vger.kernel.org;
> devicet...@vger.kernel.org; dl-linux-imx 
> Subject: Re: [PATCH v2 03/12] staging: typec: tcpci: support port config
> passed via dt
> 
> On Mon, Mar 05, 2018 at 08:53:00AM +, Jun Li wrote:
> > > On Mon, Feb 26, 2018 at 02:30:53PM +, Jun Li wrote:
> > > > > > +   child = of_get_child_by_name(tcpci->dev->of_node,
> "connector");
> > > > > > +   if (!child) {
> > > > > > +   dev_err(tcpci->dev, "failed to get connector node.\n");
> > > > > > +   return -EINVAL;
> > > > > > +   }
> > > > >
> > > > > Why do you need separate child node for the connector? You will
> > > > > always have only one connector per tcpc, i.e. the tcpci already
> > > > > represents the connector and all its capabilities.
> > > > >
> > > > This is my original idea, my understanding is Rob expects those
> > > > properties should apply for a common usb connector node[1], that
> > > > way I need add a child node for it, sorry I didn't make the
> > > > dt-binding patches come first in this series, please see patch 10,11.
> > > >
> > > > [1]
> > > >
> > >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fp
> > > at
> > > >
> > >
> chwork.kernel.org%2Fpatch%2F10231447%2F=02%7C01%7Cjun.li%40
> > > nxp.co
> > > >
> > >
> m%7Ce37ed8b084374e241d2e08d57dd1b02a%7C686ea1d3bc2b4c6fa92cd9
> > > 9c5c30163
> > > >
> > >
> 5%7C0%7C0%7C636553261972212376=hSNiAfXoTTzK3TjjkjWo7OJL7
> > > %2B3gDHT
> > > > I8NO0FQviDd4%3D=0
> > >
> > > But was the idea really to put properties like the TCPC capabilities
> > > under the usb connector child node? That will force us to extract
> > > the same properties in two different methods in every USB Type-C
> > > driver. One extracting them from DT, and another from other FW
> interfaces and build-in properties.
> > >
> > > To avoid that, let's just expect to get these properties in the node
> > > for tcpc, not the usb connector child.
> >
> > I misunderstood it's better to put typec props under connector node in
> > all cases so we can have a unified typec description. As Rob comments
> > that's only required for multiple connectors for one controller, not
> > for simple connector like my case, I will put those props under tcpc node.
> 
> Hold on! I was not considering multi-port PD/Type-C controllers.
> 
> I'm wrong about the child node forcing us to have two methods for
> extracting the properties in the drivers. It does mean we can not use
> device_property* functions as the child node is not (yet) bind to any struct
> device, but we can still use fwnode_property* functions, which is fine.
> 

Yes, fwnode_property* function can be used in this case.

> So it actually does make sense to define those properties for the
> "connector" node instead of TCPC parent. They are generic "Type-C"
> properties (right?), so we may want to use them with multiport devices as
> well.
> 

Yes, that's the idea of my v2, I will keep this but via fwnode_property*.

> 
> Br,
> 
> --
> heikki

Re: [PATCH v2 1/1] usb: xhci: do not create and register shared_hcd when USB3.0 is disabled

[Thang Q. Nguyen]

On Mon, Jan 29, 2018 at 5:24 PM, Thang Q. Nguyen  wrote:
> From: Tung Nguyen 
>
> Currently, hcd->shared_hcd always creates and registers to the usb-core.
> If, for some reasons, USB3 downstream port is disabled, no roothub port for
> USB3.0 is found. This causes kernel to display an error:
> hub 2-0:1.0: config failed, hub doesn't have any ports! (err -19)
> This patch checks and registers shared_hcd if USB3.0 downstream
> port is available.
>
> Signed-off-by: Tung Nguyen 
> ---
>  drivers/usb/host/xhci-plat.c |  9 ++---
>  drivers/usb/host/xhci.c  | 13 +
>  2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 6f03830..bdb3975 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -293,9 +293,12 @@ static int xhci_plat_probe(struct platform_device *pdev)
> if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> xhci->shared_hcd->can_do_streams = 1;
>
> -   ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> -   if (ret)
> -   goto dealloc_usb2_hcd;
> +   /* Just add the shared_hcd when USB3.0 downstream port is available */
> +   if (xhci->num_usb3_ports > 0) {
> +   ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> +   if (ret)
> +   goto dealloc_usb2_hcd;
> +   }
>
> device_enable_async_suspend(>dev);
> pm_runtime_put_noidle(>dev);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 1eeb339..9d3b1ab 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -611,6 +611,19 @@ int xhci_run(struct usb_hcd *hcd)
> if (ret)
> xhci_free_command(xhci, command);
> }
> +   /*
> +* In case that the USB3.0 downstream port is not available
> +* No one triggers to start the xHC which should be done
> +* before finishing xhci_run
> +*/
> +   if (xhci->num_usb3_ports == 0) {
> +   if (xhci_start(xhci)) {
> +   xhci_halt(xhci);
> +   return -ENODEV;
> +   }
> +   xhci->cmd_ring_state = CMD_RING_STATE_RUNNING;
> +   }
> +
> xhci_dbg_trace(xhci, trace_xhci_dbg_init,
> "Finished xhci_run for USB2 roothub");
>
> --
> 1.8.3.1
>
Hi,
Do you have any comment on the patch?
--
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 10/12] dt-bindings: connector: add properties for typec power delivery

[Andrzej Hajda]

On 05.03.2018 08:00, Jun Li wrote:
>
>> -Original Message-
>> From: Andrzej Hajda [mailto:a.ha...@samsung.com]
>> Sent: 2018年2月27日 16:41
>> To: Jun Li ; gre...@linuxfoundation.org;
>> robh...@kernel.org; heikki.kroge...@linux.intel.com; li...@roeck-us.net
>> Cc: mark.rutl...@arm.com; yue...@google.com; Peter Chen
>> ; garsi...@embeddedor.com; o_leve...@orange.fr;
>> shufan_...@richtek.com; linux-usb@vger.kernel.org;
>> devicet...@vger.kernel.org; dl-linux-imx 
>> Subject: Re: [PATCH v2 10/12] dt-bindings: connector: add properties for
>> typec power delivery
>>
>> On 26.02.2018 12:49, Li Jun wrote:
>>> In case of usb-c-connector with power delivery support, add bingdings
>>> supported by current typec driver, so user can pass all those
>>> properties via dt.
>>>
>>> Signed-off-by: Li Jun 
>>> ---
>>> Changes for v2:
>>> - Added typec properties are based on general usb connector bindings[1]
>>>   proposed by Andrzej Hajda.
>>> - Use the standard unit suffixes as defined in property-units.txt.
>>>
>>> [1]
>>>
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
>> chwork.kernel.org%2Fpatch%2F10231447%2F=02%7C01%7Cjun.li%40
>> nxp.co
>> m%7Cccf14a36ca6445ee5f1108d57dbde3c7%7C686ea1d3bc2b4c6fa92cd99
>> c5c30163
>> 5%7C0%7C0%7C636553176880292197=2Pi0AtiwAqHQE3rGl%2Bo49K
>> 7yZZcp%2B
>>> 5bvJAknRBMGTrk%3D=0
>>>
>>>  .../bindings/connector/usb-connector.txt   | 43
>> ++
>>>  1 file changed, 43 insertions(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> index e1463f1..242f6df 100644
>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> @@ -15,6 +15,30 @@ Optional properties:
>>>  - type: size of the connector, should be specified in case of USB-A, USB-B
>>>non-fullsize connectors: "mini", "micro".
>>>
>>> +Required properties for usb-c-connector with power delivery support:
>>> +- port-type: should be one of "source", "sink" or "dual".
>>> +- default-role: preferred power role if port-type is "dual"(drp),
>>> +should be
>>> +  "sink" or "source".
>> Since port carries data and power, it would be better to explicitly mention 
>> it
>> in names of properties which can be ambiguous.
>> Maybe instead of 'port-type' you can use 'power-role', for example.
> Port-type is align with the name of typec coding and ABI, 'power-role'
> is more like for the current role rather than the port's ability.

I am not sure what are you exactly mean by "coding and ABI", but I guess
it is just about Power-Delivery part of USB-C. And if you want to put
this property into USB node without mark it is related to PD part of USB
connector, you risk confusion with possible USB data related properties.
Simple question, what if somebody wants to add property describing USB
data capabilities (DFP, UFP, DRD), how should it be named?

>
>> Other thing is that default-role is required only in case of DRP, so maybe
>> better would be to squash it in power-role, for example:
>>     power-role: should be on of "source", "sink", "dual-source", "dual-sink",
>> in case of dual roles suffix determines preferred role.
> I don't know how much this squash can benefit, "dual-source/sink" is not
> directly showing its meaning by name.

Some benefit is simpler binding, no conditionally-required property.

Another question is that USB TypeC specification describes 7 different
"behavioral models" [1]:
- Source-only
- Source (Default) – strong preference toward being a Source but
subsequently
capable of becoming a Sink using USB PD swap mechanisms.
- Sink-only
- Sink (Default) – strong preference toward being a Sink but
subsequently capable of
becoming a Source using USB PD swap mechanisms.
- DRP: Toggling (Source/Sink)
- DRP: Sourcing Device
- DRP: Sinking Host

How it maps to your proposed props?

[1]: USB Type-C specification release 1.3, chapter 4.5.1.4.

Regards
Andrzej

>
>>
>>> +- src-pdos: An array of u32 with each entry providing supported power
>>> +  source data object(PDO), the detailed bit definitions of PDO can be
>>> +found
>>> +  in "Universal Serial Bus Power Delivery Specification" chapter
>>> +6.4.1.2
>>> +  Source_Capabilities Message, the order of each entry(PDO) should
>>> +follow
>>> +  the PD spec chapter 6.4.1. Required for power source and power dual
>> role.
>>> +- snk-pdos: An array of u32 with each entry providing supported power
>>> +  sink data object(PDO), the detailed bit definitions of PDO can be
>>> +found in
>>> +  "Universal Serial Bus Power Delivery Specification" chapter 6.4.1.3
>>> +Sink
>>> +  Capabilities Message, the order of each entry(PDO) should follow
>>> +the PD
>>> +  spec chapter 6.4.1. Required for power sink and power dual role.
>> We should avoid magic numbers, magic numbers are 

[RFC PATCH] cdc-acm: do not drop data from fast devices

[Romain Izard]

There are some devices using their USB CDC-ACM interfaces as a debug port
that are able to send data at a very high speed, but with the current
driver implementation it is not possible to receive it when using a
relatively slow embedded system without dropping an important part of the
data.

The existing driver uses the throttling mechanism of the TTY line
discipline to regulate the speed of the data transmitted from the port to
the upper layers. This throttling prevents URBs from being resubmitted,
slowing down the transfer on the USB line. But the existing code does not
work correctly when the internal bufferring gets filled.

The TTY buffer is 4096 bytes large, throttling when there are only 128
free bytes left, and unthrottling when there are only 128 bytes available.
But the TTY buffer is filled from an intermediate flip buffer that
contains up to 64 KiB of data, and each time unthrottle() is called 16
URBs are scheduled by the driver, sending up to 16 KiB to the flip buffer.
As the result of tty_insert_flip_string() is not checked in the URB
reception callback, data can be lost when the flip buffer is filled faster
than the TTY is emptied.

Moreover, as the URB callbacks are called from a tasklet context, whereas
throttling is called from the system workqueue, it is possible for the
throttling to be delayed by high traffic in the tasklet. As a result,
completed URBs can be resubmitted even if the flip buffer is full, and we
request more data from the device only to drop it immediately.

To prevent this problem, the URBs whose data did not reach the flip buffer
are placed in a waiting list, which is only processed when the serial port
is unthrottled.

Signed-off-by: Romain Izard 

--

This is working when using the normal line discipline on ttyACM. But
there is a big hole in this: other line disciplines do not use throttling
and thus unthrottle is never called. The URBs will never get resubmitted,
and the port is dead. Unfortunately, there is no notification when the
flip buffer is ready to receive new data, so the only alternative would
be to schedule a timer polling the flip buffer. But with an additional
asynchronous process in the mix, the code starts to be very brittle.

I believe this issue is not limited to ttyACM. As the TTY layer is not
performance-oriented, it can be easy to overwhelm devices with a low
available processing power. In my case, a modem sending a sustained 2 MB/s
on a debug port (exported as a CDC-ACM port) was enough to trigger the
issue. The code handling the CDC-ACM class and the generic USB serial port
is very similar when it comes to URB handling, so all drivers that rely on
that code have the same issue.

But in general, it seems to me that there is no code in the kernel that
checks the return value of tty_insert_flip_string(). This means that we
are working with the assumption that the kernel will consume the data
faster than the source can send it, or that the upper layer will be
able or willing to throttle it fast enough to avoid losing data.


---
 drivers/usb/class/cdc-acm.c | 80 -
 drivers/usb/class/cdc-acm.h |  4 +++
 2 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 7b366a6c0b49..833fa0a43ddd 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -150,12 +150,18 @@ static inline int acm_set_control(struct acm *acm, int 
control)
 static void acm_kill_urbs(struct acm *acm)
 {
int i;
+   struct acm_rb *rb, *t;
 
usb_kill_urb(acm->ctrlurb);
for (i = 0; i < ACM_NW; i++)
usb_kill_urb(acm->wb[i].urb);
for (i = 0; i < acm->rx_buflimit; i++)
usb_kill_urb(acm->read_urbs[i]);
+   list_for_each_entry_safe(rb, t, >wait_list, node) {
+   set_bit(rb->index, >read_urbs_free);
+   rb->offset = 0;
+   list_del_init(>node);
+   }
 }
 
 /*
@@ -454,14 +460,27 @@ static int acm_submit_read_urbs(struct acm *acm, gfp_t 
mem_flags)
return 0;
 }
 
-static void acm_process_read_urb(struct acm *acm, struct urb *urb)
+static int acm_process_read_urb(struct acm *acm, struct urb *urb)
 {
+   int flipped, remainder;
+   struct acm_rb *rb = urb->context;
if (!urb->actual_length)
-   return;
+   return 0;
+   flipped = tty_insert_flip_string(>port,
+   urb->transfer_buffer + rb->offset,
+   urb->actual_length - rb->offset);
+   rb->offset += flipped;
+   remainder = urb->actual_length - rb->offset;
+   if (remainder != 0)
+   dev_dbg(>data->dev,
+   "remaining data: usb %d len %d offset %d flipped %d\n",
+   rb->index, urb->actual_length, rb->offset, flipped);
+   else
+   rb->offset = 0;
 
-   tty_insert_flip_string(>port, 

Re: [PATCH v2 03/12] staging: typec: tcpci: support port config passed via dt

[Heikki Krogerus]

On Mon, Mar 05, 2018 at 08:53:00AM +, Jun Li wrote:
> > On Mon, Feb 26, 2018 at 02:30:53PM +, Jun Li wrote:
> > > > > + child = of_get_child_by_name(tcpci->dev->of_node, "connector");
> > > > > + if (!child) {
> > > > > + dev_err(tcpci->dev, "failed to get connector node.\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > >
> > > > Why do you need separate child node for the connector? You will
> > > > always have only one connector per tcpc, i.e. the tcpci already
> > > > represents the connector and all its capabilities.
> > > >
> > > This is my original idea, my understanding is Rob expects those
> > > properties should apply for a common usb connector node[1], that way I
> > > need add a child node for it, sorry I didn't make the dt-binding
> > > patches come first in this series, please see patch 10,11.
> > >
> > > [1]
> > >
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> > >
> > chwork.kernel.org%2Fpatch%2F10231447%2F=02%7C01%7Cjun.li%40
> > nxp.co
> > >
> > m%7Ce37ed8b084374e241d2e08d57dd1b02a%7C686ea1d3bc2b4c6fa92cd9
> > 9c5c30163
> > >
> > 5%7C0%7C0%7C636553261972212376=hSNiAfXoTTzK3TjjkjWo7OJL7
> > %2B3gDHT
> > > I8NO0FQviDd4%3D=0
> > 
> > But was the idea really to put properties like the TCPC capabilities under 
> > the
> > usb connector child node? That will force us to extract the same properties
> > in two different methods in every USB Type-C driver. One extracting them
> > from DT, and another from other FW interfaces and build-in properties.
> > 
> > To avoid that, let's just expect to get these properties in the node for 
> > tcpc,
> > not the usb connector child.
> 
> I misunderstood it's better to put typec props under connector node in all 
> cases
> so we can have a unified typec description. As Rob comments that's only 
> required
> for multiple connectors for one controller, not for simple connector like my 
> case,
> I will put those props under tcpc node.

Hold on! I was not considering multi-port PD/Type-C controllers.

I'm wrong about the child node forcing us to have two methods for
extracting the properties in the drivers. It does mean we can not use
device_property* functions as the child node is not (yet) bind to any
struct device, but we can still use fwnode_property* functions, which
is fine.

So it actually does make sense to define those properties for the
"connector" node instead of TCPC parent. They are generic "Type-C"
properties (right?), so we may want to use them with multiport
devices as well.


Br,

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

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Roger Quadros]

Felipe,

On 05/03/18 10:49, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros  writes:
>>> Roger Quadros  writes:
 In the following test we get stuck by sleeping forever in _dwc3_set_mode()
 after which dual-role switching doesn't work.

 On dra7-evm's dual-role port,
 - Load g_zero gadget driver and enumerate to host
 - suspend to mem
 - disconnect USB cable to host and connect otg cable with Pen drive in it.
 - resume system
 - we sleep indefinitely in _dwc3_set_mode due to.
   dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()->
dwc3_gadget_stop()->wait_event_lock_irq()

 Let's clear the DWC3_EP_END_TRANSFER_PENDING flag on all endpoints
 so we don't wait in dwc3_gadget_stop().

 Signed-off-by: Roger Quadros 
 ---
  drivers/usb/dwc3/gadget.c | 14 ++
  1 file changed, 14 insertions(+)

 diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
 index 2bda4eb..0a360da 100644
 --- a/drivers/usb/dwc3/gadget.c
 +++ b/drivers/usb/dwc3/gadget.c
 @@ -3273,6 +3273,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)
  
  void dwc3_gadget_exit(struct dwc3 *dwc)
  {
 +  int epnum;
 +  unsigned long flags;
 +
 +  spin_lock_irqsave(>lock, flags);
 +  for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
 +  struct dwc3_ep  *dep = dwc->eps[epnum];
 +
 +  if (!dep)
 +  continue;
 +
 +  dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
 +  }
 +  spin_unlock_irqrestore(>lock, flags);
 +
usb_del_gadget_udc(>gadget);
dwc3_gadget_free_endpoints(dwc);
>>>
>>> free endpoints is a better place for this. It's already going to free
>>> the memory anyway. Might as well clear all flags to 0 there.
>>>
>>
>> But it won't solve the deadlock issue. Since dwc3_gadget_free_endpoints()
>> is called after usb_del_gadget_udc() and the deadlock happens when
>>
>> usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()
>>
>> and DWC3_EP_END_TRANSFER_PENDING flag is set.
> 
> indeed. Iterating twice over the entire endpoint list seems
> wasteful. Perhaps we just shouldn't wait when removing the UDC since
> that's essentially what this patch will do, right? If you clear the flag
> before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
> will do nothing. Might as well remove it.
> 

This means that we will never wait for DWC3_EP_END_TRANSFER_PENDING to clear
in dwc3_gadget_stop() like we used to. This is perfectly fine, right?

It makes sense to me as dwc3_gadget_stop() calls __dwc3_gadget_stop() which
masks all interrupts and nobody will ever clear that flag if it was set.

And there is no point in clearing the DWC3_EP_END_TRANSFER_PENDING flag
in dwc3_gadget_free_endpoints() since we're freeing the dwc3_ep memory there.

dwc3_gadget_init_endpoints() will start with a clean slate.

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
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: [PATH 0/4] usbip: make vhci_hcd.* objects independent of vhci_hcd.0

[Salvador Fandiño]

On 02/21/2018 01:35 AM, Shuah Khan wrote:

Hi Salvador,

On 01/30/2018 01:36 AM, Salvador Fandino wrote:

Let me start by explaining the problem that have motivated me to write
this patches:

I work on the QVD, a virtual desktop platform for Linux. This software
runs Linux desktops (i.e. XFCE, KDE) and their applications inside LXC
containers, and makes then available through the network to remote
users.

Supporting USB devices is a common feature customers have been
requesting us for a long time (in order to use, for instance, remote
signature pads, bar-code scanners, fingerprint readers, etc.). So, we
have been working on that feature using the USB/IP layer on the
kernel.

Connecting and disconnecting devices and transferring data works
seamless for the devices listed above. But we also want to make the
usbip operations private to the container where they are run.  For
instance, it is unacceptable for our product, that one user could list
the devices connected by other users or access them.

We can control how can access every device using cgroups once those
are attached, but the usbip layer is not providing any mechanism for
controlling who can attach, detach or list the devices.


Did you explore a solution to add a mechanism for access control to
usbip?


Could you elaborate on that?

For "usbip", do you mean the user space tools? If that is the case, I 
don't think it would be enough. My aim is to limit vhci usage from 
containers and I have no control about what runs inside the containers. 
So, a mangled usbip tool-set could be used by a malicious user to 
circumvent any access control set there.


IMO, there is no other choice but to control access to VHCI at the 
kernel level.







So, we got the idea that in order to enforce that remote usbip devices
are only visible inside the container where they were imported, we
could conveniently mount-bind inside every container just one of the
vhci_hcd directories below /sys/devices/platform. So that it is as if
every container had a vhci_hcd just for itself (and also, we restrict
access to the matching USB ports in cgroups).

Unfortunately, all the vhci_hcd.* devices are controlled through
attributes in vhci_hcd.0 making our approach fail and so... well, that
is what this patch series changes. It makes every vhci_hcd device
controllable through attributes inside its own sysfs directory.>
The first patch, does that in the kernel, and the second and third
patches change user space, adapting the libusbip and the usbip tools
code respectively.

Then there is a fourth patch, that allows to create much more USB
hubs per machine. It was limited to 64 and we are running thousands of
containers (every one requiring a hub) per host.

These changes are not completely backward compatible. In the sysfs
side, besides moving around the attribute files, now the port numbers
go from 0 to CONFIG_USBIP_VHCI_HC_PORTS * 2 - 1 and are reused for
every vhci_hcd device. I could have maintained the absolute numeration
but I think reusing the numbers is a better and simpler approach.


Not being able to maintain backwards compatibility is an issue. This is
a considerable change to the user interface.


Well, it is true that it is a considerable change to the user interface 
breaking backward compatibility, but as I had already stated, that 
interface was broken until a couple of months ago, when my coworker, 
Juan Zea, reported it and nobody had noticed it before. So, I don't 
think we are going to affect too many people.


Note also that the user interface does not change when only vhci_hcd.0 
is used.



Regards
--
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/12] staging: typec: tcpci: support port config passed via dt

[Jun Li]

Hi

> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: 2018年2月27日 19:03
> To: Jun Li 
> Cc: gre...@linuxfoundation.org; robh...@kernel.org; li...@roeck-us.net;
> a.ha...@samsung.com; mark.rutl...@arm.com; yue...@google.com;
> Peter Chen ; garsi...@embeddedor.com;
> o_leve...@orange.fr; shufan_...@richtek.com; linux-usb@vger.kernel.org;
> devicet...@vger.kernel.org; dl-linux-imx 
> Subject: Re: [PATCH v2 03/12] staging: typec: tcpci: support port config
> passed via dt
> 
> Hi,
> 
> On Mon, Feb 26, 2018 at 02:30:53PM +, Jun Li wrote:
> > > > +   child = of_get_child_by_name(tcpci->dev->of_node, "connector");
> > > > +   if (!child) {
> > > > +   dev_err(tcpci->dev, "failed to get connector node.\n");
> > > > +   return -EINVAL;
> > > > +   }
> > >
> > > Why do you need separate child node for the connector? You will
> > > always have only one connector per tcpc, i.e. the tcpci already
> > > represents the connector and all its capabilities.
> > >
> > This is my original idea, my understanding is Rob expects those
> > properties should apply for a common usb connector node[1], that way I
> > need add a child node for it, sorry I didn't make the dt-binding
> > patches come first in this series, please see patch 10,11.
> >
> > [1]
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >
> chwork.kernel.org%2Fpatch%2F10231447%2F=02%7C01%7Cjun.li%40
> nxp.co
> >
> m%7Ce37ed8b084374e241d2e08d57dd1b02a%7C686ea1d3bc2b4c6fa92cd9
> 9c5c30163
> >
> 5%7C0%7C0%7C636553261972212376=hSNiAfXoTTzK3TjjkjWo7OJL7
> %2B3gDHT
> > I8NO0FQviDd4%3D=0
> 
> But was the idea really to put properties like the TCPC capabilities under the
> usb connector child node? That will force us to extract the same properties
> in two different methods in every USB Type-C driver. One extracting them
> from DT, and another from other FW interfaces and build-in properties.
> 
> To avoid that, let's just expect to get these properties in the node for tcpc,
> not the usb connector child.

I misunderstood it's better to put typec props under connector node in all cases
so we can have a unified typec description. As Rob comments that's only required
for multiple connectors for one controller, not for simple connector like my 
case,
I will put those props under tcpc node.

Jun Li
> 
> 
> Thanks,
> 
> --
> heikki

Re: [PATCH v2] usb: phy: mxs: Fix NULL pointer dereference on i.MX23/28

[Felipe Balbi]

Fabio Estevam  writes:

> Hi Felipe,
>
> On Mon, Jan 22, 2018 at 10:28 AM, Fabio Estevam  wrote:
>> Hi Felipe,
>>
>> On Thu, Jan 18, 2018 at 12:22 AM, Fabio Estevam  wrote:
>>> From: Fabio Estevam 
>>>
>>> Commit e93650994a95 ("usb: phy: mxs: add usb charger type detection")
>>> causes the following kernel hang on i.MX28:
>>>
>>> [2.207973] usbcore: registered new interface driver usb-storage
>>> [2.235659] Unable to handle kernel NULL pointer dereference at virtual 
>>> address 0188
>>> [2.244195] pgd = (ptrval)
>>> [2.246994] [0188] *pgd=
>>> [2.250676] Internal error: Oops: 5 [#1] ARM
>>> [2.254979] Modules linked in:
>>> [2.258089] CPU: 0 PID: 1 Comm: swapper Not tainted 
>>> 4.15.0-rc8-next-20180117-2-g75d5f21 #7
>>> [2.266724] Hardware name: Freescale MXS (Device Tree)
>>> [2.271921] PC is at regmap_read+0x0/0x5c
>>> [2.275977] LR is at mxs_phy_charger_detect+0x34/0x1dc
>>>
>>> mxs_phy_charger_detect() makes accesses to the anatop registers via regmap,
>>> however i.MX23/28 do not have such registers, which causes a NULL pointer
>>> dereference.
>>>
>>> Fix the issue by doing a NULL check on the 'regmap' pointer.
>>>
>>> Fixes: e93650994a95 ("usb: phy: mxs: add usb charger type detection")
>>> Signed-off-by: Fabio Estevam 
>>
>> Could this one be applied to 4.15 final so that we avoid a boot
>> regression on i.MX23/28?
>
> A gentle ping.

499350865387f8b8c40a9e9453a9a7eb3cec5dc4

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>> Roger Quadros  writes:
>>> In the following test we get stuck by sleeping forever in _dwc3_set_mode()
>>> after which dual-role switching doesn't work.
>>>
>>> On dra7-evm's dual-role port,
>>> - Load g_zero gadget driver and enumerate to host
>>> - suspend to mem
>>> - disconnect USB cable to host and connect otg cable with Pen drive in it.
>>> - resume system
>>> - we sleep indefinitely in _dwc3_set_mode due to.
>>>   dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()->
>>> dwc3_gadget_stop()->wait_event_lock_irq()
>>>
>>> Let's clear the DWC3_EP_END_TRANSFER_PENDING flag on all endpoints
>>> so we don't wait in dwc3_gadget_stop().
>>>
>>> Signed-off-by: Roger Quadros 
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 14 ++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 2bda4eb..0a360da 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -3273,6 +3273,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>>  
>>>  void dwc3_gadget_exit(struct dwc3 *dwc)
>>>  {
>>> +   int epnum;
>>> +   unsigned long flags;
>>> +
>>> +   spin_lock_irqsave(>lock, flags);
>>> +   for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>>> +   struct dwc3_ep  *dep = dwc->eps[epnum];
>>> +
>>> +   if (!dep)
>>> +   continue;
>>> +
>>> +   dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
>>> +   }
>>> +   spin_unlock_irqrestore(>lock, flags);
>>> +
>>> usb_del_gadget_udc(>gadget);
>>> dwc3_gadget_free_endpoints(dwc);
>> 
>> free endpoints is a better place for this. It's already going to free
>> the memory anyway. Might as well clear all flags to 0 there.
>> 
>
> But it won't solve the deadlock issue. Since dwc3_gadget_free_endpoints()
> is called after usb_del_gadget_udc() and the deadlock happens when
>
> usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()
>
> and DWC3_EP_END_TRANSFER_PENDING flag is set.

indeed. Iterating twice over the entire endpoint list seems
wasteful. Perhaps we just shouldn't wait when removing the UDC since
that's essentially what this patch will do, right? If you clear the flag
before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
will do nothing. Might as well remove it.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v2 02/12] usb: typec: add API to get sink and source config

[Jun Li]

> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: 2018年2月26日 21:32
> To: Jun Li 
> Cc: gre...@linuxfoundation.org; robh...@kernel.org; li...@roeck-us.net;
> a.ha...@samsung.com; mark.rutl...@arm.com; yue...@google.com;
> Peter Chen ; garsi...@embeddedor.com;
> o_leve...@orange.fr; shufan_...@richtek.com; linux-usb@vger.kernel.org;
> devicet...@vger.kernel.org; dl-linux-imx 
> Subject: Re: [PATCH v2 02/12] usb: typec: add API to get sink and source
> config
> 
> Hi,
> 
> On Mon, Feb 26, 2018 at 07:49:09PM +0800, Li Jun wrote:
> > This patch add 2 APIs to get sink and source power config from
> > firmware description.
> >
> > Signed-off-by: Li Jun 
> > ---
> >  drivers/usb/typec/tcpm.c | 43
> > +++
> >  include/linux/usb/tcpm.h |  2 ++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index
> > f4d563e..409f1d0 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -12,6 +12,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> 
> No need for that. There is nothing DT only specific here.
> 
> As in 01/12, you only have device properties here, so use the unified device
> property API instead of of_property_* functions.

I will use device property API, also as the below sink props:
"max-snk-microvolt"
"max-snk-microamp"
"max-snk-microwatt-hours"
"op-snk-microwatt-hours"
are redundant now, only PDO is required, I will combine the below 2 APIs
to be one:
int tcpm_get_pdos(struct device *dev, bool sink, struct tcpc_config *tcfg)

Jun Li
> 
> >  #include 
> >  #include 
> >  #include 
> > @@ -3589,6 +3590,48 @@ static int tcpm_copy_vdos(u32 *dest_vdo,
> const u32 *src_vdo,
> > return nr_vdo;
> >  }
> >
> > +int tcpm_of_get_src_config(struct device_node *np, struct tcpc_config
> > +*tcfg) {
> > +   int ret;
> > +
> > +   ret = of_property_read_variable_u32_array(np, "src-pdos",
> > +   tcfg->src_pdo, 1, PDO_MAX_OBJECTS);
> > +   if (ret > 0)
> > +   tcfg->nr_src_pdo = ret;
> > +
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tcpm_of_get_pdos);
> > +
> > +int tcpm_of_get_snk_config(struct device_node *np, struct tcpc_config
> > +*tcfg) {
> > +   int ret;
> > +
> > +   ret = of_property_read_variable_u32_array(np, "snk-pdos",
> > +   tcfg->snk_pdo, 1, PDO_MAX_OBJECTS);
> > +   if (ret > 0)
> > +   tcfg->nr_snk_pdo = ret;
> > +   else
> > +   return ret;
> > +
> > +   ret = of_property_read_u32(np, "max-snk-microvolt",
> >max_snk_mv);
> > +   if (ret)
> > +   return ret;
> > +
> > +   ret = of_property_read_u32(np, "max-snk-microamp",
> >max_snk_ma);
> > +   if (ret)
> > +   return ret;
> > +
> > +   ret = of_property_read_u32(np, "max-snk-microwatt-hours",
> > +  >max_snk_mw);
> > +   if (ret)
> > +   return ret;
> > +
> > +   return of_property_read_u32(np, "op-snk-microwatt-hours",
> > +   >operating_snk_mw);
> > +}
> > +EXPORT_SYMBOL_GPL(tcpm_of_get_pdos);
> > +
> >  int tcpm_update_source_capabilities(struct tcpm_port *port, const u32
> *pdo,
> > unsigned int nr_pdo)
> >  {
> > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
> > ca1c0b5..962eff1 100644
> > --- a/include/linux/usb/tcpm.h
> > +++ b/include/linux/usb/tcpm.h
> > @@ -191,6 +191,8 @@ int tcpm_update_sink_capabilities(struct
> tcpm_port *port, const u32 *pdo,
> >   unsigned int max_snk_ma,
> >   unsigned int max_snk_mw,
> >   unsigned int operating_snk_mw);
> > +int tcpm_of_get_src_config(struct device_node *np, struct tcpc_config
> > +*tcfg); int tcpm_of_get_snk_config(struct device_node *np, struct
> > +tcpc_config *tcfg);
> >
> >  void tcpm_vbus_change(struct tcpm_port *port);  void
> > tcpm_cc_change(struct tcpm_port *port);
> 
> Thanks,
> 
> --
> heikki

Re: [usb-next PATCH v11 5/8] usb: host: xhci-mtk: remove custom USB PHY handling

[Sean Wang]

Hi, Martin

Tested-by: Sean Wang 

I've tested the series with U2 storage and U3 ethernet devices on both
boards MT7623 BPI-R2 and MT7622 RFB1 using xhci-mtk driver, they are
still working well.

Below is related logs for test devices probing



# [   42.590356] usb 2-1: new SuperSpeed USB device number 2 using
xhci-mtk
[   42.719883] usb-storage 2-1:1.0: USB Mass Storage device detected
[   42.726339] scsi host2: usb-storage 2-1:1.0
[   43.815572] scsi 2:0:0:0: Direct-Access Kingston DataTraveler 3.0
PMAP PQ: 0 ANSI: 6
[   44.792938] sd 2:0:0:0: [sda] 30728832 512-byte logical blocks: (15.7
GB/14.7 GiB)
[   44.800658] sd 2:0:0:0: [sda] Write Protect is off
[   44.805582] sd 2:0:0:0: [sda] No Caching mode page found
[   44.810888] sd 2:0:0:0: [sda] Assuming drive cache: write through



# [  134.270617] usb 2-1: new SuperSpeed USB device number 3 using
xhci-mtk
[  134.664163] ax88179_178a 2-1:1.0 eth1: register 'ax88179_178a' at
usb-1a1c.usb-1, ASIX AX88179 USB 3.0 Gigabit Ethernet,
00:11:6b:68:4c:9e


On Sat, 2018-03-03 at 22:43 +0100, Martin Blumenstingl wrote:
> The new PHY wrapper is now wired up in the core HCD code. This means
> that PHYs are now controlled (initialized, enabled, disabled, exited)
> without requiring any host-driver specific code.
> Remove the custom USB PHY handling from the xhci-mtk driver as the core
> HCD code now handles this.
> 
> Signed-off-by: Martin Blumenstingl 
> ---
>  drivers/usb/host/xhci-mtk.c | 98 
> +
>  1 file changed, 2 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> index b0ab4d5e2751..7334da9e9779 100644
> --- a/drivers/usb/host/xhci-mtk.c
> +++ b/drivers/usb/host/xhci-mtk.c
> @@ -14,7 +14,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -352,62 +351,6 @@ static const struct xhci_driver_overrides 
> xhci_mtk_overrides __initconst = {
>  
>  static struct hc_driver __read_mostly xhci_mtk_hc_driver;
>  
> -static int xhci_mtk_phy_init(struct xhci_hcd_mtk *mtk)
> -{
> - int i;
> - int ret;
> -
> - for (i = 0; i < mtk->num_phys; i++) {
> - ret = phy_init(mtk->phys[i]);
> - if (ret)
> - goto exit_phy;
> - }
> - return 0;
> -
> -exit_phy:
> - for (; i > 0; i--)
> - phy_exit(mtk->phys[i - 1]);
> -
> - return ret;
> -}
> -
> -static int xhci_mtk_phy_exit(struct xhci_hcd_mtk *mtk)
> -{
> - int i;
> -
> - for (i = 0; i < mtk->num_phys; i++)
> - phy_exit(mtk->phys[i]);
> -
> - return 0;
> -}
> -
> -static int xhci_mtk_phy_power_on(struct xhci_hcd_mtk *mtk)
> -{
> - int i;
> - int ret;
> -
> - for (i = 0; i < mtk->num_phys; i++) {
> - ret = phy_power_on(mtk->phys[i]);
> - if (ret)
> - goto power_off_phy;
> - }
> - return 0;
> -
> -power_off_phy:
> - for (; i > 0; i--)
> - phy_power_off(mtk->phys[i - 1]);
> -
> - return ret;
> -}
> -
> -static void xhci_mtk_phy_power_off(struct xhci_hcd_mtk *mtk)
> -{
> - unsigned int i;
> -
> - for (i = 0; i < mtk->num_phys; i++)
> - phy_power_off(mtk->phys[i]);
> -}
> -
>  static int xhci_mtk_ldos_enable(struct xhci_hcd_mtk *mtk)
>  {
>   int ret;
> @@ -488,8 +431,6 @@ static int xhci_mtk_probe(struct platform_device *pdev)
>   struct xhci_hcd *xhci;
>   struct resource *res;
>   struct usb_hcd *hcd;
> - struct phy *phy;
> - int phy_num;
>   int ret = -ENODEV;
>   int irq;
>  
> @@ -529,16 +470,6 @@ static int xhci_mtk_probe(struct platform_device *pdev)
>   return ret;
>   }
>  
> - mtk->num_phys = of_count_phandle_with_args(node,
> - "phys", "#phy-cells");
> - if (mtk->num_phys > 0) {
> - mtk->phys = devm_kcalloc(dev, mtk->num_phys,
> - sizeof(*mtk->phys), GFP_KERNEL);
> - if (!mtk->phys)
> - return -ENOMEM;
> - } else {
> - mtk->num_phys = 0;
> - }
>   pm_runtime_enable(dev);
>   pm_runtime_get_sync(dev);
>   device_enable_async_suspend(dev);
> @@ -596,23 +527,6 @@ static int xhci_mtk_probe(struct platform_device *pdev)
>   mtk->has_ippc = false;
>   }
>  
> - for (phy_num = 0; phy_num < mtk->num_phys; phy_num++) {
> - phy = devm_of_phy_get_by_index(dev, node, phy_num);
> - if (IS_ERR(phy)) {
> - ret = PTR_ERR(phy);
> - goto put_usb2_hcd;
> - }
> - mtk->phys[phy_num] = phy;
> - }
> -
> - ret = xhci_mtk_phy_init(mtk);
> - if (ret)
> - goto put_usb2_hcd;
> -
> - ret = xhci_mtk_phy_power_on(mtk);
> - if (ret)
> - goto exit_phys;
> -
>   device_init_wakeup(dev, 

Re: [PATCH v5 1/6] dt-bindings: add bindings for USB physical connector

[Andrzej Hajda]

On 02.03.2018 14:13, Heikki Krogerus wrote:
> Hi,
>
> On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
>> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
>> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
>> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
>> +
>> +ccic: s2mm005@33 {
>> +...
>> +usb_con: connector {
>> +compatible = "usb-c-connector";
>> +label = "USB-C";
> Is this child node really necessary? There will never be more then
> one connector per CC line.

But there can be more connectors/cc-lines per IC, for example EZ-PD CCG5[1].

[1]:
http://www.cypress.com/products/ez-pd-ccg5-two-port-usb-type-c-and-power-delivery

>
> We should prefer device_graph* functions over of_graph* and
I guess you mean fwnode_graph* functions.
> acpi_graph* functions in the drivers so we don't have to handle the
> same thing multiple times with separate APIs. Is it still possible if
> there is that connector child node?

Bindings proposed here are OF bindings, I suppose the most important is
to follow OF specification and guidelines and these bindings tries to
follow it.
It looks like it should not be a problem for fwnode framework to handle
such bindings, but it is just my guess. I have not seen any fwnode*
specification I am not sure what is the real purpose of this framework,
but it seems to be just in-kernel abstraction for different firmware
standards (OF, ACPI), so even if it lacks at the moment some
functionality it should not be a barrier for OF bindings.

Regards
Andrzej

>
>> +ports {
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +port@0 {
>> +reg = <0>;
>> +usb_con_hs: endpoint {
>> +remote-endpoint = <_usbc_hs>;
>> +};
>> +};
>> +port@1 {
>> +reg = <1>;
>> +usb_con_ss: endpoint {
>> +remote-endpoint = <_phy_ss>;
>> +};
>> +};
>> +port@2 {
>> +reg = <2>;
>> +usb_con_sbu: endpoint {
>> +remote-endpoint = <_aux>;
>> +};
>> +};
>> +};
>> +};
>> +};
>
> Thanks,
>

--
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: howto debug xhci driver?

[Felipe Balbi]

Hi,

Bin Liu  writes:
> I am relatively new to xhci and its driver. I am trying to get a xhci
> driver runtime log to understand how it handles usb transactions, but I
> don't get much information with dynamic debug (module xhci_hcd) or
> enabling xhci trace events. Is there any other methods you guys use to
> debug xhci driver?

tracepoints, the best thing since sliced bread ;-)

> BTY, the issue I am trying to debug is when reading bulk IN data from a
> USB2.0 device, if the device doesn't have data to transmit and NAKs the
> IN packet, after 4 pairs of IN-NAK transactions, xhci stops sending
> further IN tokens until the next SOF, which leaves ~90us gape on the
> bus.
>
> But when reading data from a USB2.0 thumb drive, this issue doesn't
> happen, even if the device NAKs the IN tokens, xhci still keeps sending
> IN tokens, which is way more than 4 pairs of IN-NAK transactions.

Thumb drive has Bulk endpoints, what is the other device's transfer type?

> Any one has a clue on what causes xhci to stop sending IN tokens after
> the device NAK'd 4 times?

tracepoints, please :-)

-- 
balbi
--
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