linux-kernel - Re: [PATCH] fsl/usb: Workaround for USB erratum-A005697

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]

Date:	Mon, 27 Jul 2015 10:41:53 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Nikhil Badola <nikhil.badola@...escale.com>
cc:	linux-kernel@...r.kernel.org, <linux-usb@...r.kernel.org>,
	<gregkh@...uxfoundation.org>,
	Ramneek Mehresh <ramneek.mehresh@...escale.com>
Subject: Re: [PATCH] fsl/usb: Workaround for USB erratum-A005697

On Mon, 27 Jul 2015, Nikhil Badola wrote:

> As per USB specification, in the Suspend state, the status bit does
> not change until the port is suspended. However, there may be a delay
> in suspending a port if there is a transaction currently in progress
> on the bus.
> 
> In the USBDR controller, the PORTSCx[SUSP] bit changes immediately when
> the application sets it and not when the port is actually suspended
> 
> Workaround for this issue involves waiting for a minimum of 10ms to
> allow the controller to go into SUSPEND state before proceeding ahead

Why is this delay needed?  When ehci_bus_suspend() is called there are 
never any transactions in progress, except possibly for a SOF 
transaction (which takes less than 1 us, nowhere near 10 ms).

> Signed-off-by: Ramneek Mehresh <ramneek.mehresh@...escale.com>
> Signed-off-by: Nikhil Badola <nikhil.badola@...escale.com>
> ---
>  drivers/usb/host/ehci-fsl.c      |  3 +++
>  drivers/usb/host/ehci-hub.c      |  2 ++
>  drivers/usb/host/ehci.h          | 12 ++++++++++++
>  drivers/usb/host/fsl-mph-dr-of.c |  5 +++++
>  include/linux/fsl_devices.h      |  1 +
>  5 files changed, 23 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 202dafb..8904aae 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -278,6 +278,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
>  		out_be32(non_ehci + FSL_SOC_USB_SNOOP2, 0x80000000 | SNOOP_SIZE_2GB);
>  	}
>  
> +	if (pdata->has_fsl_erratum_a005697 == 1)
> +		ehci->has_fsl_susp_errata = 1;
> +
>  	if ((pdata->operating_mode == FSL_USB2_DR_HOST) ||
>  			(pdata->operating_mode == FSL_USB2_DR_OTG))
>  		if (ehci_fsl_setup_phy(hcd, pdata->phy_mode, 0))
> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> index 22abb68..7eac923 100644
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
> @@ -303,6 +303,8 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
>  						USB_PORT_STAT_HIGH_SPEED)
>  				fs_idle_delay = true;
>  			ehci_writel(ehci, t2, reg);
> +			if (ehci_has_fsl_susp_errata(ehci))
> +				usleep_range(10000, 20000);
>  			changed = 1;
>  		}
>  	}

This is wrong for two reasons.  First, you must not sleep while holding 
a spinlock.  Second, your sleep is in the wrong place.  It should occur 
later, at this point:

	if ((changed && ehci->has_tdi_phy_lpm) || fs_idle_delay) {
		/*
		 * Wait for HCD to enter low-power mode or for the bus
		 * to switch to full-speed idle.
		 */
		usleep_range(5000, 5500);
	}

You can check your quirk flag here and increase the length of this 
sleep to 10 ms.

Alan Stern

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