public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Ivan Mironov <mironov.ivan@gmail.com>
To: linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Alan Stern <stern@rowland.harvard.edu>,
	Martin Liu <liumartin@google.com>,
	YueHaibing <yuehaibing@huawei.com>,
	Mathias Nyman <mathias.nyman@linux.intel.com>,
	Nicolas Boichat <drinkcat@chromium.org>,
	Jon Flatley <jflat@chromium.org>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>,
	Benson Leung <bleung@chromium.org>,
	Harry Pan <harry.pan@intel.com>,
	Jack Stocker <jackstocker.93@gmail.com>,
	Danilo Krummrich <danilokrummrich@dk-develop.de>,
	Samuel Sadok <samuel.sadok@bluewin.ch>,
	Ivan Mironov <mironov.ivan@gmail.com>
Subject: [RFC PATCH 1/2] usb: core: Add support of disabling SS link before system suspend
Date: Thu, 14 Feb 2019 02:13:22 +0500	[thread overview]
Message-ID: <20190213211323.6072-2-mironov.ivan@gmail.com> (raw)
In-Reply-To: <20190213211323.6072-1-mironov.ivan@gmail.com>

Some Apple MacBooks contain internal SD card reader connected to the USB
3.0 bus. Example: MacBook Pro Retina mid 2015, which contains USB card
reader with ID 05ac:8406.

Currently, such card reader works only after a reboot and completely
disappears from system after the first system suspend/resume cycle.
Also, any subsequent attempts to suspend are starting to fail.

There is a known way to circumvent the suspend problem: removing device
using /sys/devices/*/*/usb*/*-*/remove helps. But this unbreaks only
suspend, not the card reader itself (it remains absent).

When trying to fix both suspend and card reader device, I found that the
only working method is to set link state to disabled during suspend
procedure.

This patch adds new quirk for USB devices:
USB_QUIRK_DISABLE_LINK_ON_SUSPEND. When enabled, it changes the usual
suspend procedure for USB 3.0 devices by setting link state to DISABLED
instead of U3. To "resume" from disabled state, new code sets link state
to RX_DETECT and also enables reset for device.

As usual, this quirk may be enabled manually by adding
'usbcore.quirks=$VID:$PID:p' to the kernel parameters.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=111201
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202509
Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
---
 drivers/usb/core/driver.c  |  6 +++
 drivers/usb/core/hub.c     | 84 ++++++++++++++++++++++++++++++++++++--
 drivers/usb/core/quirks.c  |  3 ++
 include/linux/usb.h        |  3 ++
 include/linux/usb/quirks.h |  9 ++++
 5 files changed, 101 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 53564386ed57..1a1ee1ba7a63 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1145,6 +1145,10 @@ static int usb_suspend_device(struct usb_device *udev, pm_message_t msg)
 			udev->state == USB_STATE_SUSPENDED)
 		goto done;
 
+	if (!PMSG_IS_AUTO(msg) &&
+			(udev->quirks & USB_QUIRK_DISABLE_LINK_ON_SUSPEND))
+		udev->disable_link_on_suspend = 1;
+
 	/* For devices that don't have a driver, we do a generic suspend. */
 	if (udev->dev.driver)
 		udriver = to_usb_device_driver(udev->dev.driver);
@@ -1188,6 +1192,8 @@ static int usb_resume_device(struct usb_device *udev, pm_message_t msg)
 
  done:
 	dev_vdbg(&udev->dev, "%s: status %d\n", __func__, status);
+	if (!status)
+		udev->disable_link_on_suspend = 0;
 	return status;
 }
 
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 1d1e61e980f3..c2e4e23500d3 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3230,8 +3230,22 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 			goto err_ltm;
 	}
 
+	if (udev->disable_link_on_suspend && !hub_is_superspeed(hub->hdev)) {
+		dev_dbg(&udev->dev,
+				"disabling link unsupported on <USB 3.0\n");
+		udev->disable_link_on_suspend = 0;
+	}
+
+	if (udev->disable_link_on_suspend) {
+		status = hub_set_port_link_state(hub, port1,
+				USB_SS_PORT_LS_SS_DISABLED);
+		if (status) {
+			dev_dbg(&port_dev->dev, "can't disable link\n");
+			udev->disable_link_on_suspend = 0;
+		}
+	}
 	/* see 7.1.7.6 */
-	if (hub_is_superspeed(hub->hdev))
+	else if (hub_is_superspeed(hub->hdev))
 		status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3);
 
 	/*
@@ -3431,6 +3445,32 @@ static int wait_for_connected(struct usb_device *udev,
 	return status;
 }
 
+static int wait_for_link(struct usb_device *udev,
+		struct usb_hub *hub, int port1,
+		u16 *portchange, u16 *portstatus)
+{
+	int status = 0, delay_ms = 0;
+
+	while (delay_ms < 2000) {
+		status = hub_port_status(hub, port1, portstatus, portchange);
+
+		if (status || ((*portstatus & USB_PORT_STAT_LINK_STATE) ==
+					USB_SS_PORT_LS_POLLING) ||
+				(*portstatus & USB_PORT_STAT_CONNECTION))
+			break;
+
+		msleep(20);
+		delay_ms += 20;
+	}
+	dev_dbg(&udev->dev, "waited %dms for link, status %d\n", delay_ms,
+			status);
+
+	if (delay_ms >= 2000)
+		status = -ENODEV;
+
+	return status;
+}
+
 /*
  * usb_port_resume - re-activate a suspended usb device's upstream port
  * @udev: device to re-activate, not a root hub
@@ -3484,8 +3524,43 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 
 	usb_lock_port(port_dev);
 
-	/* Skip the initial Clear-Suspend step for a remote wakeup */
 	status = hub_port_status(hub, port1, &portstatus, &portchange);
