Re: [PATCH 1/2] USB: c67x00: move URB private data allocation from under spinlock
> "Max" == Max Filippov writes: > This fixes the following warning: > BUG: sleeping function called from invalid context at mm/slub.c:940 > in_atomic(): 1, irqs_disabled(): 1, pid: 17, name: khubd > CPU: 0 PID: 17 Comm: khubd Not tainted 3.12.0-4-g938dd60-dirty #1 >__might_sleep+0xbe/0xc0 >kmem_cache_alloc_trace+0x36/0x170 >c67x00_urb_enqueue+0x5c/0x254 >usb_hcd_submit_urb+0x66e/0x724 >usb_submit_urb+0x2ac/0x308 >usb_start_wait_urb+0x2c/0xb8 >usb_control_msg+0x8c/0xa8 >hub_port_init+0x191/0x718 >hub_thread+0x804/0xe14 >kthread+0x72/0x78 >ret_from_kernel_thread+0x8/0xc > Signed-off-by: Max Filippov Acked-by: Peter Korsgaard -- Bye, Peter Korsgaard -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] USB: c67x00: add proper delays to HPI read/write
> "Max" == Max Filippov writes:
> According to CY7C67300 specification HPI read and write cycle duration
> Tcyc must be at least 6T long, where T is 1/48MHz, which is 125ns.
> Without this delay fast host processor cannot write to chip registers.
> Add proper ndelay to hpi_{read,write}_reg.
It would be good to add a sensible named define (with the above
description) as a comment and use that instead of the magic 125 constant
in the code.
With that fixed:
Acked-by: Peter Korsgaard
--
Bye, Peter Korsgaard
--
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: Using both usbmon and ftrace?
Hi Alan,
Many thanks for your response!
On 2013-12-25 02:49, Alan Stern wrote:
>
> This was a long and rather rambling message.
Sorry about that, it was my attempt at being detailed :)
> The main points I
> gathered from it were:
>
> You are using a very old kernel version. You really should use
> something more up-to-date, if at all possible.
>
Yes; however, my current project is a part of a series that were done on those
kernel versions, so I want to keep the kernel version the same.
> You are trying to combine ftrace with usbmon. I know extremely
> little about ftrace, so I can't help you there.
>
Yes, I am.
> You want to know how to correlate kernel time values with the
> timestamps produced by usbmon.
>
Yes, I do.
> The last part is easy enough to do. You can call the following
> subroutine to get a usbmon-style timestamp value, which can then be
> added to an ftrace message or simply printed in the kernel log:
Fantastic, that will do!
Many thanks again,
Cheers!
>
> #include
>
> static unsigned usbmon_timestamp(void)
> {
> struct timeval tval;
> unsigned stamp;
>
> do_gettimeofday(&tval);
> stamp = tval.tv_sec & 0xFFF;
> stamp = stamp * 100 + tval.tv_usec;
> return stamp;
> }
>
> For example,
>
> pr_info("The usbmon time is: %u\n", usbmon_timestamp());
>
> Alan Stern
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo-u79uwxl29ty76z2rm5m...@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/2] Fixes for CY7C67x00 host controller
Hello, there are two fixes for the CY7C67x00 host controller: one fixes memory allocation with non-atomic flags in the atomic context, the other adds proper delays to the register access methods which makes possible to use the chip with fast host CPU. Changes v1 -> v2: - add HPI_T_CYC_NS macro with a comment why it is needed; - add Peter's Acked-bys. Thanks for the review, Peter. Max Filippov (2): USB: c67x00: move URB private data allocation from under spinlock USB: c67x00: add proper delays to HPI read/write drivers/usb/c67x00/c67x00-ll-hpi.c | 10 ++ drivers/usb/c67x00/c67x00-sched.c | 18 +- 2 files changed, 19 insertions(+), 9 deletions(-) -- 1.8.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] USB: c67x00: move URB private data allocation from under spinlock
This fixes the following warning:
BUG: sleeping function called from invalid context at mm/slub.c:940
in_atomic(): 1, irqs_disabled(): 1, pid: 17, name: khubd
CPU: 0 PID: 17 Comm: khubd Not tainted 3.12.0-4-g938dd60-dirty #1
__might_sleep+0xbe/0xc0
kmem_cache_alloc_trace+0x36/0x170
c67x00_urb_enqueue+0x5c/0x254
usb_hcd_submit_urb+0x66e/0x724
usb_submit_urb+0x2ac/0x308
usb_start_wait_urb+0x2c/0xb8
usb_control_msg+0x8c/0xa8
hub_port_init+0x191/0x718
hub_thread+0x804/0xe14
kthread+0x72/0x78
ret_from_kernel_thread+0x8/0xc
Signed-off-by: Max Filippov
Acked-by: Peter Korsgaard
---
drivers/usb/c67x00/c67x00-sched.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/c67x00/c67x00-sched.c
b/drivers/usb/c67x00/c67x00-sched.c
index aa49162..d9cfe50 100644
--- a/drivers/usb/c67x00/c67x00-sched.c
+++ b/drivers/usb/c67x00/c67x00-sched.c
@@ -372,6 +372,13 @@ int c67x00_urb_enqueue(struct usb_hcd *hcd,
struct c67x00_hcd *c67x00 = hcd_to_c67x00_hcd(hcd);
int port = get_root_port(urb->dev)-1;
+ /* Allocate and initialize urb private data */
+ urbp = kzalloc(sizeof(*urbp), mem_flags);
+ if (!urbp) {
+ ret = -ENOMEM;
+ goto err_urbp;
+ }
+
spin_lock_irqsave(&c67x00->lock, flags);
/* Make sure host controller is running */
@@ -384,13 +391,6 @@ int c67x00_urb_enqueue(struct usb_hcd *hcd,
if (ret)
goto err_not_linked;
- /* Allocate and initialize urb private data */
- urbp = kzalloc(sizeof(*urbp), mem_flags);
- if (!urbp) {
- ret = -ENOMEM;
- goto err_urbp;
- }
-
INIT_LIST_HEAD(&urbp->hep_node);
urbp->urb = urb;
urbp->port = port;
@@ -453,11 +453,11 @@ int c67x00_urb_enqueue(struct usb_hcd *hcd,
return 0;
err_epdata:
- kfree(urbp);
-err_urbp:
usb_hcd_unlink_urb_from_ep(hcd, urb);
err_not_linked:
spin_unlock_irqrestore(&c67x00->lock, flags);
+ kfree(urbp);
+err_urbp:
return ret;
}
--
1.8.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] USB: c67x00: add proper delays to HPI read/write
According to CY7C67300 specification HPI read and write cycle duration
Tcyc must be at least 6T long, where T is 1/48MHz, which is 125ns.
Without this delay fast host processor cannot write to chip registers.
Add proper ndelay to hpi_{read,write}_reg.
Signed-off-by: Max Filippov
Acked-by: Peter Korsgaard
---
Changes v1 -> v2:
- add HPI_T_CYC_NS macro with a comment why it is needed
drivers/usb/c67x00/c67x00-ll-hpi.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/usb/c67x00/c67x00-ll-hpi.c
b/drivers/usb/c67x00/c67x00-ll-hpi.c
index 3a1ca4d..cd2c6ed 100644
--- a/drivers/usb/c67x00/c67x00-ll-hpi.c
+++ b/drivers/usb/c67x00/c67x00-ll-hpi.c
@@ -22,6 +22,7 @@
*/
#include
+#include
#include
#include
#include
@@ -73,13 +74,22 @@ struct c67x00_lcp_int_data {
#define HPI_ADDR 2
#define HPI_STATUS 3
+/*
+ * According to CY7C67300 specification (tables 140 and 141) HPI read and
+ * write cycle duration Tcyc must be at least 6T long, where T is 1/48MHz,
+ * which is 125ns.
+ */
+#define HPI_T_CYC_NS 125
+
static inline u16 hpi_read_reg(struct c67x00_device *dev, int reg)
{
+ ndelay(HPI_T_CYC_NS);
return __raw_readw(dev->hpi.base + reg * dev->hpi.regstep);
}
static inline void hpi_write_reg(struct c67x00_device *dev, int reg, u16 value)
{
+ ndelay(HPI_T_CYC_NS);
__raw_writew(value, dev->hpi.base + reg * dev->hpi.regstep);
}
--
1.8.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] usb/core: fix NULL pointer dereference in recursively_mark_NOTATTACHED
On Tue, 24 Dec 2013, Alan Stern wrote: > > This is my analysis, how do you think? > > There is another reason why usb_hub_to_struct_hub() could return NULL: > if usb_get_intfdata(hdev->actconfig->interface[0]) is NULL. This could > happen if recursively_mark_NOTATTACHED() is called _while_ > hub_disconnect() is running. > > There should be locking to prevent this, but there isn't. That's what > we need to fix. It's not an easy problem. Can you figure out a > correct solution? I think this will fix it. Take a close look and do some careful testing. Alan Stern Index: usb-3.13/drivers/usb/core/hub.c === --- usb-3.13.orig/drivers/usb/core/hub.c +++ usb-3.13/drivers/usb/core/hub.c @@ -1622,11 +1622,14 @@ static void hub_disconnect(struct usb_in hub->error = 0; hub_quiesce(hub, HUB_DISCONNECT); - usb_set_intfdata (intf, NULL); - for (i = 0; i < hdev->maxchild; i++) usb_hub_remove_port_device(hub, i + 1); + + /* Avoid races with recursively_mark_NOTATTACHED() */ + spin_lock_irq(&device_state_lock); hub->hdev->maxchild = 0; + usb_set_intfdata(intf, NULL); + spin_unlock_irq(&device_state_lock); if (hub->hdev->speed == USB_SPEED_HIGH) highspeed_hubs--; -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: xhci_hcd and Canon Lide 110 not playing well together
Hey Alan,
On Mo, 2013-12-23 at 15:38 -0500, Alan Stern wrote:
> On Mon, 23 Dec 2013, Matthias [ISO-8859-1] Blsing wrote:
> > Looking at the strace alone, I see, that the USBDEVFS_SETCONFIGURATION
> > ioctl calls take excessive time.
> >
>
> You're right; those calls took an excessively long time. The usbmon
> traces show that the communication with the device was very quick. I
> don't know where the rest of the time was spent.
>
> The most likely possibility is locking the bandwidth mutex. But that
> wouldn't require much time either, unless something else was going on
> at the same time.
>
> You could try adding a few statements of the form
>
> dev_info(&dev->dev, "[Your message here]\n");
>
> at various spots inside drivers/usb/core/message.c's
> usb_reset_configuration() routine. Maybe you'll be able to see where
> everything slows down. (Be sure to set CONFIG_PRINTK_TIME when you
> build the kernel.)
>
The slow down can be tracked to:
usb_enable_interface(dev, intf, true);
I springled comments like this:
dev_info(&dev->dev, "T1\n");
/* re-init hc/hcd interface/endpoint state */
for (i = 0; i < config->desc.bNumInterfaces; i++) {
struct usb_interface *intf = config->interface[i];
struct usb_host_interface *alt;
dev_info(&dev->dev, "T2-1\n");
alt = usb_altnum_to_altsetting(intf, 0);
dev_info(&dev->dev, "T2-2\n");
/* No altsetting 0? We'll assume the first altsetting.
* We could use a GetInterface call, but if a device is
* so non-compliant that it doesn't have altsetting 0
* then I wouldn't trust its reply anyway.
*/
if (!alt)
alt = &intf->altsetting[0];
if (alt != intf->cur_altsetting) {
dev_info(&dev->dev, "T2-X1\n");
remove_intf_ep_devs(intf);
dev_info(&dev->dev, "T2-X2\n");
usb_remove_sysfs_intf_files(intf);
dev_info(&dev->dev, "T2-X3\n");
}
intf->cur_altsetting = alt;
dev_info(&dev->dev, "T2-3\n");
usb_enable_interface(dev, intf, true);
dev_info(&dev->dev, "T2-4\n");
if (device_is_registered(&intf->dev)) {
dev_info(&dev->dev, "T2-5\n");
usb_create_sysfs_intf_files(intf);
dev_info(&dev->dev, "T2-6\n");
create_intf_ep_devs(intf);
dev_info(&dev->dev, "T2-7\n");
}
}
dev_info(&dev->dev, "T3\n");
And got the attached log (minus my inline comments). As you can see it
starts in the 5. call there it starts with a 5 second delay betewen T2-3
and T2-4. In the next round 15s is reached and there it stays.
My tests of a second run support this and are finalized by xsane
freezing and the final GPF.
Maybe this helps.
Greetings
Matthias
=> Start
Dec 25 21:16:09 athena kernel: [ 104.886377] usb 3-2: T1
Dec 25 21:16:09 athena kernel: [ 104.886380] usb 3-2: T2-1
Dec 25 21:16:09 athena kernel: [ 104.886381] usb 3-2: T2-2
Dec 25 21:16:09 athena kernel: [ 104.886382] usb 3-2: T2-3
Dec 25 21:16:09 athena kernel: [ 104.886647] usb 3-2: T2-4
Dec 25 21:16:09 athena kernel: [ 104.886648] usb 3-2: T2-5
Dec 25 21:16:09 athena kernel: [ 104.886649] usb 3-2: T2-6
Dec 25 21:16:09 athena kernel: [ 104.886650] usb 3-2: T2-7
Dec 25 21:16:09 athena kernel: [ 104.886651] usb 3-2: T3
Dec 25 21:16:15 athena kernel: [ 111.329474] usb 3-2: T1
Dec 25 21:16:15 athena kernel: [ 111.329478] usb 3-2: T2-1
Dec 25 21:16:15 athena kernel: [ 111.329479] usb 3-2: T2-2
Dec 25 21:16:15 athena kernel: [ 111.329481] usb 3-2: T2-3
Dec 25 21:16:15 athena kernel: [ 111.329756] usb 3-2: T2-4
Dec 25 21:16:15 athena kernel: [ 111.329758] usb 3-2: T2-5
Dec 25 21:16:15 athena kernel: [ 111.329760] usb 3-2: T2-6
Dec 25 21:16:15 athena kernel: [ 111.329762] usb 3-2: T2-7
Dec 25 21:16:15 athena kernel: [ 111.329763] usb 3-2: T3
Dec 25 21:16:15 athena kernel: [ 111.390151] usb 3-2: T1
Dec 25 21:16:15 athena kernel: [ 111.390154] usb 3-2: T2-1
Dec 25 21:16:15 athena kernel: [ 111.390155] usb 3-2: T2-2
Dec 25 21:16:15 athena kernel: [ 111.390156] usb 3-2: T2-3
Dec 25 21:16:15 athena kernel: [ 111.390428] usb 3-2: T2-4
Dec 25 21:16:15 athena kernel: [ 111.390429] usb 3-2: T2-5
Dec 25 21:16:15 athena kernel: [ 111.390430] usb 3-2: T2-6
Dec 25 21:16:15 athena kernel: [ 111.390431] usb 3-2: T2-7
Dec 25 21:16:15 athena kernel: [ 111.390432] usb 3-2: T3
Dec 25 21:16:15 athena kernel: [ 111.470847] usb 3-2: T1
Dec 25 21:16:15 athena kernel: [ 111.470850] usb 3-2: T2-1
Dec 25 21:16:15 athena kernel: [ 111.470851] usb 3-2: T2-2
Dec 25 21:16:15 athena kernel: [ 111.470852] usb 3-2: T2-3
Dec 25 21:16:15 athe
RE: [PATCH] usb/core: fix NULL pointer dereference in recursively_mark_NOTATTACHED
> On Tue, 24 Dec 2013, Alan Stern wrote: > I think this will fix it. Take a close look and do some careful testing. > > Alan Stern > > Index: usb-3.13/drivers/usb/core/hub.c > == > = > --- usb-3.13.orig/drivers/usb/core/hub.c > +++ usb-3.13/drivers/usb/core/hub.c > @@ -1622,11 +1622,14 @@ static void hub_disconnect(struct usb_in > hub->error = 0; > hub_quiesce(hub, HUB_DISCONNECT); > > - usb_set_intfdata (intf, NULL); > - > for (i = 0; i < hdev->maxchild; i++) > usb_hub_remove_port_device(hub, i + 1); > + > + /* Avoid races with recursively_mark_NOTATTACHED() */ > + spin_lock_irq(&device_state_lock); > hub->hdev->maxchild = 0; > + usb_set_intfdata(intf, NULL); > + spin_unlock_irq(&device_state_lock); > > if (hub->hdev->speed == USB_SPEED_HIGH) > highspeed_hubs--; > Sorry for late response. Agree with you. I will test your patch carefully and let you know the result. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: xhci_hcd and Canon Lide 110 not playing well together
On Wed, 25 Dec 2013, Matthias [ISO-8859-1] Bl�sing wrote:
> The slow down can be tracked to:
>
> usb_enable_interface(dev, intf, true);
>
> I springled comments like this:
>
>
> dev_info(&dev->dev, "T1\n");
> /* re-init hc/hcd interface/endpoint state */
> for (i = 0; i < config->desc.bNumInterfaces; i++) {
> struct usb_interface *intf = config->interface[i];
> struct usb_host_interface *alt;
> dev_info(&dev->dev, "T2-1\n");
>
> alt = usb_altnum_to_altsetting(intf, 0);
> dev_info(&dev->dev, "T2-2\n");
>
> /* No altsetting 0? We'll assume the first altsetting.
>* We could use a GetInterface call, but if a device is
>* so non-compliant that it doesn't have altsetting 0
>* then I wouldn't trust its reply anyway.
>*/
> if (!alt)
> alt = &intf->altsetting[0];
>
> if (alt != intf->cur_altsetting) {
> dev_info(&dev->dev, "T2-X1\n");
> remove_intf_ep_devs(intf);
> dev_info(&dev->dev, "T2-X2\n");
> usb_remove_sysfs_intf_files(intf);
> dev_info(&dev->dev, "T2-X3\n");
> }
> intf->cur_altsetting = alt;
> dev_info(&dev->dev, "T2-3\n");
> usb_enable_interface(dev, intf, true);
> dev_info(&dev->dev, "T2-4\n");
> if (device_is_registered(&intf->dev)) {
> dev_info(&dev->dev, "T2-5\n");
> usb_create_sysfs_intf_files(intf);
> dev_info(&dev->dev, "T2-6\n");
> create_intf_ep_devs(intf);
> dev_info(&dev->dev, "T2-7\n");
> }
> }
> dev_info(&dev->dev, "T3\n");
>
> And got the attached log (minus my inline comments). As you can see it
> starts in the 5. call there it starts with a 5 second delay betewen T2-3
> and T2-4. In the next round 15s is reached and there it stays.
>
> My tests of a second run support this and are finalized by xsane
> freezing and the final GPF.
>
> Maybe this helps.
Okay, now we know that usb_enable_interface takes a long time. That
routine does nothing but call usb_enable_endpoint, which does nothing
but call usb_hcd_reset_endpoint, which calls the xhci_endpoint_reset
routine.
So we have traced the problem into xhci-hcd. You could trace it
farther down if you want, but at this point it probably would be more
productive to see what Sarah has to say. She may know about some new
changes that are not yet available in the development kernel.
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
