->is_power_on can now be reliably determined by looking at the power policy and the runtime status. ->did_runtime_put is handled automatically by the pm_runtime parent-child relationship. A few other cleanups fall out: 1/ No longer any need for usb_hub_set_port_power 2/ hub_power_on() only ever re-enables ports that should otherwise be powered Yes, this is still racy, but no worse than the current solution, synchronization to come. Cc: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> --- drivers/usb/core/hub.c | 95 ++++++++--------------------------------------- drivers/usb/core/hub.h | 14 ++----- drivers/usb/core/port.c | 11 ++++- 3 files changed, 29 insertions(+), 91 deletions(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index e06a457094fd..2e0812eff0ae 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -409,7 +409,7 @@ int usb_clear_port_feature(struct usb_device *hdev, int port1, int feature) /* * USB 2.0 spec Section 11.24.2.13 */ -static int set_port_feature(struct usb_device *hdev, int port1, int feature) +int usb_set_port_feature(struct usb_device *hdev, int port1, int feature) { return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0), USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1, @@ -426,7 +426,7 @@ static void set_port_led( int selector ) { - int status = set_port_feature(hub->hdev, (selector << 8) | port1, + int status = usb_set_port_feature(hub->hdev, (selector << 8) | port1, USB_PORT_FEAT_INDICATOR); if (status < 0) dev_dbg (hub->intfdev, @@ -731,34 +731,6 @@ static void hub_tt_work(struct work_struct *work) } /** - * usb_hub_set_port_power - control hub port's power state - * @hdev: USB device belonging to the usb hub - * @hub: target hub - * @port1: port index - * @set: expected status - * - * call this function to control port's power via setting or - * clearing the port's PORT_POWER feature. - * - * Return: 0 if successful. A negative error code otherwise. - */ -int usb_hub_set_port_power(struct usb_device *hdev, struct usb_hub *hub, - int port1, bool set) -{ - int ret; - struct usb_port *port_dev = hub->ports[port1 - 1]; - - if (set) - ret = set_port_feature(hdev, port1, USB_PORT_FEAT_POWER); - else - ret = usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER); - - if (!ret) - port_dev->power_is_on = set; - return ret; -} - -/** * usb_hub_clear_tt_buffer - clear control/bulk TT state in high speed hub * @urb: an URB associated with the failed or incomplete split transaction * @@ -836,11 +808,9 @@ static unsigned hub_power_on(struct usb_hub *hub, bool do_delay) dev_dbg(hub->intfdev, "trying to enable port power on " "non-switchable hub\n"); for (port1 = 1; port1 <= hub->hdev->maxchild; port1++) - if (hub->ports[port1 - 1]->power_is_on) - set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER); - else - usb_clear_port_feature(hub->hdev, port1, - USB_PORT_FEAT_POWER); + if (usb_port_power_enabled(hub->ports[port1 - 1])) + usb_set_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); @@ -872,7 +842,7 @@ static int hub_hub_status(struct usb_hub *hub, static int hub_set_port_link_state(struct usb_hub *hub, int port1, unsigned int link_status) { - return set_port_feature(hub->hdev, + return usb_set_port_feature(hub->hdev, port1 | (link_status << 3), USB_PORT_FEAT_LINK_STATE); } @@ -1092,6 +1062,9 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) struct usb_device *udev = hub->ports[port1 - 1]->child; u16 portstatus, portchange; + if (!usb_port_power_enabled(hub->ports[port1 - 1])) + continue; + portstatus = portchange = 0; status = hub_port_status(hub, port1, &portstatus, &portchange); if (udev || (portstatus & USB_PORT_STAT_CONNECTION)) @@ -1174,17 +1147,10 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type) 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 - /* Don't set the change_bits when the device - * was powered off. - */ - if (port_dev->power_is_on) - set_bit(port1, hub->change_bits); - + set_bit(port1, hub->change_bits); } else { /* The power session is gone; tell khubd */ usb_set_device_state(udev, USB_STATE_NOTATTACHED); @@ -2062,16 +2028,9 @@ void usb_disconnect(struct usb_device **pdev) usb_hcd_synchronize_unlinks(udev); if (udev->parent) { - struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent); - struct usb_port *port_dev = hub->ports[udev->portnum - 1]; struct usb_device *hdev = udev->parent; sysfs_remove_link(&hdev->dev.kobj, dev_name(&udev->dev)); - - if (!port_dev->did_runtime_put) - pm_runtime_put(&port_dev->dev); - else - port_dev->did_runtime_put = false; } usb_remove_ep_devs(&udev->ep0); @@ -2373,16 +2332,12 @@ int usb_new_device(struct usb_device *udev) /* Create link files between child device and usb port device. */ if (udev->parent) { - struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent); - struct usb_port *port_dev = hub->ports[udev->portnum - 1]; struct usb_device *hdev = udev->parent; err = sysfs_create_link(&hdev->dev.kobj, &udev->dev.kobj, dev_name(&udev->dev)); if (err) goto fail; - - pm_runtime_get_sync(&port_dev->dev); } (void) usb_create_ep_devs(&udev->dev, &udev->ep0, udev); @@ -2669,7 +2624,7 @@ static int hub_port_reset(struct usb_hub *hub, int port1, /* Reset the port */ for (i = 0; i < PORT_RESET_TRIES; i++) { - status = set_port_feature(hub->hdev, port1, (warm ? + status = usb_set_port_feature(hub->hdev, port1, (warm ? USB_PORT_FEAT_BH_PORT_RESET : USB_PORT_FEAT_RESET)); if (status == -ENODEV) { @@ -2965,7 +2920,6 @@ static unsigned wakeup_enabled_descendants(struct usb_device *udev) int usb_port_suspend(struct usb_device *udev, pm_message_t msg) { struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent); - struct usb_port *port_dev = hub->ports[udev->portnum - 1]; int port1 = udev->portnum; int status; bool really_suspend = true; @@ -3020,7 +2974,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) * descendants is enabled for remote wakeup. */ else if (PMSG_IS_AUTO(msg) || wakeup_enabled_descendants(udev) > 0) - status = set_port_feature(hub->hdev, port1, + status = usb_set_port_feature(hub->hdev, port1, USB_PORT_FEAT_SUSPEND); else { really_suspend = false; @@ -3059,11 +3013,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) { - pm_runtime_put_sync(&port_dev->dev); - port_dev->did_runtime_put = true; - } - usb_mark_last_busy(hub->hdev); return status; } @@ -3189,21 +3138,10 @@ static int finish_port_resume(struct usb_device *udev) int usb_port_resume(struct usb_device *udev, pm_message_t msg) { struct usb_hub *hub = usb_hub_to_struct_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)) @@ -3347,7 +3285,7 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg) if (hub_is_superspeed(hdev) && hdev->do_remote_wakeup) { /* Enable hub to send remote wakeup for all ports. */ for (port1 = 1; port1 <= hdev->maxchild; port1++) { - status = set_port_feature(hdev, + status = usb_set_port_feature(hdev, port1 | USB_PORT_FEAT_REMOTE_WAKE_CONNECT | USB_PORT_FEAT_REMOTE_WAKE_DISCONNECT | @@ -3578,7 +3516,7 @@ static int usb_set_lpm_timeout(struct usb_device *udev, return -EINVAL; } - ret = set_port_feature(udev->parent, + ret = usb_set_port_feature(udev->parent, USB_PORT_LPM_TIMEOUT(timeout) | udev->portnum, feature); if (ret < 0) { @@ -4449,7 +4387,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, /* maybe switch power back on (e.g. root hub was reset) */ if ((wHubCharacteristics & HUB_CHAR_LPSM) < 2 && !port_is_power_on(hub, portstatus)) - set_port_feature(hdev, port1, USB_PORT_FEAT_POWER); + usb_set_port_feature(hdev, port1, USB_PORT_FEAT_POWER); if (portstatus & USB_PORT_STAT_ENABLE) goto done; @@ -4722,7 +4660,8 @@ static void hub_events(void) /* deal with port status changes */ for (i = 1; i <= hdev->maxchild; i++) { - if (test_bit(i, hub->busy_bits)) + if (test_bit(i, hub->busy_bits) + || !usb_port_power_enabled(hub->ports[i-1])) continue; connect_change = test_bit(i, hub->change_bits); wakeup_change = test_and_clear_bit(i, hub->wakeup_bits); diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h index 9ea075d1b7a3..3eb51068c39f 100644 --- a/drivers/usb/core/hub.h +++ b/drivers/usb/core/hub.h @@ -88,8 +88,6 @@ 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_device *child; @@ -99,24 +97,20 @@ struct usb_port { struct dev_state *port_owner; enum usb_port_connect_type connect_type; u8 portnum; - unsigned power_is_on:1; - unsigned did_runtime_put:1; }; #define to_usb_port(_dev) \ container_of(_dev, struct usb_port, dev) -extern int usb_hub_create_port_device(struct usb_hub *hub, - int port1); -extern void usb_hub_remove_port_device(struct usb_hub *hub, - int port1); -extern int usb_hub_set_port_power(struct usb_device *hdev, struct usb_hub *hub, - int port1, bool set); +extern int usb_hub_create_port_device(struct usb_hub *hub, int port1); +bool usb_port_power_enabled(struct usb_port *port_dev); +extern void usb_hub_remove_port_device(struct usb_hub *hub, int port1); extern struct usb_hub *usb_hub_to_struct_hub(struct usb_device *hdev); extern int hub_port_debounce(struct usb_hub *hub, int port1, bool must_be_connected); extern int usb_clear_port_feature(struct usb_device *hdev, int port1, int feature); +extern int usb_set_port_feature(struct usb_device *hdev, int port1, int feature); static inline int hub_port_debounce_be_connected(struct usb_hub *hub, int port1) diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index 3c20cea08d50..89f451fa0ab8 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -84,6 +84,12 @@ static bool is_power_policy_on(struct usb_port *port_dev) return false; } +bool usb_port_power_enabled(struct usb_port *port_dev) +{ + return is_power_policy_on(port_dev) + || pm_runtime_active(&port_dev->dev); +} + static int usb_port_runtime_poweron(struct device *dev) { struct usb_port *port_dev = to_usb_port(dev); @@ -105,7 +111,7 @@ static int usb_port_runtime_poweron(struct device *dev) set_bit(port1, hub->busy_bits); - retval = usb_hub_set_port_power(hdev, hub, port1, true); + retval = usb_set_port_feature(hdev, port1, USB_PORT_FEAT_POWER); if (port_dev->child && !retval) { /* * Attempt to wait for usb hub port to be reconnected in order @@ -140,7 +146,7 @@ static int usb_port_runtime_poweroff(struct device *dev) return 0; set_bit(port1, hub->busy_bits); - retval = usb_hub_set_port_power(hdev, hub, port1, false); + retval = usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER); usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION); usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE); clear_bit(port1, hub->busy_bits); @@ -178,7 +184,6 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1) 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; -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html