On 2012年12月19日 23:39, Alan Stern wrote:
> On Wed, 19 Dec 2012, lantianyu wrote:
>
>>>> I just find busy_bits is set or clear in the usb_port_resume() and
>>>> usb_reset_and_verify_device(). So currently we should keep my changes
>>>> mutually exclusive with them, right?
>>>
>>> Don't forget about what happens when a device is removed.
>> I am not very clear about this since busy_bits is not changed during device
>> being
>> removed. Could you elaborate? Thanks.
>
> What happens if the device is unplugged while some thread is using
> busy_bits? Will the hub driver realize that the device is gone?
>
> Alan Stern
>
Hi Alan:
Ok. I get. The port resume/suspend only take place when device is
suspending, suspended and resuming. If the device was unplugged during
busy_bits is set, it would be disconnected when resuming the device
since the resume operation would be failed.
I refresh my patch according to our previous discussion. The
needs_debounce flag is not useful because we move debouce to port's
resume. I also add a check of "port_dev->power_is_on" before set
change_bits in the hub_activate(). When device is power off, the port
status and port change will be 0 and change_bits will be set in the
original hub_activate() code which will cause device to be disconnected.
So add check to keep change_bits unset during device have been power off.
Index: usb/drivers/usb/core/hub.c
===================================================================
--- usb.orig/drivers/usb/core/hub.c
+++ usb/drivers/usb/core/hub.c
@@ -26,6 +26,7 @@
#include <linux/mutex.h>
#include <linux/freezer.h>
#include <linux/random.h>
+#include <linux/pm_qos.h>
#include <asm/uaccess.h>
#include <asm/byteorder.h>
@@ -108,7 +109,7 @@ MODULE_PARM_DESC(use_both_schemes,
DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
-#define HUB_DEBOUNCE_TIMEOUT 1500
+#define HUB_DEBOUNCE_TIMEOUT 2000
#define HUB_DEBOUNCE_STEP 25
#define HUB_DEBOUNCE_STABLE 100
@@ -127,7 +128,7 @@ static inline char *portspeed(struct usb
}
/* Note that hdev or one of its children must be locked! */
-static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
+struct usb_hub *hdev_to_hub(struct usb_device *hdev)
{
if (!hdev || !hdev->actconfig || !hdev->maxchild)
return NULL;
@@ -393,7 +394,7 @@ static int clear_hub_feature(struct usb_
/*
* USB 2.0 spec Section 11.24.2.2
*/
-static int clear_port_feature(struct usb_device *hdev, int port1, int
feature)
+int clear_port_feature(struct usb_device *hdev, int port1, int feature)
{
return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
@@ -718,6 +719,9 @@ int usb_hub_set_port_power(struct usb_de
bool set)
{
int ret;
+ struct usb_hub *hub = hdev_to_hub(hdev);
+ struct usb_port *port_dev
+ = hub->ports[port1 - 1];
if (set)
ret = set_port_feature(hdev, port1,
@@ -725,6 +729,9 @@ int usb_hub_set_port_power(struct usb_de
else
ret = clear_port_feature(hdev, port1,
USB_PORT_FEAT_POWER);
+
+ if (!ret)
+ port_dev->power_is_on = set;
return ret;
}
@@ -804,7 +811,11 @@ static unsigned hub_power_on(struct usb_
dev_dbg(hub->intfdev, "trying to enable port power on "
"non-switchable hub\n");
for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
- set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
+ if (hub->ports[port1 - 1]->power_is_on)
+ set_port_feature(hub->hdev, port1,
USB_PORT_FEAT_POWER);
+ else
+ clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_POWER);
/* Wait at least 100 msec for power to become stable */
delay = max(pgood_delay, (unsigned) 100);
@@ -1136,10 +1147,14 @@ static void hub_activate(struct usb_hub
set_bit(port1, hub->change_bits);
} else if (udev->persist_enabled) {
+ struct usb_port *port_dev = hub->ports[port1 - 1];
+
#ifdef CONFIG_PM
udev->reset_resume = 1;
#endif
- set_bit(port1, hub->change_bits);
+ /* Don't set the change_bits when the device was
power off */
+ if (port_dev->power_is_on)
+ set_bit(port1, hub->change_bits);
} else {
/* The power session is gone; tell khubd */
@@ -2835,6 +2850,7 @@ EXPORT_SYMBOL_GPL(usb_enable_ltm);
int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
{
struct usb_hub *hub = hdev_to_hub(udev->parent);
{
struct usb_hub *hub = hdev_to_hub(udev->parent);
+ struct usb_port *port_dev = hub->ports[udev->portnum - 1];
int port1 = udev->portnum;
int status;
@@ -2929,6 +2945,20 @@ int usb_port_suspend(struct usb_device *
udev->port_is_suspended = 1;
msleep(10);
}
+
+ /*
+ * Check whether current status is meeting requirement of
+ * usb port power off mechanism
+ */
+ if (!udev->do_remote_wakeup
+ && (dev_pm_qos_flags(&port_dev->dev,
+ PM_QOS_FLAG_NO_POWER_OFF) != PM_QOS_FLAGS_ALL)
+ && udev->persist_enabled
+ && !status) {
+ pm_runtime_put_sync(&port_dev->dev);
+ port_dev->did_runtime_put = true;
+ }
+
usb_mark_last_busy(hub->hdev);
return status;
}
@@ -3049,10 +3079,22 @@ static int finish_port_resume(struct usb
int usb_port_resume(struct usb_device *udev, pm_message_t msg)
{
struct usb_hub *hub = hdev_to_hub(udev->parent);
+ struct usb_port *port_dev = hub->ports[udev->portnum - 1];
int port1 = udev->portnum;
int status;
u16 portchange, portstatus;
+ if (port_dev->did_runtime_put) {
+ status = pm_runtime_get_sync(&port_dev->dev);
+ port_dev->did_runtime_put = false;
+ if (status < 0) {
+ dev_dbg(&udev->dev, "can't resume usb port,
status %d\n",
+ status);
+ return status;
+ }
+ }
+
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, &portstatus, &portchange);
if (status == 0 && !port_is_suspended(hub, portstatus))
@@ -3733,7 +3775,7 @@ EXPORT_SYMBOL_GPL(usb_enable_ltm);
* every 25ms for transient disconnects. When the port status has been
* unchanged for 100ms it returns the port status.
*/
-static int hub_port_debounce(struct usb_hub *hub, int port1)
+int hub_port_debounce(struct usb_hub *hub, int port1, bool
must_be_connected)
{
int ret;
int total_time, stable_time = 0;
@@ -3747,7 +3789,9 @@ static int hub_port_debounce(struct usb_
if (!(portchange & USB_PORT_STAT_C_CONNECTION) &&
(portstatus & USB_PORT_STAT_CONNECTION) ==
connection) {
- stable_time += HUB_DEBOUNCE_STEP;
+ if (!must_be_connected || (connection
+ == USB_PORT_STAT_CONNECTION))
+ stable_time += HUB_DEBOUNCE_STEP;
if (stable_time >= HUB_DEBOUNCE_STABLE)
break;
} else {
@@ -4260,7 +4304,7 @@ static void hub_port_connect_change(stru
if (portchange & (USB_PORT_STAT_C_CONNECTION |
USB_PORT_STAT_C_ENABLE)) {
- status = hub_port_debounce(hub, port1);
+ status = hub_port_debounce(hub, port1, false);
if (status < 0) {
if (printk_ratelimit())
dev_err(hub_dev, "connect-debounce failed, "
Index: usb/drivers/usb/core/hub.h
===================================================================
--- usb.orig/drivers/usb/core/hub.h
+++ usb/drivers/usb/core/hub.h
@@ -80,6 +80,8 @@ struct usb_hub {
* @port_owner: port's owner
* @connect_type: port's connect type
* @portnum: port index num based one
+ * @power_is_on: port's power state
+ * @did_runtime_put: port has done pm_runtime_put().
*/
struct usb_port {
struct usb_port {
struct usb_device *child;
@@ -87,6 +89,8 @@ struct usb_port {
struct dev_state *port_owner;
enum usb_port_connect_type connect_type;
u8 portnum;
+ bool power_is_on;
+ bool did_runtime_put;
};
#define to_usb_port(_dev) \
@@ -98,4 +102,9 @@ extern void usb_hub_remove_port_device(s
int port1);
extern int usb_hub_set_port_power(struct usb_device *hdev,
int port1, bool set);
+extern struct usb_hub *hdev_to_hub(struct usb_device *hdev);
+extern int hub_port_debounce(struct usb_hub *hub, int port1,
+ bool must_be_connected);
+extern int clear_port_feature(struct usb_device *hdev,
+ int port1, int feature);
Index: usb/drivers/usb/core/port.c
===================================================================
--- usb.orig/drivers/usb/core/port.c
+++ usb/drivers/usb/core/port.c
@@ -74,9 +74,31 @@ static int usb_port_runtime_resume(struc
struct usb_port *port_dev = to_usb_port(dev);
struct usb_device *hdev =
to_usb_device(dev->parent->parent);
+ struct usb_hub *hub = hdev_to_hub(hdev);
+ int port1 = port_dev->portnum;
+ int retval;
- return usb_hub_set_port_power(hdev, port_dev->portnum,
+ set_bit(port1, hub->busy_bits);
+ retval = usb_hub_set_port_power(hdev, port1,
true);
+
+ if (port_dev->child && !retval) {
+ /*
+ * Wait for usb hub port to be reconnected in order to make
+ * the resume procedure successful.
+ */
+ retval = hub_port_debounce(hub, port1, true);
+ if (retval < 0) {
+ dev_dbg(&port_dev->dev, "can't get reconnection
after setting port power on, status %d\n",
+ retval);
+ clear_bit(port1, hub->busy_bits);
+ return retval;
+ }
+ clear_port_feature(hdev, port1,
+ USB_PORT_FEAT_C_ENABLE);
+ }
+ clear_bit(port1, hub->busy_bits);
+ return retval;
}
static int usb_port_runtime_suspend(struct device *dev)
@@ -84,13 +106,22 @@ static int usb_port_runtime_suspend(stru
struct usb_port *port_dev = to_usb_port(dev);
struct usb_device *hdev =
to_usb_device(dev->parent->parent);
+ struct usb_hub *hub = hdev_to_hub(hdev);
+ int port1 = port_dev->portnum;
+ int retval;
if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
== PM_QOS_FLAGS_ALL)
return -EAGAIN;
- return usb_hub_set_port_power(hdev, port_dev->portnum,
- false);
+ set_bit(port1, hub->busy_bits);
+ retval = usb_hub_set_port_power(hdev, port1, false);
+ clear_port_feature(hdev, port1,
+ USB_PORT_FEAT_C_CONNECTION);
+ clear_port_feature(hdev, port1,
+ USB_PORT_FEAT_C_ENABLE);
+ clear_bit(port1, hub->busy_bits);
+ return retval;
}
static const struct dev_pm_ops usb_port_pm_ops = {
@@ -120,6 +151,7 @@ int usb_hub_create_port_device(struct us
hub->ports[port1 - 1] = port_dev;
port_dev->portnum = port1;
+ port_dev->power_is_on = true;
port_dev->dev.parent = hub->intfdev;
port_dev->dev.groups = port_dev_group;
port_dev->dev.type = &usb_port_device_type;
--
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
