Re: [RFC] Delay EHCI initialization until khubd is idle

[Frans Pop <elendil@xxxxxxxxx>]

On Friday 05 September 2008, Alan Stern wrote:
> On Fri, 5 Sep 2008, Frans Pop wrote:
> > The "dead keyboard" issue discussed in the other thread does IMO
> > demonstrate that there is a real risk of issues because of the
> > current lack of coordination between drivers which makes the outcome
> > sensitive to miniscule timing differences and races.
>
> The "dead keyboard" problem is only tangentially related to EHCI
> initialization.  Below is a patch for 2.6.27-rc5 which removes the
> transient-disconnect handling that caused the "dead keyboard" problem.
> You might want to test it; even without the "delay EHCI init" patch it
> should eliminate any more dead keyboards.

Hi Alan,

I've been running with this patch on several machines basically since you 
sent it. I have not seen the "dead keyboard" problem since, nor have I 
seen other issues related to USB.

Sorry for not replying sooner, but it is harder to prove a negative in 
this case than a positive and I had not booted the relevant system very 
often. I've just done a series of reboots without seeing the problem, but 
I still can't be 100% sure. You seemed confident in your analysis though.

Feel free to add a
Tested-by: Frans Pop <elendil@xxxxxxxxx>

Thanks,
FJP

P.S. I'll also give your other patch to quieten errors caused by ehci take 
over a try. I really appreciate the fact that you're staying with that 
subject.

> Index: usb-2.6/drivers/usb/core/hub.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/hub.c
> +++ usb-2.6/drivers/usb/core/hub.c
> @@ -2683,35 +2683,17 @@ static void hub_port_connect_change(stru
>  				USB_PORT_STAT_C_ENABLE);
>  #endif
>
> -	/* Try to use the debounce delay for protection against
> -	 * port-enable changes caused, for example, by EMI.
> -	 */
> -	if (portchange & (USB_PORT_STAT_C_CONNECTION |
> -				USB_PORT_STAT_C_ENABLE)) {
> -		status = hub_port_debounce(hub, port1);
> -		if (status < 0) {
> -			if (printk_ratelimit())
> -				dev_err (hub_dev, "connect-debounce failed, "
> -						"port %d disabled\n", port1);
> -			portstatus &= ~USB_PORT_STAT_CONNECTION;
> -		} else {
> -			portstatus = status;
> -		}
> -	}
> -
>  	/* Try to resuscitate an existing device */
>  	udev = hdev->children[port1-1];
>  	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 */
> -		} else if (!udev->persist_enabled) {
> -			status = -ENODEV;	/* Mustn't resuscitate */
>
>  #ifdef CONFIG_USB_SUSPEND
> -		} else if (udev->state == USB_STATE_SUSPENDED) {
> +		} else if (udev->state == USB_STATE_SUSPENDED &&
> +				udev->persist_enabled) {
>  			/* For a suspended device, treat this as a
>  			 * remote wakeup event.
>  			 */
> @@ -2726,7 +2708,7 @@ static void hub_port_connect_change(stru
>  #endif
>
>  		} else {
> -			status = usb_reset_device(udev);
> +			status = -ENODEV;	/* Don't resuscitate */
>  		}
>  		usb_unlock_device(udev);
>
> @@ -2741,6 +2723,19 @@ static void hub_port_connect_change(stru
>  		usb_disconnect(&hdev->children[port1-1]);
>  	clear_bit(port1, hub->change_bits);
>
> +	if (portchange & (USB_PORT_STAT_C_CONNECTION |
> +				USB_PORT_STAT_C_ENABLE)) {
> +		status = hub_port_debounce(hub, port1);
> +		if (status < 0) {
> +			if (printk_ratelimit())
> +				dev_err (hub_dev, "connect-debounce failed, "
> +						"port %d disabled\n", port1);
> +			portstatus &= ~USB_PORT_STAT_CONNECTION;
> +		} else {
> +			portstatus = status;
> +		}
> +	}
> +
>  	/* Return now if debouncing failed or nothing is connected */
>  	if (!(portstatus & USB_PORT_STAT_CONNECTION)) {
>
> @@ -2748,7 +2743,7 @@ static void hub_port_connect_change(stru
>  		if ((wHubCharacteristics & HUB_CHAR_LPSM) < 2
>  				&& !(portstatus & (1 << USB_PORT_FEAT_POWER)))
>  			set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
> -
> +
>  		if (portstatus & USB_PORT_STAT_ENABLE)
>    			goto done;
>  		return;

--
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