fix some hubs and hid devices at startup

PATCH Johannes Erdfelt 4 participants 12 messages Mar 11, 2002 – May 15, 2002 USB

Share & tools

Johannes Erdfelt

author

2002-03-11 22:08 UTC #0 lore raw

Quoted 6 lines · Greg KH

Since the latest USB patch went in that added a proper delay to the hub connection sequence, a lot of people have been reporting odd problems with some USB hubs, keyboards, and mice. I've seen this problem too. It is usually fixed by just replugging the device in, but not all of the time.

Quoted 8 lines

The reason seems to be that usbmodules is getting called from the hotplug package at the same time the hub driver is talking to the device. Since we don't have control endpoint locking in the kernel USB code yet, this messes things up :)
The patch below, from Olaf Hering, seems to fix the problem. Also just removing the usbmodules binary seems to fix it for other people, but that's not really a good fix.

The hub driver doesn't talk to devices, so I'll assume you mean the hub thread, which is actually calling the core for enumeration (setting address, fetching descriptors, etc)

But the enumeration should be finished by the time the hotplug stuff finishes.

In that case, nothing should be stomping over each other.

Is it that devices don't like how quickly we let the driver talk to it after enumeration?

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

David Brownell

2002-03-12 00:20 UTC #1 lore raw

Johannes replied to Greg:

Quoted 10 lines

Quoted 5 lines
Since the latest USB patch went in that added a proper delay to the hub connection sequence, a lot of people have been reporting odd problems with some USB hubs, keyboards, and mice. I've seen this problem too. It is usually fixed by just replugging the device in, but not all of the time.
.... deleted patch (adds delay in userland) and comments ....
Is it that devices don't like how quickly we let the driver talk to it after enumeration?

I'm likewise puzzled by this. Were these devices that worked previously? If so it'd seem to be the kernel "delay" patch that's at fault somehow. And if not, it'd seem like that "delay" patch doesn't work quite well enough on those devices (which might be more marginal than it seems at first).

It doesn't make sense to me that if more delays are added in the kernel code, userland needs to see even more.

- Dave

Greg KH

2002-03-12 20:27 UTC #2 lore raw

In reply to Johannes Erdfelt

Quoted 4 lines · Johannes Erdfelt

The hub driver doesn't talk to devices, so I'll assume you mean the hub thread, which is actually calling the core for enumeration (setting address, fetching descriptors, etc)

Sorry, yes this is what I meant.

Quoted 4 lines

But the enumeration should be finished by the time the hotplug stuff finishes.
In that case, nothing should be stomping over each other.

In looking at this some more I agree, it doesn't look like this could race with the hub thread.

Quoted 2 lines

Is it that devices don't like how quickly we let the driver talk to it after enumeration?

Ah, this is probably more likely.

The problem looks like the usbdevfs calls are happening at the same time the hid driver is also talking to the device.

Olaf, is this what your log messages show?

thanks,

greg k-h

Greg KH

2002-03-12 20:29 UTC #3 lore raw

In reply to Johannes Erdfelt

Quoted 9 lines · David Brownell

I'm likewise puzzled by this. Were these devices that worked previously? If so it'd seem to be the kernel "delay" patch that's at fault somehow. And if not, it'd seem like that "delay" patch doesn't work quite well enough on those devices (which might be more marginal than it seems at first).
It doesn't make sense to me that if more delays are added in the kernel code, userland needs to see even more.

I agree, but Olaf and others prove that this works.

I'm more puzzled why usbmodules is needing to talk to a device. I thought usbmodules was only for cold-plug stuff. Have any ideas?

thanks,

greg k-h

David Brownell

2002-03-12 22:15 UTC #4 lore raw

In reply to Johannes Erdfelt

Quoted 4 lines

Quoted 2 lines
It doesn't make sense to me that if more delays are added in the kernel code, userland needs to see even more.
I agree, but Olaf and others prove that this works.

There was that hypothesis that it's really control traffic interference ... I notice that usb_control_msg() chose against the "keep the setup data in struct usb_device and lock it with a semaphore" strategy (it kmallocs the setup buffer), so I could see how that'd happen.

"usbmodules" asks the device for its config descriptor, rather than reading /proc/bus/usb/BBB/DDD, so HCDs that don't handle control queuing (I think OHCI behaves there, and EHCI, but not the UHCIs) might suffer from concurrent control messages from it and from HID ...

I think the best that can be done for now would be a
hack: add a lock in usb_device that must be held by whoever submits a control request, and released when that request completes. The non-hack solution is to support control queuing in all HCDs.

What kind of lock? Maybe that binary semaphore. Code that needs to issue control requests in_interrupt() would need to be modified, as would usb_control_msg().

Quoted 2 lines

I'm more puzzled why usbmodules is needing to talk to a device. I thought usbmodules was only for cold-plug stuff. Have any ideas?

