linux-kernel - Re: [PATCH v3] usb: Retry port status check on resume to work around RH bugs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1501281033420.25648-100000@netrider.rowland.org>
Date:	Wed, 28 Jan 2015 10:33:56 -0500 (EST)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Julius Werner <jwerner@...omium.org>
cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	<linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	Dan Williams <dan.j.williams@...el.com>,
	Douglas Anderson <dianders@...omium.org>,
	Tomasz Figa <tfiga@...omium.org>,
	Wu Liang feng <wulf@...k-chips.com>
Subject: Re: [PATCH v3] usb: Retry port status check on resume to work around
 RH bugs

On Tue, 27 Jan 2015, Julius Werner wrote:

> The EHCI controller on the RK3288 SoC is violating basic parts of the
> USB spec and thereby unable to properly resume a suspended port. It does
> not start SOF generation within 3ms of finishing resume signaling, so
> the attached device will drop of the bus again. This is a particular
> problem with runtime PM, where accessing the device will trigger a
> resume that immediately makes it unavailable (and reenumerate with a new
> handle).
> 
> Thankfully, the persist feature is generally able to work around stuff
> like that. Unfortunately, it doesn't quite work in this particular case
> because the controller will turn off the CurrentConnectStatus bit for an
> instant while the device is reconnecting, which causes the kernel to
> conclude that it permanently disappeared. This patch adds a tiny retry
> mechanism to the core port resume code which will catch this case and
> shouldn't have any notable impact on other controllers.
> 
> Signed-off-by: Julius Werner <jwerner@...omium.org>
> ---
>  drivers/usb/core/hub.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index aeb50bb..26bdd96 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2896,10 +2896,12 @@ static int port_is_suspended(struct usb_hub *hub, unsigned portstatus)
>   */
>  static int check_port_resume_type(struct usb_device *udev,
>  		struct usb_hub *hub, int port1,
> -		int status, unsigned portchange, unsigned portstatus)
> +		int status, u16 portchange, u16 portstatus)
>  {
>  	struct usb_port *port_dev = hub->ports[port1 - 1];
> +	int retries = 3;
>  
> + retry:
>  	/* Is a warm reset needed to recover the connection? */
>  	if (status == 0 && udev->reset_resume
>  		&& hub_port_warm_reset_required(hub, port1, portstatus)) {
> @@ -2907,10 +2909,17 @@ static int check_port_resume_type(struct usb_device *udev,
>  	}
>  	/* Is the device still present? */
>  	else if (status || port_is_suspended(hub, portstatus) ||
> -			!port_is_power_on(hub, portstatus) ||
> -			!(portstatus & USB_PORT_STAT_CONNECTION)) {
> +			!port_is_power_on(hub, portstatus)) {
>  		if (status >= 0)
>  			status = -ENODEV;
> +	} else if (!(portstatus & USB_PORT_STAT_CONNECTION)) {
> +		if (retries--) {
> +			usleep_range(200, 300);
> +			status = hub_port_status(hub, port1, &portstatus,
> +							     &portchange);
> +			goto retry;
> +		}
> +		status = -ENODEV;
>  	}
>  
>  	/* Can't do a normal resume if the port isn't enabled,

Acked-by: Alan Stern <stern@...land.harvard.edu>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/