+	if (status)
+		dev_dbg(&udev->dev,
+				"port status: first read failed, status %d\n",
+				status);
+
+	if (status == 0 && udev->disable_link_on_suspend) {
+		if ((portstatus & USB_PORT_STAT_LINK_STATE) ==
+				USB_SS_PORT_LS_SS_DISABLED) {
+			status = hub_set_port_link_state(hub, port1,
+					USB_SS_PORT_LS_RX_DETECT);
+			if (status) {
+				dev_dbg(&port_dev->dev, "can't enable link\n");
+				goto SuspendCleared;
+			}
+
+			status = wait_for_link(udev, hub, port1, &portchange,
+					&portstatus);
+			if (status) {
+				dev_dbg(&port_dev->dev,
+						"wait for link failed\n");
+				goto SuspendCleared;
+			}
+			if (portstatus & USB_PORT_STAT_CONNECTION) {
+				dev_dbg(&port_dev->dev,
+						"connected after enabling link\n");
+				udev->disable_link_on_suspend = 0;
+			}
+
+			set_bit(port1, hub->warm_reset_bits);
+		} else {
+			dev_dbg(&port_dev->dev, "link is not disabled\n");
+			udev->disable_link_on_suspend = 0;
+		}
+	}
+
+	/* Skip the initial Clear-Suspend step for a remote wakeup */
 	if (status == 0 && !port_is_suspended(hub, portstatus)) {
 		if (portchange & USB_PORT_STAT_C_SUSPEND)
 			pm_wakeup_event(&udev->dev, 0);
@@ -3530,7 +3605,7 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 		}
 	}
 
-	if (udev->persist_enabled)
+	if (udev->persist_enabled && !udev->disable_link_on_suspend)
 		status = wait_for_connected(udev, hub, &port1, &portchange,
 				&portstatus);
 
@@ -4069,7 +4144,8 @@ static int usb_disable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 	if (usb_set_lpm_timeout(udev, state, 0))
 		return -EBUSY;
 
-	usb_set_device_initiated_lpm(udev, state, false);
+	if (!udev->disable_link_on_suspend)
+		usb_set_device_initiated_lpm(udev, state, false);
 
 	if (hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state))
 		dev_warn(&udev->dev, "Could not disable xHCI %s timeout, "
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 8bc35d53408b..c26ba784dc54 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -131,6 +131,9 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp)
 			case 'o':
 				flags |= USB_QUIRK_HUB_SLOW_RESET;
 				break;
+			case 'p':
+				flags |= USB_QUIRK_DISABLE_LINK_ON_SUSPEND;
+				break;
 			/* Ignore unrecognized flag characters */
 			}
 		}
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 5e49e82c4368..44dff19a7839 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -610,6 +610,8 @@ struct usb3_lpm_parameters {
  * @do_remote_wakeup:  remote wakeup should be enabled
  * @reset_resume: needs reset instead of resume
  * @port_is_suspended: the upstream port is suspended (L2 or U3)
+ * @disable_link_on_suspend: link was disabled/should be disabled before
+ *	suspend
  * @wusb_dev: if this is a Wireless USB device, link to the WUSB
  *	specific data for the device.
  * @slot_id: Slot ID assigned by xHCI
@@ -699,6 +701,7 @@ struct usb_device {
 	unsigned do_remote_wakeup:1;
 	unsigned reset_resume:1;
 	unsigned port_is_suspended:1;
+	unsigned disable_link_on_suspend:1;
 #endif
 	struct wusb_dev *wusb_dev;
 	int slot_id;
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index a1be64c9940f..d23f14145782 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -69,4 +69,13 @@
 /* Hub needs extra delay after resetting its port. */
 #define USB_QUIRK_HUB_SLOW_RESET		BIT(14)
 
+/*
+ * Disable link on device's port before system suspend and then
+ * enable it again after resume. This also enables device reset
+ * after resume.
+ *
+ * Has effect only on USB 3.0 devices connected to USB 3.0 ports.
+ */
+#define USB_QUIRK_DISABLE_LINK_ON_SUSPEND	BIT(15)
+
 #endif /* __LINUX_USB_QUIRKS_H */
-- 
2.20.1


  reply	other threads:[~2019-02-13 21:13 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-13 21:13 [RFC PATCH 0/2] Fix for the internal card reader and suspend on MacBooks Ivan Mironov
2019-02-13 21:13 ` Ivan Mironov [this message]
2019-02-13 21:13 ` [RFC PATCH 2/2] usb: quirks: Add quirk to fix " Ivan Mironov
2019-02-14  1:40 ` [RFC PATCH 0/2] Fix for the internal " Ivan Mironov
2019-02-14 17:32   ` Ivan Mironov
2019-02-25 13:52     ` Ivan Mironov
2019-02-14 15:03 ` Mathias Nyman
2019-02-14 17:20   ` Ivan Mironov
2019-02-18 18:50 ` Eric Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190213211323.6072-2-mironov.ivan@gmail.com \
    --to=mironov.ivan@gmail.com \
    --cc=bleung@chromium.org \
    --cc=danilokrummrich@dk-develop.de \
    --cc=drinkcat@chromium.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=harry.pan@intel.com \
    --cc=jackstocker.93@gmail.com \
    --cc=jflat@chromium.org \
    --cc=kai.heng.feng@canonical.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=liumartin@google.com \
    --cc=mathias.nyman@linux.intel.com \
    --cc=samuel.sadok@bluewin.ch \
    --cc=stern@rowland.harvard.edu \
    --cc=yuehaibing@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox