linux-kernel - [RFC PATCH] USB: hub: fix port suspend/resume

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

Message-Id: <20200227095040.10208-1-m.felsch@pengutronix.de>
Date:   Thu, 27 Feb 2020 10:50:40 +0100
From:   Marco Felsch <m.felsch@...gutronix.de>
To:     stern@...land.harvard.edu, gregkh@...uxfoundation.org,
        Thinh.Nguyen@...opsys.com, harry.pan@...el.com,
        nobuta.keiya@...itsu.com, malat@...ian.org,
        kai.heng.feng@...onical.com, chiasheng.lee@...el.com,
        andreyknvl@...gle.com, heinzelmann.david@...il.com
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...gutronix.de
Subject: [RFC PATCH] USB: hub: fix port suspend/resume

At the momemnt the usb-port driver has only runime_pm hooks.
Suspending the port and turn off the VBUS supply should be triggered by
the hub device suspend callback usb_port_suspend() which calls the
pm_runtime_put_sync() if all pre-conditions are meet. This mechanism
don't work correctly due to the global PM behaviour, for more information
see [1]. According [1] I added the suspend/resume callbacks for the port
device to fix this. While on it I replaced the #ifdef's by
__maybe_unused.

[1] https://www.spinics.net/lists/linux-usb/msg190537.html

Signed-off-by: Marco Felsch <m.felsch@...gutronix.de>
---
 drivers/usb/core/hub.c  | 13 -------------
 drivers/usb/core/port.c | 39 +++++++++++++++++++++++++++++++--------
 2 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3405b146edc9..c294484e478d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3323,10 +3323,6 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 		usb_set_device_state(udev, USB_STATE_SUSPENDED);
 	}
 
-	if (status == 0 && !udev->do_remote_wakeup && udev->persist_enabled
-			&& test_and_clear_bit(port1, hub->child_usage_bits))
-		pm_runtime_put_sync(&port_dev->dev);
-
 	usb_mark_last_busy(hub->hdev);
 
 	usb_unlock_port(port_dev);
@@ -3514,15 +3510,6 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 	int		status;
 	u16		portchange, portstatus;
 
-	if (!test_and_set_bit(port1, hub->child_usage_bits)) {
-		status = pm_runtime_get_sync(&port_dev->dev);
-		if (status < 0) {
-			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
-					status);
-			return status;
-		}
-	}
-
 	usb_lock_port(port_dev);
 
 	/* Skip the initial Clear-Suspend step for a remote wakeup */
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index bbbb35fa639f..9efa6b2ef31b 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -187,8 +187,7 @@ static void usb_port_device_release(struct device *dev)
 	kfree(port_dev);
 }
 
-#ifdef CONFIG_PM
-static int usb_port_runtime_resume(struct device *dev)
+static int __maybe_unused usb_port_runtime_resume(struct device *dev)
 {
 	struct usb_port *port_dev = to_usb_port(dev);
 	struct usb_device *hdev = to_usb_device(dev->parent->parent);
@@ -244,7 +243,7 @@ static int usb_port_runtime_resume(struct device *dev)
 	return retval;
 }
 
-static int usb_port_runtime_suspend(struct device *dev)
+static int __maybe_unused usb_port_runtime_suspend(struct device *dev)
 {
 	struct usb_port *port_dev = to_usb_port(dev);
 	struct usb_device *hdev = to_usb_device(dev->parent->parent);
@@ -283,7 +282,33 @@ static int usb_port_runtime_suspend(struct device *dev)
 
 	return retval;
 }
-#endif
+
+static int __maybe_unused _usb_port_suspend(struct device *dev)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+	struct usb_device *udev = port_dev->child;
+	int retval;
+
+	if (!udev->do_remote_wakeup && udev->persist_enabled)
+		retval = usb_port_runtime_suspend(dev);
+
+	/* Do not force the user to enable the power-off feature */
+	if (retval && retval != -EAGAIN)
+		return retval;
+
+	return 0;
+}
+
+static int __maybe_unused _usb_port_resume(struct device *dev)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+	struct usb_device *udev = port_dev->child;
+
+	if (!udev->do_remote_wakeup && udev->persist_enabled)
+		return usb_port_runtime_resume(dev);
+
+	return 0;
+}
 
 static void usb_port_shutdown(struct device *dev)
 {
@@ -294,10 +319,8 @@ static void usb_port_shutdown(struct device *dev)
 }
 
 static const struct dev_pm_ops usb_port_pm_ops = {
-#ifdef CONFIG_PM
-	.runtime_suspend =	usb_port_runtime_suspend,
-	.runtime_resume =	usb_port_runtime_resume,
-#endif
+	SET_SYSTEM_SLEEP_PM_OPS(_usb_port_suspend, _usb_port_resume)
+	SET_RUNTIME_PM_OPS(usb_port_runtime_suspend, usb_port_runtime_resume, NULL)
 };
 
 struct device_type usb_port_device_type = {
-- 
2.20.1