Re: [PATCH] usb: storage: Add ums-cros-aoa driver
> Why not do the mode switch from userspace? I thought we were trying to move > all the mode-switching stuff in that direction. I need to tie in to the main USB mass storage driver in a way that I think would make it too complicated to move the mode switching part to userspace. See the part I'm adding to initializers.c... that one has to be in the kernel since it operates on the device after the mode switch when it is claimed by the normal USB storage driver. But the mode switch part has to communicate information to it to make sure it picks up the right device (just relying on the normal USB device matching isn't enough in this case, because all Android devices in AOA mode identify themselves with that well-known VID/PID... I don't know if there's any other kernel driver using this protocol today, but there may be at some point and then it becomes important to make sure you really grab the device you meant to grab). Some of that information (the 'route' field) isn't even directly available from userspace (I could use the device name string instead and that would roughly come out to the same thing, but seems less clean to me). So I could either do the mode switch in userspace and add a big custom sysfs interface to the usb-storage driver to allow userspace to configure all this, or I can add a small kernel shim driver like in this patch. Considering how little code the shim driver actually needs I expect it would come out to roughly the same amount of kernel code in both cases, and I feel like this version is much simpler to follow and fits cleaner in the existing "unusual device" driver infrastructure.
[PATCH] usb: storage: Add ums-cros-aoa driver
This patch adds a new "unusual" USB mass storage device driver. This
driver will be used for a virtual USB storage device presented by an
Android phone running the 'Chrome OS Recovery'* Android app. This app
uses the Android Open Accessory (AOA) API to talk directly to a USB host
attached to the phone.
The AOA protocol requires the host to send a custom vendor command on
EP0 to "switch" the phone into "AOA mode" (making it drop off the bus
and reenumerate with different descriptors). The ums-cros-aoa driver is
just a small stub driver to send these vendor commands. It identifies
the device it should operate on by VID/PID passed in through a module
parameter (e.g. from the bootloader). After the phone is in AOA mode,
the normal USB mass storage stack will recognize it by its special
VID/PID like any other "unusual dev". An initializer function will
further double-check that the device is the device previously operated
on by ums-cros-aoa.
*NOTE: The Android app is still under development and will be released
at a later date. I'm submitting this patch now so that the driver name
and module parameters can be set in stone already, because I have to
bake them into bootloader code that is not field-updatable.
Signed-off-by: Julius Werner
---
drivers/usb/storage/Kconfig| 12 +++
drivers/usb/storage/Makefile | 2 +
drivers/usb/storage/cros-aoa.c | 129 +
drivers/usb/storage/initializers.c | 34
drivers/usb/storage/initializers.h | 4 +
drivers/usb/storage/unusual_devs.h | 18
6 files changed, 199 insertions(+)
create mode 100644 drivers/usb/storage/cros-aoa.c
diff --git a/drivers/usb/storage/Kconfig b/drivers/usb/storage/Kconfig
index 59aad38b490a6..cc901ee2bb766 100644
--- a/drivers/usb/storage/Kconfig
+++ b/drivers/usb/storage/Kconfig
@@ -184,6 +184,18 @@ config USB_STORAGE_ENE_UB6250
To compile this driver as a module, choose M here: the
module will be called ums-eneub6250.
+config USB_STORAGE_CROS_AOA
+ tristate "Support for connecting to Chrome OS Recovery Android app"
+ default n
+ depends on USB_STORAGE
+ help
+ Say Y here if you want to connect an Android phone running the Chrome
+ OS Recovery app to this device and mount the image served by that app
+ as a virtual storage device. Unless you're building for Chrome OS, you
+ probably want to say N.
+
+ If this driver is compiled as a module, it will be named ums-cros-aoa.
+
endif # USB_STORAGE
config USB_UAS
diff --git a/drivers/usb/storage/Makefile b/drivers/usb/storage/Makefile
index a67ddcbb4e249..f734741d4658b 100644
--- a/drivers/usb/storage/Makefile
+++ b/drivers/usb/storage/Makefile
@@ -17,6 +17,7 @@ usb-storage-y += usual-tables.o
usb-storage-$(CONFIG_USB_STORAGE_DEBUG) += debug.o
obj-$(CONFIG_USB_STORAGE_ALAUDA) += ums-alauda.o
+obj-$(CONFIG_USB_STORAGE_CROS_AOA) += ums-cros-aoa.o
obj-$(CONFIG_USB_STORAGE_CYPRESS_ATACB) += ums-cypress.o
obj-$(CONFIG_USB_STORAGE_DATAFAB) += ums-datafab.o
obj-$(CONFIG_USB_STORAGE_ENE_UB6250) += ums-eneub6250.o
@@ -31,6 +32,7 @@ obj-$(CONFIG_USB_STORAGE_SDDR55) += ums-sddr55.o
obj-$(CONFIG_USB_STORAGE_USBAT)+= ums-usbat.o
ums-alauda-y := alauda.o
+ums-cros-aoa-y := cros-aoa.o
ums-cypress-y := cypress_atacb.o
ums-datafab-y := datafab.o
ums-eneub6250-y:= ene_ub6250.o
diff --git a/drivers/usb/storage/cros-aoa.c b/drivers/usb/storage/cros-aoa.c
new file mode 100644
index 0..269e9193209d9
--- /dev/null
+++ b/drivers/usb/storage/cros-aoa.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2 WITH Linux-syscall-note
+/*
+ * Driver for Chrome OS Recovery via Android Open Accessory
+ *
+ * (c) 2019 Google LLC (Julius Werner )
+ *
+ * This driver connects to an Android device via the Android Open Accessory
+ * protocol to use it as a USB storage back-end. It is used for system recovery
+ * on Chrome OS. The descriptors sent are specific to the Chrome OS Recovery
app
+ * for Android. The driver is inert unless activated by boot firmware with an
+ * explicit kernel command line parameter.
+ */
+
+#include
+#include
+#include
+
+#include "initializers.h"
+
+#define DRV_NAME "ums-cros-aoa"
+
+MODULE_DESCRIPTION("Driver for Chrome OS Recovery via Android Open Accessory");
+MODULE_AUTHOR("Julius Werner ");
+MODULE_LICENSE("GPL");
+
+#define AOA_GET_PROTOCOL 51
+#define AOA_SET_STRING 52
+#define AOA_START 53
+
+#define AOA_STR_MANUFACTURER 0
+#define AOA_STR_MODEL 1
+#define AOA_STR_DESCRIPTION2
+#define AOA_STR_VERSION3
+#define AOA_STR_URI4
+#define AOA_STR_SERIAL 5
+
+#define CROS_MANUF "Google"
+#define CROS_MODEL "
Re: usb: dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s
> d9a14b00 339317035 C Ii:1:004:1 -32:1 0 > d9a14b00 339317049 S Ii:1:004:1 -115:1 10 < > d9a14b00 339318040 C Ii:1:004:1 -32:1 0 > d9a14b00 339318057 S Ii:1:004:1 -115:1 10 < > d9a14b00 339319042 C Ii:1:004:1 -32:1 0 > d9a14b00 339319056 S Ii:1:004:1 -115:1 10 < > d9a14b00 339329551 C Ii:1:004:1 -32:1 0 > d9a14b00 339329571 S Ii:1:004:1 -115:1 10 < > d9a14b00 339330586 C Ii:1:004:1 -32:1 0 > d9a14b00 339330601 S Ii:1:004:1 -115:1 10 < > d9a14b00 339331035 C Ii:1:004:1 -32:1 0 Sorry for necromancing an old thread, but I just happened to read through this and thought someone might care: If I read that right, the usbmon output shows that the interrupt endpoint is stalled (keeps returning -EPIPE). A STALL is a special device-side USB condition that tells the host something is wrong and will persist until cleared manually. It seems that the driver isn't prepared for this (see drivers/usb/serial/pl2303.c#pl2303_read_int_callback) and just keeps resubmitting the URB, so it will stall again as fast as the endpoint allows it to. This may be the reason why you get so many transfers that it overwhelms the CPU. A fix would be to catch -EPIPE in that function and handle it explicitly (with either a CLEAR_STALL to the endpoint or a full USB reset... would have to look at the documentation for PL2303 to see what the stall actually means and how you're supposed to treat it). -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] usb: dwc2: hcd: fix split schedule issue
> To handle things smarter, I think I need to research how to deal with > hubs attached to hubs attached to hubs. For instance: > > dwc2 > -> multi_tt hub > -> single_tt hub > -> device 1 > -> device 2 Keep in mind that there's always at most one (active) TT between host and device. The TT is the point where high-speed traffic is translated to low-/full-speed traffic, so after that you cannot translate again. With multiple hubs you either have -> high-speed 2.0 hub (TT inactive / irrelevant for this path) -> multi or single TT 2.0 hub -> device or -> multi or single TT 2.0 hub -> full-speed 1.1 hub -> device All the information you need should already be in struct usb_device. If udev->tt->multi == 0, then it must be scheduled in the same group as all other devices it shares udev->tt (the same pointer address) with. If udev->tt->multi == 1, then it belongs in the same group as all that have the same udev->tt and the same udev->ttport. There's even a udev->tt->hcpriv where you could link a data structure (array) in to keep track of these matching devices. I agree that this is a nice-to-have optimization, though... it's more important to get the thing stable, and I think it's fine to assume that all low-/full-speed transfers go on the same bus for the first iteration. -- 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/2] usb: dwc2: host: Fix missing device insertions
Another point to note, which I think is what prevents the flow Alan
suggested from working, is this little snippet in DWC2's hub_control
GetPortStatus callback:
if (!hsotg->flags.b.port_connect_status) {
/*
* The port is disconnected, which means the core is
* either in device mode or it soon will be.
Just
* return 0's for the remainder of the port status
* since the port register can't be read if the core
* is in device mode.
*/
*(__le32 *)buf = cpu_to_le32(port_status);
break;
}
This is before it actually checks the register state of the port. So
it relies on dwc2_hcd_connect() and dwc2_hcd_disconnect() to be called
in the right order to give the correct answer for
USB_PORT_STAT_CONNECTION. This could maybe be improved but I don't
know what kind of weird interactions with device mode operation made
that snippet necessary in the first place.
--
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: [REPOST PATCH 3/3] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
> But I don't see how you will make it work when the root hub itself is > not enabled for wakeup and a non-hub device plugged into one of the > root hub's ports is enabled. > > It seems like you would need a usb_hcd_wakeup_not_needed(hcd, port) > subroutine. We'd just put that in the Tegra platform driver, I guess. Iterate over all ports, call usb_wakeup_enabled_descendants() on the connected device (if any) and disable that port's PHY if it returns 0 or wasn't connected. Since usb_wakeup_enabled_descendants() also counts do_remote_wakeup from the device itself and is safe to call even on non-hubs, that should work for all cases. -- 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: [REPOST PATCH 3/3] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
> Doug, how would you feel about reworking the patch that exports > usb_wakeup_enabled_descendants()? Instead of doing it that way, create > and export a new subroutine in hcd.c called > usb_hcd_wakeup_not_needed(), or something similar. We have a use case with another host controller (Tegra, which I think is still in the process of being upstreamed) where we are able to power down PHYs (and thus reduce power consumption) per port. I think we should prefer the more flexible 'number of wake devices in subtree' interface to be able to support cases like that. (And for the simple case, 'if (usb_hcd_wakeup_not_needed(hcd))' and 'if (!usb_wakeup_enabled_descendants(hcd->self.root_hub))' look pretty similar anyway.) -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 18/22] usb: dwc2: host: don't use dma_alloc_coherent with irqs disabled
->qh->dw_align_buf_size,
> + chan->ep_is_in ?
> + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> + if (chan->ep_is_in)
> + memcpy(urb->buf + frame_desc->offset +
> + qtd->isoc_split_offset,
> + chan->qh->dw_align_buf,
> + frame_desc->actual_length);
> }
> break;
> case DWC2_HC_XFER_FRAME_OVERRUN:
> @@ -584,13 +594,18 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state(
> chan, chnum, qtd, halt_status, NULL);
>
> /* Non DWORD-aligned buffer case handling */
> - if (chan->align_buf && frame_desc->actual_length &&
> - chan->ep_is_in) {
> + if (chan->align_buf && frame_desc->actual_length) {
> dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n",
> __func__);
> - memcpy(urb->buf + frame_desc->offset +
> - qtd->isoc_split_offset, chan->qh->dw_align_buf,
> - frame_desc->actual_length);
> + dma_unmap_single(hsotg->dev,
> chan->qh->dw_align_buf_dma,
> + chan->qh->dw_align_buf_size,
> + chan->ep_is_in ?
> + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> + if (chan->ep_is_in)
> + memcpy(urb->buf + frame_desc->offset +
> + qtd->isoc_split_offset,
> + chan->qh->dw_align_buf,
> + frame_desc->actual_length);
> }
>
> /* Skip whole frame */
> @@ -926,6 +941,8 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg
> *hsotg,
>
> if (chan->align_buf) {
> dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
> + dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
> + chan->qh->dw_align_buf_size, DMA_FROM_DEVICE);
> memcpy(qtd->urb->buf + frame_desc->offset +
>qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
> }
> @@ -1155,8 +1172,14 @@ static void dwc2_update_urb_state_abn(struct
> dwc2_hsotg *hsotg,
> /* Non DWORD-aligned buffer case handling */
> if (chan->align_buf && xfer_length && chan->ep_is_in) {
> dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
> - memcpy(urb->buf + urb->actual_length, chan->qh->dw_align_buf,
> - xfer_length);
> + dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
> + chan->qh->dw_align_buf_size,
> + chan->ep_is_in ?
> + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> + if (chan->ep_is_in)
> + memcpy(urb->buf + urb->actual_length,
> + chan->qh->dw_align_buf,
> + xfer_length);
> }
>
> urb->actual_length += xfer_length;
> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
> index 63207dc..3735ae6 100644
> --- a/drivers/usb/dwc2/hcd_queue.c
> +++ b/drivers/usb/dwc2/hcd_queue.c
> @@ -231,9 +231,10 @@ void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct
> dwc2_qh *qh)
> {
> if (hsotg->core_params->dma_desc_enable > 0)
> dwc2_hcd_qh_free_ddma(hsotg, qh);
> - else if (qh->dw_align_buf)
> - dma_free_coherent(hsotg->dev, qh->dw_align_buf_size,
> - qh->dw_align_buf, qh->dw_align_buf_dma);
> + else if (qh->dw_align_buf) {
> + kfree(qh->dw_align_buf);
> + qh->dw_align_buf_dma = (dma_addr_t)0;
> + }
> kfree(qh);
> }
>
> --
> 2.3.3
Otherwise, I think this looks good now, thanks!
Reviewed-by: Julius Werner
--
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 v1 18/20] usb: dwc2: host: don't use dma_alloc_coherent with irqs disabled
>> > - qh->dw_align_buf = dma_alloc_coherent(hsotg->dev, buf_size,
>> > -
>> > &qh->dw_align_buf_dma,
>> > - GFP_ATOMIC);
>> > + qh->dw_align_buf = kzalloc(buf_size, GFP_ATOMIC);
>>
>> Shouldn't this also set GFP_DMA? [...]
>
> No, it should be GFP_ATOMIC, as this can be reached from interrupt handler.
Are the two mutually exclusive? I meant that I think this should be
'kzalloc(buf_size, GFP_DMA | GFP_ATOMIC)', because I wouldn't be sure
that GFP_ATOMIC always returns DMA-able memory on systems that may
have limitations there.
>> > }
>> > }
>> >
>> > + qh->dw_align_buf_dma = dma_map_single(hsotg->dev,
>> > + qh->dw_align_buf, qh->dw_align_buf_size,
>> > + DMA_TO_DEVICE);
>>
>> Documentation/DMA-API.txt says that you must always use the same
>> arguments for matching dma_map_single() and dma_unmap_single()... so I
>> think this and all the unmaps should use DMA_BIDIRECTIONAL.
>
> The mapping is wrong. It should consider endpoint direction. Something like
> chan->ep_is_in ? DMA_FROM_DEVICE : DMA_TO_DEVICE
Yeah, I think this should work as well.
However, looking at this again I think there are more problems on
unmapping. Since some of those calls are guarded by chan->ep_is_in,
you will not unmap the memory out OUT transfers. DMA map/unmap calls
must always be balanced.
I think in all the cases where the code previously had something like
if (chan->align_buf && chan->ep_is_in) {
memcpy(...);
...;
}
you'll need to change it into
if (chan->align_buf) {
if (chan->ep_is_in) {
memcpy(...);
dma_unmap_single(..., DMA_FROM_DEVICE);
...
} else {
dma_unmap_single(..., DMA_TO_DEVICE);
}
}
You might also want to double-check all abnormal error paths to ensure
you're not leaking a DMA mapping. The previous code might not have
been so careful (since for a really bad error you usually don't care
about memcpy()ing the receive buffer back), but if you use DMA
mappings you have to.
--
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 v1 18/20] usb: dwc2: host: don't use dma_alloc_coherent with irqs disabled
[once more, without Gmail being stupid]
Nice! This also solves problems with exhausting coherent DMA memory
when you plug too many devices in.
On Tue, Mar 17, 2015 at 2:54 AM, Mian Yousaf Kaukab
wrote:
> From: Gregory Herrero
>
> Align buffer must be allocated using kmalloc since irqs are disabled.
> Coherency is handled through dma_map_single which can be used with irqs
> disabled.
>
> Signed-off-by: Gregory Herrero
> ---
> drivers/usb/dwc2/hcd.c | 7 ---
> drivers/usb/dwc2/hcd_intr.c | 10 ++
> drivers/usb/dwc2/hcd_queue.c | 7 ---
> 3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index fd2ad25..54f58c1 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -710,9 +710,7 @@ static int dwc2_hc_setup_align_buf(struct dwc2_hsotg
> *hsotg, struct dwc2_qh *qh,
> /* 3072 = 3 max-size Isoc packets */
> buf_size = 3072;
>
> - qh->dw_align_buf = dma_alloc_coherent(hsotg->dev, buf_size,
> - &qh->dw_align_buf_dma,
> - GFP_ATOMIC);
> + qh->dw_align_buf = kzalloc(buf_size, GFP_ATOMIC);
Shouldn't this also set GFP_DMA? (And does it need to be kzalloc()? I
think kmalloc() should be enough and might avoid another memory
transfer.)
> if (!qh->dw_align_buf)
> return -ENOMEM;
> qh->dw_align_buf_size = buf_size;
> @@ -737,6 +735,9 @@ static int dwc2_hc_setup_align_buf(struct dwc2_hsotg
> *hsotg, struct dwc2_qh *qh,
> }
> }
>
> + qh->dw_align_buf_dma = dma_map_single(hsotg->dev,
> + qh->dw_align_buf, qh->dw_align_buf_size,
> DMA_TO_DEVICE);
Documentation/DMA-API.txt says that you must always use the same
arguments for matching dma_map_single() and dma_unmap_single()... so I
think this and all the unmaps should use DMA_BIDIRECTIONAL.
> +
> chan->align_buf = qh->dw_align_buf_dma;
> return 0;
> }
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index 6927bba..22f1476 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -468,6 +468,8 @@ static int dwc2_update_urb_state(struct dwc2_hsotg *hsotg,
> /* Non DWORD-aligned buffer case handling */
> if (chan->align_buf && xfer_length && chan->ep_is_in) {
> dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
> + dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
> + chan->qh->dw_align_buf_size, DMA_FROM_DEVICE);
> memcpy(urb->buf + urb->actual_length, chan->qh->dw_align_buf,
>xfer_length);
> }
> @@ -559,6 +561,8 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state(
> chan->ep_is_in) {
> dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n",
> __func__);
> + dma_unmap_single(hsotg->dev,
> chan->qh->dw_align_buf_dma,
> + chan->qh->dw_align_buf_size, DMA_FROM_DEVICE);
> memcpy(urb->buf + frame_desc->offset +
>qtd->isoc_split_offset, chan->qh->dw_align_buf,
>frame_desc->actual_length);
> @@ -588,6 +592,8 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state(
> chan->ep_is_in) {
> dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n",
> __func__);
> + dma_unmap_single(hsotg->dev,
> chan->qh->dw_align_buf_dma,
> + chan->qh->dw_align_buf_size, DMA_FROM_DEVICE);
> memcpy(urb->buf + frame_desc->offset +
>qtd->isoc_split_offset, chan->qh->dw_align_buf,
>frame_desc->actual_length);
> @@ -926,6 +932,8 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg
> *hsotg,
>
> if (chan->align_buf) {
> dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
> + dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
> + chan->qh->dw_align_buf_size, DMA_FROM_DEVICE);
> memcpy(qtd->urb->buf + frame_desc->offset +
>qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
> }
> @@ -1155,6 +1163,8 @@ static void dwc2_update_urb_state_abn(struct dwc2_hsotg
> *hsotg,
> /* Non DWORD-aligned buffer case handling */
> if (chan->align_buf && xfer_length && chan->ep_is_in) {
> dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
> + dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
> +
Re: [RFC PATCH v1] usb: dwc2: reduce dwc2 driver probe time
> @@ -2703,7 +2703,7 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg) > gusbcfg = readl(hsotg->regs + GUSBCFG); > gusbcfg &= ~GUSBCFG_FORCEHOSTMODE; > writel(gusbcfg, hsotg->regs + GUSBCFG); > - usleep_range(10, 15); > + usleep_range(25000, 5); The point of usleep_range() is to coalesce multiple timer interrupts in idle systems for power efficiency. It's pretty pointless/harmful during probe anyway and there's almost never a reason to make the span larger than a few milliseconds. You should reduce this to something reasonable (e.g. usleep_range(25000, 26000) or even usleep_range(25000, 25000)) to save another chunk of time. Same applies to other delays above. > do you know what's the upper boundary for AHB clock ? How fast can it > be? It's not wise to change timers because "it works on my RK3288 > board", you need to guarantee that this won't break anybody else. But this code is already a loop that spins on the AHBIdle bit, right? It should work correctly regardless of the delay. The only question is whether the code could be more efficient with a longer sleep... but since the general recommendation is to delay for ranges less than 10us, and the AHB clock would need to be lower than 100KHz (the ones I see are usually in the range of tens or hundreds of MHz) to take longer than that, this seems reasonable to me. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] usb: Retry port status check on resume to work around RH bugs
The EHCI controller on the RK3288 SoC is violating basic parts of the
USB spec and thereby unable to properly resume a suspended port. It does
not start SOF generation within 3ms of finishing resume signaling, so
the attached device will drop of the bus again. This is a particular
problem with runtime PM, where accessing the device will trigger a
resume that immediately makes it unavailable (and reenumerate with a new
handle).
Thankfully, the persist feature is generally able to work around stuff
like that. Unfortunately, it doesn't quite work in this particular case
because the controller will turn off the CurrentConnectStatus bit for an
instant while the device is reconnecting, which causes the kernel to
conclude that it permanently disappeared. This patch adds a tiny retry
mechanism to the core port resume code which will catch this case and
shouldn't have any notable impact on other controllers.
Signed-off-by: Julius Werner
---
drivers/usb/core/hub.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index aeb50bb..26bdd96 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2896,10 +2896,12 @@ static int port_is_suspended(struct usb_hub *hub,
unsigned portstatus)
*/
static int check_port_resume_type(struct usb_device *udev,
struct usb_hub *hub, int port1,
- int status, unsigned portchange, unsigned portstatus)
+ int status, u16 portchange, u16 portstatus)
{
struct usb_port *port_dev = hub->ports[port1 - 1];
+ int retries = 3;
+ retry:
/* Is a warm reset needed to recover the connection? */
if (status == 0 && udev->reset_resume
&& hub_port_warm_reset_required(hub, port1, portstatus)) {
@@ -2907,10 +2909,17 @@ static int check_port_resume_type(struct usb_device
*udev,
}
/* Is the device still present? */
else if (status || port_is_suspended(hub, portstatus) ||
- !port_is_power_on(hub, portstatus) ||
- !(portstatus & USB_PORT_STAT_CONNECTION)) {
+ !port_is_power_on(hub, portstatus)) {
if (status >= 0)
status = -ENODEV;
+ } else if (!(portstatus & USB_PORT_STAT_CONNECTION)) {
+ if (retries--) {
+ usleep_range(200, 300);
+ status = hub_port_status(hub, port1, &portstatus,
+&portchange);
+ goto retry;
+ }
+ status = -ENODEV;
}
/* Can't do a normal resume if the port isn't enabled,
--
2.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: Retry port status check on resume to work around RH bugs
The EHCI controller on the RK3288 SoC is violating basic parts of the
USB spec and thereby unable to properly resume a suspended port. It does
not start SOF generation within 3ms of finishing resume signaling, so
the attached device will drop off the bus again. This is a particular
problem with runtime PM, where accessing the device will trigger a
resume that immediately makes it unavailabe (and reenumerate with a new
handle).
Thankfully, the persist feature is generally able to work around stuff
like that. Unfortunately, it doesn't quite work in this particular case
because the controller will turn off the CurrentConnectStatus bit for an
instant while the device is reconnecting, which causes the kernel to
conclude that it permanently disappeared. This patch adds a tiny retry
mechanism to the core port resume code which will catch this case and
shouldn't have any notable impact on other controllers.
Signed-off-by: Julius Werner
---
drivers/usb/core/hub.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index aeb50bb..d1d0a66 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2896,10 +2896,12 @@ static int port_is_suspended(struct usb_hub *hub,
unsigned portstatus)
*/
static int check_port_resume_type(struct usb_device *udev,
struct usb_hub *hub, int port1,
- int status, unsigned portchange, unsigned portstatus)
+ int status, u16 portchange, u16 portstatus)
{
struct usb_port *port_dev = hub->ports[port1 - 1];
+ int retries = 3;
+retry:
/* Is a warm reset needed to recover the connection? */
if (status == 0 && udev->reset_resume
&& hub_port_warm_reset_required(hub, port1, portstatus)) {
@@ -2909,8 +2911,15 @@ static int check_port_resume_type(struct usb_device
*udev,
else if (status || port_is_suspended(hub, portstatus) ||
!port_is_power_on(hub, portstatus) ||
!(portstatus & USB_PORT_STAT_CONNECTION)) {
- if (status >= 0)
+ if (status >= 0) {
+ if (retries--) {
+ usleep_range(200, 300);
+ status = hub_port_status(hub, port1,
+ &portstatus, &portchange);
+ goto retry;
+ }
status = -ENODEV;
+ }
}
/* Can't do a normal resume if the port isn't enabled,
--
2.1.2
--
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: Retry port status check on resume to work around RH bugs
> You should use a sleeping function call, not a delay. Whoops... yes, of course. Guess too much firmware work made me forget what multithreading is there for a moment. Will send a v2. -- 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: Retry port status check on resume to work around RH bugs
The EHCI controller on the RK3288 SoC is violating basic parts of the
USB spec and thereby unable to properly resume a suspended port. It does
not start SOF generation within 3ms of finishing resume signaling, so
the attached device will drop off the bus again. This is a particular
problem with runtime PM, where accessing the device will trigger a
resume that immediately makes it unavailabe (and reenumerate with a new
handle).
Thankfully, the persist feature is generally able to work around stuff
like that. Unfortunately, it doesn't quite work in this particular case
because the controller will turn off the CurrentConnectStatus bit for an
instant while the device is reconnecting, which causes the kernel to
conclude that it permanently disappeared. This patch adds a tiny retry
mechanism to the core port resume code which will catch this case and
shouldn't have any notable impact on other controllers.
Signed-off-by: Julius Werner
---
drivers/usb/core/hub.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index aeb50bb..d1d0a66 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2896,10 +2896,12 @@ static int port_is_suspended(struct usb_hub *hub,
unsigned portstatus)
*/
static int check_port_resume_type(struct usb_device *udev,
struct usb_hub *hub, int port1,
- int status, unsigned portchange, unsigned portstatus)
+ int status, u16 portchange, u16 portstatus)
{
struct usb_port *port_dev = hub->ports[port1 - 1];
+ int retries = 3;
+retry:
/* Is a warm reset needed to recover the connection? */
if (status == 0 && udev->reset_resume
&& hub_port_warm_reset_required(hub, port1, portstatus)) {
@@ -2909,8 +2911,15 @@ static int check_port_resume_type(struct usb_device
*udev,
else if (status || port_is_suspended(hub, portstatus) ||
!port_is_power_on(hub, portstatus) ||
!(portstatus & USB_PORT_STAT_CONNECTION)) {
- if (status >= 0)
+ if (status >= 0) {
+ if (retries--) {
+ udelay(200);
+ status = hub_port_status(hub, port1,
+ &portstatus, &portchange);
+ goto retry;
+ }
status = -ENODEV;
+ }
}
/* Can't do a normal resume if the port isn't enabled,
--
2.1.2
--
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: ehci-platform: Support ehci reset after resume quirk
>> The EHCI controller doesn't properly detect the case when > > "The" EHCI controller? I don't know what EHCI controller you're > talking about, but my controllers don't have any trouble detecting > device removal during suspend. This is similar to other SoC-based controllers that loose state in suspend, like on Samsung's Exynos. Usually we catch that with the FLAG_CF check in ehci_resume(). In the case of RK3288 it unfortunately leaves the CF flag (and other things, like PORTSC[PED] bits) set, although it doesn't react to any events correctly. If the device looses its power session the controller won't notice and then on resume get stuck trying to send resume signals to something that had been reset (or newly plugged in). I think since we can't trust the controller to do anything right, the safest thing is to fall back to the solution of resetting everything (at least we know that works), and since the FLAG_CF check doesn't work we need another solution to mark which controllers are affected. > Isn't this solution too extreme? What if the device was a flash > storage drive and it wasn't unplugged during suspend? This patch would > force it to be removed, messing up any mounted filesystems, when there > was no need. > > Can you find a better way to work around the problem? As Doug said, I think persist is the solution. We have essentially the same case: all we know is that there is now a device connected to the same port that a device had been connected during suspend... but with no guarantees whether it is the same device or in the same state. By forcing people to use persist, we acknowledge that this has the same risks (e.g. data corruption if a mounted mass storage device was swapped out for another one), and we benefit from the same safety checks like comparing the serial number. -- 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: Re: [PATCH v2] usb: dwc2: resume root hub when device detect with suspend state
On Wed, Nov 19, 2014 at 1:47 AM, 李云志 wrote: > hi Julius & Alan > > Shall we use dwc2's private status "hsotg->lx_state" here instesd of > "hcd->state" for checking root hub is in suspend state ? > I see the EHCI driver do something like this(ehci->rh_state == > EHCI_RH_SUSPENDED) before resume the root hub. It's not this simple, because lx_state only relates to the status of the one port on the DWC2. That may be suspended even though the root hub is not. The USB core differentiates between suspending individual ports, and suspending the whole root hub (which should automatically suspend all ports in a host-controller-specific manner). This distinction may seem silly on DWC2 because there is only one port to suspend, but you still need to make it so that the USB core doesn't get confused. So the different things you need to keep track of are: * is the one port individually suspended (through the hub_control(SetPortFeature(PORT_SUSPEND)) method) * is the root hub suspended (through calling bus_suspend()) * if the root hub gets suspended (through bus_suspend()), had the port already been suspended before that (through a earlier hub_control())... this is the thing I mentioned in your other patch You can decide whether you want to bake that all into one variable somehow or make it three, but we need to be able to tell all of these things apart. The third bullet point will also require you to prevent races of hub_control() with bus_suspend() (not sure if the kernel already guarantees that or not), so I think we may have to rethink the way the spinlock works (because it currently doesn't cover that). -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: dwc2: resume root hub when device detect with suspend state
>> You should be aware that it's not safe to use hcd->state for anything >> in a host controller driver. That field is owned by usbcore, not by >> the HCD, and it is not protected by any locks. >> >> Thus, for example, hcd->state does not get set to HC_STATE_SUSPENDED >> until some time after the bus_suspend routine has returned. A >> port-change interrupt might occur during that time interval. Looks like there is explicit code in hcd_bus_suspend() to check for that race condition right after setting hcd->state, or do I misinterpret that (the "Did we race with a root-hub wakeup event?" part)? Also, it seems xhci_bus_suspend() explicitly sets 'hcd->state = HC_STATE_SUSPENDED' before giving up the spinlock for some undocumented reason, maybe to avoid exactly this problem. We could just copy that trick if the hcd.c solution isn't enough (although the DWC2 bus_suspend/bus_resume in the other patch don't grab that spinlock right now, where I'm also not so sure if that's a good idea...). -- 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: dwc2: resume root hub when device detect with suspend state
On Mon, Nov 17, 2014 at 5:14 AM, Kever Yang wrote: > After we implement the bus_suspend/resume, auto suspend id enabled. > The root hub will be auto suspend if there is no device connected, > we need to resume the root hub when a device connect detect. > > This patch tested on rk3288. > > Signed-off-by: Roy Li > Signed-off-by: Kever Yang > --- > > drivers/usb/dwc2/hcd_intr.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c > index 551ba87..c8299fd 100644 > --- a/drivers/usb/dwc2/hcd_intr.c > +++ b/drivers/usb/dwc2/hcd_intr.c > @@ -355,6 +355,13 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) > hprt0_modify |= HPRT0_CONNDET; > > /* > +* Check if root hub is in suspend state > +* if root hub in suspend, resume it. > +*/ > + if ((bus->root_hub) && (hcd->state == HC_STATE_SUSPENDED)) What is bus->root_hub checking for? Is there any chance that this could be NULL here? > + usb_hcd_resume_root_hub(hcd); > + > + /* > * The Hub driver asserts a reset when it sees port connect > * status change flag > */ > -- > 1.9.1 Seems sensible in general. Does this actually fix the problem Doug was reporting? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb: dwc2: add bus suspend/resume for dwc2
> I will figure out how to make dwc2 detect the device connect after auto > suspend, > or disable the auto suspend feature for the dwc2 hcd. I think auto-suspend of the root hub device (which is what calls bus_suspend, but is not the host controller device itself) is expected to always happen and not really meant to be disabled. I'm surprised that the controller would fail to come back up, though. Does removing the PCGCTL part make it work? That's the only thing I can think of (but then again the function should immediately return if the port is not in L0, so if there is nothing plugged in the suspend shouldn't really do anything). Another thing might be that the port connect interrupt does not correctly resume the root hub. I don't really know many details about how that works, and it seems pretty complicated. But I can see that all other HCDs seem to call usb_hcd_resume_root_hub() from their interrupt handlers, which we don't. There's also a usb_hcd_start_port_resume() in EHCI which I'm not familiar with, that seems to have been added recently. And finally, it seems that all normal host controllers (UHCI/OHCI/EHCI/XHCI) now do the usb_hcd->uses_new_polling thing (where you're supposed to call hcd_poll_rh_status() from the HCD code)... the old polling code still seems to be in place, but without any relevant driver using I wouldn't be so sure if it's still functional. +Alan who should know HCD/core interactions much better and might have some ideas what the DWC2 driver is still missing right now. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: dwc2: add bus suspend/resume for dwc2
>> The contract for bus_suspend() is that it will suspend all ports not >> yet suspended, keep track of those ports and then only resume those in >> bus_resume() (compare, for example, how XHCI keeps track of that with >> xhci_bus_state.bus_suspended in xhci_bus_suspend/resume()). So you >> need something here to remember whether this function suspended the >> port or whether it had already been suspended, and then only resume >> the port in bus_resume() in the former case. > > In fact, the dwc2 controller only support one port, so the hprt0 > is the only one port we need to care. Yes, I know, but that one port still needs to play by the rules the USB core expects. All I'm saying is: if the port was already suspended during bus_suspend(), then the next bus_resume() should not resume it. The rest looks good to me now. But in order to get it really working, I think we'll still need the actual driver.pm suspend/resume methods, at least for the HCD_FLAG_HW_ACCESSIBLE and the usb_root_hub_lost_power() handling (probably better in a separate 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] usb: dwc2: add bus suspend/resume for dwc2
On Wed, Nov 5, 2014 at 5:30 PM, Kever Yang wrote:
> Hcd controller needs bus_suspend/resume, dwc2 controller make
> root hub generate suspend/resume signal with hprt0 register
> when work in host mode.
> After the root hub enter suspend, we can make controller enter
> low power state with PCGCTL register.
You say you do this, but I don't actually see you doing it (for the
not-connected case)?
>
> We also update the lx_state for hsotg state.
>
> This patch has tested on rk3288 with suspend/resume.
>
> Signed-off-by: Kever Yang
> ---
>
> Changes in v2:
> - update commit message
> - make dwc2 suspend/resume sourcecode work
>
> drivers/usb/dwc2/hcd.c | 78
> +++---
> 1 file changed, 67 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 0a0e6f0..01a415b 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -1471,6 +1471,30 @@ static void dwc2_port_suspend(struct dwc2_hsotg
> *hsotg, u16 windex)
> }
> }
>
> +static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
> +{
> + u32 hprt0;
> +
> + /* After clear the Stop PHY clock bit, we should wait for a moment
> +* for PLL work stable with clock output.
> +*/
> + writel(0, hsotg->regs + PCGCTL);
> + usleep_range(2000, 4000);
> +
> + hprt0 = dwc2_read_hprt0(hsotg);
> + hprt0 |= HPRT0_RES;
> + writel(hprt0, hsotg->regs + HPRT0);
> + hprt0 &= ~HPRT0_SUSP;
> + /* according to USB2.0 Spec 7.1.7.7, the host must send the resume
> +* signal for at least 20ms
> +*/
> + usleep_range(2, 25000);
> +
> + hprt0 &= ~HPRT0_RES;
> + writel(hprt0, hsotg->regs + HPRT0);
> + hsotg->lx_state = DWC2_L0;
> +}
> +
> /* Handles hub class-specific requests */
> static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
> u16 wvalue, u16 windex, char *buf, u16
> wlength)
> @@ -1516,17 +1540,7 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg
> *hsotg, u16 typereq,
> case USB_PORT_FEAT_SUSPEND:
> dev_dbg(hsotg->dev,
> "ClearPortFeature USB_PORT_FEAT_SUSPEND\n");
> - writel(0, hsotg->regs + PCGCTL);
> - usleep_range(2, 4);
> -
> - hprt0 = dwc2_read_hprt0(hsotg);
> - hprt0 |= HPRT0_RES;
> - writel(hprt0, hsotg->regs + HPRT0);
> - hprt0 &= ~HPRT0_SUSP;
> - usleep_range(10, 15);
> -
> - hprt0 &= ~HPRT0_RES;
> - writel(hprt0, hsotg->regs + HPRT0);
I'm curious why this didn't change lx_state back to DWC2_L0 before...
Paul, do you know?
> + dwc2_port_resume(hsotg);
> break;
>
> case USB_PORT_FEAT_POWER:
> @@ -2299,6 +2313,44 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
> usleep_range(1000, 3000);
> }
>
> +static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> +{
> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> + u32 hprt0;
> +
> + if (!((hsotg->op_state == OTG_STATE_B_HOST) ||
> + (hsotg->op_state == OTG_STATE_A_HOST)))
> + return 0;
> +
> + if (hsotg->lx_state != DWC2_L0)
What if the port is in L1 state? I don't think the driver supports LPM
right now, but the DWC2_L1 enum is defined so it may one day in the
future. Let's maybe at least add a TODO.
> + return 0;
In your original ChromiumOS version of this patch, you also set
PCGCTL_STOPPCLK here if the port was not connected. Is there a reason
that changed (does it not actually save power or something)?
> +
> + hprt0 = dwc2_read_hprt0(hsotg);
> + if (hprt0 & HPRT0_CONNSTS)
> + dwc2_port_suspend(hsotg, 1);
The contract for bus_suspend() is that it will suspend all ports not
yet suspended, keep track of those ports and then only resume those in
bus_resume() (compare, for example, how XHCI keeps track of that with
xhci_bus_state.bus_suspended in xhci_bus_suspend/resume()). So you
need something here to remember whether this function suspended the
port or whether it had already been suspended, and then only resume
the port in bus_resume() in the former case. Note that
dwc2_port_suspend() changes lx_state to DWC_L2 (at least in the
version I'm looking at right now), so you can't just rely on that
unless you explicitly set it back to something else here.
> +
> + return 0;
> +}
> +
> +static int _dwc2_hcd_resume(struct usb_hcd *hcd)
> +{
> + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> + u32 hprt0;
> +
> + if (!((hsotg->op_state == OTG_STATE_B_HOST) ||
> + (hsotg->op_state == OTG_STATE_A_HOST)))
> + return 0;
> +
> + if (hsotg->lx_state !=
Re: [PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800
>> right, use that to call phy_init() at the right time, then you need to >> add a new ->calibrate() method which, likely, will only be used by you >> ;-) > so you mean, the xhci should itself call phy_init() at a time suitable, > so that ->calibrate() is not required at all ? I'm not sure if that's a good idea because it would require phy_init() and calibrate() to always mean the same thing. The calibrate() for Exynos5420 needs to be called both during boot and after system resume, but there might be other platforms that don't want to completely shutdown and reinit their PHYs during suspend/resume with the same functions used for boot. For example, Tegra5 (proposed driver at http://www.spinics.net/lists/linux-usb/msg113093.html) can power-gate the PHY during suspend, but that's not running the same code as in the phy_init()/phy_shutdown() methods (the posted patch doesn't contain all power-gating code yet, but you can get an idea about how it has to work from https://chromium.googlesource.com/chromiumos/third_party/kernel-next/+/chromeos-3.10/drivers/usb/host/xhci-tegra.c). It would also mean that the initial phy_init() call must always come after the XHCI reset, and I am not sure if that would fit nicely with all platforms. > right, and what's more generic than adding the support for PHYs straight into > xHCI ? Well, if we are going to have a calibrate() method (as in "stuff the PHY does after every controller reset if it needs to"), we have to add it either to the XHCI stack or the USB core. I thought the USB core would be more generic, because in theory it's possible that e.g. an EHCI controller might have a similar requirement. (Also, we have the gen_phy pointer in a USB core structure (struct usb_hcd), so I thought making this feature generic to the USB core and thus available to all kinds of host controllers would be more natural.) -- 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: [RFTv3 PATCH] xhci: rework cycle bit checking for new dequeue pointers
ate->new_cycle_state = hw_dequeue & 0x1;
> - if (ep_ring->first_seg == ep_ring->first_seg->next &&
> - cur_td->last_trb < state->new_deq_ptr)
> - state->new_cycle_state ^= 0x1;
> + do {
> + if (!cycle_found && xhci_trb_virt_to_dma(new_seg, new_deq)
> + == (dma_addr_t)(hw_dequeue & ~0xf)) {
> + cycle_found = true;
> + if (td_last_trb_found)
> + break;
> + }
> + if (new_deq == cur_td->last_trb)
> + td_last_trb_found = true;
>
> - state->new_deq_ptr = cur_td->last_trb;
> - xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> - "Finding segment containing last TRB in TD.");
> - state->new_deq_seg = find_trb_seg(state->new_deq_seg,
> - state->new_deq_ptr, &state->new_cycle_state);
> - if (!state->new_deq_seg) {
> - WARN_ON(1);
> - return;
> - }
> + if (cycle_found &&
> + TRB_TYPE_LINK_LE32(new_deq->generic.field[3]) &&
> + new_deq->generic.field[3] & cpu_to_le32(LINK_TOGGLE))
> + state->new_cycle_state ^= 0x1;
> +
> + next_trb(xhci, ep_ring, &new_seg, &new_deq);
> +
> + /* Search wrapped around, bail out */
> + if (new_deq == ep->ring->dequeue) {
> + xhci_err(xhci, "Failed finding new dequeue state\n");
> + break;
> + }
> +
> + } while (!cycle_found || !td_last_trb_found);
nit: two spaces between cycle_found and ||
>
> - /* Increment to find next TRB after last_trb. Cycle if appropriate. */
> - trb = &state->new_deq_ptr->generic;
> - if (TRB_TYPE_LINK_LE32(trb->field[3]) &&
> - (trb->field[3] & cpu_to_le32(LINK_TOGGLE)))
> - state->new_cycle_state ^= 0x1;
> - next_trb(xhci, ep_ring, &state->new_deq_seg, &state->new_deq_ptr);
> + state->new_deq_seg = new_seg;
> + state->new_deq_ptr = new_deq;
>
> /* Don't update the ring cycle state for the producer (us). */
> xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> --
> 1.8.3.2
>
Great, I think this should work! Let's see what Maciej says after testing it.
Reviewed-by: Julius Werner
--
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: [RFTv2 PATCH] xhci: rework cycle bit checking for new dequeue pointers
I don't think this works. As I understand it, ep_ring->cycle_state contains the current cycle state at the enqueue pointer, not the dequeue pointer (it gets updated in inc_enq() but not in inc_deq() for transfer rings). That's the only reason we need to pull it out of the Endpoint Context at all... because we have long since overwritten our own software copy, when we originally enqueued the TD (and an arbitrary amount of further ones after it). I think if we really want to play it safe, any solution for this must keep track of and only stop searching after both passing last_trb and reaching hw_dequeue (taking the cycle_state off the latter), since there might be cases where either of them could be more than one TRB after the other. Maybe if you add another last_found variable and depend the loop on both you could implement that in a clear way? (Also, I'm wondering if we should just drop the code that skips whole segments to make it simpler, since I can hardly think of any real-world examples where a single TD would cover a whole segment.) -- 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: [RFT PATCH] xhci: rework cycle bit checking for new dequeue pointers
On Fri, Jul 25, 2014 at 8:47 AM, Mathias Nyman
wrote:
> Signed-off-by: Mathias Nyman
> ---
> drivers/usb/host/xhci-ring.c | 95
> +++-
> 1 file changed, 31 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 749fc68..35da18c 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -364,32 +364,6 @@ static void ring_doorbell_for_active_rings(struct
> xhci_hcd *xhci,
> }
> }
>
> -/*
> - * Find the segment that trb is in. Start searching in start_seg.
> - * If we must move past a segment that has a link TRB with a toggle cycle
> state
> - * bit set, then we will toggle the value pointed at by cycle_state.
> - */
> -static struct xhci_segment *find_trb_seg(
> - struct xhci_segment *start_seg,
> - union xhci_trb *trb, int *cycle_state)
> -{
> - struct xhci_segment *cur_seg = start_seg;
> - struct xhci_generic_trb *generic_trb;
> -
> - while (cur_seg->trbs > trb ||
> - &cur_seg->trbs[TRBS_PER_SEGMENT - 1] < trb) {
> - generic_trb = &cur_seg->trbs[TRBS_PER_SEGMENT - 1].generic;
> - if (generic_trb->field[3] & cpu_to_le32(LINK_TOGGLE))
> - *cycle_state ^= 0x1;
> - cur_seg = cur_seg->next;
> - if (cur_seg == start_seg)
> - /* Looped over the entire list. Oops! */
> - return NULL;
> - }
> - return cur_seg;
> -}
> -
> -
> static struct xhci_ring *xhci_triad_to_transfer_ring(struct xhci_hcd *xhci,
> unsigned int slot_id, unsigned int ep_index,
> unsigned int stream_id)
> @@ -459,9 +433,11 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
> struct xhci_virt_device *dev = xhci->devs[slot_id];
> struct xhci_virt_ep *ep = &dev->eps[ep_index];
> struct xhci_ring *ep_ring;
> - struct xhci_generic_trb *trb;
> + struct xhci_segment *new_seg;
> + union xhci_trb *new_deq, *tmp_trb;
> dma_addr_t addr;
> u64 hw_dequeue;
> + bool cycle_found = false;
>
> ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
> ep_index, stream_id);
> @@ -486,45 +462,36 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
> hw_dequeue = le64_to_cpu(ep_ctx->deq);
> }
>
> - /* Find virtual address and segment of hardware dequeue pointer */
> - state->new_deq_seg = ep_ring->deq_seg;
> - state->new_deq_ptr = ep_ring->dequeue;
> - while (xhci_trb_virt_to_dma(state->new_deq_seg, state->new_deq_ptr)
> - != (dma_addr_t)(hw_dequeue & ~0xf)) {
> - next_trb(xhci, ep_ring, &state->new_deq_seg,
> - &state->new_deq_ptr);
> - if (state->new_deq_ptr == ep_ring->dequeue) {
> - WARN_ON(1);
> - return;
> - }
> - }
> + new_seg = ep_ring->deq_seg;
> + new_deq = ep_ring->dequeue;
> + state->new_cycle_state = hw_dequeue & 0x1;
> /*
> -* Find cycle state for last_trb, starting at old cycle state of
> -* hw_dequeue. If there is only one segment ring, find_trb_seg() will
> -* return immediately and cannot toggle the cycle state if this search
> -* wraps around, so add one more toggle manually in that case.
> +* We want to find the pointer, segment and cycle state of the
> +* new trb (the one after current TD's last_trb). We know the
> +* cycle state at hw_dequeue, so walk the ring until it's found.
> +* Once found we can jump a whole segments, but carefully cross them
> +* to check for cycle toggles.
> */
> - state->new_cycle_state = hw_dequeue & 0x1;
> - if (ep_ring->first_seg == ep_ring->first_seg->next &&
> - cur_td->last_trb < state->new_deq_ptr)
> - state->new_cycle_state ^= 0x1;
> -
> - state->new_deq_ptr = cur_td->last_trb;
> - xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> - "Finding segment containing last TRB in TD.");
> - state->new_deq_seg = find_trb_seg(state->new_deq_seg,
> - state->new_deq_ptr, &state->new_cycle_state);
> - if (!state->new_deq_seg) {
> - WARN_ON(1);
> - return;
> - }
> -
> - /* Increment to find next TRB after last_trb. Cycle if appropriate. */
> - trb = &state->new_deq_ptr->generic;
> - if (TRB_TYPE_LINK_LE32(trb->field[3]) &&
> - (trb->field[3] & cpu_to_le32(LINK_TOGGLE)))
> - state->new_cycle_state ^= 0x1;
> - next_trb(xhci, ep_ring, &state->new_deq_seg, &state->new_deq_ptr);
> + do {
> + if (cycle_found) {
> + if
Re: [PATCH] usb: xhci: Fix Set TR Dequeue Pointer cycle state for quirky xHCs
> Hmm. Wouldn't it be safer to have a quirk for this, and only enable > the workaround if the Asmedia controller is detected? This code is so > complicated that it is difficult to see whether this could have a > harmful effect on controllers without the bug. Sorry for making it complicated, but it kinda has been like that before already... I don't think the new patch adds much confusion on its own. I would really advise against making this a quirk: checking for it in every case essentially doesn't cost us anything (just one more register compare that is negligible against all the cache-coherent memory accesses of walking the ring), and hardcoding a quirk would mean that we have to identify and add every host controller that does this individually. I still haven't seen anything in the XHCI spec that actually forbids this behavior, so it might be a perfectly legal interpretation and who knows how many host controllers chose to do it that way. Until we find them all, we would have some really bad and hard to track down bugs on those controllers (we really just got lucky this time that Maciej was able to catch it in a bisect). I think it's better to program the driver defensively and able to deal with unexpected situations in general. -- 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: Possible bug in xhci-ring.c
Hi Maciej, On Tue, Jul 15, 2014 at 11:29 AM, Maciej Puzio wrote: > Julius, I tested the patch on kernel 3.2.61, on two USB 3.0 host > controllers (Asmedia and NEC), and four USB 3.0 devices (three of > which were previously triggering the issue, and one worked fine). On > the Asmedia controller, the patch fixes the regression, but with one > of the devices (Areca ARC-5040) I still occasionally get the following > messages in the log: > > Jul 15 12:34:50 ubuntu kernel: [ 1855.902804] usb 4-1: reset > SuperSpeed USB device number 2 using xhci_hcd > Jul 15 12:34:50 ubuntu kernel: [ 1855.920190] xhci_hcd :03:00.0: > xHCI xhci_drop_endpoint called with disabled ep 880423628480 > Jul 15 12:34:50 ubuntu kernel: [ 1855.920197] xhci_hcd :03:00.0: > xHCI xhci_drop_endpoint called with disabled ep 8804236284c0 > > These messages appear out of nowhere, seemingly without any cause, > usually some time after the device has been plugged in (time varies > from 30 sec to 30 min). This worries me a little, because these exact > messages were one of the symptoms of the regression. However, the > device seems to work fine and remains accessible. Without your patch, > such messages were logged every 30 seconds, and the device was not > accessible until they stopped. > I did not notice any problems with other devices on the Asmedia > controller (with the patch), nor with any devices on the NEC > controller (with or without the patch). I *think* those messages are harmless. I've seen them often enough in other logs, they shouldn't have anything to do with this bug. You would probably see them without either of my two patches as well. > I have not yet tested any other kernel version; I intend to compile > and test the newest available kernel tomorrow. Where should I add the > "Tested-by" tag? Just respond with 'Tested-by: Your Name ' to the patch email and the the maintainer should merge that into the commit message when he picks it up (at least that's how I've seen this done before). -- 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: Possible bug in xhci-ring.c
Hi Maciej, The patch is up here, I think you should've been CCed on it: https://lkml.org/lkml/2014/7/8/571 I've only been able to test it on 3.8 with a normal controller, so please test it on both 3.2 and later-than-3.2 with both your quirky and a normal host controller (if you can), and add your Tested-by: tag if it works. Thanks, Julius -- 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 4/4] xhci: Allow xHCI drivers to be built as separate modules
> Nope - since CONFIG_USB_XHCI_MVEBU can be 'y' or 'm' we need the ifneq > here (which matches against both) to ensure xhci-mvebu.o is built is > part of xhci-plat-hcd.o. Oh... does it make sense to have it tristate at all, then? Looks like was never really buildable as an independent module (and still won't be after this patch), so I bet that was just a mistake when the Kconfig for it was first written that should be fixed. (Ideally, xhci-plat.c should probably go away in favor of dwc3/host.c and xhci-mvebu.c implementing the wrapper to xhci_init_driver() directly, but that is a longer-term goal.) -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
On Mon, Jul 14, 2014 at 5:49 AM, Vivek Gautam wrote:
> The host controller by itself may sometimes need to handle PHY
> and/or calibrate some of the PHY settings to get full support out
> of the PHY controller. The PHY core provides a calibration
> funtionality now to do so.
> Therefore, facilitate getting the two possible PHYs, viz.
> USB 2.0 type (UTMI+) and USB 3.0 type (PIPE3).
>
> Signed-off-by: Vivek Gautam
> ---
> drivers/usb/host/xhci-plat.c | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 1a0cf9f..d097d60 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -180,6 +181,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
> goto put_hcd;
> }
>
> + /* Get possile USB 2.0 type PHY (UTMI+) available with xhci */
> + hcd->gen_phy = devm_phy_get(&pdev->dev, "usb2-phy");
> + if (IS_ERR(hcd->gen_phy)) {
> + ret = PTR_ERR(hcd->gen_phy);
> + if (ret != -ENOSYS && ret != -ENODEV)
> + dev_dbg(&pdev->dev, "no usb2 phy configured\n");
nit: This message is not really accurate anymore, right? If there is
no phy configured, you get ENODEV and (correctly) skip the message
completely. What you probably want is dev_warn(..., "error retrieving
usb2 phy: %d\n"); or something like that.
> + }
> +
> ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> if (ret)
> goto disable_clk;
> @@ -209,6 +218,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
> if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> xhci->shared_hcd->can_do_streams = 1;
>
> + /* Get possile USB 3.0 type PHY (PIPE3) available with xhci */
> + xhci->shared_hcd->gen_phy = devm_phy_get(&pdev->dev, "usb3-phy");
> + if (IS_ERR(xhci->shared_hcd->gen_phy)) {
> + ret = PTR_ERR(xhci->shared_hcd->gen_phy);
> + if (ret != -ENOSYS && ret != -ENODEV)
> + dev_dbg(&pdev->dev, "no usb3 phy configured\n");
> + }
> +
> ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> if (ret)
> goto put_usb3_hcd;
> --
> 1.7.10.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] xhci: Allow xHCI drivers to be built as separate modules
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile > index af89a90..bafba71 100644 > --- a/drivers/usb/host/Makefile > +++ b/drivers/usb/host/Makefile > @@ -15,19 +15,19 @@ fhci-$(CONFIG_FHCI_DEBUG) += fhci-dbg.o > xhci-hcd-y := xhci.o xhci-mem.o > xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o > xhci-hcd-y += xhci-trace.o > -xhci-hcd-$(CONFIG_PCI) += xhci-pci.o > > -ifneq ($(CONFIG_USB_XHCI_PLATFORM), ) > - xhci-hcd-y += xhci-plat.o > +xhci-plat-hcd-y := xhci-plat.o > ifneq ($(CONFIG_USB_XHCI_MVEBU), ) > - xhci-hcd-y += xhci-mvebu.o > -endif nit: Can do this even simpler now, just "xhci-plat-hcd-$(CONFIG_USB_XHCI_MVEBU) += xhci-mvebu.o" without the ifneq. > + xhci-plat-hcd-y += xhci-mvebu.o > endif > > obj-$(CONFIG_USB_WHCI_HCD) += whci/ > > obj-$(CONFIG_PCI) += pci-quirks.o > > +obj-$(CONFIG_USB_XHCI_PCI) += xhci-pci.o > +obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o > + > obj-$(CONFIG_USB_EHCI_HCD) += ehci-hcd.o > obj-$(CONFIG_USB_EHCI_PCI) += ehci-pci.o > obj-$(CONFIG_USB_EHCI_HCD_PLATFORM)+= ehci-platform.o Great patch series, thank you! -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] usb: host: xhci-plat: Caibrate PHY post host reset
On Wed, Jul 9, 2014 at 3:01 AM, Vivek Gautam wrote:
> Some quirky PHYs may require to be calibrated post the host
> controller initialization.
> The USB 3.0 DRD PHY on Exynos5420/5800 systems, coming along with
> Synopsys's DWC3 controller, is one such PHY which needs to be
> calibrated post xhci's reset at initialization time and at
> resume time, to get the controller work at SuperSpeed.
>
> Signed-off-by: Vivek Gautam
> ---
> drivers/usb/host/xhci-plat.c | 39 +--
> 1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index e50bd7d..decf349 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -35,7 +35,27 @@ static void xhci_plat_quirks(struct device *dev, struct
> xhci_hcd *xhci)
> /* called during probe() after chip reset completes */
> static int xhci_plat_setup(struct usb_hcd *hcd)
> {
> - return xhci_gen_setup(hcd, xhci_plat_quirks);
> + struct device *parent;
> + int ret;
> +
> + ret = xhci_gen_setup(hcd, xhci_plat_quirks);
> + if (ret) {
> + dev_err(hcd->self.controller, "xhci setup failed\n");
> + return ret;
> + }
> +
> + parent = hcd->self.controller->parent;
> + if (of_device_is_compatible(parent->of_node, "synopsys,dwc3") ||
> + of_device_is_compatible(parent->of_node, "snps,dwc3")) {
> + if (!IS_ERR(hcd->gen_phy)) {
> + ret = phy_calibrate(hcd->gen_phy);
> + if (ret < 0 && ret != -ENOTSUPP)
> + dev_err(hcd->self.controller,
> + "failed to calibrate USB PHY\n");
> + }
> + }
Here as well, is it really necessary to special-case it so much? I'd
say if there is a PHY and it has a calibrate function bound we call
it, and if not we just go ahead.
I also think that this would fit better in core/hcd.c since it's not
really XHCI specific... it's conceivable that an EHCI controller might
also need to tune some PHY settings after reset (in fact Tegra does
something similar, although it already has another hack for that now),
so if we introduce this general facility why not offer it to
everyone?.
> +
> + return ret;
> }
>
> static int xhci_plat_start(struct usb_hcd *hcd)
> @@ -288,8 +308,23 @@ static int xhci_plat_resume(struct device *dev)
> {
> struct usb_hcd *hcd = dev_get_drvdata(dev);
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + int ret;
> +
> + ret = xhci_resume(xhci, 0);
> + if (ret)
> + return ret;
>
> - return xhci_resume(xhci, 0);
> + if (of_device_is_compatible(dev->parent->of_node, "synopsys,dwc3") ||
> + of_device_is_compatible(dev->parent->of_node, "snps,dwc3")) {
> + if (!IS_ERR(hcd->gen_phy)) {
> + ret = phy_calibrate(hcd->gen_phy);
> + if (ret < 0 && ret != -ENOTSUPP)
> + dev_err(hcd->self.controller,
> + "failed to calibrate USB PHY\n");
> + }
> + }
> +
> + return ret;
> }
>
> static const struct dev_pm_ops xhci_plat_pm_ops = {
> --
> 1.7.10.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
On Wed, Jul 9, 2014 at 3:01 AM, Vivek Gautam wrote:
> The host controller by itself may sometimes need to handle PHY
> and/or calibrate some of the PHY settings to get full support out
> of the PHY controller. The PHY core provides a calibration
> funtionality now to do so.
> Therefore, facilitate getting the two possible PHYs, viz.
> USB 2.0 type (UTMI+) and USB 3.0 type (PIPE3), provided
> by the parent - Synopsys's DWC3 controller
>
> Signed-off-by: Vivek Gautam
> ---
> drivers/usb/host/xhci-plat.c | 36
> 1 file changed, 36 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 29d8adb..e50bd7d 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "xhci.h"
> @@ -101,6 +102,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> struct clk *clk;
> int ret;
> int irq;
> + struct device *parent;
>
> if (usb_disabled())
> return -ENODEV;
> @@ -165,6 +167,23 @@ static int xhci_plat_probe(struct platform_device *pdev)
> goto unmap_registers;
> }
>
> + parent = pdev->dev.parent;
> + /*
> +* Get possile USB 2.0 type PHY (UTMI+) registered by xhci's parent:
> +* Synopsys-dwc3
> +*/
> + if (of_device_is_compatible(parent->of_node, "synopsys,dwc3") ||
> + of_device_is_compatible(parent->of_node, "snps,dwc3")) {
> + hcd->gen_phy = devm_phy_get(&pdev->dev, "usb2-phy");
> + if (IS_ERR(hcd->gen_phy)) {
> + ret = PTR_ERR(hcd->gen_phy);
> + if (ret != -ENOSYS && ret != -ENODEV) {
> + dev_err(&pdev->dev, "no usb2 phy
> configured\n");
> + return ret;
> + }
> + }
> + }
Why does this need to check for DWC3? I think this code should be as
generic as possible. Can't you just devm_phy_get("usb2-phy"), and keep
going with a dev_dbg() message if it fails? If the platform has a phy
it will find it, if not that's fine too.
Looks like Heikki's patch assigns the phy names in DWC3-specific code,
so I'm not sure if they are supposed to be specific to that
controller... but DWC3 is the only merged XHCI controller this applys
to right now, so why not make that a general convention? The concept
of having one "usb2-phy" and one "usb3-phy" is probably common across
most xHC implementations (unless they share a single phy in which case
they could just leave one of them unset), so it will be much easier to
handle if they all chose the same two names for those (and we can
avoid a big list of special cases here).
> +
> ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> if (ret)
> goto disable_clk;
> @@ -191,6 +210,23 @@ static int xhci_plat_probe(struct platform_device *pdev)
> if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> xhci->shared_hcd->can_do_streams = 1;
>
> + /*
> +* Get possile USB 3.0 type PHY (PIPE3) registered by xhci's parent:
> +* Synopsys-dwc3
> +*/
> + if (of_device_is_compatible(parent->of_node, "synopsys,dwc3") ||
> + of_device_is_compatible(parent->of_node, "snps,dwc3")) {
> + xhci->shared_hcd->gen_phy = devm_phy_get(&pdev->dev,
> +"usb3-phy");
> + if (IS_ERR(xhci->shared_hcd->gen_phy)) {
> + ret = PTR_ERR(xhci->shared_hcd->gen_phy);
> + if (ret != -ENOSYS && ret != -ENODEV) {
> + dev_err(&pdev->dev, "no usb3 phy
> configured\n");
> + return ret;
> + }
> + }
> + }
> +
> ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> if (ret)
> goto put_usb3_hcd;
> --
> 1.7.10.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
[PATCH] usb: xhci: Fix Set TR Dequeue Pointer cycle state for quirky xHCs
Commit 1f81b6d22 "usb: xhci: Prefer endpoint context dequeue pointer
over stopped_trb" changed the logic in xhci_find_new_dequeue_state() to
only use the Endpoint Context's TR Dequeue Pointer instead of the last
Event TRB's TRB Pointer as a basis from which to infer the host
controller state. This has uncovered a bug with certain Asmedia xHCs
which seem to advance their TR Dequeue Pointer one TRB behind the one
that caused a Stall Error.
This confuses the cycle state calculation since cur_td->last_trb is now
behind hw_dequeue (which the algorithm interprets as a single huge TD
that wraps around the whole transfer ring). This patch counteracts that
by explicitly looking for last_trb when searching for hw_dequeue from
the old software dequeue pointer. If it is found, we toggle the cycle
state once more to balance out the incorrect toggle that will happen
later.
In order to make this work for both single and multi segment rings, this
patch also expands the detection of wrap-around TDs to work on the
latter ones (which cannot normally happen because the kernel prevents
that case to allow for ring expansion, but it's how things appear to be
in the quirky case and allowing the code to handle it makes it easier to
generalize this fix across all kernels). The code will now toggle the
cycle state whenever last_trb is before hw_dequeue on the same segment,
regardless of how many segments there are in the ring.
This patch should be backported to kernels as old as 2.6.31 that contain
the commit ae636747146ea97efa18e04576acd3416e2514f5 "USB: xhci: URB
cancellation support."
Cc: sta...@vger.kernel.org
Signed-off-by: Julius Werner
---
drivers/usb/host/xhci-ring.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 749fc68..50abc68 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -489,8 +489,17 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
/* Find virtual address and segment of hardware dequeue pointer */
state->new_deq_seg = ep_ring->deq_seg;
state->new_deq_ptr = ep_ring->dequeue;
+ state->new_cycle_state = hw_dequeue & 0x1;
while (xhci_trb_virt_to_dma(state->new_deq_seg, state->new_deq_ptr)
!= (dma_addr_t)(hw_dequeue & ~0xf)) {
+ /*
+* Quirky HCs can advance hw_dequeue behind cur_td->last_trb.
+* That violates the assumptions of the TD wrap around check
+* below, so toggle the cycle state once more to cancel it out.
+*/
+ if (state->new_deq_ptr == cur_td->last_trb)
+ state->new_cycle_state ^= 1;
+
next_trb(xhci, ep_ring, &state->new_deq_seg,
&state->new_deq_ptr);
if (state->new_deq_ptr == ep_ring->dequeue) {
@@ -500,12 +509,11 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
}
/*
* Find cycle state for last_trb, starting at old cycle state of
-* hw_dequeue. If there is only one segment ring, find_trb_seg() will
-* return immediately and cannot toggle the cycle state if this search
-* wraps around, so add one more toggle manually in that case.
+* hw_dequeue. If last_trb is on the current segment before hw_dequeue,
+* that means we wrap around the whole ring, but find_trb_seq() will
+* return immediately. Toggle the cycle state manually in that case.
*/
- state->new_cycle_state = hw_dequeue & 0x1;
- if (ep_ring->first_seg == ep_ring->first_seg->next &&
+ if (state->new_deq_seg->trbs < cur_td->last_trb &&
cur_td->last_trb < state->new_deq_ptr)
state->new_cycle_state ^= 0x1;
--
1.8.3.2
--
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: Possible bug in xhci-ring.c
Thanks Maciej, that was exactly what I needed! Looks like the problem in your situation is that last_trb is actually (chronologically) before hw_dequeue, which the code doesn't expect. You enqueue a single-TRB-TD at 2f45e080 (so last_trb points there as well), which encounters a Stall Error. I would guess that the Event TRB for that error still returns 2f45e080 in its TRB Pointer (which the old code stored in the stopped_trb variable that I removed), but the hw_dequeue value we read from the Endpoint Context is 2f45e090. I don't see this behavior on a DesignWare and an Intel xHC that I have here, so I guess your Asmedia host controller is simply quirky/broken. I think it should really store the failed TRB in the TR Dequeue Pointer of the Endpoint Context when encountering an error, and not just advance to the next TRB on its own... but I have a hard time finding a decisive rule for that in the XHCI spec right now (it makes this clear for the Stop Endpoint Command, but not really for errors). Sarah, Mathias, do you know if this is specified somewhere or is it really left up to the implementation? Nevertheless, we should try to accommodate for this somehow. I think the only way to catch it is to already look for last_trb on first search for hw_dequeue, and cycle the bit once more if we encounter it first (to counteract the incorrect cycling later on). Unfortunately that will make this whole thing yet more complicated, but it should work. I'll try to submit a patch tomorrow, but you will need to test it on that host controller (for both 3.2 and later). > Some additional thoughts: > >> The piece of code you pointed out only affects single-segment >> transfer rings. I think the kernel generally switched to using >> multi-segment transfer rings for everything in commit 2fdcd47b69 >> "xHCI: Allocate 2 segments for transfer ring", which explains why >> the problem doesn't affect kernels after 3.2 > > Does this mean that in kernels after 3.2 the problematic code line > (that toggles new_cycle_state) is never executed, i.e. dead code? Well... yes, it's never executed, but I wouldn't call it dead code. It's just coincidence that the kernel currently doesn't use single-segment rings, there's no intrinsic reason for it and it might change in the future. I think it's still a good idea to keep the case working. Also, really the only reason later kernels work for that controller is because we don't handle the case where a TD wraps all the way around a whole multi-segment ring back to the same segment. We should do the extra cycle in that case as well (and a normal transfer would look like that on your controller), so I think we should fix both of these bug and make sure the cycle state is correct in every conceivable setup. -- 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: Possible bug in xhci-ring.c
Thanks for the report, this sounds very troubling. The piece of code you pointed out only affects single-segment transfer rings. I think the kernel generally switched to using multi-segment transfer rings for everything in commit 2fdcd47b69 "xHCI: Allocate 2 segments for transfer ring", which explains why the problem doesn't affect kernels after 3.2 (and it also means that I didn't test this case directly). Nevertheless, we should of course make that case working. I still think the logic in the commit is correct, it's just unfortunately quite confusing (the code flow wasn't very readable to begin with and I didn't do much to improve it). If you take the surrounding code into account, the value of state->new_deq_ptr changes quite a bit over time, and points to a different position in the two cases: in the old code, it refers to the actual, final "new dequeue pointer" (the TRB that the ring will be set to start at with the Set TR Dequeue Pointer command), which should be one after cur->last_trb. So we check if that TRB is in front of the stopped_trb (although it should chronologically come after it), and conclude that we wrapped around. In my patch, state->new_deq_pointer actually points at the virtual address of hw_dequeue (which is equivalent to the removed stopped_trb) at that time. We haven't jumped it forward to its final position yet, but we can still check if cur->last_trb (which should come chronologically after it) is in front of it on the ring, and conclude that we will wrap around once we jump forward. (For reference, the "chronological" order of things should be [ep_ring->dequeue], which is zero or more TRBs before [hw_dequeue], which is the same as [stopped_trb], which is zero or more TRBs before cur->last_trb, which is exactly one TRB before [state->new_deq_ptr at the end of the function].) If you have a setup that easily reproduces this bug, could you please just gather a big load of debug output so we can get a better idea of what it's doing? You'll need to add the CONFIG_USB_XHCI_HCD_DEBUGGING Kconfig and then make all the dev_dbg() in that file work (easiest way is to just '#define DEBUG' in xhci.h). Also add an explicit call of xhci_debug_ring(xhci, ep_ring) to the top of the function and a few more prints to see the initial values of hw_dequeue, state->new_deq_seg, state->new_deq_ptr and state->new_cycle_state, and how they change over the course of the function. That will hopefully help us figure out where the logic is messed up, because I can't really spot a mistake in there yet. On Tue, Jul 1, 2014 at 11:34 AM, Maciej Puzio wrote: > Hi, I am writing about commit 1f81b6d22a5980955b01e08cf27fb745dc9b686f > "usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb". > This commit has been introduced in kernel 3.15-rc3, and subsequently > backported to older kernels. In kernel 3.2.59 (used by Ubuntu 12.04 LTS), > and newer kernels in this branch, it causes a regression with varying > symptoms, if some triggering conditions are met. The symptoms appear > when the USB device is plugged into the USB 3.0 port, and may include > the device not being recognized, or the device being recognized after a > long (18 minute) delay, accompanied with various errors being logged. > If the device is plugged in during boot, system may be unable to > boot. The symptoms, or lack thereof, are highly depended on USB 3.0 > hardware, and on kernel version. So far, all observed cases involved > USB 3.0 controllers from Asmedia ASM1042 family. Even with this > controller, some USB devices do not trigger the regression, but a > large number do, from a SD card reader, to an external HDD, to a RAID > eclosure. For details, please see bug reports [1], [2] and [3]. The > regression is not reproducible in kernel 3.16.0-rc3, but the code > affected by the commit remained essentially the same (as in 3.15-rc3 > and 3.2.59), which gives a reason to believe that the bug is masked > by another change and the regression is avoided by coincidence. > > I would like to focus here on results of my debugging of this issue. > I believe that I have narrowed down the cause of the problem to a > specific line, and I would like to ask people knowledgeable with xhci > code if my assumptions and conclusions are correct. > > Commit: 1f81b6d22a5980955b01e08cf27fb745dc9b686f > File: drivers/usb/host/xhci-ring.c > Function: xhci_find_new_dequeue_state > > The regression appears to be caused by state->new_cycle_state having > wrong value (0 rather than 1) at the end of execution of > xhci_find_new_dequeue_state() function. Inside this function, the > value is toggled between 0 and 1 under various conditions. It seems > that regression is caused by flipping the value of new_cycle_state > one time too many. The commit in question refactored the body of this > function, and among other changes, replaced lines > > if (ep_ring->first_seg == ep_ring->first_seg->next && > state->new_deq_ptr < dev->eps[ep_index].stopped_trb)
Re: [PATCH v1 6/9] usb: xhci: Add NVIDIA Tegra XHCI host-controller driver
> +static const struct hc_driver tegra_xhci_hc_driver = {
> + .description = "tegra-xhci-hcd",
> + .product_desc = "Tegra xHCI Host Controller",
> + .hcd_priv_size =sizeof(struct xhci_hcd *),
> +
> + /*
> +* generic hardware linkage
> +*/
> + .irq = xhci_irq,
> + .flags =HCD_MEMORY | HCD_USB3 | HCD_SHARED,
> +
> + /*
> +* basic lifecycle operations
> +*/
> + .reset =tegra_xhci_setup,
> + .start =xhci_run,
> + .stop = xhci_stop,
> + .shutdown = xhci_shutdown,
> +
> + /*
> +* managing i/o requests and associated device resources
> +*/
> + .urb_enqueue = xhci_urb_enqueue,
> + .urb_dequeue = xhci_urb_dequeue,
> + .alloc_dev =xhci_alloc_dev,
> + .free_dev = xhci_free_dev,
> + .alloc_streams =xhci_alloc_streams,
> + .free_streams = xhci_free_streams,
> + .add_endpoint = xhci_add_endpoint,
> + .drop_endpoint =xhci_drop_endpoint,
> + .endpoint_reset = xhci_endpoint_reset,
> + .check_bandwidth = xhci_check_bandwidth,
> + .reset_bandwidth = xhci_reset_bandwidth,
> + .address_device = xhci_address_device,
> + .enable_device =xhci_enable_device,
> + .update_hub_device =xhci_update_hub_device,
> + .reset_device = xhci_discover_or_reset_device,
> +
> + /*
> +* scheduling support
> +*/
> + .get_frame_number = xhci_get_frame,
> +
> + /* Root hub support */
> + .hub_control = xhci_hub_control,
> + .hub_status_data = xhci_hub_status_data,
> + .bus_suspend = xhci_bus_suspend,
> + .bus_resume = xhci_bus_resume,
> +};
I know I missed the first round of discussion where this was
suggested, but I don't think it's a good idea to pull the whole
hc_driver structure out into every platform implementation. It will
lead to duplication, then to future additions only being applied to
some of the implementations and everything getting out of sync. This
is already a problem with the PCI/plat split (e.g. the LPM functions
were only added to xhci-pci even though they should apply to both).
Also, if I'm not mistaken this code would fail to compile as a module
(you are referencing lots of symbols that are internal to the xhci-hcd
module).
I think at the very least you should add a function
"xhci_default_driver(struct hc_driver *driver)" to xhci-plat.c (or
even better to xhci.c and use it for PCI as well) that initializes all
function pointers to the default (internal) symbols, and can then be
overridden afterwards.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] usb: host: xhci-plat: Add support to get PHYs
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 9ffecd5..453d89e 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1582,6 +1582,9 @@ struct xhci_hcd {
> u32 port_status_u0;
> /* Compliance Mode Timer Triggered every 2 seconds */
> #define COMP_MODE_RCVRY_MSECS 2000
> + /* phys for the controller */
> + struct phy *phy2_gen;
> + struct phy *phy3_gen;
> };
I don't think adding new variables here and restricting most of this
logic to xhci-plat.c (in the next patch) is the best way to do it.
There's no conceptual reason why other host controllers (e.g. xhci-pci
or even EHCI) could not have a similar need to tune their PHY after
reset. PHYs are universal to all host controllers.
There is already a 'phy' member in struct usb_hcd which I think is
mostly unused right now. I think it would be much less
confusing/redundant to reuse that member for this purpose (you could
still set it up from xhci_plat_probe(), and then call it from
hcd_bus_resume() or something like that). Since XHCI host controllers
already conveniently have two struct usb_hcd (one for 2.0 and one for
3.0), you can cleanly store references to your two PHYs in there.
--
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] ARM: dts: Enable USB 3503 hub on exynos5250-snow
> Ok, there was already a patch posted by you for this[1], which had > quite a much discussion > on it. > Would you like to give some pointers based on that ? > One that Olof had suggested was to use gpio-reset driver which is yet > to make to mainline. > But i think with that too we need to take care of the timing for > resetting the hub before PHY > gets reset. Oh, right, I remember now. Seems like there wasn't much consensus on how to solve the problem there, and I think I didn't really have time to work on it any more either. I think coupling in another driver to reset the device isn't a bad idea... this could even be usb3503 if you modified it a little, and it could also address Tomasz' concerns from that thread that the solution should be extensible to other HSIC devices that need a different reset sequence. But you need some mechanism to hook the two drivers together and make sure phy-samsung-usb2 synchronously calls the reset function at exactly the right point to ensure the timing works right. -- 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] ARM: dts: Enable USB 3503 hub on exynos5250-snow
We originally tried using this driver on ChromiumOS and never got it to work reliably. IIRC the issue was that if the hub had already been initialized by firmware, the USB stack might enumerate it before the usb3503 driver is probed and then the later reset will silently disrupt that connection. (I think I tried to force the 3503 to probe earlier as well, and there was some other issue with that although I don't recall the details.) This will not be an issue for the Snow and Peach_Pi(t) boards (since neither of them shipped with firmware that supports this hub), but it will be an issue for Spring and Skate. On ChromiumOS we decided to carry a local (and admittedly ugly) patch to pull that reset line from the USB PHY driver instead, since that's the only way I could get it to work in all cases (see http://crosreview.com/58963). This doesn't mean I'm against this patch per se, just wanted to point out the trade-offs. -- 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: [regression 3.15-rc3] Resume from s4 broken by 1f81b6d22a5980955b01e08cf27fb745dc9b686f
Hmmm... very odd. I unfortunately don't have a machine that can easily do S4 at hand, but I did test this on an IVB with XHCI_RESET_ON_RESUME in S3 (essentially the same code path), and I didn't run into any problems. How exactly does your machine fail on resume? Is it a kernel crash or just a hang? Can you try getting some debug output (by setting 'echo N > /sys/module/printk/parameters/console_suspend' and trying to catch the crash on the screen or a serial line, or maybe through pstore)? I really don't see much that could go wrong with this patch, so without more info it will be hard to understand your problem. Also, I noticed that you have two HID devices plugged in during suspend. Does it make a difference if you have different devices (e.g. a mass storage stick) or none at all? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: xhci: Correct last context entry calculation for Configure Endpoint
The current XHCI driver recalculates the Context Entries field in the
Slot Context on every add_endpoint() and drop_endpoint() call. In the
case of drop_endpoint(), it seems to assume that the add_flags will
always contain every endpoint for the new configuration, which is not
necessarily correct if you don't make assumptions about how the USB core
uses the add_endpoint/drop_endpoint interface (add_flags only contains
endpoints that are new additions in the new configuration).
Furthermore, EP0_FLAG is not consistently set in add_flags throughout
the lifetime of a device. This means that when all endpoints are
dropped, the Context Entries field can be set to 0 (which is invalid and
may cause a Parameter Error) or -1 (which is interpreted as 31 and
causes the driver to keep using the old, incorrect value).
The only surefire way to set this field right is to also take all
existing endpoints into account, and to force the value to 1 (meaning
only EP0 is active) if no other endpoint is found. This patch implements
that as a single step in the final check_bandwidth() call and removes
the intermediary calculations from add_endpoint() and drop_endpoint().
Signed-off-by: Julius Werner
---
drivers/usb/host/xhci.c | 51 +
1 file changed, 18 insertions(+), 33 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 924a6cc..fec6423 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1562,12 +1562,10 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
struct xhci_hcd *xhci;
struct xhci_container_ctx *in_ctx, *out_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
- struct xhci_slot_ctx *slot_ctx;
- unsigned int last_ctx;
unsigned int ep_index;
struct xhci_ep_ctx *ep_ctx;
u32 drop_flag;
- u32 new_add_flags, new_drop_flags, new_slot_info;
+ u32 new_add_flags, new_drop_flags;
int ret;
ret = xhci_check_args(hcd, udev, ep, 1, true, __func__);
@@ -1614,24 +1612,13 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
ctrl_ctx->add_flags &= cpu_to_le32(~drop_flag);
new_add_flags = le32_to_cpu(ctrl_ctx->add_flags);
- last_ctx = xhci_last_valid_endpoint(le32_to_cpu(ctrl_ctx->add_flags));
- slot_ctx = xhci_get_slot_ctx(xhci, in_ctx);
- /* Update the last valid endpoint context, if we deleted the last one */
- if ((le32_to_cpu(slot_ctx->dev_info) & LAST_CTX_MASK) >
- LAST_CTX(last_ctx)) {
- slot_ctx->dev_info &= cpu_to_le32(~LAST_CTX_MASK);
- slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(last_ctx));
- }
- new_slot_info = le32_to_cpu(slot_ctx->dev_info);
-
xhci_endpoint_zero(xhci, xhci->devs[udev->slot_id], ep);
- xhci_dbg(xhci, "drop ep 0x%x, slot id %d, new drop flags = %#x, new add
flags = %#x, new slot info = %#x\n",
+ xhci_dbg(xhci, "drop ep 0x%x, slot id %d, new drop flags = %#x, new add
flags = %#x\n",
(unsigned int) ep->desc.bEndpointAddress,
udev->slot_id,
(unsigned int) new_drop_flags,
- (unsigned int) new_add_flags,
- (unsigned int) new_slot_info);
+ (unsigned int) new_add_flags);
return 0;
}
@@ -1654,11 +1641,9 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
struct xhci_hcd *xhci;
struct xhci_container_ctx *in_ctx, *out_ctx;
unsigned int ep_index;
- struct xhci_slot_ctx *slot_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
u32 added_ctxs;
- unsigned int last_ctx;
- u32 new_add_flags, new_drop_flags, new_slot_info;
+ u32 new_add_flags, new_drop_flags;
struct xhci_virt_device *virt_dev;
int ret = 0;
@@ -1673,7 +1658,6 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
return -ENODEV;
added_ctxs = xhci_get_endpoint_flag(&ep->desc);
- last_ctx = xhci_last_valid_endpoint(added_ctxs);
if (added_ctxs == SLOT_FLAG || added_ctxs == EP0_FLAG) {
/* FIXME when we have to issue an evaluate endpoint command to
* deal with ep0 max packet size changing once we get the
@@ -1739,24 +1723,14 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
*/
new_drop_flags = le32_to_cpu(ctrl_ctx->drop_flags);
- slot_ctx = xhci_get_slot_ctx(xhci, in_ctx);
- /* Update the last valid endpoint context, if we just added one past */
- if ((le32_to_cpu(slot_ctx->dev_info) & LAST_CTX_MASK) <
- LAST_CTX(last_ctx)) {
- slot_ctx->dev_info &= cpu_to_le32(~LAST_CTX_MASK);
- slot_ctx->dev_in
Re: [PATCH] usb: xhci: Correct last context entry calculation for Configure Endpoint
Okay, thanks, sounds good. I personally don't care that much about the stable backport. Let me respin this with a fixed commit message and the type change Felipe suggested. -- 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: xhci: Correct last context entry calculation for Configure Endpoint
*bump* Sarah, Mathias, can we decide how to proceed with this? I think the section Alan quoted is a pretty good argument in favor of my interpretation (although of course this would not be the first time that two sections of a spec contradict each other). But more importantly, we have a case that just generates a clearly wrong Slot Context right now (the one that only drops endpoints), and I see no way how you could construct a correct Slot Context for that case with Sarah's interpretation. We need to resolve that somehow. -- 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/5] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb
> You don't actually add the stable@ tag here, why not? > > You have read Documentation/stable_kernel_rules.txt for how stable > kernel trees work, so why not add the label here? Sorry... I actually didn't know about that file, I just added the "should be backported" line because I have seen that in other commits. I will adhere to those rules in the future. The patch is a little over the line limit (150+ lines with context, although most of it are scattered single-line changes). The issue it solves is that a USB device may silently stop working until it is disconnected, not sure if that's enough to qualify as "oh, that's not good". I'll let you decide whether it still should be queued up for stable backports or not. -- 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: xhci: Prefer endpoint context dequeue pointer over stopped_trb
+hdegoede
> I tried to apply this patch on top of 3.15-rc1, but it fails because of the
> streams support added to xhci_find_new_dequeue_state()
>
> After some manual editing the interesting parts of
> xhci_find_new_dequeue_state() looks like this:
>
> @@ -577,46 +568,57 @@ void xhci_find_new_dequeue_state(struct xhci_hcd
> *xhci,
> if (ep->ep_state & EP_HAS_STREAMS) {
> struct xhci_stream_ctx *ctx =
> &ep->stream_info->stream_ctx_array[stream_id];
> - state->new_cycle_state = 0x1 &
> le64_to_cpu(ctx->stream_ring);
> + hw_dequeue = le64_to_cpu(ctx->stream_ring);
> } else {
> struct xhci_ep_ctx *ep_ctx
>
> = xhci_get_ep_ctx(xhci, dev->out_ctx, ep_index);
> - state->new_cycle_state = 0x1 & le64_to_cpu(ep_ctx->deq);
> + hw_dequeue = le64_to_cpu(ep_ctx->deq);
> }
>
> + /* Find virtual address and segment of hardware dequeue pointer */
>
> + state->new_deq_seg = ep_ring->deq_seg;
> + state->new_deq_ptr = ep_ring->dequeue;
> + while (xhci_trb_virt_to_dma(state->new_deq_seg, state->new_deq_ptr)
> + != (dma_addr_t)(hw_dequeue & ~0x1)) {
> + next_trb(xhci, ep_ring, &state->new_deq_seg,
> + &state->new_deq_ptr);
> + if (state->new_deq_ptr == ep_ring->dequeue) {
> + WARN_ON(1);
> + return;
> + }
> + }
>
> Also the comparison of the dequeue pointers, using (hw_dequeue & ~0x1) might
> have some troubles with streams. Endpoint context TR dequeue pointer LO
> field has bits 3:1 reserved (probably zero) but stream context uses those
> bits. Would it make sense to use (hw_dequeue & ~0xf) here instead?
Ah, yes, looks like that patch wasn't in Linus' tree yet back when I
wrote this. I think your merge looks pretty good... just use
(hw_dequeue & ~0xf) instead of (hw_dequeue & ~0x1) to get the pointer
as you said, and this should work fine.
> But I'm still concerned about the dequeue pointer in the streams case.
> streams may be nested, we might be pointing at another stream context
> instead of the dequeue pointer.
>
> So there's still some work needed. Are you interested in re-working this to
> fit on top of 3.15-rc1 or should I add it to my todo list?
Hmmm... maybe the stream_id parameter is already pointing to the
correct secondary stream (if applicable) so you can rely on having a
normal dequeue pointer there? The xhci_triad_to_transfer_ring()
function seems to make the same assumption... At any rate, if there is
a problem here it would also be in the original c4bedb77e ("xhci: For
streams the css flag most be read from the stream-ctx on ep stop")
already, so I think you should follow up with that patch's author and
fix it in a separate commit if necessary.
I unfortunately don't have a device using streams to test with, so I
couldn't do more for this patch than you've already done. I think if
you change the hw_dequeue masking as you said that should guarantee
that the patch at least won't make things worse for streams, and that
should be good enough.
--
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: xhci: Prefer endpoint context dequeue pointer over stopped_trb
Hi Mathias, > The patch looks fine. Mathias is taking over for xHCI driver > maintainership in 3.15. He's currently handling queuing bug fix patches > for 3.14 while I finish queueing feature patches for 3.15. Mathias, > will you test and queue this up for 3.14? Did you pick this patch up anywhere yet or are there still issues with it? I just want to make sure it doesn't slip through the cracks. (Maybe I just didn't see it yet... are you still queueing patches in sarah/xhci.git or do you have your own repository somewhere?) -- 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: xhci: Correct last context entry calculation for Configure Endpoint
> http://marc.info/?l=linux-usb&m=137158978503741&w=2 > > There's an xHCI spec ambiguity: Does the last valid context entry refer > to the last valid endpoint context in the *input* device context or the > *output* device context? > > The code currently assumes it refers to the input device context, namely > the endpoints we're adding or changing. If hardware needs the last > valid endpoint context for the re-calculated *output* device context, > then yes, this needs to be changed. However, based on spec errata, I > believe that's not the intent of the spec authors: > > http://marc.info/?l=linux-kernel&m=137208958411696&w=2 Oh, okay, it didn't even occur to me to interpret it that way. It seems very odd since then Context Entries is essentially redundant with the information already provided by the Add Context flags. > What is the impact if we calculate the valid last valid endpoint context > for the input context? Do you have evidence of hardware misbehaving? > If so, which hardware? I haven't actually seen a problem from this, it just seemed like the right thing to do for me when looking at it. The only real error we had was when the command fails due to Context Entries being 0. However, the question remains: What is the right value for Context Entries when we have no Add Context flags, or only the SLOT_FLAG? It should be perfectly legal to just drop a bunch of endpoints without adding/changing anything else, such as when you switch a UVC interface back to alternate setting 0 (which has no endpoints). Then the Input Context really ends at the Slot Context (DCI = 0), but Context Entries = 0 is very clearly forbidden in the spec. I guess we could just force it to 1 there and it would probably work, but that would technically be incorrect since the EP0 context is not updated and not part of the Add Context flags. -- 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: xhci: Correct last context entry calculation for Configure Endpoint
The current XHCI driver recalculates the Context Entries field in the
Slot Context on every add_endpoint() and drop_endpoint() call. In the
case of drop_endpoint(), it seems to assume that the add_flags will
always contain every endpoint for the new configuration, which is not
necessarily correct if you don't make assumptions about how the USB core
uses the add_endpoint/drop_endpoint interface (add_flags only contains
endpoints that are new additions in the new configuration).
Furthermore, EP0_FLAG is not consistently set in add_flags throughout
the lifetime of a device. This means that when all endpoints are
dropped, the Context Entries field can be set to 0 (which is invalid and
may cause a Parameter Error) or -1 (which is interpreted as 31 and
causes the driver to keep using the old, incorrect value).
The only surefire way to set this field right is to also take all
existing endpoints into account, and to force the value to 1 (meaning
only EP0 is active) if no other endpoint is found. This patch implements
that as a single step in the final check_bandwidth() call and removes
the intermediary calculations from add_endpoint() and drop_endpoint().
This patch should be backported to kernels as old as 2.6.31 that contain
the commit f94e0186312b0fc39f41eed4e21836ed74b7efe1 "USB: xhci:
Bandwidth allocation support".
Signed-off-by: Julius Werner
---
drivers/usb/host/xhci.c | 51 +
1 file changed, 18 insertions(+), 33 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 924a6cc..e7d9dfa 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1562,12 +1562,10 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
struct xhci_hcd *xhci;
struct xhci_container_ctx *in_ctx, *out_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
- struct xhci_slot_ctx *slot_ctx;
- unsigned int last_ctx;
unsigned int ep_index;
struct xhci_ep_ctx *ep_ctx;
u32 drop_flag;
- u32 new_add_flags, new_drop_flags, new_slot_info;
+ u32 new_add_flags, new_drop_flags;
int ret;
ret = xhci_check_args(hcd, udev, ep, 1, true, __func__);
@@ -1614,24 +1612,13 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
ctrl_ctx->add_flags &= cpu_to_le32(~drop_flag);
new_add_flags = le32_to_cpu(ctrl_ctx->add_flags);
- last_ctx = xhci_last_valid_endpoint(le32_to_cpu(ctrl_ctx->add_flags));
- slot_ctx = xhci_get_slot_ctx(xhci, in_ctx);
- /* Update the last valid endpoint context, if we deleted the last one */
- if ((le32_to_cpu(slot_ctx->dev_info) & LAST_CTX_MASK) >
- LAST_CTX(last_ctx)) {
- slot_ctx->dev_info &= cpu_to_le32(~LAST_CTX_MASK);
- slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(last_ctx));
- }
- new_slot_info = le32_to_cpu(slot_ctx->dev_info);
-
xhci_endpoint_zero(xhci, xhci->devs[udev->slot_id], ep);
- xhci_dbg(xhci, "drop ep 0x%x, slot id %d, new drop flags = %#x, new add
flags = %#x, new slot info = %#x\n",
+ xhci_dbg(xhci, "drop ep 0x%x, slot id %d, new drop flags = %#x, new add
flags = %#x\n",
(unsigned int) ep->desc.bEndpointAddress,
udev->slot_id,
(unsigned int) new_drop_flags,
- (unsigned int) new_add_flags,
- (unsigned int) new_slot_info);
+ (unsigned int) new_add_flags);
return 0;
}
@@ -1654,11 +1641,9 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
struct xhci_hcd *xhci;
struct xhci_container_ctx *in_ctx, *out_ctx;
unsigned int ep_index;
- struct xhci_slot_ctx *slot_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
u32 added_ctxs;
- unsigned int last_ctx;
- u32 new_add_flags, new_drop_flags, new_slot_info;
+ u32 new_add_flags, new_drop_flags;
struct xhci_virt_device *virt_dev;
int ret = 0;
@@ -1673,7 +1658,6 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
return -ENODEV;
added_ctxs = xhci_get_endpoint_flag(&ep->desc);
- last_ctx = xhci_last_valid_endpoint(added_ctxs);
if (added_ctxs == SLOT_FLAG || added_ctxs == EP0_FLAG) {
/* FIXME when we have to issue an evaluate endpoint command to
* deal with ep0 max packet size changing once we get the
@@ -1739,24 +1723,14 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct
usb_device *udev,
*/
new_drop_flags = le32_to_cpu(ctrl_ctx->drop_flags);
- slot_ctx = xhci_get_slot_ctx(xhci, in_ctx);
- /* Update the last valid endpoint context, if we just added one past */
- if ((le32_to_cpu(slot_ctx->d
Re: usbnet: driver_info->stop required to stop USB interrupts?
>> Can you please explain why we need to check if the waitqueue is active? > > and add a comment that answers the above question. Oh the braces!!! Well, that's just mean... I expect that we don't really need the waitqueue_active() check there as long as we fix the patch to make sure the control flow in the rest of the statements actually stays the same. (That's why I really like to put comments for an else block next to or under the opening brace, instead of above with another empty line...) -- 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: usbnet: driver_info->stop required to stop USB interrupts?
> I tried adding the following change on top of your patch but believe > the plumbing still isn't quite correct since the USB device (eth0) is > reporting a link but no TX or RX of traffic: > @@ -805,6 +807,9 @@ int usbnet_open (struct net_device *net) > goto done; > } > > + /* usbnet_bh() expects the spinlock to be initialized. */ > + init_waitqueue_head(&dev->wait); > + > /* hard_mtu or rx_urb_size may change in reset() */ > usbnet_update_max_qlen(dev); I think a better place for this would be in usbnet_probe() (together with all the other dev->xxx initialization). I'm not quite sure how that could cause the transfer problems you are seeing, but at least you will no longer initialize the waitqueue multiple times on multiple usbnet_open (which is probably a bad idea). -- 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: usbnet: driver_info->stop required to stop USB interrupts?
Hi Oliver, Any update on the state of this patch? Did it get picked up for merge somewhere? Do you agree with my suggestion of sticking closer to the original logic instead of adding that autopm_get(), or do you maybe want to add some more reviewers to confirm your code? I don't really care that much which way we choose in the end, I just want to make sure this bug gets fixed and we don't forget about it. Thanks, Julius -- 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: usbnet: driver_info->stop required to stop USB interrupts?
I don't know enough about how runtime PM works in this driver to really review that patch, sorry. (Would this do a complete resume of the device if we call usbnet_stop() while it was suspended? Is that what we want?) I think you could have also preserved the original logic of using dev->wait as a flag if you had just replaced 'if (!dev->wait &&' with 'if (!waitqueue_active(&dev->wait) &&' to check whether the waitqueue is empty. -- 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: usbnet: driver_info->stop required to stop USB interrupts?
I think usbnet_stop() raced with the dev->bh tasklet, which by itself might not be a problem (usbnet_stop() later kills the tasklet itself, so it should expect that it can be running before that). The issue is that it calls usbnet_terminate_urbs() before that, which temporarily installs a waitqueue in dev->wait in order to be able to wait on the tasklet to run and finish up some queues. The waiting itself looks okay, but the access to 'dev->wait' is totally unprotected and can race arbitrarily. I think in this case usbnet_bh() managed to succeed it's dev->wait check just before usbnet_terminate_urbs() sets it back to NULL. The latter then finishes and the waitqueue_t structure on its stack gets overwritten by other functions halfway through the wake_up() call in usbnet_bh(). I think the best solution would be to just make dev->wait a directly embedded structure inside struct usbnet instead of a pointer to something stack-allocated. usbnet_bh() could just call wake_up() unconditionally (if empty it will be a noop), and then one other check for !dev->wait could be replaced with a call to waitqueue_active(). Then the waitqueue-internal locks should be enough to protect all accesses. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] usb: Make DELAY_INIT quirk wait 100ms between Get Configuration requests
The DELAY_INIT quirk only reduces the frequency of enumeration failures
with the Logitech HD Pro C920 and C930e webcams, but does not quite
eliminate them. We have found that adding a delay of 100ms between the
first and second Get Configuration request makes the device enumerate
perfectly reliable even after several weeks of extensive testing. The
reasons for that are anyone's guess, but since the DELAY_INIT quirk
already delays enumeration by a whole second, wating for another 10th of
that isn't really a big deal for the one other device that uses it, and
it will resolve the problems with these webcams.
Signed-off-by: Julius Werner
---
drivers/usb/core/config.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 8d72f0c..062967c 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -717,6 +717,10 @@ int usb_get_configuration(struct usb_device *dev)
result = -ENOMEM;
goto err;
}
+
+ if (dev->quirks & USB_QUIRK_DELAY_INIT)
+ msleep(100);
+
result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno,
bigbuffer, length);
if (result < 0) {
--
1.8.3.2
--
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: Make DELAY_INIT quirk wait 100ms between Get Configuration requests
> What am I supposed to do with this line? :) > Oh damn, sorry, forgot to take that out on the second one. We have this really annoying commit hook that adds them automatically. v2 incoming... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] usb: Make DELAY_INIT quirk wait 100ms between Get Configuration requests
The DELAY_INIT quirk only reduces the frequency of enumeration failures
with the Logitech HD Pro C920 and C930e webcams, but does not quite
eliminate them. We have found that adding a delay of 100ms between the
first and second Get Configuration request makes the device enumerate
perfectly reliable even after several weeks of extensive testing. The
reasons for that are anyone's guess, but since the DELAY_INIT quirk
already delays enumeration by a whole second, wating for another 10th of
that isn't really a big deal for the one other device that uses it, and
it will resolve the problems with these webcams.
Change-Id: Ibf738426307fe8ef362768db2decc9bc2b30a930
Signed-off-by: Julius Werner
---
drivers/usb/core/config.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 8d72f0c..062967c 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -717,6 +717,10 @@ int usb_get_configuration(struct usb_device *dev)
result = -ENOMEM;
goto err;
}
+
+ if (dev->quirks & USB_QUIRK_DELAY_INIT)
+ msleep(100);
+
result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno,
bigbuffer, length);
if (result < 0) {
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: Add device quirk for Logitech HD Pro Webcams C920 and C930e
We've encountered a rare issue when enumerating two Logitech webcams
after a reboot that doesn't power cycle the USB ports. They are spewing
random data (possibly some leftover UVC buffers) on the second
(full-sized) Get Configuration request of the enumeration phase. Since
the data is random this can potentially cause all kinds of odd behavior,
and since it occasionally happens multiple times (after the kernel
issues another reset due to the garbled configuration descriptor), it is
not always recoverable. Set the USB_DELAY_INIT quirk that seems to work
around the issue.
Signed-off-by: Julius Werner
---
drivers/usb/core/quirks.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 8f37063..739ee8e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -47,6 +47,10 @@ static const struct usb_device_id usb_quirk_list[] = {
/* Microsoft LifeCam-VX700 v2.0 */
{ USB_DEVICE(0x045e, 0x0770), .driver_info = USB_QUIRK_RESET_RESUME },
+ /* Logitech HD Pro Webcams C920 and C930e */
+ { USB_DEVICE(0x046d, 0x082d), .driver_info = USB_QUIRK_DELAY_INIT },
+ { USB_DEVICE(0x046d, 0x0843), .driver_info = USB_QUIRK_DELAY_INIT },
+
/* Logitech Quickcam Fusion */
{ USB_DEVICE(0x046d, 0x08c1), .driver_info = USB_QUIRK_RESET_RESUME },
--
1.8.3.2
--
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 13/16] usb: force warm reset to break link re-connect livelock
Acked-by: Julius Werner -- 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: xhci: Prefer endpoint context dequeue pointer over stopped_trb
We have observed a rare cycle state desync bug after Set TR Dequeue
Pointer commands on Intel LynxPoint xHCs (resulting in an endpoint that
doesn't fetch new TRBs and thus an unresponsive USB device). It always
triggers when a previous Set TR Dequeue Pointer command has set the
pointer to the final Link TRB of a segment, and then another URB gets
enqueued and cancelled again before it can be completed. Further
investigation showed that the xHC had returned the Link TRB in the TRB
Pointer field of the Transfer Event (CC == Stopped -- Length Invalid),
but when xhci_find_new_dequeue_state() later accesses the Endpoint
Context's TR Dequeue Pointer field it is set to the first TRB of the
next segment.
The driver expects those two values to be the same in this situation,
and uses the cycle state of the latter together with the address of the
former. This should be fine according to the XHCI specification, since
the endpoint ring should be stopped when returning the Transfer Event
and thus should not advance over the Link TRB before it gets restarted.
However, real-world XHCI implementations apparently don't really care
that much about these details, so the driver should follow a more
defensive approach to try to work around HC spec violations.
This patch removes the stopped_trb variable that had been used to store
the TRB Pointer from the last Transfer Event of a stopped TRB. Instead,
xhci_find_new_dequeue_state() now relies only on the Endpoint Context,
requiring a small amount of additional processing to find the virtual
address corresponding to the TR Dequeue Pointer. Some other parts of the
function were slightly rearranged to better fit into this model.
This patch should be backported to kernels as old as 2.6.31 that contain
the commit ae636747146ea97efa18e04576acd3416e2514f5 "USB: xhci: URB
cancellation support."
Signed-off-by: Julius Werner
---
drivers/usb/host/xhci-ring.c | 66
drivers/usb/host/xhci.c | 1 -
drivers/usb/host/xhci.h | 2 --
3 files changed, 30 insertions(+), 39 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a0b248c..b8277c7 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -549,6 +549,7 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
struct xhci_generic_trb *trb;
struct xhci_ep_ctx *ep_ctx;
dma_addr_t addr;
+ u64 hw_dequeue;
ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
ep_index, stream_id);
@@ -558,56 +559,56 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
stream_id);
return;
}
- state->new_cycle_state = 0;
- xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
- "Finding segment containing stopped TRB.");
- state->new_deq_seg = find_trb_seg(cur_td->start_seg,
- dev->eps[ep_index].stopped_trb,
- &state->new_cycle_state);
- if (!state->new_deq_seg) {
- WARN_ON(1);
- return;
- }
/* Dig out the cycle state saved by the xHC during the stop ep cmd */
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"Finding endpoint context");
ep_ctx = xhci_get_ep_ctx(xhci, dev->out_ctx, ep_index);
- state->new_cycle_state = 0x1 & le64_to_cpu(ep_ctx->deq);
+ hw_dequeue = le64_to_cpu(ep_ctx->deq);
+
+ /* Find virtual address and segment of hardware dequeue pointer */
+ state->new_deq_seg = ep_ring->deq_seg;
+ state->new_deq_ptr = ep_ring->dequeue;
+ while (xhci_trb_virt_to_dma(state->new_deq_seg, state->new_deq_ptr)
+ != (dma_addr_t)(hw_dequeue & ~0x1)) {
+ next_trb(xhci, ep_ring, &state->new_deq_seg,
+ &state->new_deq_ptr);
+ if (state->new_deq_ptr == ep_ring->dequeue) {
+ WARN_ON(1);
+ return;
+ }
+ }
+ /*
+* Find cycle state for last_trb, starting at old cycle state of
+* hw_dequeue. If there is only one segment ring, find_trb_seg() will
+* return immediately and cannot toggle the cycle state if this search
+* wraps around, so add one more toggle manually in that case.
+*/
+ state->new_cycle_state = hw_dequeue & 0x1;
+ if (ep_ring->first_seg == ep_ring->first_seg->next &&
+ cur_td->last_trb < state->new_deq_ptr)
+ state->new_cycle_state ^= 0x1;
state->new_deq_ptr = cur_td->last_trb;
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"Finding segment containing
Re: [PATCH v4 13/14] usb: force warm reset to break resume livelock
>> We don't need to change hub_port_debounce() right away... that was >> just an additional suggestion to make things more efficient for >> SuperSpeed devices in general. I think for now (in order to solve >> Dan's problem), it would be best if he just calls hub_port_debounce() >> in usb_port_runtime_resume() (which should bring a connected device >> back in the majority of cases), and always queues up a warm reset if >> that fails. (This is essentially what his patch is already doing, just >> get rid of the extra check for USB_SS_PORT_LS_POLLING in the error >> path which I think might be a little too specific and overlook cases >> where the RxDetect/Polling cycle just happens to be at RxDetect in >> that moment.) > > That's the plan, and I also want to test a usb device quirk flag to > unconditionally warm reset on power-session loss since it does seem to > be device specifc in my case. For what it's worth, I think you might not need a quirk flag since you are not really loosing anything in that case. If the first hub_port_debounce() fails to reconnect, the device is either quirky or there is no device attached at all. In the first case we want the warm reset, and in the second case the warm reset is pointless but also doesn't cost us anything (assuming it runs totally asynchronous, which I think your patch guarantees). The only case where we really need to avoid wasting those 100ms is on ports which are connected and already usable, but those should be caught by hub_port_debounce() already. -- 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 13/14] usb: force warm reset to break resume livelock
> Julius, are you sure the Synopsys host will actually power off the > ports? The Intel hosts need some special ACPI methods, so I'm not sure > if Dan's issue with ports after power on could even be seen on the > Synopsys host. > > The Synopsys issue (as I remember it, please correct me if I'm wrong) > would only be triggered if the host lost power during S3, and was halted > and reset after a register restore failure. I think the solution we > agreed to was to implement a Synopsys host quirk that would warm reset > all ports unconditionally on register restore failure. Was that right? > > Then there's Dan's issue. Dan, does the port go into SS.Inactive before > the host starts to cycle between RxDetect and Polling and U0 for this > case? Sorry, I didn't mean to pull the Synopsys issue into this thread... we should probably keep that separate in https://lkml.org/lkml/2013/12/9/336 , or this will get too confusing. Some aspects of that issue are definitely different, e.g. there's no port power being turned off there (on the contrary, the problem is that the power session stays alive during suspend but the xHC gets reset and forgets about it). I just wanted to point out that both issues can lead to the same state (by different means) where the port status cycles between RxDetect and Polling, because they both reset the host controller's port state to RxDetect in a way that the device might not notice (or not react correctly to). I think whenever the host port state gets forced back to RxDetect while the device is in SS.Inactive (or anything that can lead to SS.Inactive after a few unexpected packets) you will get into this state, and you will only get back out of there through a warm reset (or by physically cutting off VBUS). > Hans also ran into this issue (or at least a variation of it), and > proposed a patch to fix it. > > https://git.kernel.org/cgit/linux/kernel/git/sarah/xhci.git/commit/?h=for-usb-next-streams&id=3fd14185404e3a298a3f6b6c6f21ff8d41bb2747 > > Can you take a look at it, and see if it would address your issue? I > think it will catch the case where we transition from SS.Inactive -> > RxDetect -> Polling. I don't think that's targeting the same problem. Hans seems to be describing a situation where the device connects again even though he doesn't want it to -- in our case, the device doesn't connect even though we want that. His patch shouldn't do anything for our issue since he checks for PORT_STAT_CONNECTION, and that bit will be unset when the host port is stuck in RxDetect/Polling. > > It is a valid transitional state, unfortunately, but in a working case > > it should resolve itself within a few milliseconds (probably less than > > 10). Maybe we should try to differentiate between USB 2.0 and 3.0 > > devices in hub_port_debounce()? I think due to the built-in link > > training in USB 3.0, the classic debouncing doesn't really make sense > > anymore (and wastes a lot of time since SuperSpeed links can train > > really fast when they work). > > > > As for this patch, I think the best approach would be to wait for the > > device to come back in usb_port_runtime_resume() (through > > hub_port_debounce() or something else), and if it doesn't show up > > always set the bit to warm reset the port (regardless of LTSSM state, > > since even if it says RxDetect I wouldn't be sure that there is really > > nothing connected). We could then also use those bits in the "lost > > power" case of xhci_resume() to try and work around the problems with > > that Synopsys controller. > > That's a lot of changes to the hub core. Would an xHCI quirk be > simpler? Is there some scenario I'm missing that the S3 resume quirk > wouldn't handle? We don't need to change hub_port_debounce() right away... that was just an additional suggestion to make things more efficient for SuperSpeed devices in general. I think for now (in order to solve Dan's problem), it would be best if he just calls hub_port_debounce() in usb_port_runtime_resume() (which should bring a connected device back in the majority of cases), and always queues up a warm reset if that fails. (This is essentially what his patch is already doing, just get rid of the extra check for USB_SS_PORT_LS_POLLING in the error path which I think might be a little too specific and overlook cases where the RxDetect/Polling cycle just happens to be at RxDetect in that moment.) -- 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 13/14] usb: force warm reset to break resume livelock
>> I believe I am seeing a "polling livelock" scenario as described by Julius. > > Julius was talking about what happens when the host controller itself > gets reset (and therefore remembers nothing about any device) whereas > the device still thinks it is in U3. Is that the scenario you're > encountering? I thought you were working on normal runtime PM. When you turn the power back on for a port, it should start out in RxDetect and switch to Polling as it detects Rx terminations. If the downstream device is unhappy for any reason (e.g. in SS.Inactive or still in U3) and sends no or wrong responses to the LFPS Polling, the hub's port will either move to ComplianceMode or keep cycling back and forth between RxDetect and Polling. The latter is especially dangerous because it's hard to detect (if you just sample the port status you might see RxDetect, which would also be expected if there is nothing connected at all), so I'm thinking an unconditional warm reset might be unavoidable. That is why we proposed to go that route for the Synopsys controller, and I think the same will apply to this situation (since I think turning off a PortPower bit in XHCI will make the controller "forget" a previous U3 state and return to RxDetect when you turn it back on again, even though the actual VBUS line to the device may not have been disabled after all). >> > One other thought (I don't know if it is the right thing to do) is that >> > we might _always_ perform a warm reset after powering-on a SuperSpeed >> > port, without bothering to call hub_port_debounce_be_connected. >> >> I'm leaning in that direction. However, the decision comes down to >> the relative occurrence frequency of devices that fall into this trap >> vs those that successfully recover and would suffer the additional >> latency of a warm reset. > > Is a warm reset significantly longer than an ordinary reset? We have > to do some kind of reset in any case. After all, the power session > _has_ been interrupted. (Assuming the power switching worked...) USB 3.0 ports don't need to be reset on connect as a matter of course. The should usually just start training themselves and eventually become ready as soon as the wires touch. An extra warm reset would add 80-120ms delay to the port resume. (In comparison, a hot reset should not take more than 12ms, probably even much less.) >> >> With this in place we may want to consider reducing the timeout and >> >> relying on warm reset for recovery. >> > >> > Why? I'm not familiar with the intricacies of USB-3 link state >> > changes, but there seem to be only two possibilities: >> > >> > Either PORT_LS_POLLING is a valid state to be in while >> > trying to establish a SuperSpeed connection, in which case >> > we don't want to reduce the timeout, >> > >> > Or it isn't a valid state, in which case we should abort >> > the debounce immediately. It is a valid transitional state, unfortunately, but in a working case it should resolve itself within a few milliseconds (probably less than 10). Maybe we should try to differentiate between USB 2.0 and 3.0 devices in hub_port_debounce()? I think due to the built-in link training in USB 3.0, the classic debouncing doesn't really make sense anymore (and wastes a lot of time since SuperSpeed links can train really fast when they work). As for this patch, I think the best approach would be to wait for the device to come back in usb_port_runtime_resume() (through hub_port_debounce() or something else), and if it doesn't show up always set the bit to warm reset the port (regardless of LTSSM state, since even if it says RxDetect I wouldn't be sure that there is really nothing connected). We could then also use those bits in the "lost power" case of xhci_resume() to try and work around the problems with that Synopsys controller. -- 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: core: get config and string descriptors for unauthorized devices
>> I'd like to know if this is going to be considered for merge or if >> I'll have to look elsewhere for a solution. > > It's queued up for 3.14-rc1 (you can always check linux-next for this > type of thing...) Do you also need/want this in older (i.e. stable) > kernel releases? Oh right... sorry, I should've had a closer look first. (I guess Google doesn't index git.kernel.org that well or I would've found it.) I personally don't care about the stable backport, but it does work around a rare use-after-free bug of the udev->config pointer (when the usb_set_configuration(-1) in usb_deauthorize_device() fails because the device is already unplugged, but it still runs usb_destroy_configuration() afterwards), so you might want to consider it. > You are using wireless usb? Nope, but we are using the sysfs "authorized" node to force a reconfiguration and driver rebinding in certain cases. -- 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: core: get config and string descriptors for unauthorized devices
Hi, has there been any movement on this patch? It happens to fix a problem that we have been experiencing with races between authorize/deauthorize and device removal. I'd like to know if this is going to be considered for merge or if I'll have to look elsewhere for a solution. -- 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: core: Add warm reset while reset-resuming SuperSpeed HUBs
>> ...although, the spec says that it does not wait for the port resets >> to complete. As far as I can see re-issuing a warm reset and waiting >> is the only way to guarantee the core times the recovery. Presumably >> the portstatus debounce in hub_activate() mitigates this, but that >> 100ms is less than a full reset timeout. It's definitely not just a timing issue for us. I can't reproduce all the same cases as Vikas, but when I attach a USB analyzer to the ones I do see the host controller doesn't even start sending a reset. >>> The xHCI spec requires that when the xHCI host is reset, a USB reset is >>> driven down the USB 3.0 ports. If hot reset fails, the port may migrate >>> to warm reset. See table 32 in the xHCI spec, in the definition of >>> HCRST. It sounds like this host doesn't drive a USB reset down USB 3.0 >>> ports at all on host controller reset? Oh, interesting, I hadn't seen that yet. So I guess the spec itself is fine if it were followed to the letter. I did some more tests about this on my Exynos machine: when I put a device to autosuspend (U3) and manually poke the xHC reset bit, I do see an automatic warm reset on the analyzer and the ports manage to retrain to U0. But after a system suspend/resume which calls xhci_reset() in the process, there is no reset on the wire. I also noticed that it doesn't drive a reset (even after manual poking) when there is no device connected on the other end of the analyzer. So this might be our problem: maybe these host controllers (Synopsys DesignWare) issue the spec-mandated warm reset only on ports where they think there is a device attached. But after a system suspend/resume (where the whole IP block on the SoC was powered down), the host controller cannot know that there is still a device with an active power session attached, and therefore doesn't drive the reset on its own. Even though this is a host controller bug, we still have to deal with it somehow. I guess we could move the code into xhci_plat_resume() and hide it behind a quirk to lessen the impact. But since reset_resume is not a common case for most host controllers, it's hard to say if this is DesignWare specific or a more widespread implementation mistake. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: core: Make sure usb_set_configuration(-1) cannot fail
> Sorry for not getting back to this sooner. Ironically, it looks like > this change isn't needed any more, thanks to Thomas Pugliese's patch: > > http://marc.info/?l=linux-usb&m=13866180520&w=2 Yes... thanks for pointing it out, that would make this patch obsolete. I'll wait a few days with this to see if that patch gets accepted as it is. -- 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: core: Add warm reset while reset-resuming SuperSpeed HUBs
> I don't know what you mean by "fails". The system goes to sleep and > then later on wakes up, doesn't it? > > Do you mean that the Jetflash device gets disconnected when the system > wakes up? That's _supposed_ to happen under those circumstances. > When hub_activate() sees HUB_RESET_RESUME, all child devices get > disconnected except those where udev->persist_enabled is set. This patch was written in response to the same bug as my "usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED" submission. My patch only helps when the port gets stuck in Compliance Mode, but Vikas reports that he can sometimes see it stuck in Polling or Recovery states as well. The underlying issue is a deadlock in the USB 3.0 link training state machine when the host controller is unilaterally reset on resume (without driving a reset on the bus). The host port starts out back in Rx.Detect without remembering anything about its previous state, but the device is still in U3. The host detects Rx terminations, moves to Polling and starts sending LFPS link training packets, but the device doesn't expect those and interprets them as link problems (moving to Recovery). What happens next seems to be device specific, but apparently the device can end up in SS.Inactive while the host port gets stuck in Polling or Recovery (or some kind of livelock between those). This patch tries to warm reset all USB 3.0 ports on reset-resume (after xhci_reset() was called) that had devices connected to them before suspend. This seems to be the only way to ensure the devices' state machines get back to a well-defined state that the host can work with. I don't think this is a specific hardware bug, it's just an unfortunate design flaw that the USB 3.0 spec doesn't account for a root hub port being reset independently of its connected device. I think Sarah is correct that it could be limited to root hubs, though. -- 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 RFC 2/4] xhci: Add quirk for DWC3-Exynos controller
On Tue, Dec 10, 2013 at 2:55 AM, Vivek Gautam wrote:
> The DWC3-exynos eXtensible host controller on Exynos5420 SoC
> is quirky in a way that the PHY needs to be tuned to get it
> working at SuperSpeed.
> By default this PHY works as High-speed phy and therefore
> detects even Super-speed devices as high-speed ones.
> So, the PHY needs to be tuned after controller has been reset.
>
> Adding a xHCI quirk for this purpose.
>
> Signed-off-by: Vivek Gautam
> ---
> drivers/usb/host/xhci-plat.c | 19 +++
> drivers/usb/host/xhci.h |1 +
> 2 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index d9c169f..395c9e9 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -21,6 +21,25 @@
>
> static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> {
> + struct device *parent_dev;
> + struct device *vendor_parent_dev;
> +
> + parent_dev = dev->parent;
> + vendor_parent_dev = parent_dev->parent;
Is this guaranteed to exist for all current and future xhci-plat
controllers or should you maybe check one or both for NULL?
> +
> + /*
> +* The DWC3-exynos host controller on Exynos5420 SoC is quirky
> +* in a way that the PHY needs to be tuned to get it working
> +* at SuperSpeed. By default this PHY works as High-speed phy
> +* and so detects even Super-speed devices as high-speed ones.
> +* Therefor the PHY needs to be tuned after controller
> +* has been reset, since a controller reset actually forces the
> +* PHY to fall back to its default state wherein it works as
> +* High-Speed PHY only.
> +*/
> + if (!strstr(dev_name(vendor_parent_dev), "exynos"))
> + xhci->quirks |= XHCI_DWC3_EXYNOS;
> +
> /*
> * As of now platform drivers don't provide MSI support so we ensure
> * here that the generic code does not try to make a pci_dev from our
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 7807f62..f69af8c 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1560,6 +1560,7 @@ struct xhci_hcd {
> #define XHCI_PLAT (1 << 16)
> #define XHCI_SLOW_SUSPEND (1 << 17)
> #define XHCI_SPURIOUS_WAKEUP (1 << 18)
> +#define XHCI_DWC3_EXYNOS (1 << 19)
I think in general it's better to name quirks after what they do
rather than a specific device, that way you might be able to reuse
them. How about XHCI_TUNE_AFTER_RESET? (Or you could leave out the
quirk completely and just differentiate by whether the PHY implements
the tune() method or not.)
> unsigned intnum_active_eps;
> unsigned intlimit_active_eps;
> /* There are two roothubs to keep track of bus suspend info for */
> --
> 1.7.6.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: core: Make sure usb_set_configuration(-1) cannot fail
usb_deauthorize_device() tries to unset the configuration of a USB
device and then unconditionally blows away the configuration descriptors
with usb_destroy_configuration(). This is bad if the
usb_set_configuration() call failed before the configuration could be
correctly unset, since pointers like udev->actconfig can keep pointing
to the now invalid memory. We have encountered hard to reproduce crashes
from devices disconnected during suspend due to this, where khubd's
disconnect handling races with userspace tools that change authorization
on resume.
It seems desirable that a USB device can always be marked as
unconfigured (reclaiming its bandwidth) in the kernel, regardless of
communication problems. This patch changes usb_set_configuration() to
ignore all failures in the case where no new configuration is being set.
Signed-off-by: Julius Werner
---
drivers/usb/core/message.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index bb31597..f980b6d 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1706,7 +1706,8 @@ static void __usb_queue_reset_device(struct work_struct
*ws)
* underlying call that failed. On successful completion, each interface
* in the original device configuration has been destroyed, and each one
* in the new configuration has been probed by all relevant usb device
- * drivers currently known to the kernel.
+ * drivers currently known to the kernel. Guaranteed not to fail if the
+ * device is to be unconfigured (@configuration = -1).
*/
int usb_set_configuration(struct usb_device *dev, int configuration)
{
@@ -1774,7 +1775,7 @@ free_interfaces:
/* Wake up the device so we can send it the Set-Config request */
ret = usb_autoresume_device(dev);
- if (ret)
+ if (ret && cp)
goto free_interfaces;
/* if it's already configured, clear out old state first.
@@ -1797,7 +1798,7 @@ free_interfaces:
* installed, so that the xHCI driver can recalculate the U1/U2
* timeouts.
*/
- if (dev->actconfig && usb_disable_lpm(dev)) {
+ if (dev->actconfig && usb_disable_lpm(dev) && cp) {
dev_err(&dev->dev, "%s Failed to disable LPM\n.", __func__);
mutex_unlock(hcd->bandwidth_mutex);
ret = -ENOMEM;
--
1.7.12.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: core: Abort deauthorization if unsetting configuration fails
> Instead, how about changing usb_set_configuration() so that it will > never fail when the new config is -1? Except perhaps for -ENODEV > errors (the device has been disconnected), which > usb_deauthorize_device() could check for. Yes, that should work as well. It's really just one autoresume and one disable_lpm that can fail in that case so it shouldn't be too intrusive. I would prefer not to special-case ENODEV though, no need to add more complexity than necessary. I will write up a new version for that tomorrow. -- 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: core: Abort deauthorization if unsetting configuration fails
usb_deauthorize_device() tries to unset the configuration of a USB
device and then unconditionally blows away the configuration descriptors
with usb_destroy_configuration(). This is bad if the
usb_set_configuration() call failed before the configuration could be
correctly unset, since pointers like udev->actconfig can keep pointing
to the now invalid memory. We have encountered hard to reproduce crashes
from devices disconnected during suspend due to this, where khubd's
disconnect handling races with userspace tools that change authorization
on resume.
This patch ensures that usb_deauthorize_device() aborts when
usb_set_configuration() fails, and the underlying status code (such as
ENODEV) is returned to userspace.
Signed-off-by: Julius Werner
---
drivers/usb/core/hub.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a7c04e2..ade315f 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2420,13 +2420,17 @@ fail:
*/
int usb_deauthorize_device(struct usb_device *usb_dev)
{
+ int result = 0;
+
usb_lock_device(usb_dev);
if (usb_dev->authorized == 0)
goto out_unauthorized;
- usb_dev->authorized = 0;
- usb_set_configuration(usb_dev, -1);
+ result = usb_set_configuration(usb_dev, -1);
+ if (result)
+ goto error_deconfigure;
+ usb_dev->authorized = 0;
kfree(usb_dev->product);
usb_dev->product = kstrdup("n/a (unauthorized)", GFP_KERNEL);
kfree(usb_dev->manufacturer);
@@ -2437,9 +2441,10 @@ int usb_deauthorize_device(struct usb_device *usb_dev)
usb_destroy_configuration(usb_dev);
usb_dev->descriptor.bNumConfigurations = 0;
+error_deconfigure:
out_unauthorized:
usb_unlock_device(usb_dev);
- return 0;
+ return result;
}
--
1.7.12.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: xhci-plat: Enable USB 2.0 hardware LPM support for platform xHCs
*ping* Is anyone still reading this or should I resubmit? Sorry for being annoying, just please let me know if this is already considered to get picked up at the next opportunity or if you've intentionally decided against it for now. I want to make sure it didn't fall through the cracks somewhere. -- 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: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED
I'm not sure if it's worth it further looking into this for the SUSPENDED state (Sarah's post sounds like that shouldn't happen)... but at any rate, that would be orthogonal to my patch, right? I'm really only interested in the NOTATTACHED case, which solves a real issue on my system. Do you still have questions about that before you'd consider picking it up, Greg? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED
This patch adds a check for USB_STATE_NOTATTACHED to the
hub_port_warm_reset_required() workaround for ports that end up in
Compliance Mode in hub_events() when trying to decide which reset
function to use. Trying to call usb_reset_device() with a NOTATTACHED
device will just fail and leave the port broken.
Signed-off-by: Julius Werner
---
drivers/usb/core/hub.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e6b682c..0188056 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4799,8 +4799,9 @@ static void hub_events(void)
hub->ports[i - 1]->child;
dev_dbg(hub_dev, "warm reset port %d\n", i);
- if (!udev || !(portstatus &
- USB_PORT_STAT_CONNECTION)) {
+ if (!udev ||
+ !(portstatus & USB_PORT_STAT_CONNECTION) ||
+ udev->state == USB_STATE_NOTATTACHED) {
status = hub_port_reset(hub, i,
NULL, HUB_BH_RESET_TIME,
true);
--
1.8.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED
> I don't know either. But Sarah has said that ports can spontaneously > go into Compliance Mode for no apparent reason. If that can happen, > maybe it can happen while the port is in U3 and the device is > suspended. In such cases, though, you'd need to do a reset-resume > rather than a simple reset. Okay. Looks like this is a more complicated question so let's keep it out of this patch. We could always add another check to handle USB_STATE_SUSPENDED later. > I think keeping dev_dbg() is best. If you're searching for the > solution to a problem, you should have debugging enabled and so you > ought to see the message. Sure, I'll submit a second version in a moment. -- 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: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED
> Who makes those calls, drivers? Any specific ones that you know need to be fixed? Well, the one I'm worried about is the one this patch is fixing, in hub_events(). I have seen this happen when having certain USB3 devices plugged into a host controller that always looses power on suspend-to-ram (dwc3-exynos). Since the host controller resets itself the port no longer has PORT_STAT_ENABLE set and that causes hub_activate() to mark the device as USB_STATE_NOTATTACHED. The next time khubd runs hub_events(), the port may be in Compliance Mode (because the unexpected HC reset can confuse the link state machines on both sides) and the kernel correctly tries to reset it, but doesn't take the fact into account that usb_reset_device() doesn't work on NOTATTACHED devices. > But what can a user do if those messages show up? Nothing. I was just thinking this might help developers follow the kernel decisions better and understand a potential problem faster (e.g. if the user posted his log in a bug report somewhere). > What if the device is in USB_STATE_SUSPENDED? I'm not sure that is possible at that point in hub_events(), I don't know of a way that could lead to this situation. I could still add the check just to be sure if you want it, though. > Not at all. If a device is unplugged, its state changes to NOTATTACHED > before the driver is unbound. During that time, the driver will see > all its URBs failing, so it may very well try to reset the device. > (For example, usbhid behaves like this.) That isn't a bug. Oh, okay, I wasn't quite sure how that plays together. Would you think it's still valuable to print it out (maybe as dev_info() instead of dev_warn()) instead of just silently ignoring the reset request? It would have certainly been useful for me to find this problem faster, but I can take it out again if you think it would result in too much noise. -- 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: xhci-plat: Enable USB 2.0 hardware LPM support for platform xHCs
*bump* Hi Sarah, is there anything else that needs to be resolved to pick this patch up? Looks like Matthias' recent LPM fixes are all in now so there should be no way this could cause any trouble? -- 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: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED
This patch adds a check for USB_STATE_NOTATTACHED to the
hub_port_warm_reset_required() workaround for ports that end up in
Compliance Mode in hub_events() when trying to decide which reset
function to use. Trying to call usb_reset_device() with a NOTATTACHED
device will just fail and leave the port broken.
Also bumped the messages about this kind of reset failure from dev_dbg()
to dev_warn() to make it easier to notice, since calling that function
with a NOTATTACHED device would almost always be a bug
Signed-off-by: Julius Werner
---
drivers/usb/core/hub.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e6b682c..0188056 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4799,8 +4799,9 @@ static void hub_events(void)
hub->ports[i - 1]->child;
dev_dbg(hub_dev, "warm reset port %d\n", i);
- if (!udev || !(portstatus &
- USB_PORT_STAT_CONNECTION)) {
+ if (!udev ||
+ !(portstatus & USB_PORT_STAT_CONNECTION) ||
+ udev->state == USB_STATE_NOTATTACHED) {
status = hub_port_reset(hub, i,
NULL, HUB_BH_RESET_TIME,
true);
@@ -5074,7 +5075,7 @@ static int usb_reset_and_verify_device(struct usb_device
*udev)
if (udev->state == USB_STATE_NOTATTACHED ||
udev->state == USB_STATE_SUSPENDED) {
- dev_dbg(&udev->dev, "device reset not allowed in state %d\n",
+ dev_warn(&udev->dev, "device reset not allowed in state %d\n",
udev->state);
return -EINVAL;
}
@@ -5237,7 +5238,7 @@ int usb_reset_device(struct usb_device *udev)
if (udev->state == USB_STATE_NOTATTACHED ||
udev->state == USB_STATE_SUSPENDED) {
- dev_dbg(&udev->dev, "device reset not allowed in state %d\n",
+ dev_warn(&udev->dev, "device reset not allowed in state %d\n",
udev->state);
return -EINVAL;
}
--
1.8.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: hub: Clear Port Reset Change during init/resume
> Did you run into an issue where port status change events weren't being > generated because the Port Reset flag was set? I'm trying to figure out > if this addresses a real issue you hit (and thus should be queued for > stable), or if this is just a precaution. As Benson said, we're seeing this on the HP Chromebook 14 (Intel LynxPoint xHC). It only happens after system suspend/resume, so I'm not sure if there even is an xHC reset involved (not by the kernel anyway, but with all that other stuff it's hard to say). Note that we build our own firmware (including SMM/ACPI handlers), so this does not directly translate to LynxPoint PCs, but I think we based some of our code off Intel reference code and guides which may have already included the problem. I've found port reset code in both our firmware resume path and ACPI _PS0 handler, but they both seem to clear all Port Change bits correctly, so we are not sure about our true root cause yet. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: hub: Clear Port Reset Change during init/resume
This patch adds the Port Reset Change flag to the set of bits that are
preemptively cleared on init/resume of a hub. In theory this bit should
never be set unexpectedly... in practice it can still happen if BIOS,
SMM or ACPI code plays around with USB devices without cleaning up
correctly. This is especially dangerous for XHCI root hubs, which don't
generate any more Port Status Change Events until all change bits are
cleared, so this is a good precaution to have (similar to how it's
already done for the Warm Port Reset Change flag).
Signed-off-by: Julius Werner
---
drivers/usb/core/hub.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e6b682c..c3dd64c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1130,6 +1130,11 @@ static void hub_activate(struct usb_hub *hub, enum
hub_activation_type type)
usb_clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_ENABLE);
}
+ if (portchange & USB_PORT_STAT_C_RESET) {
+ need_debounce_delay = true;
+ usb_clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_C_RESET);
+ }
if ((portchange & USB_PORT_STAT_C_BH_RESET) &&
hub_is_superspeed(hub->hdev)) {
need_debounce_delay = true;
--
1.7.12.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: hub: Clear Port Reset Change during init/resume
>> + if ((portchange & USB_PORT_STAT_C_RESET)) {
>
>
>Hm, why these double parens?
Oh... good question. I copied the entry below it, remove the && and
must have overlooked those. Sorry, v2 incoming...
--
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: hub: Clear Port Reset Change during init/resume
This patch adds the Port Reset Change flag to the set of bits that are
preemptively cleared on init/resume of a hub. In theory this bit should
never be set unexpectedly... in practice it can still happen if BIOS,
SMM or ACPI code plays around with USB devices without cleaning up
correctly. This is especially dangerous for XHCI root hubs, which don't
generate any more Port Status Change Events until all change bits are
cleared, so this is a good precaution to have (similar to how it's
already done for the Warm Port Reset Change flag).
Signed-off-by: Julius Werner
---
drivers/usb/core/hub.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e6b682c..c9ef5b8 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1130,6 +1130,11 @@ static void hub_activate(struct usb_hub *hub, enum
hub_activation_type type)
usb_clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_ENABLE);
}
+ if ((portchange & USB_PORT_STAT_C_RESET)) {
+ need_debounce_delay = true;
+ usb_clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_C_RESET);
+ }
if ((portchange & USB_PORT_STAT_C_BH_RESET) &&
hub_is_superspeed(hub->hdev)) {
need_debounce_delay = true;
--
1.7.12.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] xhci: Enable LPM support only for hardwired or BESL devices
> +#include "../core/usb.h" You might want to move usb_get_hub_port_connect_type() to include/linux/usb.h instead of doing this. Also, I think you need to mark it EXPORT_SYMBOL_GPL in core/hub.c or you could run into trouble when both xhci-hcd and usbcore are compiled as modules. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] USB 2.0 Link PM is broken
> That behavior was seen on the Synopsys host, not the Intel host, > correct? Yes. Looks like the L1 transitions that are not fatal on the Intel host are much longer in my trace, usually above 100ms. This would be another indication that in the Synopsys case the L1 resume is host-triggered. > Ok. Mathias has a patch to enable it for internal devices and BESL > devices. I'll send out the updated patchset shortly. Can you run lsusb > on your Intel system, and see if the webcam supports Link PM? If so, it > would be great if you could add the patches, disable auto-suspend, and > double check that the latest version of lsusb shows that the device goes > into L1. I don't have access to a system today or tomorrow, but I can > check next week if necessary. Unfortunately, none of the internal devices on our current Haswell system support LPM. Our firmware also doesn't set the right ACPI table entries to mark ports as removable. I think the easiest way to verify your patch would be a small kernel hack that would force an inserted device to look non-removable. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] USB 2.0 Link PM is broken
> My hypothesis is that the Synopsys host doesn't go into L1 if the device > NAKs a transfer, only when the bus is idle. That way, it doesn't have > to depend on L1 remote wakeup, which is broken for these devices. I > don't have a way to confirm that though. Paul, is the Synopsys host > working around these broken devices? I think the device actually NAKs the IN transfer of the MSC command in my test, the same way it does on Haswell. It does go into L1, but then there is a resume just a few dozen microseconds later. Unfortunately its very hard to differentiate between device and host-initiated resume in USB 2.0... but maybe the Synopsys controller just wakes stuff up on its own in some circumstances? There doesn't seem to be a clear guideline in the spec for when to trigger a host-initiated resume, so maybe they just do it whenever there are transfers queued up for a slot or something. Also, I only know that the device doesn't fail, I'm not that convinced that the LPM really works as it should. I see a lot of L1-suspends that only last for a few dozen microseconds (and the resume signalling after that takes more than a millisecond), which probably doesn't really save power. In some of those cases the host waits a few frames before scheduling a new command after resume, which suggests that the wakeup was earlier than necessary. > I do want to allow the Synopsys host to have USB 2.0 Link PM enabled if > the host has a way to work around these broken devices. Paul and > Julius, let's work out a solution to do this on top of these patches. > I suspect that the solution may be to add an update_device method to > xhci-plat that sets udev->usb2_hw_lpm_allowed, calls xhci_update_device, > and then calls usb_set_usb2_hardware_lpm(). We'll have to wait for the > updated patches from Mathias though. I'm not sure if it's necessary to make this decision in the kernel, since this seems to be very specific to a particular controller (we maybe should try out some external PCI controllers if anyone has one). I would be fine with just turning this on from user-space as we already do with autosuspend, where we can implement arbitrary policies for any particular HC/device combination. -- 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: xhci-plat: Enable USB 2.0 hardware LPM support for platform xHCs
> If you take a look at Table > 13: BESL/HIRD Encoding from the xHCI spec version including errata to > 08/14/2012 Could you please provide a link to that errata? I still cannot find it... but from your explanation, that design decision sounds pretty horrible. Why didn't they just choose not to set old HLC flag in BESL controllers? Seems like the only purpose it provides there is to make old drivers break. Anyway, looks like we are stuck with it now, and need to deal with those mislabeled DWC3 versions. I agree with you that we should blacklist instead of whitelist, but I don't think the device tree is the best place to put that... we would have to figure out the exact DWC3 version for every processor/SoC dtsi file to determine if they are affected, and remember to keep that up to date as we added more. I would instead propose to check for the revision register directly in the DWC3 stack. I think I could add a little check to dwc3_host_init() and hack the quirk bit into the newly created XHCI controller instance if required. However, I only have an old (unaffected) 1.85 controller for testing, so I would need Synopsys to provide me with the exact revision numbers affected (as read from the register) and to test the change for us. Also, do we want to make it an XHCI_DISABLE_USB2_LPM or a XHCI_FORCE_USB2_BESL quirk? Assuming their BESL implementation is otherwise correct except for omitting that bit, the latter one should make those controllers work better. -- 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: xhci-plat: Enable USB 2.0 hardware LPM support for platform xHCs
> So the 2.41a has BESL support, but may not set the BLC flag. What > happens if we use the HIRD encoding instead? Will things break? It > seems like we would need to disable USB 2.0 LPM on that host all > together, if it expects BESL encoding, but advertises HIRD encoding. Wait a second, just for clarity: are you saying that BESL-capable controllers do not support the old HIRD mechanism and thus just break on non-BESL aware OSes? I would've assumed that they somehow notice if software doesn't write to the new register and automatically fall back to HIRD... it seems like a weird decision to break hardware backwards compatibility like that (after all, it would mean that Linux 3.10 and older would also break on LynxPoint systems right now). -- 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: phy: samsung-usb2: Toggle HSIC GPIO from device tree
I've tried to get the 3503 driver to work in my case for quite some time, but ultimately gave up. For me, playing around with the load order was not enough to solve all issues. When you try to build a permanent, clean solution for this, you should definitely also test the case where the hub has already been initialized and configured by firmware before the kernel booted, because that brought on another bunch of issues for me. I think it ultimately only works reliably when you first reset the hub, then reset the HSIC port on the PHY side before the USB core gets a chance to talk to it again (thus one of the drivers needs to be directly connected to and call the other). -- 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/3 v5] usb: phy-samsung-usb: Simplify PMU register handling
*Ping!* Are there still unanswered concerns left with this patch? I hope my prior mails could clear up why I think that the PMU register description in the device tree for a specific PHY is represents the hardware more accurately after my patch, and my analysis of the Exynos4 situation currently not being implemented (and therefore not needing to be addressed by this patch) was correct. Please let me know if you have further objections... and if not, could we agree to have this picked up somewhere? Thanks, Julius -- 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: xhci-plat: Enable USB 2.0 hardware LPM support for platform xHCs
> You need the USB 2.0 spec errata from 2011-11 that describes the changes > made for BESL as well. It's in the USB2-LPM-Errata-final.pdf and > USB2_LinkPowerMangement_ECN[final].pdf files in this zip file: > > http://www.usb.org/developers/docs/usb_20_070113.zip > > I agree though, it's all a confusing mess for documentation on USB 2.0 > Link PM. Thanks, I hadn't found that first one yet. Do you also have a link for the updated XHCI specification or a separate erratum document describing the PORTHLPMC register referenced in the BESL patches (they don't exist on DWC3, but I'm still curious)? -- 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: xhci-plat: Enable USB 2.0 hardware LPM support for platform xHCs
> Thanks for the patch! Did you test with a USB analyzer to see if the > device was actually going into USB 2.0 Link PM? I'd like to confirm we > really aren't breaking anything for DW3 hosts by enabling this. Yes, I did. The LPM transfers on the analyzer look good and the device works as expected. My platform only runs a 3.8 derivative, though, so I now have also cherry-picked the BESL patches that went in since then to make sure they don't break things. I had problems on one device until I found the XHCI_BLC fix Mathias sent out this morning, so you should pick that one up first. (Without it LPM doesn't break completely but seems to assert resume for less time than the HIRD defines, so the device sometimes gets confused and resets. I can't figure out how BESL is really supposed to work since the XHCI spec from 8/14/12 where it's supposedly defined doesn't seem to be publicly available.) -- 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: xhci-plat: Enable USB 2.0 hardware LPM support for platform xHCs
The driver methods required for hardware LPM have only been added to the
PCI version of the XHCI driver, for no apparent reason. They seem to
work just as well with the platform driver, so let's add them to give
more devices the chance for additional power savings. Tested on the DWC3
xHC of an Exynos5420.
Signed-off-by: Julius Werner
---
drivers/usb/host/xhci-plat.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 51e22bf..7f46b5d 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -80,6 +80,10 @@ static const struct hc_driver xhci_plat_xhci_driver = {
.hub_status_data = xhci_hub_status_data,
.bus_suspend = xhci_bus_suspend,
.bus_resume = xhci_bus_resume,
+
+ /* LPM support */
+ .update_device =xhci_update_device,
+ .set_usb2_hw_lpm = xhci_set_usb2_hardware_lpm,
};
static int xhci_plat_probe(struct platform_device *pdev)
--
1.7.12.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: xhci: Use non-interruptible sleep for XHCI commands
Hi Sarah, I know you are probably swamped with patches, but this (https://patchwork.kernel.org/patch/2612831/) has been hanging a few months and I just wanted to double-check if it slipped through somewhere. I still think this is necessary (regardless of any command queue reengineering in the future) as explained in my last mail. Please let me know if (and why) you disagree, and whether you have intentionally decided to not pick this up. Same goes (essentially) for: usb: xhci: Fix Command Ring Stopped Event handling (https://patchwork.kernel.org/patch/2612821/) usb: xhci: Consolidate XHCI TD Size calculation (https://patchwork.kernel.org/patch/2548321/) 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: [PATCH] usb: phy: Cleanup error code in **_usb_get_phy_**() APIs
>> I liked the ones where we had IS_ERR_OR_NULL() here (and in all the >> ones below)... you sometimes have to handle PHYs in >> platform-independent code where you don't want to worry about if this >> platform actually has a PHY driver there or not. Any reason you >> changed that? > > The **get_phy_*() APIs never return a NULL pointer now, do we still > need to handle that in that case. > Or are we assuming that code will use these phy operations without > getting a phy in the first place ? In our 5420 PHY tune patch (which I think has not made it upstream yet), we're calling usb_phy_tune(hcd->phy) from the USB core. This pointer is usually NULL unless it has been explicitly set by the platform specific HCD driver. For situations like that I think it's convenient if you can just fire-and-forget a generic PHY method without worrying whether the particular PHY implements it or whether it has a driver at all. -- 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/3 v5] usb: phy-samsung-usb: Simplify PMU register handling
> Sorry, I don't understand what is not implemented. Without your patch, the > PHY driver handles both PMU registers of Exynos4. I don't have an Exynos4 to actually test this, so please let me know if I'm missing something here... but in order to hit the right HOST PHY register in the current upstream code, the Exynos4 code would need to have a hostphy_reg_offset of 4 somewhere in its samsung_usbphy_drvdata. In my latest checkout of Linus' tree (6c2580c) it does not (only Exynos5 sets that attribute), so it would default to 0 (thereby actually hitting the DEVICE register). If you want I can gladly provide another change on top of my patchset to fix that in the future... but it looks to me like it had been broken anyway for now. -- 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/3 v5] usb: phy-samsung-usb: Simplify PMU register handling
> I'm not sure I understand. The old documentation referred to the > USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers for a phy, and > your new version only refers to (usb device) PHY_CONTROL. Regardless of > multiple phys, you're suggesting that we describe less of each phy. > That seems like taking away usable information. Unless I've > misunderstood? Well that's just the thing that's confusing right now, and which I am trying to fix: every PHY is either DEVICE or HOST and thus has only one PMU register. The current code describes the PMU register space for all PHYs on the system in the DT entry of every PHY and then calculates which register to use with hardcoded offsets. I think it makes much more sense if every PHY only describes its own register and doesn't need to do address arithmetic later on. As Vivek said there is one exception in an old Exynos4, but that is currently not implemented in the upstream kernel anyway, and if it ever will be it's still much easier to special case one weird chip than to have a super complicated and confusing mechanism for all of them. -- 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: phy: Cleanup error code in **_usb_get_phy_**() APIs
> @@ -94,11 +94,11 @@ static int devm_usb_phy_match(struct device *dev, void
> *res, void *match_data)
> */
> struct usb_phy *devm_usb_get_phy(struct device *dev, enum usb_phy_type type)
> {
> - struct usb_phy **ptr, *phy;
> + struct usb_phy *phy = ERR_PTR(-ENOMEM), **ptr;
This looks a little roundabout, don't you think? Why don't you just
directly have 'return ERR_PTR(-ENOMEM)' down there where you put 'goto
err0'?
>
> ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
> if (!ptr)
> - return NULL;
> + goto err0;
>
> phy = usb_get_phy(type);
> if (!IS_ERR(phy)) {
> @@ -107,6 +107,7 @@ struct usb_phy *devm_usb_get_phy(struct device *dev, enum
> usb_phy_type type)
> } else
> devres_free(ptr);
>
> +err0:
> return phy;
> }
> EXPORT_SYMBOL_GPL(devm_usb_get_phy);
> struct usb_phy *devm_usb_get_phy_dev(struct device *dev, u8 index)
> {
> - struct usb_phy **ptr, *phy;
> + struct usb_phy *phy = ERR_PTR(-ENOMEM), **ptr;
Same here
>
> ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
> if (!ptr)
> - return NULL;
> + goto err0;
>
> phy = usb_get_phy_dev(dev, index);
> if (!IS_ERR(phy)) {
> @@ -267,6 +268,7 @@ struct usb_phy *devm_usb_get_phy_dev(struct device *dev,
> u8 index)
> } else
> devres_free(ptr);
>
> +err0:
> return phy;
> }
> EXPORT_SYMBOL_GPL(devm_usb_get_phy_dev);
> @@ -142,7 +142,7 @@ extern void usb_remove_phy(struct usb_phy *);
> /* helpers for direct access thru low-level io interface */
> static inline int usb_phy_io_read(struct usb_phy *x, u32 reg)
> {
> - if (x->io_ops && x->io_ops->read)
> + if (!IS_ERR(x) && x->io_ops && x->io_ops->read)
I liked the ones where we had IS_ERR_OR_NULL() here (and in all the
ones below)... you sometimes have to handle PHYs in
platform-independent code where you don't want to worry about if this
platform actually has a PHY driver there or not. Any reason you
changed that?
> return x->io_ops->read(x, reg);
>
> return -EINVAL;
> @@ -150,7 +150,7 @@ static inline int usb_phy_io_read(struct usb_phy *x, u32
> reg)
>
> static inline int usb_phy_io_write(struct usb_phy *x, u32 val, u32 reg)
> {
> - if (x->io_ops && x->io_ops->write)
> + if (!IS_ERR(x) && x->io_ops && x->io_ops->write)
> return x->io_ops->write(x, val, reg);
>
> return -EINVAL;
> @@ -159,7 +159,7 @@ static inline int usb_phy_io_write(struct usb_phy *x, u32
> val, u32 reg)
> static inline int
> usb_phy_init(struct usb_phy *x)
> {
> - if (x->init)
> + if (!IS_ERR(x) && x->init)
> return x->init(x);
>
> return 0;
> @@ -168,26 +168,27 @@ usb_phy_init(struct usb_phy *x)
> static inline void
> usb_phy_shutdown(struct usb_phy *x)
> {
> - if (x->shutdown)
> + if (!IS_ERR(x) && x->shutdown)
> x->shutdown(x);
> }
>
> static inline int
> usb_phy_vbus_on(struct usb_phy *x)
> {
> - if (!x->set_vbus)
> - return 0;
> + if (!IS_ERR(x) && x->set_vbus)
> + return x->set_vbus(x, true);
>
> - return x->set_vbus(x, true);
> + return 0;
> }
>
> static inline int
> usb_phy_vbus_off(struct usb_phy *x)
> {
> - if (!x->set_vbus)
> - return 0;
> + if (!IS_ERR(x) && x->set_vbus)
> + return x->set_vbus(x, false);
> +
> + return 0;
>
> - return x->set_vbus(x, false);
> }
>
> /* for usb host and peripheral controller drivers */
> @@ -249,8 +250,9 @@ static inline int usb_bind_phy(const char *dev_name, u8
> index,
> static inline int
> usb_phy_set_power(struct usb_phy *x, unsigned mA)
> {
> - if (x && x->set_power)
> + if (!IS_ERR(x) && x->set_power)
> return x->set_power(x, mA);
> +
> return 0;
> }
>
> @@ -258,28 +260,28 @@ usb_phy_set_power(struct usb_phy *x, unsigned mA)
> static inline int
> usb_phy_set_suspend(struct usb_phy *x, int suspend)
> {
> - if (x->set_suspend != NULL)
> + if (!IS_ERR(x) && x->set_suspend != NULL)
> return x->set_suspend(x, suspend);
> - else
> - return 0;
> +
> + return 0;
> }
>
> static inline int
> usb_phy_notify_connect(struct usb_phy *x, enum usb_device_speed speed)
> {
> - if (x->notify_connect)
> + if (!IS_ERR(x) && x->notify_connect)
> return x->notify_connect(x, speed);
> - else
> - return 0;
> +
> + return 0;
> }
>
> static inline int
> usb_phy_notify_disconnect(struct usb_phy *x, enum usb_device_speed speed)
> {
> - if (x->notify_disconnect)
> + if (!IS_ERR(x) && x->notify_disconnect)
> return x->notify_disconnect(x, speed);
> - else
> - return 0;
> +
> + return 0;
> }
O
Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling
> This breaks compatibility, both for an old kernel and a new dt and a new > kernel with an old dt. Is anyone using these bindings? They only affect Samsung SoCs and have only been upstream for half a year, so I doubt it's heavily used. > Why are we describing fewer registers now? Are they described elsewhere? > > The dt should describe the device, not only the portion of it Linux > wants to use right now. This only ever described a small section of the huge set of PMU registers anyway. Before it described up to three registers controlling different PHYs (using hardcoded offsets in the code to later find the right one)... with my patch every PHY's DT entry only describes the one register concerning itself, which makes more sense in my opinion. It will also prevent the register descriptions in different DT entries from overlapping. -- 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
