RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Peter Stuge > But what about that alignment? It seems that userspace > needs to start caring about alignment with xhci, right? No because there is a copy_to/from_user() in the middle. The ehci/ohci/uhci all require that fragments be a multiple of the usb message size (512 bytes for USB2). So everything (until very recently) would always supply suitable aligned buffers. Mostly they are page aligned. For those who haven't read the xhci spec carefully: The xhci controller removes the requirement on dma segments being aligned to usb messages. However there are two alignment requirements: 1) dma segments must not cross 64k address boundaries. This is documented clearly, even though it is a slight pain. You'd have thought the address counter could have more than 16 bits these days! There only 17 bits for the length, but a length restriction would be less of a problem. 2) The v1.00 version of the specification adds that the end of the transfer ring can only occur at a 'TD fragment' boundary. These are aligned with the payload 'bursts' - which can be sixteen 1k packets. I think that breaking the second of these causes a usb message be split into two small pieces - which will terminate bulk xfers. The asix USB3 Ge silicon gets very confused when this happens. David -- 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] tools: usb: aio example applications
On 01/30/2014 04:30 PM, Peter Stuge wrote:
> Robert Baldyga wrote:
>> v3:
> ..
>> +++ b/tools/usb/aio_multibuff/host_app/Makefile
>> @@ -0,0 +1,13 @@
>> +CC = gcc
>> +LIBUSB_CFLAGS = $(shell pkg-config --cflags libusb-1.0)
>> +LIBUSB_LIBS = $(shell pkg-config --libs libusb-1.0)
>> +WARNINGS = -Wall -Wextra
>> +CFLAGS = $(LIBUSB_CFLAGS) $(WARNINGS)
>> +LDFLAGS = $(LIBUSB_LIBS)
>> +
>> +all: test
>> +%: %.c
>> +$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS)
>> +
>> +clean:
>> +$(RM) test
>
> Nice!
>
>
>> +++ b/tools/usb/aio_multibuff/host_app/test.c
> ..
>> +cnt = libusb_get_device_list(state->ctx, &list);
>> +if (cnt < 0) {
>> +printf("no devices found\n");
>> +goto error1;
>> +}
> ..
>> +error1:
>> +libusb_free_device_list(list, 1);
>> +libusb_exit(state->ctx);
>> +return 1;
>> +}
>
> The above tries to free uninitialized memory in the error path.
Right, thanks :)
>
>
>> +for (i = 0; i < cnt; ++i) {
>> +libusb_device *dev = list[i];
>> +struct libusb_device_descriptor desc;
>> +if (libusb_get_device_descriptor(dev, &desc)) {
>> +printf("unable to get device descriptor\n");
>> +goto error1;
>> +}
>> +if (desc.idVendor == VENDOR && desc.idProduct == PRODUCT) {
>> +state->found = dev;
>> +break;
>> +}
>> +}
>> +
>> +if (state->found) {
> ...
>> +} else {
>> +printf("no devices found\n");
>> +goto error1;
>> +}
>
> A matter of taste, but I would reverse the if () condition to avoid
> the extra indent level for when a device has been found.
>
> I find that makes it more clear what part of the code handles errors
> and what part is the expected common case.
>
>
> A few other things in the same code:
>
>> +if (state->found) {
>> +printf("found device\n");
>> +
>> +printf("open device: ");
>> +if (libusb_open(state->found, &state->handle)) {
>> +printf("ERROR\n");
>> +goto error1;
>> +}
>> +printf("DONE\n");
>> +
>> +if (libusb_kernel_driver_active(state->handle, 0)) {
>> +printf("device busy.. detaching\n");
>> +if (libusb_detach_kernel_driver(state->handle, 0)) {
>> +printf("unable do deatch device\n");
>
> Typo "deatch"
>
>
>> +goto error2;
>> +}
>> +state->attached = 1;
>> +} else
>> +printf("device free from kernel\n");
>
> This isn't completely accurate, in two ways. First, it's only the
> interface and not the entire device which is claimed/detached.
> Second, it could be either another userspace program (via the
> usbfs kernel driver) or it could be a kernel driver which has
> claimed the interface.
>
> libusb_detach_kernel_driver() makes it so that no kernel driver
> (usbfs or other) is attached to the interface.
>
> libusb_claim_interface() then makes the usbfs kernel driver attach
> the interface.
>
>
>> +
>> +if (libusb_claim_interface(state->handle, 0)) {
>> +printf("failed to claim interface\n");
>> +goto error3;
>> +}
>> +} else {
>
> I still recommend the open/claim/detach logic to be:
>
> libusb_open()
> if (libusb_claim_interface() != LIBUSB_SUCCESS) {
> ret = libusb_detach_kernel_driver();
> if (ret != LIBUSB_SUCCESS) {
> printf("unable to detach kernel driver: %s\n", libusb_error_name(ret));
> goto ..
> }
> ret = libusb_claim_interface();
> if ( != LIBUSB_SUCCESS) {
> printf("cannot claim interface: %s\n", libusb_error_name(ret));
> goto ..
> }
> }
>
> If you prefer to be more conservative then don't use libusb_error_name()
> which was only introduced in late 2011 and released in 2012.
>
Thanks for feedback! I will take it into account in v4.
Best regards
Robert Baldyga
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] tools: usb: aio example applications
On 01/30/2014 11:40 AM, David Laight wrote: > From: Robert Baldyga > This patch adds two example applications showing usage of Asynchronous I/O API >> of FunctionFS. First one (aio_simple) is simple example of bidirectional data >> transfer. Second one (aio_multibuff) shows multi-buffer data transfer, which >> may to be used in high performance applications. >> >> Both examples contains userspace applications for device and for host. >> It needs libaio library on the device, and libusb library on host. > ... > >> +int main() > > You've still got K&R function definitions... > I also suspect that most coding standard want structure definitions > and #defines at the top of the file. > > David > Ok,thanks. I will fix it. Best regards Robert Baldyga Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Sarah Sharp > We need to step back and reassess the larger picture here, instead of > trying to fire-fight all the regressions caused by the link TRB commit > (35773dac5f86 "usb: xhci: Link TRB must not occur within a USB payload > burst"). Some of the breakage seems to have been related to the PM and readq/writeq changes. The main problem with that patch is that it limited the number of fragments. > We shouldn't need to make userspace start to worry about alignment at > all. libusb worked in the past, before the link TRB fix went in. We > *cannot* break userspace USB drivers. The breakage needs to be fixed in > the USB core or the xHCI driver. Userspace doesn't care since everything gets copied into aligned kernel fragments - otherwise the other usb controllers wouldn't work. > Commit 35773dac5f86 was meant to be a short-term bandaid fix, but it's > already caused at least four different regressions. Some we've fixed, > some have proposed solutions that David has sent. > > The storage layer is getting borked because it submits scatter-gather > lists longer than what will fit on a segment, and now libusb has the > same issue. One xHCI host controller stopped responding to commands, > and reverting the bandaid fix helped. The implications of this change > just keep coming in, and I'm not comfortable wall-papering over the > issues. The transfers from the storage layer are actually all 'suitably aligned'. Fragments can cross 64k boundaries, but they all start on 4k boundaries. > On the flip side, it seems that the only devices that have been helped > by the bandaid fix patch are USB ethernet devices using the ax88179_178a > driver. (Mark Lord still needs to confirm which device he uses.) I > have not seen any other reports that other USB ethernet chipsets were > broken in 3.12 by the USB networking layer adding scatter-gather > support. That is the only usbnet driver for which SG support is enabled. I believe it was all enabled because the ax88179_178a is the only one that can be expected to saturate Ge and supports TCP segmentation offload (TSO) - where the buffer is almost always fragmented. With TSO transmits are almost certainly single fragments. > It should not matter what alignment or length of scatter-gather list the > upper layers pass the xHCI driver, it should just work. I want to do > this fix right, by changing the fundamental way we queue TRBs to the > rings to fit the TD rules. We should break each TD into fragment-sized > chunks, and add a link TRB in the middle of a segment where necessary. There will always be some transfer requests that make this impossible (without using a bounce buffer). In practise nothing will send such bad transfers. To avoid excessive work you need to be told whether the transfer is aligned (everything from the block layer and libusb is) or misaligned (potentially everything from usbnet). The limits on the number length of SG list has to be different for the two types of request. > Let's do this fix the right way, instead of wall papering over the > issue. Here's what we should do: > > 1. Disable scatter-gather for the ax88179_178a driver when it's under an >xHCI host. That can be done by simple not setting the flag on the xhci driver. However you also need to double check that this disables TSO. > 2. Revert the following commits: >f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block writes. >d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many trbs >35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload burst > > 3. Dan and Mathias can work together to come up with an overall plan to >change the xHCI driver architecture to be fully compliant with the TD >fragment rules. That can be done over the next few kernel releases. Don't forget that these rules can affect isoc transfers as well. Even without SG, the buffer can cross a 64k boundary and thus need splitting into separate TRB - which might need to be in different ring segments. Easily fixable by writing the LINK early (expect that the TRB writing code looks for LINK TRBs). > The end result is that we don't destabilize storage or break userspace > USB drivers, we don't break people's xHCI host controllers, > the ax88179_178a USB ethernet devices still work under xHCI (a bit with > worse performance), and other USB ethernet devices still get the > performance improvement introduced in 3.12. -- 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: dwc3: keystone: switch to use runtime pm
The Keystone PM management layer has been implemented using PM bus for
power management clocks. As result, most of Keystone drivers don't need
to manage clocks directly. They just need to enable runtime PM and use it
to handle their PM state and clocks.
Hence, remove clock management code and switch to use runtime PM.
Signed-off-by: Grygorii Strashko
---
drivers/usb/dwc3/dwc3-keystone.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
index 1fad161..a810b41 100644
--- a/drivers/usb/dwc3/dwc3-keystone.c
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
/* USBSS register offsets */
#define USBSS_REVISION 0x
@@ -116,12 +117,10 @@ static int kdwc3_probe(struct platform_device *pdev)
kdwc3_dma_mask = dma_get_mask(dev);
dev->dma_mask = &kdwc3_dma_mask;
- kdwc->clk = devm_clk_get(kdwc->dev, "usb");
-
- error = clk_prepare_enable(kdwc->clk);
+ pm_runtime_enable(dev);
+ error = pm_runtime_get_sync(dev);
if (error < 0) {
- dev_dbg(kdwc->dev, "unable to enable usb clock, err %d\n",
- error);
+ dev_dbg(dev, "unable to enable usb dev, err %d\n", error);
return error;
}
@@ -152,7 +151,8 @@ static int kdwc3_probe(struct platform_device *pdev)
err_core:
kdwc3_disable_irqs(kdwc);
err_irq:
- clk_disable_unprepare(kdwc->clk);
+ pm_runtime_put_sync(dev);
+ pm_runtime_disable(dev);
return error;
}
@@ -172,7 +172,8 @@ static int kdwc3_remove(struct platform_device *pdev)
kdwc3_disable_irqs(kdwc);
device_for_each_child(&pdev->dev, NULL, kdwc3_remove_core);
- clk_disable_unprepare(kdwc->clk);
+ pm_runtime_put_sync(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
platform_set_drvdata(pdev, NULL);
return 0;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
David Laight wrote: > > We shouldn't need to make userspace start to worry about alignment at > > all. libusb worked in the past, before the link TRB fix went in. We > > *cannot* break userspace USB drivers. The breakage needs to be fixed in > > the USB core or the xHCI driver. > > Userspace doesn't care since everything gets copied into aligned > kernel fragments - otherwise the other usb controllers wouldn't work. OK, but not so great if someone wants to squeeze the most performance possible out of USB also from userspace. I'm going off on a tangent now but would it make sense to allow userspace to do alignment if it wants to, and have a way to tell the kernel when urb buffers are pre-aligned? //Peter -- 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 1/1] usb: Tell xhci when usb data might be misaligned
From: Peter Stuge [mailto:pe...@stuge.se] > > Userspace doesn't care since everything gets copied into aligned > > kernel fragments - otherwise the other usb controllers wouldn't work. > > OK, but not so great if someone wants to squeeze the most performance > possible out of USB also from userspace. > > I'm going off on a tangent now but would it make sense to allow > userspace to do alignment if it wants to, and have a way to tell > the kernel when urb buffers are pre-aligned? I can only see that mattering if either: 1) The userspace buffers are (say) 4n+1 aligned and the kernel decides to align the copy_from_user(). 2) The code is doing buffer-loaning. Personally I'm not at all sure how often buffer-loaning helps (given the cost of the TLB shootdowns that it often implies). I guess it might be ok if the memory doesn't have to be given a KVA and the user program avoids any COW. In any case and such code could be limited to page-aligned transfers. And/or the user code would have to know at least some of the constraints. The other usb controllers only support 'message' aligned transfers (512 bytes for USB2). All the code I've found achieves this by using page aligned (maybe 4k aligned) fragments. xhci trivially supports this (it has since the 'ring expansion' code was added). xhci can also easily support: 1) Arbitrary fragmentation for a limited number of fragments (by constraining the fragments to a single ring segment). 2) Arbitrary fragmentation provided all the fragments (except the last) exceed some minimal length (by splitting the current or previous fragment at the appropriate boundary). The code for the second is probably worth adding just in case a 4k fragment crosses a 64k boundary. Since arbitrarily fragmented packets can't be sent to other controllers it does seem sensible for the code generating the urb so say that it is (or might be) fragmented like that. David -- 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
FunctionFS: iInterface value seems to have a bug
Using the userspace test application ffs-test.c with the g_ffs module I noticed that the "iInterface" field in the Interface descriptor often has a zero value (Shown on the host side) which leads to the corresponding language string not being shown. In my tests values ranged from 0x09 to 0x0C, but mostly 0x09. It changes at every start of the application. Seems to be a bug in the module? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm
On Friday 31 January 2014 08:20 AM, Grygorii Strashko wrote:
> The Keystone PM management layer has been implemented using PM bus for
> power management clocks. As result, most of Keystone drivers don't need
> to manage clocks directly. They just need to enable runtime PM and use it
> to handle their PM state and clocks.
>
> Hence, remove clock management code and switch to use runtime PM.
>
> Signed-off-by: Grygorii Strashko
> ---
Please capture why now it allowes us to remove the clock code.
Commit 8a6720e {PM / clock_ops: fix up clk prepare/unprepare count}
Without that information, the change log will be miss-leading
> drivers/usb/dwc3/dwc3-keystone.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-keystone.c
> b/drivers/usb/dwc3/dwc3-keystone.c
> index 1fad161..a810b41 100644
> --- a/drivers/usb/dwc3/dwc3-keystone.c
> +++ b/drivers/usb/dwc3/dwc3-keystone.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
>
> /* USBSS register offsets */
> #define USBSS_REVISION 0x
> @@ -116,12 +117,10 @@ static int kdwc3_probe(struct platform_device *pdev)
> kdwc3_dma_mask = dma_get_mask(dev);
> dev->dma_mask = &kdwc3_dma_mask;
>
> - kdwc->clk = devm_clk_get(kdwc->dev, "usb");
> -
> - error = clk_prepare_enable(kdwc->clk);
> + pm_runtime_enable(dev);
> + error = pm_runtime_get_sync(dev);
> if (error < 0) {
> - dev_dbg(kdwc->dev, "unable to enable usb clock, err %d\n",
> - error);
> + dev_dbg(dev, "unable to enable usb dev, err %d\n", error);
> return error;
> }
>
> @@ -152,7 +151,8 @@ static int kdwc3_probe(struct platform_device *pdev)
> err_core:
> kdwc3_disable_irqs(kdwc);
> err_irq:
> - clk_disable_unprepare(kdwc->clk);
> + pm_runtime_put_sync(dev);
> + pm_runtime_disable(dev);
>
> return error;
> }
> @@ -172,7 +172,8 @@ static int kdwc3_remove(struct platform_device *pdev)
>
> kdwc3_disable_irqs(kdwc);
> device_for_each_child(&pdev->dev, NULL, kdwc3_remove_core);
> - clk_disable_unprepare(kdwc->clk);
> + pm_runtime_put_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> platform_set_drvdata(pdev, NULL);
>
> return 0;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MAX3421E Linux driver and DGStation
Some further digging suggests that there was support for the MAX3421 in a couple of Linux based satellite receivers, specficially the IpBox 250s and DGStation Mutant-200s The producer of the later was even subject to a GPL violation and resolution where they made a kernel patch available, although this apears 'lost' too. http://web.archive.org/web/20090709064234/http://www.dgstation.co.kr/new/news_software.php?code=27&mode=view So if anyone here was involved in that scene it migth be a good time to see if you have a copy of 'linux-2.6.17.14.DGStation.patch.tar.gz' siting on a harddisk and upload it somewhere, so we can see whether it contains this and other goodies. Cheers, Simon. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm
Hi, On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote: > The Keystone PM management layer has been implemented using PM bus for > power management clocks. As result, most of Keystone drivers don't need > to manage clocks directly. They just need to enable runtime PM and use it > to handle their PM state and clocks. > > Hence, remove clock management code and switch to use runtime PM. > > Signed-off-by: Grygorii Strashko quite a few weeks back I sent a series enabling runtime pm for all glue layers. I'll use that version instead, sorry. -- balbi signature.asc Description: Digital signature
RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: Sarah Sharp > On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: > > FWIW, the plan looks fine to me. Just adding a couple of hints to > > simplify the implementation. > > > > Sarah Sharp writes: > > > > > Let's do this fix the right way, instead of wall papering over the > > > issue. Here's what we should do: > > > > > > 1. Disable scatter-gather for the ax88179_178a driver when it's under an > > >xHCI host. > > > > No need to make this conditional. SG is only enabled in the > > ax88179_178a driver if udev->bus->no_sg_constraint is true, so it > > applies only to xHCI hosts in the first place. Leave the usbnet code alone and unset udev->bus->no_sg_constraint. David -- 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: host: xhci-plat: Use module_platform_driver()
Sarah,
On Fri, Jan 31, 2014 at 2:29 AM, Fabio Estevam wrote:
> From: Fabio Estevam
>
> Using module_platform_driver() can make the code simpler.
>
> Signed-off-by: Fabio Estevam
> ---
> Build-tested only
>
> Changes since v1:
> - Mark the patch as 1/2
>
> drivers/usb/host/xhci-plat.c | 11 +--
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 8abda5c..488e3d4 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -242,13 +242,4 @@ static struct platform_driver usb_xhci_driver = {
> },
> };
> MODULE_ALIAS("platform:xhci-hcd");
> -
> -int xhci_register_plat(void)
> -{
> - return platform_driver_register(&usb_xhci_driver);
> -}
> -
> -void xhci_unregister_plat(void)
> -{
> - platform_driver_unregister(&usb_xhci_driver);
> -}
> +module_platform_driver(usb_xhci_driver);
Please discard this series. We cannot remove xhci_register_plat() as
it is used in xhci.c
Regards,
Fabio Estevam
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm
On Friday 31 January 2014 10:19 AM, Felipe Balbi wrote: > Hi, > > On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote: >> The Keystone PM management layer has been implemented using PM bus for >> power management clocks. As result, most of Keystone drivers don't need >> to manage clocks directly. They just need to enable runtime PM and use it >> to handle their PM state and clocks. >> >> Hence, remove clock management code and switch to use runtime PM. >> >> Signed-off-by: Grygorii Strashko > > quite a few weeks back I sent a series enabling runtime pm for all glue > layers. I'll use that version instead, sorry. > That should be fine but you need to drop clk_*() related code from that patch. I assume you will send refresh version of it then. Regards, Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm
On Fri, Jan 31, 2014 at 10:43:21AM -0500, Santosh Shilimkar wrote: > On Friday 31 January 2014 10:19 AM, Felipe Balbi wrote: > > Hi, > > > > On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote: > >> The Keystone PM management layer has been implemented using PM bus for > >> power management clocks. As result, most of Keystone drivers don't need > >> to manage clocks directly. They just need to enable runtime PM and use it > >> to handle their PM state and clocks. > >> > >> Hence, remove clock management code and switch to use runtime PM. > >> > >> Signed-off-by: Grygorii Strashko > > > > quite a few weeks back I sent a series enabling runtime pm for all glue > > layers. I'll use that version instead, sorry. > > > That should be fine but you need to drop clk_*() related code > from that patch. I assume you will send refresh version of it then. why ? it makes no difference if you enable twice and disable twice. -- balbi signature.asc Description: Digital signature
Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm
On Friday 31 January 2014 10:47 AM, Felipe Balbi wrote: > On Fri, Jan 31, 2014 at 10:43:21AM -0500, Santosh Shilimkar wrote: >> On Friday 31 January 2014 10:19 AM, Felipe Balbi wrote: >>> Hi, >>> >>> On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote: The Keystone PM management layer has been implemented using PM bus for power management clocks. As result, most of Keystone drivers don't need to manage clocks directly. They just need to enable runtime PM and use it to handle their PM state and clocks. Hence, remove clock management code and switch to use runtime PM. Signed-off-by: Grygorii Strashko >>> >>> quite a few weeks back I sent a series enabling runtime pm for all glue >>> layers. I'll use that version instead, sorry. >>> >> That should be fine but you need to drop clk_*() related code >> from that patch. I assume you will send refresh version of it then. > > why ? it makes no difference if you enable twice and disable twice. > Sure but why do you want to have the clock node handling code in drivers if it is not needed. Isn't that 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
ASMedia xhci controller errors
In order to get an ASMedia xhci controller to even detect a device
being inserted I had to split the writeq() and readq().
The following patch is sufficient on amd64.
I've not yet tried to make the ax88179 card work.
David
[PATCH] xhci: Do not use 64bit accesses to the controller registers
Some controllers (eg ASMedia) do not support 64 bit accesses to
their internal registers even though they are 64bit capable.
32bit transfers in the correct order.
Signed-off-by: David Laight
---
drivers/usb/host/xhci.h | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f8416639..ee893e6 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1614,6 +1616,44 @@ static inline struct usb_hcd *xhci_to_hcd(struct
xhci_hcd *xhci)
#define xhci_warn_ratelimited(xhci, fmt, args...) \
dev_warn_ratelimited(xhci_to_hcd(xhci)->self.controller , fmt , ## args)
+/*
+ * Registers should always be accessed with double word or quad word accesses.
+ *
+ * Some xHCI implementations may support 64-bit address pointers. Registers
+ * with 64-bit address pointers should be written to with dword accesses by
+ * writing the low dword first (ptr[0]), then the high dword (ptr[1]) second.
+ * xHCI implementations that do not support 64-bit address pointers will ignore
+ * the high dword, and write order is irrelevant.
+ *
+ * Some 64bit capable controllers (eg ASMedia) do not support 64bit accesses
+ * to their own registers.
+ */
+static inline void xhci_write_64(const u64 val, __le64 __iomem *regs)
+{
+ __u32 __iomem *ptr = (__u32 __iomem *) regs;
+ u32 val_lo = lower_32_bits(val);
+ u32 val_hi = upper_32_bits(val);
+
+ writel(val_lo, ptr);
+ writel(val_hi, ptr + 1);
+}
+#define writeq writeq
+#undef writeq
+#define writeq(v, p) xhci_write_64(v, p)
+
+static inline u64 xhci_read_64(__le64 __iomem *regs)
+{
+ __u32 __iomem *ptr = (__u32 __iomem *) regs;
+ u32 val_lo = readl(ptr);
+ u32 val_hi = readl(ptr + 1);
+
+ return (u64)val_hi << 32 | val_lo;
+}
+#define readq readq
+#undef readq
+#define readq(p) xhci_read_64(p)
+
+
static inline int xhci_link_trb_quirk(struct xhci_hcd *xhci)
{
return xhci->quirks & XHCI_LINK_TRB_QUIRK;
--
1.8.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: dwc3: keystone: switch to use runtime pm
On Fri, Jan 31, 2014 at 10:50:40AM -0500, Santosh Shilimkar wrote:
> On Friday 31 January 2014 10:47 AM, Felipe Balbi wrote:
> > On Fri, Jan 31, 2014 at 10:43:21AM -0500, Santosh Shilimkar wrote:
> >> On Friday 31 January 2014 10:19 AM, Felipe Balbi wrote:
> >>> Hi,
> >>>
> >>> On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote:
> The Keystone PM management layer has been implemented using PM bus for
> power management clocks. As result, most of Keystone drivers don't need
> to manage clocks directly. They just need to enable runtime PM and use it
> to handle their PM state and clocks.
>
> Hence, remove clock management code and switch to use runtime PM.
>
> Signed-off-by: Grygorii Strashko
> >>>
> >>> quite a few weeks back I sent a series enabling runtime pm for all glue
> >>> layers. I'll use that version instead, sorry.
> >>>
> >> That should be fine but you need to drop clk_*() related code
> >> from that patch. I assume you will send refresh version of it then.
> >
> > why ? it makes no difference if you enable twice and disable twice.
> >
> Sure but why do you want to have the clock node handling code in drivers
> if it is not needed. Isn't that better ?
It might, but then way that I wanted to see it is so that I can make
assumptions about the device state. From a driver perspective, what I
would love to see is the ability to assume that when my probe gets
called the device is already enabled. So if you can make sure that
clk_enable() happens before my probe and that you call
pm_runtime_set_active() before my probe too, then I can more than
hapilly remove clk_* calls from the driver ;-)
either that or maintain the driver like so:
probe()
{
...
clk_get(dev, "fck");
clk_prepare(clk);
clk_enable(clk);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
pm_runtime_get_sync(dev);
...
}
runtime_suspend()
{
clk_disable(dev);
}
runtime_resume()
{
clk_enable(dev);
}
note that because of pm_runtime_set_active() that first
pm_runtime_get_sync() in probe() will simply increase the reference
counter without calling my ->runtime_resume() callback, which is exactly
what we want, as that would completely avoid situations of bad context
being restored because of that initial pm_runtime_get_sync().
Then, we can even make pm_runtime completely async easily, because
clk_prepare() was called only on probe() (or before it, for that
matter).
Bottomline is, if you can guarantee me that clk_get(), clk_prepare(),
clk_enable() and pm_runtime_set_active() will be called properly before
my probe, i'll be more than happy to comply with your request above as
that will greatly simplify my driver.
Just make, also, that if this clock is shared between dwc3-keystone
wrapper and dwc3 core, you clk_get() on both driver's probe.
cheers
--
balbi
signature.asc
Description: Digital signature
RE: [RFT] usb: Disable Link PM if setting device-initiated timeout fails.
From: Sarah Sharp > Yoma, can you apply this patch and see if it helps your issue? Helped a lot on my amd system with the ASMedia controller. David ... > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index 64ea21971be2..74a69b887346 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -3651,7 +3651,8 @@ static void usb_enable_link_state(struct usb_hcd > > *hcd, struct usb_device > *udev, > > > > /* Only a configured device will accept the Set Feature U1/U2_ENABLE */ > > else if (udev->actconfig) > > - usb_set_device_initiated_lpm(udev, state, true); > > + if (usb_set_device_initiated_lpm(udev, state, true)) > > + hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state); > > > > } -- 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] usb: Disable Link PM if setting device-initiated timeout fails.
From: David Laight > From: Sarah Sharp > > Yoma, can you apply this patch and see if it helps your issue? > > Helped a lot on my amd system with the ASMedia controller. I lied :-) Sometimes it is detected at 480MB - in which case it comes up immediately. Other times it is detected at 5000MB and I see a lot of errors before it is finally 'ready'. I'm not going to have time to do any further investigation until after the weekend. David -- 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] Move DWC2 driver out of staging
Hi,
On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
> The DWC2 driver should now be in good enough shape to move out of
> staging. I have stress tested it overnight on RPI running mass
> storage and Ethernet transfers in parallel, and for several days
> on our proprietary PCI-based platform.
>
> Signed-off-by: Paul Zimmerman
> ---
> v4: Also change directory path in MAINTAINERS
>
> Greg,
> I believe I have addressed all of the feedback since I last
> submitted this. Is there still time to do this before you
> close your trees?
>
> MAINTAINERS | 2 +-
> drivers/staging/Kconfig | 2 --
> drivers/staging/Makefile | 1 -
> drivers/staging/dwc2/TODO | 33
> ---
> drivers/usb/Kconfig | 2 ++
> drivers/usb/Makefile | 1 +
> drivers/{staging => usb}/dwc2/Kconfig | 0
> drivers/{staging => usb}/dwc2/Makefile| 0
> drivers/{staging => usb}/dwc2/core.c | 0
> drivers/{staging => usb}/dwc2/core.h | 0
> drivers/{staging => usb}/dwc2/core_intr.c | 0
> drivers/{staging => usb}/dwc2/hcd.c | 0
> drivers/{staging => usb}/dwc2/hcd.h | 0
> drivers/{staging => usb}/dwc2/hcd_ddma.c | 0
> drivers/{staging => usb}/dwc2/hcd_intr.c | 0
> drivers/{staging => usb}/dwc2/hcd_queue.c | 0
> drivers/{staging => usb}/dwc2/hw.h| 0
> drivers/{staging => usb}/dwc2/pci.c | 0
> drivers/{staging => usb}/dwc2/platform.c | 0
> 19 files changed, 4 insertions(+), 37 deletions(-)
> delete mode 100644 drivers/staging/dwc2/TODO
> rename drivers/{staging => usb}/dwc2/Kconfig (100%)
> rename drivers/{staging => usb}/dwc2/Makefile (100%)
> rename drivers/{staging => usb}/dwc2/core.c (100%)
> rename drivers/{staging => usb}/dwc2/core.h (100%)
> rename drivers/{staging => usb}/dwc2/core_intr.c (100%)
> rename drivers/{staging => usb}/dwc2/hcd.c (100%)
> rename drivers/{staging => usb}/dwc2/hcd.h (100%)
> rename drivers/{staging => usb}/dwc2/hcd_ddma.c (100%)
> rename drivers/{staging => usb}/dwc2/hcd_intr.c (100%)
> rename drivers/{staging => usb}/dwc2/hcd_queue.c (100%)
> rename drivers/{staging => usb}/dwc2/hw.h (100%)
> rename drivers/{staging => usb}/dwc2/pci.c (100%)
> rename drivers/{staging => usb}/dwc2/platform.c (100%)
this looks just fine, but for whatever reason it breaks sdhci on my rpi.
With today's Linus' master the dwc2 controller seems to initialize fine,
but I get this upon boot:
[1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
[1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12
That is:
struct sdhci_host *sdhci_pltfm_init(struct platform_device
*pdev,
const struct
sdhci_pltfm_data *pdata,
size_t priv_size)
{
...
iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!iomem) {
ret = -ENOMEM;
goto err;
}
...
So far it's 100% reproducible. No further infos since my root device
went away. Same behavior with bcm2835_defconfig.
Bisecting points to this commit, and if I move this driver back to
staging (revert 197ba5f406cc) usb and sdhci are both working properly.
Without the revert, this patch on top...
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index d01d0d3..eaba547 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -124,6 +124,9 @@ static int dwc2_driver_probe(struct platform_device *dev)
int retval;
int irq;
+ if (usb_disabled())
+ return -ENODEV;
+
match = of_match_device(dwc2_of_match_table, &dev->dev);
if (match && match->data) {
params = match->data;
...and "nousb" in the cmdline (with crashes without the patch), sdhci works
again. I don't see any obvious clues, any idea what's going on?
Regards,
Andre
--
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] Move DWC2 driver out of staging
Hi,
On Fri, Jan 31, 2014 at 07:12:36PM +0100, Andre Heider wrote:
> Hi,
>
> On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
> > The DWC2 driver should now be in good enough shape to move out of
> > staging. I have stress tested it overnight on RPI running mass
> > storage and Ethernet transfers in parallel, and for several days
> > on our proprietary PCI-based platform.
> >
> > Signed-off-by: Paul Zimmerman
> > ---
> > v4: Also change directory path in MAINTAINERS
> >
> > Greg,
> > I believe I have addressed all of the feedback since I last
> > submitted this. Is there still time to do this before you
> > close your trees?
> >
> > MAINTAINERS | 2 +-
> > drivers/staging/Kconfig | 2 --
> > drivers/staging/Makefile | 1 -
> > drivers/staging/dwc2/TODO | 33
> > ---
> > drivers/usb/Kconfig | 2 ++
> > drivers/usb/Makefile | 1 +
> > drivers/{staging => usb}/dwc2/Kconfig | 0
> > drivers/{staging => usb}/dwc2/Makefile| 0
> > drivers/{staging => usb}/dwc2/core.c | 0
> > drivers/{staging => usb}/dwc2/core.h | 0
> > drivers/{staging => usb}/dwc2/core_intr.c | 0
> > drivers/{staging => usb}/dwc2/hcd.c | 0
> > drivers/{staging => usb}/dwc2/hcd.h | 0
> > drivers/{staging => usb}/dwc2/hcd_ddma.c | 0
> > drivers/{staging => usb}/dwc2/hcd_intr.c | 0
> > drivers/{staging => usb}/dwc2/hcd_queue.c | 0
> > drivers/{staging => usb}/dwc2/hw.h| 0
> > drivers/{staging => usb}/dwc2/pci.c | 0
> > drivers/{staging => usb}/dwc2/platform.c | 0
> > 19 files changed, 4 insertions(+), 37 deletions(-)
> > delete mode 100644 drivers/staging/dwc2/TODO
> > rename drivers/{staging => usb}/dwc2/Kconfig (100%)
> > rename drivers/{staging => usb}/dwc2/Makefile (100%)
> > rename drivers/{staging => usb}/dwc2/core.c (100%)
> > rename drivers/{staging => usb}/dwc2/core.h (100%)
> > rename drivers/{staging => usb}/dwc2/core_intr.c (100%)
> > rename drivers/{staging => usb}/dwc2/hcd.c (100%)
> > rename drivers/{staging => usb}/dwc2/hcd.h (100%)
> > rename drivers/{staging => usb}/dwc2/hcd_ddma.c (100%)
> > rename drivers/{staging => usb}/dwc2/hcd_intr.c (100%)
> > rename drivers/{staging => usb}/dwc2/hcd_queue.c (100%)
> > rename drivers/{staging => usb}/dwc2/hw.h (100%)
> > rename drivers/{staging => usb}/dwc2/pci.c (100%)
> > rename drivers/{staging => usb}/dwc2/platform.c (100%)
>
> this looks just fine, but for whatever reason it breaks sdhci on my rpi.
> With today's Linus' master the dwc2 controller seems to initialize fine,
> but I get this upon boot:
>
> [1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
> [1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12
>
> That is:
>
> struct sdhci_host *sdhci_pltfm_init(struct platform_device
> *pdev,
> const struct
> sdhci_pltfm_data *pdata,
> size_t priv_size)
> {
> ...
>
> iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!iomem) {
> ret = -ENOMEM;
> goto err;
> }
>
> ...
>
> So far it's 100% reproducible. No further infos since my root device
> went away. Same behavior with bcm2835_defconfig.
>
> Bisecting points to this commit, and if I move this driver back to
> staging (revert 197ba5f406cc) usb and sdhci are both working properly.
>
> Without the revert, this patch on top...
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index d01d0d3..eaba547 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -124,6 +124,9 @@ static int dwc2_driver_probe(struct platform_device *dev)
> int retval;
> int irq;
>
> + if (usb_disabled())
> + return -ENODEV;
> +
> match = of_match_device(dwc2_of_match_table, &dev->dev);
> if (match && match->data) {
> params = match->data;
>
> ...and "nousb" in the cmdline (with crashes without the patch), sdhci works
> again. I don't see any obvious clues, any idea what's going on?
wait, what ? How can a driver rename cause sdhci to die ? Your error is
because you don't have that resource as part of your platform_device.
-ECONFUSED. Have you really bisected it down to Paul's patch ?
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v4] Move DWC2 driver out of staging
On Fri, Jan 31, 2014 at 12:15:26PM -0600, Felipe Balbi wrote:
> Hi,
>
> On Fri, Jan 31, 2014 at 07:12:36PM +0100, Andre Heider wrote:
> > Hi,
> >
> > On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
> > > The DWC2 driver should now be in good enough shape to move out of
> > > staging. I have stress tested it overnight on RPI running mass
> > > storage and Ethernet transfers in parallel, and for several days
> > > on our proprietary PCI-based platform.
> > >
> > > Signed-off-by: Paul Zimmerman
> > > ---
> > > v4: Also change directory path in MAINTAINERS
> > >
> > > Greg,
> > > I believe I have addressed all of the feedback since I last
> > > submitted this. Is there still time to do this before you
> > > close your trees?
> > >
> > > MAINTAINERS | 2 +-
> > > drivers/staging/Kconfig | 2 --
> > > drivers/staging/Makefile | 1 -
> > > drivers/staging/dwc2/TODO | 33
> > > ---
> > > drivers/usb/Kconfig | 2 ++
> > > drivers/usb/Makefile | 1 +
> > > drivers/{staging => usb}/dwc2/Kconfig | 0
> > > drivers/{staging => usb}/dwc2/Makefile| 0
> > > drivers/{staging => usb}/dwc2/core.c | 0
> > > drivers/{staging => usb}/dwc2/core.h | 0
> > > drivers/{staging => usb}/dwc2/core_intr.c | 0
> > > drivers/{staging => usb}/dwc2/hcd.c | 0
> > > drivers/{staging => usb}/dwc2/hcd.h | 0
> > > drivers/{staging => usb}/dwc2/hcd_ddma.c | 0
> > > drivers/{staging => usb}/dwc2/hcd_intr.c | 0
> > > drivers/{staging => usb}/dwc2/hcd_queue.c | 0
> > > drivers/{staging => usb}/dwc2/hw.h| 0
> > > drivers/{staging => usb}/dwc2/pci.c | 0
> > > drivers/{staging => usb}/dwc2/platform.c | 0
> > > 19 files changed, 4 insertions(+), 37 deletions(-)
> > > delete mode 100644 drivers/staging/dwc2/TODO
> > > rename drivers/{staging => usb}/dwc2/Kconfig (100%)
> > > rename drivers/{staging => usb}/dwc2/Makefile (100%)
> > > rename drivers/{staging => usb}/dwc2/core.c (100%)
> > > rename drivers/{staging => usb}/dwc2/core.h (100%)
> > > rename drivers/{staging => usb}/dwc2/core_intr.c (100%)
> > > rename drivers/{staging => usb}/dwc2/hcd.c (100%)
> > > rename drivers/{staging => usb}/dwc2/hcd.h (100%)
> > > rename drivers/{staging => usb}/dwc2/hcd_ddma.c (100%)
> > > rename drivers/{staging => usb}/dwc2/hcd_intr.c (100%)
> > > rename drivers/{staging => usb}/dwc2/hcd_queue.c (100%)
> > > rename drivers/{staging => usb}/dwc2/hw.h (100%)
> > > rename drivers/{staging => usb}/dwc2/pci.c (100%)
> > > rename drivers/{staging => usb}/dwc2/platform.c (100%)
> >
> > this looks just fine, but for whatever reason it breaks sdhci on my rpi.
> > With today's Linus' master the dwc2 controller seems to initialize fine,
> > but I get this upon boot:
> >
> > [1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
> > [1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12
> >
> > That is:
> >
> > struct sdhci_host *sdhci_pltfm_init(struct platform_device
> > *pdev,
> > const struct
> > sdhci_pltfm_data *pdata,
> > size_t priv_size)
> > {
> > ...
> >
> > iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!iomem) {
> > ret = -ENOMEM;
> > goto err;
> > }
> >
> > ...
> >
> > So far it's 100% reproducible. No further infos since my root device
> > went away. Same behavior with bcm2835_defconfig.
> >
> > Bisecting points to this commit, and if I move this driver back to
> > staging (revert 197ba5f406cc) usb and sdhci are both working properly.
> >
> > Without the revert, this patch on top...
> >
> > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> > index d01d0d3..eaba547 100644
> > --- a/drivers/usb/dwc2/platform.c
> > +++ b/drivers/usb/dwc2/platform.c
> > @@ -124,6 +124,9 @@ static int dwc2_driver_probe(struct platform_device
> > *dev)
> > int retval;
> > int irq;
> >
> > + if (usb_disabled())
> > + return -ENODEV;
> > +
> > match = of_match_device(dwc2_of_match_table, &dev->dev);
> > if (match && match->data) {
> > params = match->data;
> >
> > ...and "nousb" in the cmdline (with crashes without the patch), sdhci works
> > again. I don't see any obvious clues, any idea what's going on?
>
> wait, what ? How can a driver rename cause sdhci to die ? Your error is
> because you don't have that resource as part of your platform_device.
>
> -ECONFUSED. Have you really bisected it down to Paul's patch ?
Yeah, I did. Since that confused me too I tried a clean build with the
defconfig and even another
Re: Help testing for USB ethernet/xHCI regression
On Thu, Jan 30, 2014 at 09:32:49PM -0500, Mark Lord wrote: > On 14-01-30 06:26 PM, Sarah Sharp wrote: > > On Thu, Jan 30, 2014 at 05:20:40PM -0500, Mark Lord wrote: > >> On 14-01-30 04:41 PM, Sarah Sharp wrote: > >>> > >>> Mark and David, can you pull the 3.13-td-changes-reverted branch again, > >>> and see if the latest patch fixes your issue? It disables scatter > >>> gather for the ax88179_178a device, but only when it's operating at USB > >>> 3.0 speeds. > >> > >> As expected, this works just fine. > > > > Did it work when plugged into a USB 2.0 hub? > > Curiosity, NO. Dies almost immediately when run at USB 2.0 Hi-Speed. > With a USB 2.0 hub, with a USB 2.0 port on a USB 3.0 hub, > and with a USB 2.0 extension cable in place of a hub. > > Near instant hangs. > > Plugged directly to the USB 3.0 port, it works fine. Ok, that makes sense. The patch I wrote only limited scatter-gather at USB 3.0 speeds, to see if scatter-gather could work at USB 2.0 speeds. Reverting commit 3804fad45411b48233b48003e33a78f290d227c8 "USBNET: ax88179_178a: enable tso if usb host supports sg dma" is the right way to go instead. Sarah Sharp -- 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 1/1] usb: Tell xhci when usb data might be misaligned
On Fri, Jan 31, 2014 at 08:17:58AM +0800, Ming Lei wrote: > On Fri, Jan 31, 2014 at 6:15 AM, Sarah Sharp > wrote: > > On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: > >> FWIW, the plan looks fine to me. Just adding a couple of hints to > >> simplify the implementation. > >> > >> Sarah Sharp writes: > >> > >> > Let's do this fix the right way, instead of wall papering over the > >> > issue. Here's what we should do: > >> > > >> > 1. Disable scatter-gather for the ax88179_178a driver when it's under an > >> >xHCI host. > >> > >> No need to make this conditional. SG is only enabled in the > >> ax88179_178a driver if udev->bus->no_sg_constraint is true, so it > >> applies only to xHCI hosts in the first place. > > > > Ah, so you're suggesting just reverting commit > > 3804fad45411b48233b48003e33a78f290d227c8 "USBNET: ax88179_178a: enable > > tso if usb host supports sg dma"? > > If I understand the problem correctly, the current issue is that xhci driver > doesn't support the arbitrary dma length not well, but per XHCI spec, it > should be supported, right? > > If the above is correct, reverting the commit isn't correct since there isn't > any issue about the commit, so I suggest to disable the flag in xhci > for the buggy devices, and it may be enabled again if the problem is fixed. Ok, I like that plan, since it means I don't have to touch any networking code to fix this. :) I believe that means we'll have to disable the flag for all 1.0 xHCI hosts, since those are the ones that need TD fragments. > >> > 2. Revert the following commits: > >> >f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block > >> > writes. > >> >d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many > >> > trbs > >> >35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload > >> > burst > >> > > >> > 3. Dan and Mathias can work together to come up with an overall plan to > >> >change the xHCI driver architecture to be fully compliant with the TD > >> >fragment rules. That can be done over the next few kernel releases. > >> > > >> > The end result is that we don't destabilize storage or break userspace > >> > USB drivers, we don't break people's xHCI host controllers, > >> > the ax88179_178a USB ethernet devices still work under xHCI (a bit with > >> > worse performance), and other USB ethernet devices still get the > >> > performance improvement introduced in 3.12. > >> > >> No other usbnet drivers has enabled SG... Which is why you have only > >> seen this problem with the ax88179_178a devices. So there is no > >> performance improvement to keep. > > In my test environment, the patch does improve both throughput and > cpu utilization, if you search the previous email for the patch, you can > see the data. Right, I did see the performance improvement note in that commit. Do you know if the ARM A15 dual core board was using a 0.96 xHCI host, or a 1.0 host? You can find out by reloading the xHCI driver with dynamic debugging turned on: # sudo modprobe xhci_hcd dyndbg and then look for lines like: [25296.765767] xhci_hcd :00:14.0: HCIVERSION: 0x100 Sarah Sharp -- 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] Move DWC2 driver out of staging
On Fri, Jan 31, 2014 at 12:15:26PM -0600, Felipe Balbi wrote:
> Hi,
>
> On Fri, Jan 31, 2014 at 07:12:36PM +0100, Andre Heider wrote:
> > Hi,
> >
> > On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
> > > The DWC2 driver should now be in good enough shape to move out of
> > > staging. I have stress tested it overnight on RPI running mass
> > > storage and Ethernet transfers in parallel, and for several days
> > > on our proprietary PCI-based platform.
> > >
> > > Signed-off-by: Paul Zimmerman
> > > ---
> > > v4: Also change directory path in MAINTAINERS
> > >
> > > Greg,
> > > I believe I have addressed all of the feedback since I last
> > > submitted this. Is there still time to do this before you
> > > close your trees?
> > >
> > > MAINTAINERS | 2 +-
> > > drivers/staging/Kconfig | 2 --
> > > drivers/staging/Makefile | 1 -
> > > drivers/staging/dwc2/TODO | 33
> > > ---
> > > drivers/usb/Kconfig | 2 ++
> > > drivers/usb/Makefile | 1 +
> > > drivers/{staging => usb}/dwc2/Kconfig | 0
> > > drivers/{staging => usb}/dwc2/Makefile| 0
> > > drivers/{staging => usb}/dwc2/core.c | 0
> > > drivers/{staging => usb}/dwc2/core.h | 0
> > > drivers/{staging => usb}/dwc2/core_intr.c | 0
> > > drivers/{staging => usb}/dwc2/hcd.c | 0
> > > drivers/{staging => usb}/dwc2/hcd.h | 0
> > > drivers/{staging => usb}/dwc2/hcd_ddma.c | 0
> > > drivers/{staging => usb}/dwc2/hcd_intr.c | 0
> > > drivers/{staging => usb}/dwc2/hcd_queue.c | 0
> > > drivers/{staging => usb}/dwc2/hw.h| 0
> > > drivers/{staging => usb}/dwc2/pci.c | 0
> > > drivers/{staging => usb}/dwc2/platform.c | 0
> > > 19 files changed, 4 insertions(+), 37 deletions(-)
> > > delete mode 100644 drivers/staging/dwc2/TODO
> > > rename drivers/{staging => usb}/dwc2/Kconfig (100%)
> > > rename drivers/{staging => usb}/dwc2/Makefile (100%)
> > > rename drivers/{staging => usb}/dwc2/core.c (100%)
> > > rename drivers/{staging => usb}/dwc2/core.h (100%)
> > > rename drivers/{staging => usb}/dwc2/core_intr.c (100%)
> > > rename drivers/{staging => usb}/dwc2/hcd.c (100%)
> > > rename drivers/{staging => usb}/dwc2/hcd.h (100%)
> > > rename drivers/{staging => usb}/dwc2/hcd_ddma.c (100%)
> > > rename drivers/{staging => usb}/dwc2/hcd_intr.c (100%)
> > > rename drivers/{staging => usb}/dwc2/hcd_queue.c (100%)
> > > rename drivers/{staging => usb}/dwc2/hw.h (100%)
> > > rename drivers/{staging => usb}/dwc2/pci.c (100%)
> > > rename drivers/{staging => usb}/dwc2/platform.c (100%)
> >
> > this looks just fine, but for whatever reason it breaks sdhci on my rpi.
> > With today's Linus' master the dwc2 controller seems to initialize fine,
> > but I get this upon boot:
> >
> > [1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
> > [1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12
> >
> > That is:
> >
> > struct sdhci_host *sdhci_pltfm_init(struct platform_device
> > *pdev,
> > const struct
> > sdhci_pltfm_data *pdata,
> > size_t priv_size)
> > {
> > ...
> >
> > iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!iomem) {
> > ret = -ENOMEM;
> > goto err;
> > }
> >
> > ...
> >
> > So far it's 100% reproducible. No further infos since my root device
> > went away. Same behavior with bcm2835_defconfig.
> >
> > Bisecting points to this commit, and if I move this driver back to
> > staging (revert 197ba5f406cc) usb and sdhci are both working properly.
> >
> > Without the revert, this patch on top...
> >
> > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> > index d01d0d3..eaba547 100644
> > --- a/drivers/usb/dwc2/platform.c
> > +++ b/drivers/usb/dwc2/platform.c
> > @@ -124,6 +124,9 @@ static int dwc2_driver_probe(struct platform_device
> > *dev)
> > int retval;
> > int irq;
> >
> > + if (usb_disabled())
> > + return -ENODEV;
> > +
> > match = of_match_device(dwc2_of_match_table, &dev->dev);
> > if (match && match->data) {
> > params = match->data;
> >
> > ...and "nousb" in the cmdline (with crashes without the patch), sdhci works
> > again. I don't see any obvious clues, any idea what's going on?
>
> wait, what ? How can a driver rename cause sdhci to die ? Your error is
> because you don't have that resource as part of your platform_device.
>
> -ECONFUSED. Have you really bisected it down to Paul's patch ?
This silly move doesn't trigger the sdhci ENOMEM:
diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/a
Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm
On Friday 31 January 2014 11:45 AM, Felipe Balbi wrote:
> On Fri, Jan 31, 2014 at 10:50:40AM -0500, Santosh Shilimkar wrote:
>> On Friday 31 January 2014 10:47 AM, Felipe Balbi wrote:
>>> On Fri, Jan 31, 2014 at 10:43:21AM -0500, Santosh Shilimkar wrote:
On Friday 31 January 2014 10:19 AM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote:
>> The Keystone PM management layer has been implemented using PM bus for
>> power management clocks. As result, most of Keystone drivers don't need
>> to manage clocks directly. They just need to enable runtime PM and use it
>> to handle their PM state and clocks.
>>
>> Hence, remove clock management code and switch to use runtime PM.
>>
>> Signed-off-by: Grygorii Strashko
>
> quite a few weeks back I sent a series enabling runtime pm for all glue
> layers. I'll use that version instead, sorry.
>
That should be fine but you need to drop clk_*() related code
from that patch. I assume you will send refresh version of it then.
>>>
>>> why ? it makes no difference if you enable twice and disable twice.
>>>
>> Sure but why do you want to have the clock node handling code in drivers
>> if it is not needed. Isn't that better ?
>
> It might, but then way that I wanted to see it is so that I can make
> assumptions about the device state. From a driver perspective, what I
> would love to see is the ability to assume that when my probe gets
> called the device is already enabled. So if you can make sure that
> clk_enable() happens before my probe and that you call
> pm_runtime_set_active() before my probe too, then I can more than
> hapilly remove clk_* calls from the driver ;-)
>
> either that or maintain the driver like so:
>
> probe()
> {
> ...
>
> clk_get(dev, "fck");
> clk_prepare(clk);
> clk_enable(clk);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
> ...
> }
>
> runtime_suspend()
> {
> clk_disable(dev);
> }
>
> runtime_resume()
> {
> clk_enable(dev);
> }
>
> note that because of pm_runtime_set_active() that first
> pm_runtime_get_sync() in probe() will simply increase the reference
> counter without calling my ->runtime_resume() callback, which is exactly
> what we want, as that would completely avoid situations of bad context
> being restored because of that initial pm_runtime_get_sync().
>
Thanks for making your point bit clear.
> Then, we can even make pm_runtime completely async easily, because
> clk_prepare() was called only on probe() (or before it, for that
> matter).
>
> Bottomline is, if you can guarantee me that clk_get(), clk_prepare(),
> clk_enable() and pm_runtime_set_active() will be called properly before
> my probe, i'll be more than happy to comply with your request above as
> that will greatly simplify my driver.
>
Which is the case at least I see on Keystone. And hence the patch from
Grygorii works. I also noticed your proposal for wider platform to
enforce above behavior which seems to be a good idea.
> Just make, also, that if this clock is shared between dwc3-keystone
> wrapper and dwc3 core, you clk_get() on both driver's probe.
>
I understand. In summary, whichever patch you pick(yours) or Grygorii's,
its completely safe to remove the clock handling from Keystone USB driver.
Regards,
Santosh
--
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: ASMedia xhci controller errors
On Fri, Jan 31, 2014 at 04:37:47PM +, David Laight wrote:
> In order to get an ASMedia xhci controller to even detect a device
> being inserted I had to split the writeq() and readq().
> The following patch is sufficient on amd64.
I've reverted the commits that introduced the readq and writeq changes,
no need for this patch.
Sarah Sharp
> I've not yet tried to make the ax88179 card work.
>
> David
>
> [PATCH] xhci: Do not use 64bit accesses to the controller registers
>
> Some controllers (eg ASMedia) do not support 64 bit accesses to
> their internal registers even though they are 64bit capable.
>
> 32bit transfers in the correct order.
>
> Signed-off-by: David Laight
> ---
> drivers/usb/host/xhci.h | 40
> 1 file changed, 40 insertions(+)
>
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index f8416639..ee893e6 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1614,6 +1616,44 @@ static inline struct usb_hcd *xhci_to_hcd(struct
> xhci_hcd *xhci)
> #define xhci_warn_ratelimited(xhci, fmt, args...) \
> dev_warn_ratelimited(xhci_to_hcd(xhci)->self.controller , fmt , ## args)
>
> +/*
> + * Registers should always be accessed with double word or quad word
> accesses.
> + *
> + * Some xHCI implementations may support 64-bit address pointers. Registers
> + * with 64-bit address pointers should be written to with dword accesses by
> + * writing the low dword first (ptr[0]), then the high dword (ptr[1]) second.
> + * xHCI implementations that do not support 64-bit address pointers will
> ignore
> + * the high dword, and write order is irrelevant.
> + *
> + * Some 64bit capable controllers (eg ASMedia) do not support 64bit accesses
> + * to their own registers.
> + */
> +static inline void xhci_write_64(const u64 val, __le64 __iomem *regs)
> +{
> + __u32 __iomem *ptr = (__u32 __iomem *) regs;
> + u32 val_lo = lower_32_bits(val);
> + u32 val_hi = upper_32_bits(val);
> +
> + writel(val_lo, ptr);
> + writel(val_hi, ptr + 1);
> +}
> +#define writeq writeq
> +#undef writeq
> +#define writeq(v, p) xhci_write_64(v, p)
> +
> +static inline u64 xhci_read_64(__le64 __iomem *regs)
> +{
> + __u32 __iomem *ptr = (__u32 __iomem *) regs;
> + u32 val_lo = readl(ptr);
> + u32 val_hi = readl(ptr + 1);
> +
> + return (u64)val_hi << 32 | val_lo;
> +}
> +#define readq readq
> +#undef readq
> +#define readq(p) xhci_read_64(p)
> +
> +
> static inline int xhci_link_trb_quirk(struct xhci_hcd *xhci)
> {
> return xhci->quirks & XHCI_LINK_TRB_QUIRK;
> --
> 1.8.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 v4] Move DWC2 driver out of staging
On 01/31/2014 12:37 PM, Paul Zimmerman wrote:
>> From: Andre Heider [mailto:a.hei...@gmail.com]
>> Sent: Friday, January 31, 2014 11:04 AM
>> On Fri, Jan 31, 2014 at 12:15:26PM -0600, Felipe Balbi wrote:
>>> On Fri, Jan 31, 2014 at 07:12:36PM +0100, Andre Heider wrote:
On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
> The DWC2 driver should now be in good enough shape to move out of
> staging. I have stress tested it overnight on RPI running mass
> storage and Ethernet transfers in parallel, and for several days
> on our proprietary PCI-based platform.
...
this looks just fine, but for whatever reason it breaks sdhci on my rpi.
With today's Linus' master the dwc2 controller seems to initialize fine,
but I get this upon boot:
[1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
[1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12
...
>> This silly move doesn't trigger the sdhci ENOMEM:
>>
>> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
>> index b021c96..f739b80 100644
>> --- a/arch/arm/boot/dts/bcm2835.dtsi
>> +++ b/arch/arm/boot/dts/bcm2835.dtsi
>> @@ -100,6 +100,12 @@
>> status = "disabled";
>> };
>>
>> +usb {
>> +compatible = "brcm,bcm2835-usb";
>> +reg = <0x7e98 0x1>;
>> +interrupts = <1 9>;
>> +};
>> +
>> sdhci: sdhci {
>> compatible = "brcm,bcm2835-sdhci";
>> reg = <0x7e30 0x100>;
>> @@ -107,12 +113,6 @@
>> clocks = <&clk_mmc>;
>> status = "disabled";
>> };
>> -
>> -usb {
>> -compatible = "brcm,bcm2835-usb";
>> -reg = <0x7e98 0x1>;
>> -interrupts = <1 9>;
>> -};
>> };
>>
>> clocks {
>>
>> Maybe there's some kind of race, or something even messing at the .dtb
>> at runtime?
>
> Hi Greg, Steve,
>
> Would moving a USB driver from drivers/staging/ to drivers/usb/ perhaps
> cause the initialization order to change?
Yes, certainly.
> If so, maybe that has exposed
> some pre-existing interference between sdhci and dwc2?
>
> Could moving the resource in the .dts file as Andre has done also cause
> the initialization order to change?
Yes, certainly.
I'll try to look into this, this weekend.
--
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] Move DWC2 driver out of staging
> From: Andre Heider [mailto:a.hei...@gmail.com]
> Sent: Friday, January 31, 2014 11:04 AM
>
> On Fri, Jan 31, 2014 at 12:15:26PM -0600, Felipe Balbi wrote:
> > Hi,
> >
> > On Fri, Jan 31, 2014 at 07:12:36PM +0100, Andre Heider wrote:
> > > Hi,
> > >
> > > On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
> > > > The DWC2 driver should now be in good enough shape to move out of
> > > > staging. I have stress tested it overnight on RPI running mass
> > > > storage and Ethernet transfers in parallel, and for several days
> > > > on our proprietary PCI-based platform.
> > > >
> > > > Signed-off-by: Paul Zimmerman
> > > > ---
> > > > v4: Also change directory path in MAINTAINERS
> > >
> > > this looks just fine, but for whatever reason it breaks sdhci on my rpi.
> > > With today's Linus' master the dwc2 controller seems to initialize fine,
> > > but I get this upon boot:
> > >
> > > [1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
> > > [1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error
> > > -12
> > >
> > > That is:
> > >
> > > struct sdhci_host *sdhci_pltfm_init(struct platform_device
> > > *pdev,
> > > const struct
> > > sdhci_pltfm_data *pdata,
> > > size_t priv_size)
> > > {
> > > ...
> > >
> > > iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > if (!iomem) {
> > > ret = -ENOMEM;
> > > goto err;
> > > }
> > >
> > > ...
> > >
> > > So far it's 100% reproducible. No further infos since my root device
> > > went away. Same behavior with bcm2835_defconfig.
> > >
> > > Bisecting points to this commit, and if I move this driver back to
> > > staging (revert 197ba5f406cc) usb and sdhci are both working properly.
> > >
> > > Without the revert, this patch on top...
> > >
> > > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> > > index d01d0d3..eaba547 100644
> > > --- a/drivers/usb/dwc2/platform.c
> > > +++ b/drivers/usb/dwc2/platform.c
> > > @@ -124,6 +124,9 @@ static int dwc2_driver_probe(struct platform_device
> > > *dev)
> > > int retval;
> > > int irq;
> > >
> > > + if (usb_disabled())
> > > + return -ENODEV;
> > > +
> > > match = of_match_device(dwc2_of_match_table, &dev->dev);
> > > if (match && match->data) {
> > > params = match->data;
> > >
> > > ...and "nousb" in the cmdline (with crashes without the patch), sdhci
> > > works
> > > again. I don't see any obvious clues, any idea what's going on?
> >
> > wait, what ? How can a driver rename cause sdhci to die ? Your error is
> > because you don't have that resource as part of your platform_device.
> >
> > -ECONFUSED. Have you really bisected it down to Paul's patch ?
>
> This silly move doesn't trigger the sdhci ENOMEM:
>
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> index b021c96..f739b80 100644
> --- a/arch/arm/boot/dts/bcm2835.dtsi
> +++ b/arch/arm/boot/dts/bcm2835.dtsi
> @@ -100,6 +100,12 @@
> status = "disabled";
> };
>
> + usb {
> + compatible = "brcm,bcm2835-usb";
> + reg = <0x7e98 0x1>;
> + interrupts = <1 9>;
> + };
> +
> sdhci: sdhci {
> compatible = "brcm,bcm2835-sdhci";
> reg = <0x7e30 0x100>;
> @@ -107,12 +113,6 @@
> clocks = <&clk_mmc>;
> status = "disabled";
> };
> -
> - usb {
> - compatible = "brcm,bcm2835-usb";
> - reg = <0x7e98 0x1>;
> - interrupts = <1 9>;
> - };
> };
>
> clocks {
>
> Maybe there's some kind of race, or something even messing at the .dtb
> at runtime?
Hi Greg, Steve,
Would moving a USB driver from drivers/staging/ to drivers/usb/ perhaps
cause the initialization order to change? If so, maybe that has exposed
some pre-existing interference between sdhci and dwc2?
Could moving the resource in the .dts file as Andre has done also cause
the initialization order to change?
I didn't test on the RPI platform after the driver move, only on our
proprietary PCIe-based platform. I don't have my RPI with me today, so
I won't be able to debug this until the weekend.
--
Paul
--
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: [OPW] USB subsystem questions
[Cc-ing the USB mailing list as well.] On Mon, Jan 27, 2014 at 06:13:57PM +0200, Valentina Manea wrote: > Hi Sarah, Hi Valentina, > As you probably know, I am working on a set of drivers called USB/IP. > These drivers have some issues that I'm not sure how to approach and > since you are working with the USB subsystem, I figured you might be > willing to give me some suggestions about this. > > These are the issues I'm trying to solve at this point: > > 1. This [1] is the USB/IP architecture. One of the drivers is a HCD > and it is responsible for announcing when a new device is being shared > by pretending it is physically connected to the host. This works fine > but when I "detach" the device, its entry still remains in /dev. Do you mean it still remains in /dev on the client side? > Is there any mechanism a HCD uses to annouce that a device is no > longer available? I tried browsing HCDs in the kernel but didn't see > anything quite obvious. Let me describe how it works with a physical host controller, and maybe that will help you figure out how the virtual host controller should implement it. The basic concept you need to understand is that the USB core attempts to abstract the host controller, and treat it exactly like an external USB hub. This means the USB core does things like send control transfers to the "roothub", which are then intercepted and turned into calls into the host controller driver's hub_control and hub_status_data methods. It's convoluted, but it's how the USB core works. When a USB disconnect happens on a physical host controller, the host hardware detects the disconnect, and changes its port status registers to reflect the disconnect. You'll see those registers described for USB 3.0 hubs in section 10.14.2.6 Get Port Status of the USB 3.0 bus spec, in chapter 10 of the USB 2.0 bus spec. In this case, it would set the connect status change bit, and clear the connected status bit. When the connect status change bit is set, the host hardware will send an interrupt, which the host controller driver intercepts. In the xHCI driver specifically, xhci_irq() is called, we notice there's a port status change event on the event ring, and we deal with it in handle_port_status(). You'll notice that function calls usb_hcd_poll_rh_status(). This kicks khubd, telling it to go look at the ports on our roothub. This eventually causes hub_events() in drivers/usb/core/hub.c to be called. It will call into the hub_status_data() to see if any ports changed. The host driver will set a bit saying "this port has a change bit set". hub_events() will then call into the host controller's hub_control method, and get the port status for that particular port. Based on the port status, the USB core will notice the connect status is set to disconnected, and it will mark the USB device as disconnected, which will cause the /dev entry to disappear. I don't know the details of the stub driver on the server side, so I'm not sure how this translates into the virtual host controller architecture. > 2. This question somehow resembles the previous. When a device is > shared, I don't want it to be available on the host it is physically > attached to anymore. What do you mean by this? Do you mean that when the stub driver on the server starts sharing the device to the client, you don't want it to appear on the server side anymore? > Since the stub driver (in [1]) was converted by > me to a Core driver, is there any way I could tell udev that this > device is no longer available? I'm thinking that since it wouldn't > have a /dev entry anymore, it can't be used locally, only remotely. > I'm not sure this is "the way to go", it sounds a bit like a hack. Why exactly do you want the device to not show up in /dev? I would think what you really want is for the device drivers on the server side to not bind to the device, since the client side drivers should be bound to them instead. I believe there is a mechanism for usbfs drivers to mark a port as "claimed". Perhaps the stub driver could do the same? Alan Stern would have deals on that. Sarah Sharp > Any suggestions and pointers are welcome! > > Thank you, > Valentina > > [1] http://usbip.sourceforge.net/images/usbip-design.png -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm
On Fri, 31 Jan 2014, Felipe Balbi wrote:
> probe()
> {
> ...
>
> clk_get(dev, "fck");
> clk_prepare(clk);
> clk_enable(clk);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
> ...
> }
> note that because of pm_runtime_set_active() that first
> pm_runtime_get_sync() in probe() will simply increase the reference
> counter without calling my ->runtime_resume() callback, which is exactly
> what we want, as that would completely avoid situations of bad context
> being restored because of that initial pm_runtime_get_sync().
Very minor note... A slightly better way to do the same thing is:
pm_runtime_set_active(dev);
pm_runtime_get_noresume(dev);
pm_runtime_enable(dev);
The get_noresume says that you want to increment the usage counter
without performing any callbacks, and doing it before the
pm_runtime_enable avoids any window during which a runtime suspend
might somehow occur.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ASMedia xhci controller errors
On 01/31/2014 11:23 AM, Sarah Sharp wrote: > On Fri, Jan 31, 2014 at 04:37:47PM +, David Laight wrote: >> In order to get an ASMedia xhci controller to even detect a device >> being inserted I had to split the writeq() and readq(). >> The following patch is sufficient on amd64. > > I've reverted the commits that introduced the readq and writeq changes, > no need for this patch. > > Sarah Sharp Just thought I'd let David know that I tried this patch and it works for my ASMedia 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: [OPW] USB subsystem questions
On Fri, 31 Jan 2014, Sarah Sharp wrote: > > 2. This question somehow resembles the previous. When a device is > > shared, I don't want it to be available on the host it is physically > > attached to anymore. > > What do you mean by this? Do you mean that when the stub driver on the > server starts sharing the device to the client, you don't want it to > appear on the server side anymore? It's not easy to do this properly. For example, if you went all the way and _really_ made the device disappear from the server system, then the server wouldn't be able to communicate with the device any more! Obviously this would be bad for the client... > > Since the stub driver (in [1]) was converted by > > me to a Core driver, is there any way I could tell udev that this > > device is no longer available? I'm thinking that since it wouldn't > > have a /dev entry anymore, it can't be used locally, only remotely. > > I'm not sure this is "the way to go", it sounds a bit like a hack. Even if you did this, it still wouldn't completely prevent processes on the server from accessing the device. For instance, the bConfigurationValue attribute would still be available in the device's sysfs directory on the server. Also, processes on the server could still take control of the device by using usbfs. > Why exactly do you want the device to not show up in /dev? I would > think what you really want is for the device drivers on the server side > to not bind to the device, since the client side drivers should be bound > to them instead. > > I believe there is a mechanism for usbfs drivers to mark a port as > "claimed". Perhaps the stub driver could do the same? Alan Stern would > have deals on that. That might be good enough. The stub driver could claim the port that the device is plugged into by calling usb_hub_claim_port (you would have to EXPORT this function first). This would prevent the server system from automatically binding drivers to the device's interfaces whenever the client system changed the device's configuration. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm
On Fri, Jan 31, 2014 at 04:13:19PM -0500, Alan Stern wrote:
> On Fri, 31 Jan 2014, Felipe Balbi wrote:
>
> > probe()
> > {
> > ...
> >
> > clk_get(dev, "fck");
> > clk_prepare(clk);
> > clk_enable(clk);
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> > pm_runtime_get_sync(dev);
> > ...
> > }
>
> > note that because of pm_runtime_set_active() that first
> > pm_runtime_get_sync() in probe() will simply increase the reference
> > counter without calling my ->runtime_resume() callback, which is exactly
> > what we want, as that would completely avoid situations of bad context
> > being restored because of that initial pm_runtime_get_sync().
>
> Very minor note... A slightly better way to do the same thing is:
>
> pm_runtime_set_active(dev);
> pm_runtime_get_noresume(dev);
> pm_runtime_enable(dev);
>
> The get_noresume says that you want to increment the usage counter
> without performing any callbacks, and doing it before the
> pm_runtime_enable avoids any window during which a runtime suspend
> might somehow occur.
aha, that's perfect :-) Thanks Alan.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm
Hi, On Fri, Jan 31, 2014 at 02:20:48PM -0500, Santosh Shilimkar wrote: [ snip ] > > note that because of pm_runtime_set_active() that first > > pm_runtime_get_sync() in probe() will simply increase the reference > > counter without calling my ->runtime_resume() callback, which is exactly > > what we want, as that would completely avoid situations of bad context > > being restored because of that initial pm_runtime_get_sync(). > > > Thanks for making your point bit clear. no problem. > > Then, we can even make pm_runtime completely async easily, because > > clk_prepare() was called only on probe() (or before it, for that > > matter). > > > > Bottomline is, if you can guarantee me that clk_get(), clk_prepare(), > > clk_enable() and pm_runtime_set_active() will be called properly before > > my probe, i'll be more than happy to comply with your request above as > > that will greatly simplify my driver. > > > Which is the case at least I see on Keystone. And hence the patch from I was going over pm_domain.c and drivers/base/power/clock_ops.c and none of them enable pm_runtime or make sure pm_runtime_set_active() is called. > Grygorii works. I also noticed your proposal for wider platform to > enforce above behavior which seems to be a good idea. it'll take months to stabilize though ;-) > > Just make, also, that if this clock is shared between dwc3-keystone > > wrapper and dwc3 core, you clk_get() on both driver's probe. > > > I understand. In summary, whichever patch you pick(yours) or Grygorii's, > its completely safe to remove the clock handling from Keystone USB driver. alright, since I can't really test, I'll take this as a true statement. If there are any regressions I can blame you, hehehe. cheers -- balbi signature.asc Description: Digital signature
[RFC PATCH] usb: move hub init and LED blink work to power efficient workqueue
From: Shaibal Dutta
Allow the scheduler to select the best CPU to handle hub initalization
and LED blinking work. This extends idle residency times on idle CPUs
and conserves power.
This functionality is enabled when CONFIG_WQ_POWER_EFFICIENT is selected.
Cc: Greg Kroah-Hartman
Cc: Alan Stern
Cc: Sarah Sharp
Cc: Xenia Ragiadakou
Cc: Julius Werner
Cc: Krzysztof Mazur
Cc: Matthias Beyer
Cc: Dan Williams
Cc: Mathias Nyman
Cc: Thomas Pugliese
Signed-off-by: Shaibal Dutta
[zoran.marko...@linaro.org: Rebased to latest kernel. Added commit message.
Changed reference from system to power efficient workqueue for LEDs in
check_highspeed() and hub_port_connect_change().]
Signed-off-by: Zoran Markovic
---
drivers/usb/core/hub.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index babba88..ae07ffe 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -504,7 +504,8 @@ static void led_work (struct work_struct *work)
changed++;
}
if (changed)
- schedule_delayed_work(&hub->leds, LED_CYCLE_PERIOD);
+ queue_delayed_work(system_power_efficient_wq,
+ &hub->leds, LED_CYCLE_PERIOD);
}
/* use a short timeout for hub/port status fetches */
@@ -1046,8 +1047,9 @@ static void hub_activate(struct usb_hub *hub, enum
hub_activation_type type)
if (type == HUB_INIT) {
delay = hub_power_on(hub, false);
PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func2);
- schedule_delayed_work(&hub->init_work,
- msecs_to_jiffies(delay));
+ queue_delayed_work(system_power_efficient_wq,
+ &hub->init_work,
+ msecs_to_jiffies(delay));
/* Suppress autosuspend until init is done */
usb_autopm_get_interface_no_resume(
@@ -1200,8 +1202,9 @@ static void hub_activate(struct usb_hub *hub, enum
hub_activation_type type)
/* Don't do a long sleep inside a workqueue routine */
if (type == HUB_INIT2) {
PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func3);
- schedule_delayed_work(&hub->init_work,
- msecs_to_jiffies(delay));
+ queue_delayed_work(system_power_efficient_wq,
+ &hub->init_work,
+ msecs_to_jiffies(delay));
return; /* Continues at init3: below */
} else {
msleep(delay);
@@ -1214,7 +1217,8 @@ static void hub_activate(struct usb_hub *hub, enum
hub_activation_type type)
if (status < 0)
dev_err(hub->intfdev, "activate --> %d\n", status);
if (hub->has_indicators && blinkenlights)
- schedule_delayed_work(&hub->leds, LED_CYCLE_PERIOD);
+ queue_delayed_work(system_power_efficient_wq,
+ &hub->leds, LED_CYCLE_PERIOD);
/* Scan all ports that need attention */
kick_khubd(hub);
@@ -4316,7 +4320,8 @@ check_highspeed (struct usb_hub *hub, struct usb_device
*udev, int port1)
/* hub LEDs are probably harder to miss than syslog */
if (hub->has_indicators) {
hub->indicator[port1-1] = INDICATOR_GREEN_BLINK;
- schedule_delayed_work (&hub->leds, 0);
+ queue_delayed_work(system_power_efficient_wq,
+ &hub->leds, 0);
}
}
kfree(qual);
@@ -4545,7 +4550,9 @@ static void hub_port_connect_change(struct usb_hub *hub,
int port1,
if (hub->has_indicators) {
hub->indicator[port1-1] =
INDICATOR_AMBER_BLINK;
- schedule_delayed_work (&hub->leds, 0);
+ queue_delayed_work(
+ system_power_efficient_wq,
+ &hub->leds, 0);
}
status = -ENOTCONN; /* Don't retry */
goto loop_disable;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: keystone: switch to use runtime pm
On Friday 31 January 2014 05:15 PM, Felipe Balbi wrote: > Hi, > > On Fri, Jan 31, 2014 at 02:20:48PM -0500, Santosh Shilimkar wrote: > > [ snip ] > >>> note that because of pm_runtime_set_active() that first >>> pm_runtime_get_sync() in probe() will simply increase the reference >>> counter without calling my ->runtime_resume() callback, which is exactly >>> what we want, as that would completely avoid situations of bad context >>> being restored because of that initial pm_runtime_get_sync(). >>> >> Thanks for making your point bit clear. > > no problem. > >>> Then, we can even make pm_runtime completely async easily, because >>> clk_prepare() was called only on probe() (or before it, for that >>> matter). >>> >>> Bottomline is, if you can guarantee me that clk_get(), clk_prepare(), >>> clk_enable() and pm_runtime_set_active() will be called properly before >>> my probe, i'll be more than happy to comply with your request above as >>> that will greatly simplify my driver. >>> >> Which is the case at least I see on Keystone. And hence the patch from > > I was going over pm_domain.c and drivers/base/power/clock_ops.c and none > of them enable pm_runtime or make sure pm_runtime_set_active() is > called. > >> Grygorii works. I also noticed your proposal for wider platform to >> enforce above behavior which seems to be a good idea. > > it'll take months to stabilize though ;-) > >>> Just make, also, that if this clock is shared between dwc3-keystone >>> wrapper and dwc3 core, you clk_get() on both driver's probe. >>> >> I understand. In summary, whichever patch you pick(yours) or Grygorii's, >> its completely safe to remove the clock handling from Keystone USB driver. > > alright, since I can't really test, I'll take this as a true statement. > If there are any regressions I can blame you, hehehe. > No problem... :D Grygorii patch has been working well so all good with 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
[PATCH v4 03/14] usb: find internal hub tier mismatch via acpi
ACPI identifies peer ports by setting their 'group_token' and
'group_position' _PLD data to the same value. If a platform has tier
mismatch [1] , ACPI can override the default, USB3 defined, peer port
association for internal hubs. External hubs follow the default peer
association scheme.
Location data is cached as an opaque cookie in usb_port_location data.
Note that we only consider the group_token and group_position attributes
from the _PLD data as ACPI specifies that group_token is a unique
identifier.
[1]: xhci 1.1 appendix D figure 131
[2]: acpi 5 section 6.1.8
Signed-off-by: Dan Williams
---
drivers/usb/core/hub.h |6 +++
drivers/usb/core/port.c | 96 +++
drivers/usb/core/usb-acpi.c | 35 +---
drivers/usb/core/usb.h |2 +
4 files changed, 131 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 975a0ad3a348..5ba1bc16d4b7 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -76,6 +76,10 @@ struct usb_hub {
struct usb_port **ports;
};
+struct usb_port_location {
+ u32 cookie;
+};
+
/**
* struct usb port - kernel's representation of a usb port
* @child: usb device attached to the port
@@ -83,6 +87,7 @@ struct usb_hub {
* @port_owner: port's owner
* @peer: related usb2 and usb3 ports (share the same connector)
* @connect_type: port's connect type
+ * @location: opaque representation of platform connector location
* @portnum: port index num based one
* @power_is_on: port's power state
* @did_runtime_put: port has done pm_runtime_put().
@@ -93,6 +98,7 @@ struct usb_port {
struct dev_state *port_owner;
struct usb_port *peer;
enum usb_port_connect_type connect_type;
+ struct usb_port_location location;
u8 portnum;
unsigned power_is_on:1;
unsigned did_runtime_put:1;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 7fd22020d7ee..8fae3cd03305 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -149,11 +149,14 @@ struct device_type usb_port_device_type = {
static struct usb_port *find_peer_port(struct usb_hub *hub, int port1)
{
- struct usb_device *hdev = hub->hdev;
+ struct usb_device *hdev = hub ? hub->hdev : NULL;
struct usb_device *peer_hdev;
struct usb_port *peer = NULL;
struct usb_hub *peer_hub;
+ if (!hub)
+ return NULL;
+
/* set the default peer port for root hubs. Platform firmware
* can later fix this up if tier-mismatch is present. Assumes
* the primary_hcd is usb2.0 and registered first
@@ -193,6 +196,97 @@ static struct usb_port *find_peer_port(struct usb_hub
*hub, int port1)
return peer;
}
+static void reset_peer(struct usb_port *port_dev, struct usb_port *peer)
+{
+ if (!peer)
+ return;
+
+ spin_lock(&peer_lock);
+ if (port_dev->peer)
+ put_device(&port_dev->peer->dev);
+ if (peer->peer)
+ put_device(&peer->peer->dev);
+ port_dev->peer = peer;
+ peer->peer = port_dev;
+ get_device(&peer->dev);
+ get_device(&port_dev->dev);
+ spin_unlock(&peer_lock);
+}
+
+/* assumes that location data is only set for external connectors and that
+ * external hubs never have tier mismatch
+ */
+static int redo_find_peer_port(struct device *dev, void *data)
+{
+ struct usb_port *port_dev = to_usb_port(dev);
+
+ if (is_usb_port(dev)) {
+ struct usb_device *hdev = to_usb_device(dev->parent->parent);
+ struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ int port1 = port_dev->portnum;
+ struct usb_port *peer;
+
+ peer = find_peer_port(hub, port1);
+ reset_peer(port_dev, peer);
+ }
+
+ return device_for_each_child(dev, NULL, redo_find_peer_port);
+}
+
+static int pair_port(struct device *dev, void *data)
+{
+ struct usb_port *peer = data;
+ struct usb_port *port_dev = to_usb_port(dev);
+
+ if (!is_usb_port(dev)
+ || port_dev->location.cookie != peer->location.cookie)
+ return device_for_each_child(dev, peer, pair_port);
+
+ dev_dbg(dev->parent->parent, "port%d peer = %s port%d (by location)\n",
+ port_dev->portnum, dev_name(peer->dev.parent->parent),
+ peer->portnum);
+ if (port_dev->peer != peer) {
+ /* Sigh, tier mismatch has invalidated our ancestry.
+* This should not be too onerous even in deep hub
+* topologies as we will discover tier mismatch early
+* (after platform internal hubs have been enumerated),
+* before external hubs are probed.
+*/
+ reset_peer(port_dev, peer);
+ device_for_each_child(dev, NULL, redo_find_peer_port);
+
[PATCH v4 02/14] usb: assign usb3 external hub port peers
Given that root hub port peers are already established, external hub peer
ports can be determined by traversing the device topology:
1/ ascend to the parent hub and find the upstream port_dev
2/ walk ->peer to find the peer port
3/ descend to the peer hub via ->child
4/ find the port with the matching port id
Note that this assumes the port labelling scheme required by the
specification [1].
[1]: usb3 3.1 section 10.3.3
Signed-off-by: Dan Williams
---
drivers/usb/core/port.c | 23 +--
1 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 8d3ec2daf6fe..7fd22020d7ee 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -150,15 +150,15 @@ struct device_type usb_port_device_type = {
static struct usb_port *find_peer_port(struct usb_hub *hub, int port1)
{
struct usb_device *hdev = hub->hdev;
+ struct usb_device *peer_hdev;
struct usb_port *peer = NULL;
+ struct usb_hub *peer_hub;
/* set the default peer port for root hubs. Platform firmware
* can later fix this up if tier-mismatch is present. Assumes
* the primary_hcd is usb2.0 and registered first
*/
if (!hdev->parent) {
- struct usb_hub *peer_hub;
- struct usb_device *peer_hdev;
struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
struct usb_hcd *peer_hcd = hcd->primary_hcd;
@@ -170,6 +170,24 @@ static struct usb_port *find_peer_port(struct usb_hub
*hub, int port1)
peer_hub = usb_hub_to_struct_hub(peer_hdev);
if (peer_hub && port1 <= peer_hdev->maxchild)
peer = peer_hub->ports[port1 - 1];
+ } else {
+ struct usb_port *upstream;
+ struct usb_device *parent = hdev->parent;
+ struct usb_hub *parent_hub = usb_hub_to_struct_hub(parent);
+
+ if (!parent_hub)
+ return NULL;
+
+ upstream = parent_hub->ports[hdev->portnum - 1];
+ if (!upstream->peer)
+ return NULL;
+
+ peer_hdev = upstream->peer->child;
+ peer_hub = usb_hub_to_struct_hub(peer_hdev);
+ if (!peer_hub || port1 > peer_hdev->maxchild)
+ return NULL;
+
+ peer = peer_hub->ports[port1 - 1];
}
return peer;
@@ -202,6 +220,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int
port1)
spin_lock(&peer_lock);
get_device(&peer->dev);
port_dev->peer = peer;
+ WARN_ON(peer->peer);
get_device(&port_dev->dev);
peer->peer = port_dev;
spin_unlock(&peer_lock);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 04/14] usb: sysfs link peer ports
For example:
usb2/2-1/2-1:1.0/port1/peer => ../../../../usb3/3-1/3-1:1.0/port1
usb2/2-1/2-1:1.0/port2/peer => ../../../../usb3/3-1/3-1:1.0/port2
usb2/2-1/2-1:1.0/port3/peer => ../../../../usb3/3-1/3-1:1.0/port3
usb2/2-1/2-1:1.0/port4/peer => ../../../../usb3/3-1/3-1:1.0/port4
usb2/2-0:1.0/port1/peer => ../../../usb3/3-0:1.0/port1
usb2/2-0:1.0/port2/peer => ../../../usb3/3-0:1.0/port2
usb2/2-0:1.0/port3/peer => ../../../usb3/3-0:1.0/port3
usb2/2-0:1.0/port4/peer => ../../../usb3/3-0:1.0/port4
usb3/3-1/3-1:1.0/port1/peer => ../../../../usb2/2-1/2-1:1.0/port1
usb3/3-1/3-1:1.0/port2/peer => ../../../../usb2/2-1/2-1:1.0/port2
usb3/3-1/3-1:1.0/port3/peer => ../../../../usb2/2-1/2-1:1.0/port3
usb3/3-1/3-1:1.0/port4/peer => ../../../../usb2/2-1/2-1:1.0/port4
usb3/3-0:1.0/port1/peer => ../../../usb2/2-0:1.0/port1
usb3/3-0:1.0/port2/peer => ../../../usb2/2-0:1.0/port2
usb3/3-0:1.0/port3/peer => ../../../usb2/2-0:1.0/port3
usb3/3-0:1.0/port4/peer => ../../../usb2/2-0:1.0/port4
Signed-off-by: Dan Williams
---
drivers/usb/core/port.c | 40 +++-
1 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 8fae3cd03305..d3aacf093aa1 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -21,7 +21,7 @@
#include "hub.h"
-DEFINE_SPINLOCK(peer_lock);
+DEFINE_MUTEX(peer_lock);
static const struct attribute_group *port_dev_group[];
static ssize_t connect_type_show(struct device *dev,
@@ -201,16 +201,20 @@ static void reset_peer(struct usb_port *port_dev, struct
usb_port *peer)
if (!peer)
return;
- spin_lock(&peer_lock);
- if (port_dev->peer)
+ mutex_lock(&peer_lock);
+ if (port_dev->peer) {
put_device(&port_dev->peer->dev);
- if (peer->peer)
+ sysfs_remove_link(&port_dev->dev.kobj, "peer");
+ }
+ if (peer->peer) {
put_device(&peer->peer->dev);
+ sysfs_remove_link(&peer->dev.kobj, "peer");
+ }
port_dev->peer = peer;
peer->peer = port_dev;
get_device(&peer->dev);
get_device(&port_dev->dev);
- spin_unlock(&peer_lock);
+ mutex_unlock(&peer_lock);
}
/* assumes that location data is only set for external connectors and that
@@ -311,19 +315,33 @@ int usb_hub_create_port_device(struct usb_hub *hub, int
port1)
dev_dbg(&hub->hdev->dev, "port%d peer = %s\n",
port1, peer ? dev_name(peer->dev.parent->parent) : "[none]");
if (peer) {
- spin_lock(&peer_lock);
+ mutex_lock(&peer_lock);
get_device(&peer->dev);
port_dev->peer = peer;
WARN_ON(peer->peer);
get_device(&port_dev->dev);
peer->peer = port_dev;
- spin_unlock(&peer_lock);
+ mutex_unlock(&peer_lock);
}
retval = device_add(&port_dev->dev);
if (retval)
goto error_register;
+ mutex_lock(&peer_lock);
+ peer = port_dev->peer;
+ do if (peer) {
+ retval = sysfs_create_link(&port_dev->dev.kobj,
+ &peer->dev.kobj, "peer");
+ if (retval)
+ break;
+ retval = sysfs_create_link(&peer->dev.kobj,
+ &port_dev->dev.kobj, "peer");
+ } while (0);
+ mutex_unlock(&peer_lock);
+ if (retval)
+ goto error_links;
+
pm_runtime_set_active(&port_dev->dev);
/* It would be dangerous if user space couldn't
@@ -339,6 +357,8 @@ int usb_hub_create_port_device(struct usb_hub *hub, int
port1)
error_register:
put_device(&port_dev->dev);
+error_links:
+ device_unregister(&port_dev->dev);
exit:
return retval;
}
@@ -348,14 +368,16 @@ void usb_hub_remove_port_device(struct usb_hub *hub, int
port1)
struct usb_port *port_dev = hub->ports[port1 - 1];
struct usb_port *peer = port_dev->peer;
- spin_lock(&peer_lock);
+ mutex_lock(&peer_lock);
if (peer) {
peer->peer = NULL;
port_dev->peer = NULL;
put_device(&port_dev->dev);
put_device(&peer->dev);
+ sysfs_remove_link(&port_dev->dev.kobj, "peer");
+ sysfs_remove_link(&peer->dev.kobj, "peer");
}
- spin_unlock(&peer_lock);
+ mutex_unlock(&peer_lock);
device_unregister(&port_dev->dev);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 00/14] port power control fixes
Toggling port power currently leads to three unintended disconnect
scenarios that are addressed by this rework of port power recovery and
usb device resume:
1/ Superspeed devices downgrade to their hispeed connection: fix this by
preventing superspeed poweroff until the peer port is suspended.
This depends on the ability to identify peer ports (patches 1-4), and
then patch 5 implements the policy.
2/ khubd prematurely disconnects ports that are in the process of
being resumed or reset. khubd now ignores ports in the
pm-runtime-suspended state (patch 9) and holds a lock to synchronize the port
status changes of usb_port_{suspend|resume} (patch 11).
3/ Superspeed devices fail to reconnect: Seen during repeated toggles of
the port power state. Fixed by forcing a full recovery cycle of the
usb_device before allowing the next suspend / khubd run (patch 12).
Also, for devices that live lock on reconnect the port runtime resume
path now has the capability to force a reset-resume to be a
warm-reset-resume (patch 13).
New since v3 [1]:
* Addressed Sarah's comment about the FEAT_C_ENABLE patch, split it into
patch 6 and 7 [2]
* Addressed Alan's comments [3] about usb_port_{suspend|resume}
synchronization. Refactored hub_events() (patch 8) and introduced
usb_port.status_lock (patch 11) accordingly
* Clarified the documentation to indicate the perils of unbinding
usbcore interface drivers (lose ability to resume) and added more details
about the ordering policies enforced by the fix for bug 1 above.
* Added patch 10 to fix a long standing xhci bug made more prominent by the
ordering of events forced by the port status lock.
* Clarified a few changelogs and added commentary to the code
[1] v3: http://marc.info/?l=linux-usb&m=138912657113817&w=2
[2] status lock discussion: http://marc.info/?t=138912664200010&r=1&w=2
[3] split FEAT_C_ENABLE patch:
http://marc.info/?l=linux-usb&m=138989014115864&w=2
[4] don't unbind usbcore: http://marc.info/?l=linux-usb&m=138982558731254&w=2
---
[PATCH v4 01/14] usb: assign default peer ports for root hubs
[PATCH v4 02/14] usb: assign usb3 external hub port peers
[PATCH v4 03/14] usb: find internal hub tier mismatch via acpi
[PATCH v4 04/14] usb: sysfs link peer ports
[PATCH v4 05/14] usb: defer suspension of superspeed port while peer is powered
[PATCH v4 06/14] usb: don't clear FEAT_C_ENABLE on usb_port_runtime_resume
failure
[PATCH v4 07/14] usb: usb3 ports do not support FEAT_C_ENABLE
[PATCH v4 08/14] usb: refactor port handling in hub_events()
[PATCH v4 09/14] usb: synchronize port poweroff and khubd
[PATCH v4 10/14] xhci: cancel in-flight resume requests when the port is
powered off
[PATCH v4 11/14] usb: introduce port status lock
[PATCH v4 12/14] usb: guarantee child device resume on port poweron
[PATCH v4 13/14] usb: force warm reset to break resume livelock
[PATCH v4 14/14] usb: documentation for usb port power off mechanisms
Documentation/usb/power-management.txt | 236
drivers/usb/core/hub.c | 372 +++-
drivers/usb/core/hub.h | 18 ++
drivers/usb/core/port.c| 277 ++--
drivers/usb/core/usb-acpi.c| 35 ++-
drivers/usb/core/usb.h |2
drivers/usb/host/xhci-hub.c| 12 +
7 files changed, 770 insertions(+), 182 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 05/14] usb: defer suspension of superspeed port while peer is powered
ClearPortFeature(PORT_POWER) on a usb3 port places the port in either a
DSPORT.Powered-off-detect / DSPORT.Powered-off-reset loop, or the
DSPORT.Powered-off state. There is no way to ensure that RX
terminations will persist in this state, so it is possible a device will
degrade to its usb2 connection. Prevent this by blocking power-off of a
usb3 port while its usb2 peer is active, and powering on a usb3 port
before its usb2 peer.
Signed-off-by: Dan Williams
---
drivers/usb/core/port.c | 36
1 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index d3aacf093aa1..2227379be652 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -77,12 +77,19 @@ static int usb_port_runtime_resume(struct device *dev)
struct usb_device *hdev = to_usb_device(dev->parent->parent);
struct usb_interface *intf = to_usb_interface(dev->parent);
struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ struct usb_port *peer = port_dev->peer;
int port1 = port_dev->portnum;
int retval;
if (!hub)
return -EINVAL;
+ /* power on our usb3 peer before this usb2 port to prevent a usb3
+* device from degrading to its usb2 connection
+*/
+ if (!hub_is_superspeed(hdev) && peer)
+ pm_runtime_get_sync(&peer->dev);
+
usb_autopm_get_interface(intf);
set_bit(port1, hub->busy_bits);
@@ -104,6 +111,13 @@ static int usb_port_runtime_resume(struct device *dev)
clear_bit(port1, hub->busy_bits);
usb_autopm_put_interface(intf);
+
+ /* usb3 peer is marked active so we can drop the reference we took to
+* ensure ordering above
+*/
+ if (!hub_is_superspeed(hdev) && peer)
+ pm_runtime_put(&peer->dev);
+
return retval;
}
@@ -113,6 +127,7 @@ static int usb_port_runtime_suspend(struct device *dev)
struct usb_device *hdev = to_usb_device(dev->parent->parent);
struct usb_interface *intf = to_usb_interface(dev->parent);
struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ struct usb_port *peer = port_dev->peer;
int port1 = port_dev->portnum;
int retval;
@@ -123,6 +138,16 @@ static int usb_port_runtime_suspend(struct device *dev)
== PM_QOS_FLAGS_ALL)
return -EAGAIN;
+ /* block poweroff of superspeed ports while highspeed peer is on */
+ dev_WARN_ONCE(&hdev->dev, hub_is_superspeed(hdev) && !peer,
+ "port%d missing peer info\n", port1);
+
+ /* prevent a usb3 port from powering off while its usb2 peer is
+* powered on
+*/
+ if (hub_is_superspeed(hdev) && (!peer || peer->power_is_on))
+ return -EBUSY;
+
usb_autopm_get_interface(intf);
set_bit(port1, hub->busy_bits);
retval = usb_hub_set_port_power(hdev, hub, port1, false);
@@ -130,6 +155,17 @@ static int usb_port_runtime_suspend(struct device *dev)
usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE);
clear_bit(port1, hub->busy_bits);
usb_autopm_put_interface(intf);
+
+ /* Our peer usb3 port may now be able to suspend, asynchronously
+* queue a suspend request to observe that this usb2 peer port
+* is now off. There is a small race since there is no way to
+* flush this suspend, userspace is advised to order suspends.
+*/
+ if (!hub_is_superspeed(hdev) && peer) {
+ pm_runtime_get(&peer->dev);
+ pm_runtime_put(&peer->dev);
+ }
+
return retval;
}
#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 01/14] usb: assign default peer ports for root hubs
Assume that the peer of a superspeed port is the port with the same id
on the shared_hcd root hub. This identification scheme is required of
external hubs by the USB3 spec [1]. However, for root hubs, tier mismatch
may be in effect [2]. Tier mismatch can only be enumerated via platform
firmware. For now, simply perform the nominal association.
[1]: usb 3.1 section 10.3.3
[2]: xhci 1.1 appendix D
Signed-off-by: Dan Williams
---
drivers/usb/core/hub.c |5
drivers/usb/core/hub.h |6
drivers/usb/core/port.c | 64 +++
3 files changed, 64 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index babba885978d..548d327d89dd 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -36,11 +36,6 @@
#define USB_VENDOR_GENESYS_LOGIC 0x05e3
#define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND 0x01
-static inline int hub_is_superspeed(struct usb_device *hdev)
-{
- return (hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS);
-}
-
/* Protect struct usb_device->state and ->children members
* Note: Both are also protected by ->dev.sem, except that ->state can
* change to USB_STATE_NOTATTACHED even when the semaphore isn't held. */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index df629a310e44..975a0ad3a348 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -81,6 +81,7 @@ struct usb_hub {
* @child: usb device attached to the port
* @dev: generic device interface
* @port_owner: port's owner
+ * @peer: related usb2 and usb3 ports (share the same connector)
* @connect_type: port's connect type
* @portnum: port index num based one
* @power_is_on: port's power state
@@ -90,6 +91,7 @@ struct usb_port {
struct usb_device *child;
struct device dev;
struct dev_state *port_owner;
+ struct usb_port *peer;
enum usb_port_connect_type connect_type;
u8 portnum;
unsigned power_is_on:1;
@@ -123,3 +125,7 @@ static inline int hub_port_debounce_be_stable(struct
usb_hub *hub,
return hub_port_debounce(hub, port1, false);
}
+static inline int hub_is_superspeed(struct usb_device *hdev)
+{
+ return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS;
+}
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 51542f852393..8d3ec2daf6fe 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -21,6 +21,7 @@
#include "hub.h"
+DEFINE_SPINLOCK(peer_lock);
static const struct attribute_group *port_dev_group[];
static ssize_t connect_type_show(struct device *dev,
@@ -146,9 +147,37 @@ struct device_type usb_port_device_type = {
.pm = &usb_port_pm_ops,
};
+static struct usb_port *find_peer_port(struct usb_hub *hub, int port1)
+{
+ struct usb_device *hdev = hub->hdev;
+ struct usb_port *peer = NULL;
+
+ /* set the default peer port for root hubs. Platform firmware
+* can later fix this up if tier-mismatch is present. Assumes
+* the primary_hcd is usb2.0 and registered first
+*/
+ if (!hdev->parent) {
+ struct usb_hub *peer_hub;
+ struct usb_device *peer_hdev;
+ struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
+ struct usb_hcd *peer_hcd = hcd->primary_hcd;
+
+ if (!hub_is_superspeed(hdev)
+ || WARN_ON_ONCE(!peer_hcd || hcd == peer_hcd))
+ return NULL;
+
+ peer_hdev = peer_hcd->self.root_hub;
+ peer_hub = usb_hub_to_struct_hub(peer_hdev);
+ if (peer_hub && port1 <= peer_hdev->maxchild)
+ peer = peer_hub->ports[port1 - 1];
+ }
+
+ return peer;
+}
+
int usb_hub_create_port_device(struct usb_hub *hub, int port1)
{
- struct usb_port *port_dev = NULL;
+ struct usb_port *port_dev, *peer;
int retval;
port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
@@ -164,8 +193,21 @@ int usb_hub_create_port_device(struct usb_hub *hub, int
port1)
port_dev->dev.groups = port_dev_group;
port_dev->dev.type = &usb_port_device_type;
dev_set_name(&port_dev->dev, "port%d", port1);
+ device_initialize(&port_dev->dev);
+
+ peer = find_peer_port(hub, port1);
+ dev_dbg(&hub->hdev->dev, "port%d peer = %s\n",
+ port1, peer ? dev_name(peer->dev.parent->parent) : "[none]");
+ if (peer) {
+ spin_lock(&peer_lock);
+ get_device(&peer->dev);
+ port_dev->peer = peer;
+ get_device(&port_dev->dev);
+ peer->peer = port_dev;
+ spin_unlock(&peer_lock);
+ }
- retval = device_register(&port_dev->dev);
+ retval = device_add(&port_dev->dev);
if (retval)
goto error_register;
@@ -188,9 +230,19 @@ exit:
return retval;
}
-void usb_hub_remove_
[PATCH v4 06/14] usb: don't clear FEAT_C_ENABLE on usb_port_runtime_resume failure
Three reasons: 1/ It's an invalid operation on usb3 ports 2/ There's no guarantee of when / if a usb2 port has entered an error state relative to PORT_POWER request 3/ The port is active / powered at this point, so khubd will clear it as a matter of course Signed-off-by: Dan Williams --- drivers/usb/core/port.c |1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index 2227379be652..32290b8d4f8f 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -105,7 +105,6 @@ static int usb_port_runtime_resume(struct device *dev) if (retval < 0) dev_dbg(&port_dev->dev, "can't get reconnection after setting port power on, status %d\n", retval); - usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE); retval = 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 10/14] xhci: cancel in-flight resume requests when the port is powered off
Work around a remote wakeup vs port poweroff request race. A wakeup request
sets the timer, but by the time it expires the port has been turned off, so we
hit:
if ((raw_port_status & PORT_RESET) ||
!(raw_port_status & PORT_PE))
return 0x;
...in xhci_get_port_status
When userspace has set the port policy to allow poweroff the assumption is that
it no longer cares about remote wakeup requests for that port. If a request
happens to collide with autosuspend of the port we need to make sure to cancel
an in-flight request otherwise we can end up in an infinite loop.
Signed-off-by: Dan Williams
---
drivers/usb/host/xhci-hub.c | 12 ++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 9992fbfec85f..2333ec573594 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1004,9 +1004,16 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq,
u16 wValue,
xhci_disable_port(hcd, xhci, wIndex,
port_array[wIndex], temp);
break;
- case USB_PORT_FEAT_POWER:
- writel(temp & ~PORT_POWER, port_array[wIndex]);
+ case USB_PORT_FEAT_POWER: {
+ struct xhci_bus_state *bus_state;
+ bus_state = &xhci->bus_state[hcd_index(hcd)];
+ writel(temp & ~PORT_POWER, port_array[wIndex]);
+ if (test_and_clear_bit(wIndex,
&bus_state->resuming_ports)) {
+ bus_state->resume_done[wIndex] = 0;
+ xhci_dbg(xhci, "port%d: resume cancelled\n",
+wIndex);
+ }
spin_unlock_irqrestore(&xhci->lock, flags);
temp = usb_acpi_power_manageable(hcd->self.root_hub,
wIndex);
@@ -1015,6 +1022,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq,
u16 wValue,
wIndex, false);
spin_lock_irqsave(&xhci->lock, flags);
break;
+ }
default:
goto error;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 08/14] usb: refactor port handling in hub_events()
In preparation for synchronizing port handling with pm_runtime
transitions refactor port handling into its own subroutine.
We expect that clearing some status flags will be required regardless of
the port state, so handle those first and group all non-trivial actions
at the bottom of the routine.
Other cleanups include:
1/ reflowing to 80 columns
2/ replacing redundant usages of 'hub->hdev' with 'hdev'
3/ baseline debug prints on a common format of "port%d: "
Signed-off-by: Dan Williams
---
drivers/usb/core/hub.c | 235 ++--
1 files changed, 110 insertions(+), 125 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 548d327d89dd..8da2aa706005 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4653,6 +4653,110 @@ static int hub_handle_remote_wakeup(struct usb_hub
*hub, unsigned int port,
return connect_change;
}
+static void port_event(struct usb_hub *hub, int port1)
+{
+ struct usb_port *port_dev = hub->ports[port1 - 1];
+ struct usb_device *udev = port_dev->child;
+ struct usb_device *hdev = hub->hdev;
+ struct device *hub_dev = &hdev->dev;
+ int connect_change, wakeup_change;
+ u16 portstatus, portchange;
+
+ connect_change = test_bit(port1, hub->change_bits);
+ wakeup_change = test_and_clear_bit(port1, hub->wakeup_bits);
+
+ if (hub_port_status(hub, port1, &portstatus, &portchange) < 0)
+ return;
+
+ if (portchange & USB_PORT_STAT_C_CONNECTION) {
+ usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION);
+ connect_change = 1;
+ }
+
+ if (portchange & USB_PORT_STAT_C_ENABLE) {
+ if (!connect_change)
+ dev_dbg(hub_dev, "port%d: enable change, status %08x\n",
+ port1, portstatus);
+ usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE);
+
+ /*
+* EM interference sometimes causes badly shielded USB devices
+* to be shutdown by the hub, this hack enables them again.
+* Works at least with mouse driver.
+*/
+ if (!(portstatus & USB_PORT_STAT_ENABLE)
+ && !connect_change && udev) {
+ dev_err(hub_dev,
+"port%d: disabled by hub (EMI?), re-enabling...\n",
+ port1);
+ connect_change = 1;
+ }
+ }
+
+ if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
+ u16 status = 0, unused;
+
+ dev_dbg(hub_dev, "port%d: over-current change\n", port1);
+ usb_clear_port_feature(hdev, port1,
+ USB_PORT_FEAT_C_OVER_CURRENT);
+ msleep(100);/* Cool down */
+ hub_power_on(hub, true);
+ hub_port_status(hub, port1, &status, &unused);
+ if (status & USB_PORT_STAT_OVERCURRENT)
+ dev_err(hub_dev, "port%d: over-current condition\n",
+ port1);
+ }
+
+ if (portchange & USB_PORT_STAT_C_RESET) {
+ dev_dbg(hub_dev, "port%d: reset change\n", port1);
+ usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_RESET);
+ }
+ if ((portchange & USB_PORT_STAT_C_BH_RESET)
+ && hub_is_superspeed(hdev)) {
+ dev_dbg(hub_dev, "port%d: warm reset change\n", port1);
+ usb_clear_port_feature(hdev, port1,
+ USB_PORT_FEAT_C_BH_PORT_RESET);
+ }
+ if (portchange & USB_PORT_STAT_C_LINK_STATE) {
+ dev_dbg(hub_dev, "port%d: link state change\n", port1);
+ usb_clear_port_feature(hdev, port1,
+ USB_PORT_FEAT_C_PORT_LINK_STATE);
+ }
+ if (portchange & USB_PORT_STAT_C_CONFIG_ERROR) {
+ dev_warn(hub_dev, "port%d: config error\n", port1);
+ usb_clear_port_feature(hdev, port1,
+ USB_PORT_FEAT_C_PORT_CONFIG_ERROR);
+ }
+
+ if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange))
+ connect_change = 1;
+
+ /* Warm reset a USB3 protocol port if it's in
+* SS.Inactive state.
+*/
+ if (hub_port_warm_reset_required(hub, portstatus)) {
+ int status;
+
+ dev_dbg(hub_dev, "port%d: do warm reset\n", port1);
+ if (!udev || !(portstatus & USB_PORT_STAT_CONNECTION)
+ || udev->state == USB_STATE_NOTATTACHED) {
+ status = hub_port_reset(hub, port1, NULL,
+ HUB_BH_RESET_TIME, true);
+ if (status < 0)
+ hub_port_disable(hub, port1, 1);
+ } else
[PATCH v4 09/14] usb: synchronize port poweroff and khubd
If a port is powered-off, or in the process of being powered-off, prevent
khubd from operating on it. Otherwise, the following sequence of events
leading to an unintended disconnect may occur:
Events:
(0)
(1) hub 2-2:1.0: hub_resume
(2) hub 2-2:1.0: port 1: status 0301 change
(3) hub 2-2:1.0: state 7 ports 4 chg 0002 evt
(4) hub 2-2:1.0: port 1, power off status , change , 12 Mb/s
(5) usb 2-2.1: USB disconnect, device number 5
Description:
(1) hub is resumed before sending a ClearPortFeature request
(2) hub_activate() notices the port is connected and sets
hub->change_bits for the port
(3) hub_events() starts, but at the same time the port suspends
(4) hub_connect_change() sees the disabled port and triggers disconnect
Signed-off-by: Dan Williams
---
drivers/usb/core/hub.c | 55
1 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 8da2aa706005..0217299a2402 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4728,32 +4728,41 @@ static void port_event(struct usb_hub *hub, int port1)
USB_PORT_FEAT_C_PORT_CONFIG_ERROR);
}
- if (hub_handle_remote_wakeup(hub, port1, portstatus, portchange))
- connect_change = 1;
-
- /* Warm reset a USB3 protocol port if it's in
-* SS.Inactive state.
-*/
- if (hub_port_warm_reset_required(hub, portstatus)) {
- int status;
+ /* take port actions that require the port to be powered on */
+ pm_runtime_get_noresume(&port_dev->dev);
+ pm_runtime_barrier(&port_dev->dev);
+ if (pm_runtime_active(&port_dev->dev)) {
+ if (hub_handle_remote_wakeup(hub, port1,
+portstatus, portchange))
+ connect_change = 1;
- dev_dbg(hub_dev, "port%d: do warm reset\n", port1);
- if (!udev || !(portstatus & USB_PORT_STAT_CONNECTION)
- || udev->state == USB_STATE_NOTATTACHED) {
- status = hub_port_reset(hub, port1, NULL,
- HUB_BH_RESET_TIME, true);
- if (status < 0)
- hub_port_disable(hub, port1, 1);
- } else {
- usb_lock_device(udev);
- status = usb_reset_device(udev);
- usb_unlock_device(udev);
- connect_change = 0;
+ /* Warm reset a USB3 protocol port if it's in
+* SS.Inactive state.
+*/
+ if (hub_port_warm_reset_required(hub, portstatus)) {
+ int status;
+
+ dev_dbg(hub_dev, "port%d: do warm reset\n", port1);
+ if (!udev || !(portstatus & USB_PORT_STAT_CONNECTION)
+ || udev->state == USB_STATE_NOTATTACHED) {
+ status = hub_port_reset(hub, port1, NULL,
+ HUB_BH_RESET_TIME,
+ true);
+ if (status < 0)
+ hub_port_disable(hub, port1, 1);
+ } else {
+ usb_lock_device(udev);
+ status = usb_reset_device(udev);
+ usb_unlock_device(udev);
+ connect_change = 0;
+ }
}
- }
- if (connect_change)
- hub_port_connect_change(hub, port1, portstatus, portchange);
+ if (connect_change)
+ hub_port_connect_change(hub, port1,
+ portstatus, portchange);
+ }
+ pm_runtime_put_sync(&port_dev->dev);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 07/14] usb: usb3 ports do not support FEAT_C_ENABLE
The port pm_runtime implementation unconditionally clears FEAT_C_ENABLE after clearing PORT_POWER, but the bit is reserved on usb3 hub ports. We expect khubd to be suspended at this time so we need to clear any errors for usb2 ports. Signed-off-by: Dan Williams --- drivers/usb/core/port.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index 32290b8d4f8f..6225ae4772d9 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -151,7 +151,8 @@ static int usb_port_runtime_suspend(struct device *dev) set_bit(port1, hub->busy_bits); retval = usb_hub_set_port_power(hdev, hub, port1, false); usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION); - usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE); + if (!hub_is_superspeed(hdev)) + usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE); clear_bit(port1, hub->busy_bits); usb_autopm_put_interface(intf); -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 11/14] usb: introduce port status lock
In general we do not want khubd to act on port status changes that are
the result of in progress resets or port pm runtime operations.
Specifically port power control testing has been able to trigger an
unintended disconnect in hub_port_connect_change(), paraphrasing:
if ((portstatus & USB_PORT_STAT_CONNECTION) && udev &&
udev->state != USB_STATE_NOTATTACHED) {
if (portstatus & USB_PORT_STAT_ENABLE) {
/* Nothing to do */
} else if (udev->state == USB_STATE_SUSPENDED &&
udev->persist_enabled) {
...
} else {
/* Don't resuscitate */;
}
}
...by falling to the "Don't resuscitate" path or missing
USB_PORT_STAT_CONNECTION becuase usb_port_resume() was in the middle of
modifying the port status.
The lock ordering rules are now usb_lock_device() => usb_lock_port().
This is mandated by the device core which may hold the device_lock on
the usb_device before invoking usb_port_{suspend|resume} which in turn
take the status_lock on the usb_port. We attempt to hold the
status_lock for the duration of a port_event() run, and drop/re-aquire
it when needing to take the device_lock.
Signed-off-by: Dan Williams
---
drivers/usb/core/hub.c | 93 +++
drivers/usb/core/hub.h |2 +
drivers/usb/core/port.c |1 +
3 files changed, 72 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 0217299a2402..a310028e210d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2905,6 +2905,16 @@ static unsigned wakeup_enabled_descendants(struct
usb_device *udev)
(hub ? hub->wakeup_enabled_descendants : 0);
}
+static void usb_lock_port(struct usb_port *port_dev)
+{
+ mutex_lock(&port_dev->status_lock);
+}
+
+static void usb_unlock_port(struct usb_port *port_dev)
+{
+ mutex_unlock(&port_dev->status_lock);
+}
+
/*
* usb_port_suspend - suspend a usb device's upstream port
* @udev: device that's no longer in active use, not a root hub
@@ -2961,6 +2971,8 @@ int usb_port_suspend(struct usb_device *udev,
pm_message_t msg)
int status;
boolreally_suspend = true;
+ usb_lock_port(port_dev);
+
/* enable remote wakeup when appropriate; this lets the device
* wake up the upstream hub (including maybe the root hub).
*
@@ -3056,6 +3068,8 @@ int usb_port_suspend(struct usb_device *udev,
pm_message_t msg)
}
usb_mark_last_busy(hub->hdev);
+
+ usb_unlock_port(port_dev);
return status;
}
@@ -3195,6 +3209,8 @@ int usb_port_resume(struct usb_device *udev, pm_message_t
msg)
}
}
+ usb_lock_port(port_dev);
+
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, &portstatus, &portchange);
if (status == 0 && !port_is_suspended(hub, portstatus))
@@ -3262,6 +3278,8 @@ int usb_port_resume(struct usb_device *udev, pm_message_t
msg)
usb_unlocked_enable_lpm(udev);
}
+ usb_unlock_port(port_dev);
+
return status;
}
@@ -4371,17 +4389,21 @@ hub_power_remaining (struct usb_hub *hub)
* usb_reset_and_verify_device() encounters changed descriptors (as from
* a firmware download)
* caller already locked the hub
+ *
+ * It assumes the port is locked on entry and arranges for it to be
+ * always unlocked on exit
*/
-static void hub_port_connect_change(struct usb_hub *hub, int port1,
- u16 portstatus, u16 portchange)
+static void hub_port_connect_change_unlock(struct usb_hub *hub, int port1,
+ u16 portstatus, u16 portchange)
{
struct usb_device *hdev = hub->hdev;
struct device *hub_dev = hub->intfdev;
struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
unsigned wHubCharacteristics =
le16_to_cpu(hub->descriptor->wHubCharacteristics);
+ struct usb_port *port_dev = hub->ports[port1 - 1];
struct usb_device *udev;
- int status, i;
+ int status = -ENODEV, i;
unsigned unit_load;
dev_dbg (hub_dev,
@@ -4401,10 +4423,10 @@ static void hub_port_connect_change(struct usb_hub
*hub, int port1,
#endif
/* Try to resuscitate an existing device */
- udev = hub->ports[port1 - 1]->child;
+ udev = port_dev->child;
+
if ((portstatus & USB_PORT_STAT_CONNECTION) && udev &&
udev->state != USB_STATE_NOTATTACHED) {
- usb_lock_device(udev);
if (portstatus & USB_PORT_STAT_ENABLE) {
status = 0; /* Nothing to do */
@@ -4412,30 +4434,39 @@ static void hub_port_connect_change(struct usb_hub
*hub, in
[PATCH v4 14/14] usb: documentation for usb port power off mechanisms
From: Lan Tianyu describe the mechanisms for controlling port power policy and discovering the port power state. Cc: Oliver Neukum Signed-off-by: Lan Tianyu [sarah]: wordsmithing [djbw]: updates for peer port changes Signed-off-by: Dan Williams --- Documentation/usb/power-management.txt | 236 1 files changed, 236 insertions(+), 0 deletions(-) diff --git a/Documentation/usb/power-management.txt b/Documentation/usb/power-management.txt index 1392b61d6ebe..e2344baefb31 100644 --- a/Documentation/usb/power-management.txt +++ b/Documentation/usb/power-management.txt @@ -5,6 +5,25 @@ October 28, 2010 + Contents: + - + * What is Power Management? + * What is Remote Wakeup? + * When is a USB device idle? + * Forms of dynamic PM + * The user interface for dynamic PM + * Changing the default idle-delay time + * Warnings + * The driver interface for Power Management + * The driver interface for autosuspend and autoresume + * Other parts of the driver interface + * Mutual exclusion + * Interaction between dynamic PM and system PM + * xHCI hardware link PM + * USB Port Power Control + * User Interface for Port Power Control + * Suggested Userspace Port Power Policy + What is Power Management? - @@ -516,3 +535,220 @@ relevant attribute files is usb2_hardware_lpm. driver will enable hardware LPM for the device. You can write y/Y/1 or n/N/0 to the file to enable/disable USB2 hardware LPM manually. This is for test purpose mainly. + + + USB Port Power Control + -- + +In addition to suspending endpoint devices and enabling hardware +controlled link power management, the USB subsystem also has the +capability to disable power to individual ports. Power is controlled +through Set/ClearPortFeature(PORT_POWER) requests to a hub. In the case +of a root or platform-internal hub the host controller driver translates +PORT_POWER requests into platform firmware (ACPI) method calls to set +the port power state. For more background see the Linux Plumbers +Conference 2012 slides [1] and video [2]: + +Upon receiving a ClearPortFeature(PORT_POWER) request a USB port is +logically off, and may trigger the actual loss of VBUS to the port [3]. +VBUS may be maintained in the case where a hub gangs multiple ports into +a shared power well causing power to remain until all ports in the gang +are turned off. VBUS may also be maintained by hub ports configured for +a charging application. In any event a logically off port will lose +connection with its device, not respond to hotplug events, and not +respond to remote wakeup events*. + +WARNING: turning off a port may result in the inability to hot add a device. +Please see "User Interface for Port Power Control" for details. + +As far as the effect on the device itself it is similar to what a device +goes through during system suspend, i.e. the power session is lost. Any +USB device or driver that misbehaves with system suspend will be +similarly affected by a port power cycle event. For this reason the +implementation shares the same device recovery path (and honors the same +quirks) as the system resume path for the hub. + +[1]: http://dl.dropbox.com/u/96820575/sarah-sharp-lpt-port-power-off2-mini.pdf +[2]: http://linuxplumbers.ubicast.tv/videos/usb-port-power-off-kerneluserspace-api/ +[3]: USB 3.1 Section 10.12 +* wakeup note: the implementation does not allow a port connected to a + device with wakeup capability to be powered off. + + + User Interface for Port Power Control + - + +The port power control mechanism uses the PM runtime system. Poweroff is +requested by clearing the power/pm_qos_no_power_off flag of the port device +(defaults to 1). If the port is disconnected it will immediately receive a +ClearPortFeature(PORT_POWER) request. Otherwise, it will honor the pm runtime +rules and require the attached child device and all descendants to be +suspended. + +Note, some interface devices/drivers do not support autosuspend. Userspace may +need to unbind the interface drivers before the usb_device will suspend. An +unbound interface device is suspended by default. When unbinding, be careful +to unbind interface drivers, not the driver of the parent usb device. Also, +leave hub interface drivers bound. If the driver for the usb device (not +interface) is unbound the kernel is no longer able to resume the device. If a +hub interface driver is unbound, control of its child ports is lost and all +attached child-devices will disconnect. A good rule of thumb is that if the +'driver/module' link for a device points to /sys/module/usbcore then unbinding +it will interfere with port power control. + +Example of the relevant
[PATCH v4 12/14] usb: guarantee child device resume on port poweron
Make port power recovery behave similarly to the power session recovery
that occurs after a system / hub suspend event. Arrange for a
usb_device to always complete a usb_port_resume() run prior to the next
khubd run. This serves several purposes:
1/ The device may need a reset on power-session loss, without this
change port power-on recovery exposes khubd to scenarios that
usb_port_resume() is set to handle. Also, testing showed that USB3
devices that are not reset on power-session loss may eventually
downgrade their connection to the USB2 pins.
2/ This mechanism rate limits port power toggling. The minimum port
power on/off period is now gated by the child device suspend/resume
latency. This mitigates devices downgrading their connection on
perceived instability of the host connection. This ratelimiting is
really only relevant to port power control testing, but it is a nice
side effect of closing the above race.
3/ Going forward if we find that power-session recovery requires
warm-resets (http://marc.info/?t=13865923293&r=1&w=2) that is
something usb_port_resume() can drive and handle before khubd's next
evaluation of the portstatus.
4/ If the device *was* disconnected the only time we'll know for sure is
after a failed resume, so it's necessary for
usb_port_runtime_resume() to expedite a usb_port_resume() to clean up
the removed device.
1, 2, and 4 are not a problem in the system resume case because,
although the power-session is lost, khubd is frozen until after device
resume. For the runtime pm case we can use runtime-pm-synchronization
to guarantee the same sequence of events. When a ->resume_child request
is set in usb_port_runtime_resume() the port device is in the
RPM_RESUMING state. khubd executes a pm_runtime_barrier() on the port
device to flush the port recovery, holds the port active while it
resumes the child, and completes child device resume before acting on
the current portstatus.
Signed-off-by: Dan Williams
---
drivers/usb/core/hub.c | 17 +
drivers/usb/core/hub.h |2 ++
drivers/usb/core/port.c |7 +++
3 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a310028e210d..9a505978ab92 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4767,6 +4767,23 @@ static void port_event(struct usb_hub *hub, int port1)
pm_runtime_barrier(&port_dev->dev);
usb_lock_port(port_dev);
do if (pm_runtime_active(&port_dev->dev)) {
+
+ /* service child resume requests on behalf of
+* usb_port_runtime_resume()
+*/
+ if (port_dev->resume_child && udev) {
+ usb_unlock_port(port_dev);
+
+ usb_lock_device(udev);
+ usb_autoresume_device(udev);
+ usb_autosuspend_device(udev);
+ usb_unlock_device(udev);
+
+ pm_runtime_put(&port_dev->dev);
+ usb_lock_port(port_dev);
+ }
+ port_dev->resume_child = 0;
+
/* re-read portstatus now that we are in-sync with
* usb_port_{suspend|resume}
*/
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index e965474f2575..b4f397bc6957 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -91,6 +91,7 @@ struct usb_port_location {
* @status_lock: synchronize port_event() vs usb_port_{suspend|resume}
* @portnum: port index num based one
* @power_is_on: port's power state
+ * @resume_child: set at resume to sync khubd with child recovery
* @did_runtime_put: port has done pm_runtime_put().
*/
struct usb_port {
@@ -103,6 +104,7 @@ struct usb_port {
struct mutex status_lock;
u8 portnum;
unsigned power_is_on:1;
+ unsigned resume_child:1;
unsigned did_runtime_put:1;
};
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index be9c4486816a..be1e18355fec 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -105,6 +105,13 @@ static int usb_port_runtime_resume(struct device *dev)
if (retval < 0)
dev_dbg(&port_dev->dev, "can't get reconnection after
setting port power on, status %d\n",
retval);
+
+ /* keep this port awake until we have had a chance to recover
+* the child
+*/
+ pm_runtime_get_noresume(&port_dev->dev);
+ port_dev->resume_child = 1;
+ usb_kick_khubd(hdev);
retval = 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 13/14] usb: force warm reset to break resume livelock
Resuming a powered down port sometimes results in the port state being
stuck in USB_SS_PORT_LS_POLLING:
hub 3-0:1.0: debounce: port 1: total 2000ms stable 0ms status 0x2e0
port1: can't get reconnection after setting port power on, status -110
hub 3-0:1.0: port 1 status .02e0 after resume, -19
usb 3-1: can't resume, status -19
hub 3-0:1.0: logical disconnect on port 1
In the case above we wait 2 seconds for the port to reconnect and for
that entire time the port remained in the polling state. A warm reset
triggers the device to reconnect and resume as normal. With this patch
we get:
hub 3-0:1.0: debounce: port 1: total 2000ms stable 0ms status 0x2e0
usb usb3: port1 usb_port_runtime_resume requires warm reset
hub 3-0:1.0: port 1 not warm reset yet, waiting 50ms
usb 3-1: reset SuperSpeed USB device number 2 using xhci_hcd
With this in place we may want to consider reducing the timeout and
relying on warm reset for recovery. Other xHCs that fail to propagate
warm resets on hub resume may want to trigger this behavior via a quirk.
Cc: Julius Werner
Cc: Alan Stern
Cc: Vikas Sajjan
Cc: Kukjin Kim
Cc: Vincent Palatin
Cc: Lan Tianyu
Cc: Ksenia Ragiadakou
Cc: Vivek Gautam
Cc: Douglas Anderson
Cc: Felipe Balbi
Cc: Sunil Joshi
Signed-off-by: Dan Williams
---
drivers/usb/core/hub.c | 25 +
drivers/usb/core/hub.h |2 ++
drivers/usb/core/port.c | 32 ++--
3 files changed, 41 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 9a505978ab92..243cf5767433 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2512,10 +2512,12 @@ static int hub_port_reset(struct usb_hub *hub, int
port1,
/* Is a USB 3.0 port in the Inactive or Compliance Mode state?
* Port worm reset is required to recover
*/
-static bool hub_port_warm_reset_required(struct usb_hub *hub, u16 portstatus)
+static bool hub_port_warm_reset_required(struct usb_hub *hub, int port1,
+u16 portstatus)
{
return hub_is_superspeed(hub->hdev) &&
- (((portstatus & USB_PORT_STAT_LINK_STATE) ==
+ (test_bit(port1, hub->warm_reset_bits) ||
+ ((portstatus & USB_PORT_STAT_LINK_STATE) ==
USB_SS_PORT_LS_SS_INACTIVE) ||
((portstatus & USB_PORT_STAT_LINK_STATE) ==
USB_SS_PORT_LS_COMP_MOD)) ;
@@ -2555,7 +2557,7 @@ static int hub_port_wait_reset(struct usb_hub *hub, int
port1,
if ((portstatus & USB_PORT_STAT_RESET))
return -EBUSY;
- if (hub_port_warm_reset_required(hub, portstatus))
+ if (hub_port_warm_reset_required(hub, port1, portstatus))
return -ENOTCONN;
/* Device went away? */
@@ -2654,8 +2656,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
if (status < 0)
goto done;
- if (hub_port_warm_reset_required(hub, portstatus))
+ if (hub_port_warm_reset_required(hub, port1, portstatus))
warm = true;
+ clear_bit(port1, hub->warm_reset_bits);
}
/* Reset the port */
@@ -2693,7 +2696,8 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
&portstatus, &portchange) < 0)
goto done;
- if (!hub_port_warm_reset_required(hub, portstatus))
+ if (!hub_port_warm_reset_required(hub, port1,
+ portstatus))
goto done;
/*
@@ -2766,8 +2770,13 @@ static int check_port_resume_type(struct usb_device
*udev,
struct usb_hub *hub, int port1,
int status, unsigned portchange, unsigned portstatus)
{
+ /* Is a warm reset needed to recover the connection? */
+ if (udev->reset_resume
+ && hub_port_warm_reset_required(hub, port1, portstatus)) {
+ /* pass */;
+ }
/* Is the device still present? */
- if (status || port_is_suspended(hub, portstatus) ||
+ else if (status || port_is_suspended(hub, portstatus) ||
!port_is_power_on(hub, portstatus) ||
!(portstatus & USB_PORT_STAT_CONNECTION)) {
if (status >= 0)
@@ -3911,7 +3920,7 @@ int hub_port_debounce(struct usb_hub *hub, int port1,
bool must_be_connected)
"debounce: port %d: total %dms stable %dms status 0x%x\n",
port1, total_time, stable_time, portstatus);
- if (stable_time < HUB_DEBOUNCE_STABLE)
+ if (stable_time < HUB_DEBOUNCE_STABLE && !must_be_connected)
return -ETIMEDOUT;
return portstatus;
}
@@ -4796,7 +4805,7 @@ static void port_event(struct usb_hub *hub, int port1)
/* Warm reset a U
Re: usb audio breaks ohci-pci
On Thu, 30 Jan 2014, Dennis New wrote: > Indeed, "ohci_quirk_zfmicro" (as mentioned in that marc.info link) > would crash my kernel/system (I think when some graphics switch would > happen) :). So I tried "ohci_quirk_amd700", since there were already > some PCI_VENDOR_ID_ATI quirks in ohci-pci.c that were using it, but > alas, no luck. After about 6 days, the hang/crash happened again. I > wasn't able to get debugging info this time around :s. > > The mystery continues. Hmmm. Looking again at the data you collected, it appears that those quirks are not going to help. _Something_ has gone wrong, but it's hard to tell what. At first I thought maybe the OHCI controller had simply stopped generating interrupt requests, but that doesn't seem to agree with the debugging output. I'll have to write a diagnostic patch to get more information. Unfortunately the next few days are going to be pretty busy, so I won't be able to get to it for a while. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb: move hub init and LED blink work to power efficient workqueue
On Fri, 31 Jan 2014, Zoran Markovic wrote:
> From: Shaibal Dutta
>
> Allow the scheduler to select the best CPU to handle hub initalization
> and LED blinking work. This extends idle residency times on idle CPUs
> and conserves power.
>
> This functionality is enabled when CONFIG_WQ_POWER_EFFICIENT is selected.
...
> @@ -1046,8 +1047,9 @@ static void hub_activate(struct usb_hub *hub, enum
> hub_activation_type type)
> if (type == HUB_INIT) {
> delay = hub_power_on(hub, false);
> PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func2);
> - schedule_delayed_work(&hub->init_work,
> - msecs_to_jiffies(delay));
> + queue_delayed_work(system_power_efficient_wq,
> +&hub->init_work,
> +msecs_to_jiffies(delay));
I have no objection to selecting the power-efficient work queue, but
please don't change the indentation of the existing code. The style
used throughout (most of) the USB core is that continuation lines are
indented by two tab stops more than the first line of the statement.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] Move DWC2 driver out of staging
On 01/31/2014 11:12 AM, Andre Heider wrote:
> On Mon, Jan 13, 2014 at 01:50:09PM -0800, Paul Zimmerman wrote:
>> The DWC2 driver should now be in good enough shape to move out of
>> staging. I have stress tested it overnight on RPI running mass
>> storage and Ethernet transfers in parallel, and for several days
>> on our proprietary PCI-based platform.
...
> this looks just fine, but for whatever reason it breaks sdhci on my rpi.
> With today's Linus' master the dwc2 controller seems to initialize fine,
> but I get this upon boot:
>
> [1.783316] sdhci-bcm2835 2030.sdhci: sdhci_pltfm_init failed -12
> [1.794820] sdhci-bcm2835: probe of 2030.sdhci failed with error -12
>
> That is:
>
> struct sdhci_host *sdhci_pltfm_init(struct platform_device
> *pdev,
> const struct
> sdhci_pltfm_data *pdata,
> size_t priv_size)
> {
> ...
>
> iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!iomem) {
> ret = -ENOMEM;
> goto err;
> }
This is due to the following code:
static void _dwc2_hcd_endpoint_reset(struct usb_hcd *hcd,
struct usb_host_endpoint *ep)
{
struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
...
struct usb_device *udev;
...
udev = to_usb_device(hsotg->dev);
...
usb_settoggle(udev, epnum, is_out, 0);
if (is_control)
usb_settoggle(udev, epnum, !is_out, 0);
The problem is that hsotg->dev is assigned as follows:
static int dwc2_driver_probe(struct platform_device *dev)
...
hsotg = devm_kzalloc(&dev->dev, sizeof(*hsotg), GFP_KERNEL);
...
hsotg->dev = &dev->dev;
As such, it's not legal to call to_usb_device() on it.
What ends up happening, simply due to memory allocation order, is that
the memory writes inside usb_settoggle() end up setting the SDHCI struct
platform_device's num_resources to 0, so that it's call to
platform_get_resource() fails.
With the DWC2 move patch reverted, some other random piece of memory is
being corrupted, which just happens not to cause any visible problem.
Likely it's some other struct platform_device that's already had its
resources read by the time DWC2 probes and corrupts them.
(Yes, this was hard to find!)
I honestly can't see how to solve this myself, since the whole DWC2
driver doesn't seem to have a struct usb_device * hanging around that we
can stash somewhere for it to look up later. Perhaps someone more
familiar with the USB stack can help with 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 RFC 1/1] usb: Tell xhci when usb data might be misaligned
On Sat, Feb 1, 2014 at 3:00 AM, Sarah Sharp wrote: > On Fri, Jan 31, 2014 at 08:17:58AM +0800, Ming Lei wrote: >> On Fri, Jan 31, 2014 at 6:15 AM, Sarah Sharp >> wrote: >> > On Thu, Jan 30, 2014 at 10:50:21PM +0100, Bjørn Mork wrote: >> >> FWIW, the plan looks fine to me. Just adding a couple of hints to >> >> simplify the implementation. >> >> >> >> Sarah Sharp writes: >> >> >> >> > Let's do this fix the right way, instead of wall papering over the >> >> > issue. Here's what we should do: >> >> > >> >> > 1. Disable scatter-gather for the ax88179_178a driver when it's under an >> >> >xHCI host. >> >> >> >> No need to make this conditional. SG is only enabled in the >> >> ax88179_178a driver if udev->bus->no_sg_constraint is true, so it >> >> applies only to xHCI hosts in the first place. >> > >> > Ah, so you're suggesting just reverting commit >> > 3804fad45411b48233b48003e33a78f290d227c8 "USBNET: ax88179_178a: enable >> > tso if usb host supports sg dma"? >> >> If I understand the problem correctly, the current issue is that xhci driver >> doesn't support the arbitrary dma length not well, but per XHCI spec, it >> should be supported, right? >> >> If the above is correct, reverting the commit isn't correct since there isn't >> any issue about the commit, so I suggest to disable the flag in xhci >> for the buggy devices, and it may be enabled again if the problem is fixed. > > Ok, I like that plan, since it means I don't have to touch any > networking code to fix this. :) > > I believe that means we'll have to disable the flag for all 1.0 xHCI > hosts, since those are the ones that need TD fragments. > >> >> > 2. Revert the following commits: >> >> >f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block >> >> > writes. >> >> >d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many >> >> > trbs >> >> >35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload >> >> > burst >> >> > >> >> > 3. Dan and Mathias can work together to come up with an overall plan to >> >> >change the xHCI driver architecture to be fully compliant with the TD >> >> >fragment rules. That can be done over the next few kernel releases. >> >> > >> >> > The end result is that we don't destabilize storage or break userspace >> >> > USB drivers, we don't break people's xHCI host controllers, >> >> > the ax88179_178a USB ethernet devices still work under xHCI (a bit with >> >> > worse performance), and other USB ethernet devices still get the >> >> > performance improvement introduced in 3.12. >> >> >> >> No other usbnet drivers has enabled SG... Which is why you have only >> >> seen this problem with the ax88179_178a devices. So there is no >> >> performance improvement to keep. >> >> In my test environment, the patch does improve both throughput and >> cpu utilization, if you search the previous email for the patch, you can >> see the data. With SG enabled, for the iperf client test case, the average urb size for transmission will be increased from ~1500 to ~20K bytes in my test case: iperf -c $SRV -t 30 -P 4 -w 128K So I am wondering you guys do not care the improvement, maybe the CPU is powerful enough to not degrade throughout&cpu utilization not much, but there is still the potential CPU wakeup issue, which means extra CPU power consumption might be introduced after disabling SG for usbnet. > > Right, I did see the performance improvement note in that commit. Do > you know if the ARM A15 dual core board was using a 0.96 xHCI host, or a > 1.0 host? You can find out by reloading the xHCI driver with dynamic > debugging turned on: > > # sudo modprobe xhci_hcd dyndbg Looks I can't find the parameter 'dyndbg' for xhci_hcd. > and then look for lines like: > > [25296.765767] xhci_hcd :00:14.0: HCIVERSION: 0x100 I change xhci_dbg.c manually with below: diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c index b016d38..1ae1966 100644 --- a/drivers/usb/host/xhci-dbg.c +++ b/drivers/usb/host/xhci-dbg.c @@ -66,7 +66,7 @@ static void xhci_print_cap_regs(struct xhci_hcd *xhci) (unsigned int) temp); xhci_dbg(xhci, "CAPLENGTH: 0x%x\n", (unsigned int) HC_LENGTH(temp)); - xhci_dbg(xhci, "HCIVERSION: 0x%x\n", + dev_info(xhci_to_hcd(xhci)->self.controller, "HCIVERSION: 0x%x\n", (unsigned int) HC_VERSION(temp)); and got the below output: [tom@ming ~]$ dmesg | grep HCIVERSION xhci-hcd xhci-hcd.2.auto: HCIVERSION: 0x100 Thanks, -- Ming Lei -- 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
