linux-kernel - [PATCH] ehci: phy low power mode bug fixing

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: <20100119163131.0af9f540@dxy2>
Date:	Tue, 19 Jan 2010 16:31:31 +0800
From:	alek du <alek.du@...el.com>
To:	Arnd Bergmann <arnd@...db.de>, Greg KH <greg@...ah.com>
CC:	Julia Lawall <julia@...u.dk>, Andreas Mohr <andi@...as.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Pan, Jacob jun" <jacob.jun.pan@...el.com>,
	Alan Stern <stern@...land.harvard.edu>
Subject: [PATCH] ehci: phy low power mode bug fixing

This patch should fix the bug and I tested it with Moorestown platform...
But the strange thing is that even the original code does not trigger the
"scheduling while atomic" issue...

>From ca7833d854867e91d6aa6bccf1f3224862b3a25c Mon Sep 17 00:00:00 2001
From: Alek Du <alek.du@...el.com>
Date: Tue, 19 Jan 2010 16:05:15 +0800
Subject: [PATCH] ehci: phy low power mode bug fixing

1. There are two msleep calls inside two spin lock sections, need to unlock
   and lock again after msleep.
2. Save a extra status reg setting.

Signed-off-by: Alek Du <alek.du@...el.com>
---
 drivers/usb/host/ehci-hub.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 2c6571c..7d77fbb 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -178,7 +178,9 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
 			if (hostpc_reg) {
 				u32	t3;
 
+				spin_unlock_irq(&ehci->lock);
 				msleep(5);/* 5ms for HCD enter low pwr mode */
+				spin_lock_irq(&ehci->lock);
 				t3 = ehci_readl(ehci, hostpc_reg);
 				ehci_writel(ehci, t3 | HOSTPC_PHCD, hostpc_reg);
 				t3 = ehci_readl(ehci, hostpc_reg);
@@ -886,17 +888,18 @@ static int ehci_hub_control (
 			if ((temp & PORT_PE) == 0
 					|| (temp & PORT_RESET) != 0)
 				goto error;
-			ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
+
 			/* After above check the port must be connected.
 			 * Set appropriate bit thus could put phy into low power
 			 * mode if we have hostpc feature
 			 */
+			temp &= ~PORT_WKCONN_E;
+			temp |= PORT_WKDISC_E | PORT_WKOC_E;
+			ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
 			if (hostpc_reg) {
-				temp &= ~PORT_WKCONN_E;
-				temp |= (PORT_WKDISC_E | PORT_WKOC_E);
-				ehci_writel(ehci, temp | PORT_SUSPEND,
-							status_reg);
+				spin_unlock_irqrestore(&ehci->lock, flags);
 				msleep(5);/* 5ms for HCD enter low pwr mode */
+				spin_lock_irqsave(&ehci->lock, flags);
 				temp1 = ehci_readl(ehci, hostpc_reg);
 				ehci_writel(ehci, temp1 | HOSTPC_PHCD,
 					hostpc_reg);
-- 
1.6.3.3


On Tue, 19 Jan 2010 06:25:10 +0800
Arnd Bergmann <arnd@...db.de> wrote:

> On Monday 18 January 2010, Julia Lawall wrote:
> > > That code looks indeed broken as was added las July as part of
> > > 331ac6b288d9 "USB: EHCI: Add Intel Moorestown EHCI controller
> > > HOSTPCx extensions and support phy low power mode". The reason
> > > that this hasn't triggered is probably the lack of Moorestown
> > > machines in the field.
> > 
> > The fix is just msleep -> mdelay?
> 
> No, that would just kill the warning but still leave horrible code.
> There should really be some way to move the sleeping operation
> outside of the spinlock.
> 
> From a brief look at the code, I think the sequence could be converted
> from
> 
> lock();
> suspend();
> sleep();
> clock_disable();
> unlock();
> 
> to
> 
> lock();
> again:
> suspend();
> unlock();
> sleep();
> lock();
> if (!suspended())
>    goto again;
> clock_disable();
> unlock();
> 
> 	Arnd

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