On Thu, Apr 12, 2012 at 03:03:33PM -0400, Alan Stern wrote: > On Thu, 12 Apr 2012, Sarah Sharp wrote: > > > xHCI does a similar thing of only returning a non-zero status when we're > > in the middle of resume. I think it does that for both USB 2.0 and USB > > 3.0 ports. There are also a couple other port status change events that > > we hide from the USB core, because there is no equivalent port status > > bit in external hubs. > > > > Will the USB core now handle a non-zero return status from > > hub_status_data() when none of the hub change bits are set? > > Yes. More precisely, you could always return a non-zero value from > hub_status_data with none of the change bits set, but until now that > would have no effect. In particular, it wouldn't cause khubd to check > any port statuses. > > With this patch, it's still true that khubd won't check the port > statuses if none of the change bits are set. Rather, the effect is to > tell usbcore to resume the root hub (assuming you return the non-zero > value just after the root hub has been suspended). > > The idea behind this is simple enough. The HCD generally knows when a > port resume (due to a remote-wakeup request) _starts_. But the only > way it has to tell the core is by setting the suspend-change status > bit, and it's not allowed to do that until the resume _ends_. Now the > HCD will be able to say that a port resume is in progress by returning > a non-zero value, even though the suspend-change bit isn't set. Ok, sure, makes sense. > > If so, that > > simplifies the xHCI hub architecture quite a bit. I think that means we > > should set the polling bit and return a non-zero value whenever a change > > bit is set. > > You should simply call usb_hcd_poll_rh_status whenever you learn that a > status-change bit has been set. Aren't you doing this already? Yes. However, the code before this patch meant that if the USB core doesn't see a change bit (because we're hiding it), it would have suspended the hub. > Polling should usually be avoided. The only reason for polling in > ehci-hcd is because it uses the poll as an opportunity to turn off > reset or resume signalling (and turn on the corresponding status-change > bit at the same time). Every other status-change event is detected in > the IRQ handler. > > > That will take care of some of the issues we've been seeing > > with xHCI ports seeming "dead" because the xHCI driver didn't clear a > > port change bit when it should have. > > Can you give more details about these issues? This patch wasn't > intended to help with problems like that. The issue I was thinking of would happen on some pre-release Intel xHCI hosts. When the user disconnected a device, the host would report an "Inactive" state. Then the USB core would issue a warm port reset, which would timeout before the warm port reset change bit was set. We missed a spot in the hub status handler that was supposed to look for the warm port reset change bit, and so the port was suspended while the bit was set. Since the xHCI host doesn't interrupt again when a previous port status change bit is still set, we would never get an interrupt when a device signaled a remote wakeup. In fact, we would never get reports of disconnects or connects from the port ever again. So the port would seem "dead" after the device was removed. With the current change to the USB core hub status handler, I think I can just return an error code whenever a port status change bit is set. Then the port will never be suspended with no way to know when a device resumes. But that only handles the case of a lost port resume change bit. We still need to handle other types of lost port status change bit interrupts. I believe we had the discussion some time ago that I should set the polling bit when any port status change bit is still set at the end of any of the hub functions. That way, if we miss clearing a change bit somewhere deep in the hub code, the USB core will know it needs to poll the status later. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
