linux-kernel - [PATCH] usb: chipidea: Fix missing resume call after suspend

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-next>] [day] [month] [year] [list]

Date:   Fri, 21 Apr 2017 12:10:53 +0200
From:   Bernhard Walle <bernhard@...lle.de>
To:     Peter.Chen@....com
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bernhard Walle <bernhard@...lle.de>
Subject: [PATCH] usb: chipidea: Fix missing resume call after suspend

We have a i.MX53-based hardware (quite similar to the i.MX53 QSB from
Freescale/NXP). I'm reading the ..../ci_hdrc.0/gadget/suspended sysfs
file to find out whether a PC is connected to the USB gadget. With old
kernel versions, this worked. However, with kernel 4.9 this didn't work.

When the host is suspended once, it never sets back the suspended status
to 0. The reason is that this seems to be done in the resume handler,
which should be executed in the interrupt handler:

udc_irq:

...
     if (USBi_PCI & intr) {
             ci->gadget.speed = hw_port_is_high_speed(ci) ?
                     USB_SPEED_HIGH : USB_SPEED_FULL;
             if (ci->suspended && ci->driver->resume) {
                     spin_unlock(&ci->lock);
                     ci->driver->resume(&ci->gadget);
                     spin_lock(&ci->lock);
                     ci->suspended = 0;
             }
     }
...

However, ci->suspended is already 0 here because _gadget_stop_activity
is called before. So the resume handler never gets called. The obvious
solution is to not touch ci->suspended in _gadget_stop_activity and
to trust the interrupt handler to set it back (and to modify it to set
ci->suspended to 0 even if ci->driver->resume is NULL).

This code works on my platform. However, since I didn't write the driver
and since I have no deep understanding of it, I cannot determine if
there are any negative side effects, so I hope to get some review here.

Signed-off-by: Bernhard Walle <bernhard@...lle.de>
---
 drivers/usb/chipidea/udc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index f88e9157fad0..0c780ba39b37 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -712,7 +712,6 @@ static int _gadget_stop_activity(struct usb_gadget *gadget)
 	spin_lock_irqsave(&ci->lock, flags);
 	ci->gadget.speed = USB_SPEED_UNKNOWN;
 	ci->remote_wakeup = 0;
-	ci->suspended = 0;
 	spin_unlock_irqrestore(&ci->lock, flags);

 	/* flush all endpoints */
@@ -1845,10 +1844,12 @@ static irqreturn_t udc_irq(struct ci_hdrc *ci)
 		if (USBi_PCI & intr) {
 			ci->gadget.speed = hw_port_is_high_speed(ci) ?
 				USB_SPEED_HIGH : USB_SPEED_FULL;
-			if (ci->suspended && ci->driver->resume) {
-				spin_unlock(&ci->lock);
-				ci->driver->resume(&ci->gadget);
-				spin_lock(&ci->lock);
+			if (ci->suspended) {
+				if (ci->driver->resume) {
+					spin_unlock(&ci->lock);
+					ci->driver->resume(&ci->gadget);
+					spin_lock(&ci->lock);
+				}
 				ci->suspended = 0;
 			}
 		}
-- 
2.12.2