Usbmodules is used to cope with /sbin/hotplug not providing a complete interface descriptor dump: it only shows the first of N inferface descriptors. With coldplug, it doesn't run, and doesn't provide any descriptors at all -- so it must be used.

However, even for the hotplug case "usbmodules" gives a more complete module list. In the case of an audio device with a sound/ISO interface and a control/HID interface, it'll pick up the HID.

- Dave

Greg KH

2002-03-12 22:52 UTC #5 lore raw

In reply to Johannes Erdfelt

Quoted 10 lines · David Brownell

Quoted 4 lines
Quoted 2 lines
It doesn't make sense to me that if more delays are added in the kernel code, userland needs to see even more.
I agree, but Olaf and others prove that this works.
There was that hypothesis that it's really control traffic interference ... I notice that usb_control_msg() chose against the "keep the setup data in struct usb_device and lock it with a semaphore" strategy (it kmallocs the setup buffer), so I could see how that'd happen.

It's always been this way, right?

Quoted 12 lines

"usbmodules" asks the device for its config descriptor, rather than reading /proc/bus/usb/BBB/DDD, so HCDs that don't handle control queuing (I think OHCI behaves there, and EHCI, but not the UHCIs) might suffer from concurrent control messages from it and from HID ...
I think the best that can be done for now would be a
hack: add a lock in usb_device that must be held by whoever submits a control request, and released when that request completes. The non-hack solution is to support control queuing in all HCDs.

Quoted 3 lines

What kind of lock? Maybe that binary semaphore. Code that needs to issue control requests in_interrupt() would need to be modified, as would usb_control_msg().

Yes, this would be the best way. I don't think it's a hack, as some drivers (like the usb-storage one) have said that they really need to lock the control pipe for a while to prevent things from happening out of order (for very dumb devices.)

Quoted 9 lines

Usbmodules is used to cope with /sbin/hotplug not providing a complete interface descriptor dump: it only shows the first of N inferface descriptors. With coldplug, it doesn't run, and doesn't provide any descriptors at all -- so it must be used.
However, even for the hotplug case "usbmodules" gives a more complete module list. In the case of an audio device with a sound/ISO interface and a control/HID interface, it'll pick up the HID.

Ah, thanks for explaining this, makes more sense now.

greg k-h

David Brownell

2002-03-13 01:52 UTC #6 lore raw

In reply to Johannes Erdfelt

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
It doesn't make sense to me that if more delays are added in the kernel code, userland needs to see even more.
I agree, but Olaf and others prove that this works.

Quoted 7 lines

Quoted 5 lines
There was that hypothesis that it's really control traffic interference ... I notice that usb_control_msg() chose against the "keep the setup data in struct usb_device and lock it with a semaphore" strategy (it kmallocs the setup buffer), so I could see how that'd happen.
It's always been this way, right?

No, the kmalloc() is newish. Previously it did stack allocation (bad since dma-from-stack isn't sure to work on all systems). I wonder whether adding semaphore plus eight bytes of buffer (vs just semaphore) would even make the "struct usb_device" objects go into a different kmalloc cache ... :)

Quoted 8 lines

Quoted 8 lines
...
I think the best that can be done for now would be a
hack: add a lock in usb_device that must be held by whoever submits a control request, and released when that request completes. The non-hack solution is to support control queuing in all HCDs.

Quoted 8 lines

Quoted 3 lines
What kind of lock? Maybe that binary semaphore. Code that needs to issue control requests in_interrupt() would need to be modified, as would usb_control_msg().
Yes, this would be the best way. I don't think it's a hack, as some drivers (like the usb-storage one) have said that they really need to lock the control pipe for a while to prevent things from happening out of order (for very dumb devices.)

Seems like a hack to me since it pessimizes driver performance for conforming devices in favor of an unknown (and presumed small :) set of nonconforming ones. Plus, I seem to recall Matt saying that the original motivation was a reading that the storage spec required that -- but a later reading said it didn't require that.

That said -- I think we'd be interested in a patch prepared to implement that "hack", since it's the best that's likely to show up near-term. I presume Olaf (and others) could verify whether it really solved the problem.

- Dave

Olaf Hering

2002-03-14 13:21 UTC #7 lore raw

In reply to Johannes Erdfelt

Quoted 19 lines · Greg KH

Quoted 4 lines · Johannes Erdfelt
The hub driver doesn't talk to devices, so I'll assume you mean the hub thread, which is actually calling the core for enumeration (setting address, fetching descriptors, etc)
Sorry, yes this is what I meant.
Quoted 4 lines
But the enumeration should be finished by the time the hotplug stuff finishes.
In that case, nothing should be stomping over each other.
In looking at this some more I agree, it doesn't look like this could race with the hub thread.
Quoted 2 lines
Is it that devices don't like how quickly we let the driver talk to it after enumeration?

Quoted 6 lines

Ah, this is probably more likely.
The problem looks like the usbdevfs calls are happening at the same time the hid driver is also talking to the device.
Olaf, is this what your log messages show?

It seems so. I think a more correct fix would be a lock in the usbcore code to hold usbdevfs requests when "some bus action" is going on, like this renumbering. You said there is no lock right now for this case?

Gruss Olaf

Signature · 9 lines

 $ man clone

BUGS
       Main feature not yet implemented...

_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Greg KH

2002-03-15 18:28 UTC #8 lore raw

In reply to Johannes Erdfelt

Quoted 8 lines · Olaf Hering

Quoted 4 lines
The problem looks like the usbdevfs calls are happening at the same time the hid driver is also talking to the device.
Olaf, is this what your log messages show?
It seems so. I think a more correct fix would be a lock in the usbcore code to hold usbdevfs requests when "some bus action" is going on, like this renumbering. You said there is no lock right now for this case?

This is correct and has been on the list of things to fix in the usb code for a while.

thanks,

greg k-h

David Brownell

2002-05-15 20:25 UTC #9 lore raw

In reply to Johannes Erdfelt

Quoted 3 lines

What is the status of this bug in the hotplug related functions? We added a "sleep 3" in the add routing in usb.rc, but that can only be a workaround.

Have you verified that it only happens with the UHCI drivers?

To my understanding, HCDs that queue control traffic will not have this problem. So OHCI or EHCI shouldn't need that timeout. Or said differently, the issue isn't with hotplug, but generally with control traffic.

- Dave

_______________________________________________________________

Have big pipes? SourceForge.net is looking for download mirrors. We supply the hardware. You get the recognition. Email Us: [email protected]
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Olaf Hering

2002-05-15 20:37 UTC #10 lore raw

In reply to Johannes Erdfelt

Quoted 5 lines · David Brownell

Quoted 3 lines
What is the status of this bug in the hotplug related functions? We added a "sleep 3" in the add routing in usb.rc, but that can only be a workaround.
Have you verified that it only happens with the UHCI drivers?

I see it only on intel with uhci, yes.

Gruss Olaf

Signature · 13 lines

 $ man clone

BUGS
       Main feature not yet implemented...

_______________________________________________________________

Have big pipes? SourceForge.net is looking for download mirrors. We supply
the hardware. You get the recognition. Email Us: [email protected]
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Johannes Erdfelt

author

2002-05-15 21:37 UTC #11 lore raw

In reply to Johannes Erdfelt

Quoted 28 lines · Olaf Hering

Quoted 4 lines · Greg KH
Quoted 3 lines · Olaf Hering
Quoted 3 lines
The problem looks like the usbdevfs calls are happening at the same time the hid driver is also talking to the device.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Olaf, is this what your log messages show?
It seems so. I think a more correct fix would be a lock in the usbcore code to hold usbdevfs requests when "some bus action" is going on, like this renumbering. You said there is no lock right now for this case?
This is correct and has been on the list of things to fix in the usb code for a while.
What is the status of this bug in the hotplug related functions? We added a "sleep 3" in the add routing in usb.rc, but that can only be a workaround.
My idea was that we queue the hotplug events somehow and wait for all "bus traffic" to complete, the stuff that occours when the usb-uhci driver is loaded. Something like
insmod usbcore;insmod usb-uhci;modprobe hid;insmod keybdev;insmod mousedev should work. And it did not with 2.4.19pre2/3, I got timeouts. However, typing the commands manually to get some delay, and it worked. So I think that it is not strictly related to usbdevfs, that might be another bug.

Quoted 4 lines

However, after reading the whole thread again, it seems that queuing the events is maybe not the best solution. What should be done to get this fixed? (The delay and locking should be in the kernel and not in the hotplug package).

Well, it shouldn't be giving you back ETIMEDOUT. That's indicitive of another bug, not the queueing.

However, it looks like uhci.c (and probably usb-uhci.c too since the code was based on it) didn't correctly match control pipe's and prevent 2 URB's from being queued, 1 in and 1 out.

Can you try this patch?

It should atleast start returning -ENXIO (-6 on x86).

drivers/usb/uhci.c +8 -3

@@ -1470,9 +1470,14 @@
 
 		tmp = tmp->next;
 
-		if (u->dev = urb->dev && u->pipe = urb->pipe &&
-		    u->status = -EINPROGRESS)
-			return u;
+		if (u->dev = urb->dev && u->status = -EINPROGRESS) {
+			/* For control, ignore the direction */
+			if (usb_pipecontrol(urb->pipe) &&
+			    (u->pipe & ~USB_DIR_IN) = (urb->pipe & ~USB_DIR_IN))
+				return u;
+			else if (u->pipe = urb->pipe)
+				return u;
+		}
 	}
 
 	return NULL;

_______________________________________________________________