From 1493138af1463112e42eebcdab5db61452821e97 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Thu, 5 Jan 2012 15:39:57 +0100 Subject: USB: code cleanup in suspend/resume path (3rd try) Do the cleanup to avoid behaviorial parameters Linus requested. Signed-off-by: Oliver Neukum Acked-by: Alan Stern Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/driver.c | 93 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 63 insertions(+), 30 deletions(-) (limited to 'drivers/usb/core/driver.c') diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index d40ff9568813..b7dfdecc7fda 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -958,13 +958,8 @@ void usb_rebind_intf(struct usb_interface *intf) int rc; /* Delayed unbind of an existing driver */ - if (intf->dev.driver) { - struct usb_driver *driver = - to_usb_driver(intf->dev.driver); - - dev_dbg(&intf->dev, "forced unbind\n"); - usb_driver_release_interface(driver, intf); - } + if (intf->dev.driver) + usb_forced_unbind_intf(intf); /* Try to rebind the interface */ if (!intf->dev.power.is_prepared) { @@ -977,15 +972,13 @@ void usb_rebind_intf(struct usb_interface *intf) #ifdef CONFIG_PM -#define DO_UNBIND 0 -#define DO_REBIND 1 - -/* Unbind drivers for @udev's interfaces that don't support suspend/resume, - * or rebind interfaces that have been unbound, according to @action. +/* Unbind drivers for @udev's interfaces that don't support suspend/resume + * There is no check for reset_resume here because it can be determined + * only during resume whether reset_resume is needed. * * The caller must hold @udev's device lock. */ -static void do_unbind_rebind(struct usb_device *udev, int action) +static void unbind_no_pm_drivers_interfaces(struct usb_device *udev) { struct usb_host_config *config; int i; @@ -996,23 +989,53 @@ static void do_unbind_rebind(struct usb_device *udev, int action) if (config) { for (i = 0; i < config->desc.bNumInterfaces; ++i) { intf = config->interface[i]; - switch (action) { - case DO_UNBIND: - if (intf->dev.driver) { - drv = to_usb_driver(intf->dev.driver); - if (!drv->suspend || !drv->resume) - usb_forced_unbind_intf(intf); - } - break; - case DO_REBIND: - if (intf->needs_binding) - usb_rebind_intf(intf); - break; + + if (intf->dev.driver) { + drv = to_usb_driver(intf->dev.driver); + if (!drv->suspend || !drv->resume) + usb_forced_unbind_intf(intf); } } } } +/* Unbind drivers for @udev's interfaces that failed to support reset-resume. + * These interfaces have the needs_binding flag set by usb_resume_interface(). + * + * The caller must hold @udev's device lock. + */ +static void unbind_no_reset_resume_drivers_interfaces(struct usb_device *udev) +{ + struct usb_host_config *config; + int i; + struct usb_interface *intf; + + config = udev->actconfig; + if (config) { + for (i = 0; i < config->desc.bNumInterfaces; ++i) { + intf = config->interface[i]; + if (intf->dev.driver && intf->needs_binding) + usb_forced_unbind_intf(intf); + } + } +} + +static void do_rebind_interfaces(struct usb_device *udev) +{ + struct usb_host_config *config; + int i; + struct usb_interface *intf; + + config = udev->actconfig; + if (config) { + for (i = 0; i < config->desc.bNumInterfaces; ++i) { + intf = config->interface[i]; + if (intf->needs_binding) + usb_rebind_intf(intf); + } + } +} + static int usb_suspend_device(struct usb_device *udev, pm_message_t msg) { struct usb_device_driver *udriver; @@ -1302,7 +1325,12 @@ int usb_suspend(struct device *dev, pm_message_t msg) { struct usb_device *udev = to_usb_device(dev); - do_unbind_rebind(udev, DO_UNBIND); + unbind_no_pm_drivers_interfaces(udev); + + /* From now on we are sure all drivers support suspend/resume + * but not necessarily reset_resume() + * so we may still need to unbind and rebind upon resume + */ choose_wakeup(udev, msg); return usb_suspend_both(udev, msg); } @@ -1313,15 +1341,20 @@ int usb_resume(struct device *dev, pm_message_t msg) struct usb_device *udev = to_usb_device(dev); int status; - /* For PM complete calls, all we do is rebind interfaces */ + /* For PM complete calls, all we do is rebind interfaces + * whose needs_binding flag is set + */ if (msg.event == PM_EVENT_ON) { if (udev->state != USB_STATE_NOTATTACHED) - do_unbind_rebind(udev, DO_REBIND); + do_rebind_interfaces(udev); status = 0; /* For all other calls, take the device back to full power and * tell the PM core in case it was autosuspended previously. - * Unbind the interfaces that will need rebinding later. + * Unbind the interfaces that will need rebinding later, + * because they fail to support reset_resume. + * (This can't be done in usb_resume_interface() + * above because it doesn't own the right set of locks.) */ } else { status = usb_resume_both(udev, msg); @@ -1329,7 +1362,7 @@ int usb_resume(struct device *dev, pm_message_t msg) pm_runtime_disable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); - do_unbind_rebind(udev, DO_REBIND); + unbind_no_reset_resume_drivers_interfaces(udev); } } -- cgit v1.2.3 From 98d9a82e5f753a2483d7b4638802d60e94e5d2e4 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Wed, 11 Jan 2012 08:38:35 +0100 Subject: USB: cleanup the handling of the PM complete call This eliminates the last instance of a function's behavior controlled by a parameter as Linus hates such things. Signed-off-by: Oliver Neukum Acked-by: Alan Stern Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/driver.c | 37 ++++++++++++++++++++----------------- drivers/usb/core/usb.c | 2 +- drivers/usb/core/usb.h | 1 + 3 files changed, 22 insertions(+), 18 deletions(-) (limited to 'drivers/usb/core/driver.c') diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index b7dfdecc7fda..d77daf3683da 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -1336,34 +1336,37 @@ int usb_suspend(struct device *dev, pm_message_t msg) } /* The device lock is held by the PM core */ -int usb_resume(struct device *dev, pm_message_t msg) +int usb_resume_complete(struct device *dev) { - struct usb_device *udev = to_usb_device(dev); - int status; + struct usb_device *udev = to_usb_device(dev); /* For PM complete calls, all we do is rebind interfaces * whose needs_binding flag is set */ - if (msg.event == PM_EVENT_ON) { - if (udev->state != USB_STATE_NOTATTACHED) - do_rebind_interfaces(udev); - status = 0; + if (udev->state != USB_STATE_NOTATTACHED) + do_rebind_interfaces(udev); + return 0; +} - /* For all other calls, take the device back to full power and +/* The device lock is held by the PM core */ +int usb_resume(struct device *dev, pm_message_t msg) +{ + struct usb_device *udev = to_usb_device(dev); + int status; + + /* For all calls, take the device back to full power and * tell the PM core in case it was autosuspended previously. * Unbind the interfaces that will need rebinding later, * because they fail to support reset_resume. * (This can't be done in usb_resume_interface() - * above because it doesn't own the right set of locks.) + * above because it doesn't own the right set of locks.) */ - } else { - status = usb_resume_both(udev, msg); - if (status == 0) { - pm_runtime_disable(dev); - pm_runtime_set_active(dev); - pm_runtime_enable(dev); - unbind_no_reset_resume_drivers_interfaces(udev); - } + status = usb_resume_both(udev, msg); + if (status == 0) { + pm_runtime_disable(dev); + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + unbind_no_reset_resume_drivers_interfaces(udev); } /* Avoid PM error messages for devices disconnected while suspended diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 8ca9f994a280..c74ba7bbc748 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -274,7 +274,7 @@ static int usb_dev_prepare(struct device *dev) static void usb_dev_complete(struct device *dev) { /* Currently used only for rebinding interfaces */ - usb_resume(dev, PMSG_ON); /* FIXME: change to PMSG_COMPLETE */ + usb_resume_complete(dev); } static int usb_dev_suspend(struct device *dev) diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h index 45e8479c377d..71648dcbe438 100644 --- a/drivers/usb/core/usb.h +++ b/drivers/usb/core/usb.h @@ -56,6 +56,7 @@ extern void usb_major_cleanup(void); extern int usb_suspend(struct device *dev, pm_message_t msg); extern int usb_resume(struct device *dev, pm_message_t msg); +extern int usb_resume_complete(struct device *dev); extern int usb_port_suspend(struct usb_device *dev, pm_message_t msg); extern int usb_port_resume(struct usb_device *dev, pm_message_t msg); -- cgit v1.2.3 From cef9bc56e1e944afd11f96de569657117a138c6d Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Tue, 24 Jan 2012 13:34:41 -0500 Subject: Dynamic ID addition doesn't need get_driver() As part of the removal of get_driver()/put_driver(), this patch (as1511) changes all the places that add dynamic IDs for drivers. Since these additions are done by writing to the drivers' sysfs attribute files, and the attributes are removed when the drivers are unregistered, there is no reason to take an extra reference to the drivers. The one exception is the pci-stub driver, which calls pci_add_dynid() as part of its registration. But again, there's no reason to take an extra reference here, because the driver can't be unloaded while it is being registered. Signed-off-by: Alan Stern CC: Dmitry Torokhov CC: Jiri Kosina CC: Jesse Barnes CC: Dominik Brodowski Signed-off-by: Greg Kroah-Hartman --- drivers/hid/hid-core.c | 6 +----- drivers/pci/pci-driver.c | 2 -- drivers/pcmcia/ds.c | 5 +---- drivers/usb/core/driver.c | 5 +---- 4 files changed, 3 insertions(+), 15 deletions(-) (limited to 'drivers/usb/core/driver.c') diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index af08ce7207d9..bce53fa0e166 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1619,11 +1619,7 @@ static ssize_t store_new_id(struct device_driver *drv, const char *buf, list_add_tail(&dynid->list, &hdrv->dyn_list); spin_unlock(&hdrv->dyn_lock); - ret = 0; - if (get_driver(&hdrv->driver)) { - ret = driver_attach(&hdrv->driver); - put_driver(&hdrv->driver); - } + ret = driver_attach(&hdrv->driver); return ret ? : count; } diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 3623d65f8b86..ff540477fe8b 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -72,9 +72,7 @@ int pci_add_dynid(struct pci_driver *drv, list_add_tail(&dynid->node, &drv->dynids.list); spin_unlock(&drv->dynids.lock); - get_driver(&drv->driver); retval = driver_attach(&drv->driver); - put_driver(&drv->driver); return retval; } diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c index 749c2a16012c..059699f6363d 100644 --- a/drivers/pcmcia/ds.c +++ b/drivers/pcmcia/ds.c @@ -127,10 +127,7 @@ pcmcia_store_new_id(struct device_driver *driver, const char *buf, size_t count) list_add_tail(&dynid->node, &pdrv->dynids.list); mutex_unlock(&pdrv->dynids.lock); - if (get_driver(&pdrv->drv)) { - retval = driver_attach(&pdrv->drv); - put_driver(&pdrv->drv); - } + retval = driver_attach(&pdrv->drv); if (retval) return retval; diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index d40ff9568813..54c493b4226b 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -71,10 +71,7 @@ ssize_t usb_store_new_id(struct usb_dynids *dynids, list_add_tail(&dynid->node, &dynids->list); spin_unlock(&dynids->lock); - if (get_driver(driver)) { - retval = driver_attach(driver); - put_driver(driver); - } + retval = driver_attach(driver); if (retval) return retval; -- cgit v1.2.3 From ed283e9f0a2cc0541870828c76c6c6997c51a318 Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Tue, 24 Jan 2012 14:35:13 -0500 Subject: USB/PCI/PCMCIA: Clean up new_id and remove_id sysfs attribute routines This patch (as1514) cleans up some places where new_id and remove_id sysfs attributes are created and deleted. Handling both attributes in a single routine rather than a pair of routines makes the code smaller. It also prevents certain kinds of errors, like one we currently have in the USB subsystem: The removeid attribute is often created even when newid isn't (because the driver's no_dynamid_id flag is set). In the case of the PCMCIA subsystem, the newid attribute is created but never explicitly deleted. The patch adds a deletion routine. Signed-off-by: Alan Stern Acked-by: Jesse Barnes Acked-by: Dominik Brodowski Signed-off-by: Greg Kroah-Hartman --- drivers/pci/pci-driver.c | 50 +++++++++++++------------------------- drivers/pcmcia/ds.c | 6 +++++ drivers/usb/core/driver.c | 61 ++++++++++++++++------------------------------- 3 files changed, 43 insertions(+), 74 deletions(-) (limited to 'drivers/usb/core/driver.c') diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index ff540477fe8b..8d9616b821ca 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -188,43 +188,34 @@ store_remove_id(struct device_driver *driver, const char *buf, size_t count) static DRIVER_ATTR(remove_id, S_IWUSR, NULL, store_remove_id); static int -pci_create_newid_file(struct pci_driver *drv) +pci_create_newid_files(struct pci_driver *drv) { int error = 0; - if (drv->probe != NULL) - error = driver_create_file(&drv->driver, &driver_attr_new_id); - return error; -} - -static void pci_remove_newid_file(struct pci_driver *drv) -{ - driver_remove_file(&drv->driver, &driver_attr_new_id); -} -static int -pci_create_removeid_file(struct pci_driver *drv) -{ - int error = 0; - if (drv->probe != NULL) - error = driver_create_file(&drv->driver,&driver_attr_remove_id); + if (drv->probe != NULL) { + error = driver_create_file(&drv->driver, &driver_attr_new_id); + if (error == 0) { + error = driver_create_file(&drv->driver, + &driver_attr_remove_id); + if (error) + driver_remove_file(&drv->driver, + &driver_attr_new_id); + } + } return error; } -static void pci_remove_removeid_file(struct pci_driver *drv) +static void pci_remove_newid_files(struct pci_driver *drv) { driver_remove_file(&drv->driver, &driver_attr_remove_id); + driver_remove_file(&drv->driver, &driver_attr_new_id); } #else /* !CONFIG_HOTPLUG */ -static inline int pci_create_newid_file(struct pci_driver *drv) +static inline int pci_create_newid_files(struct pci_driver *drv) { return 0; } -static inline void pci_remove_newid_file(struct pci_driver *drv) {} -static inline int pci_create_removeid_file(struct pci_driver *drv) -{ - return 0; -} -static inline void pci_remove_removeid_file(struct pci_driver *drv) {} +static inline void pci_remove_newid_files(struct pci_driver *drv) {} #endif /** @@ -1136,18 +1127,12 @@ int __pci_register_driver(struct pci_driver *drv, struct module *owner, if (error) goto out; - error = pci_create_newid_file(drv); + error = pci_create_newid_files(drv); if (error) goto out_newid; - - error = pci_create_removeid_file(drv); - if (error) - goto out_removeid; out: return error; -out_removeid: - pci_remove_newid_file(drv); out_newid: driver_unregister(&drv->driver); goto out; @@ -1166,8 +1151,7 @@ out_newid: void pci_unregister_driver(struct pci_driver *drv) { - pci_remove_removeid_file(drv); - pci_remove_newid_file(drv); + pci_remove_newid_files(drv); driver_unregister(&drv->driver); pci_free_dynids(drv); } diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c index 059699f6363d..249b8895807d 100644 --- a/drivers/pcmcia/ds.c +++ b/drivers/pcmcia/ds.c @@ -157,6 +157,11 @@ pcmcia_create_newid_file(struct pcmcia_driver *drv) return error; } +static void +pcmcia_remove_newid_file(struct pcmcia_driver *drv) +{ + driver_remove_file(&drv->drv, &driver_attr_new_id); +} /** * pcmcia_register_driver - register a PCMCIA driver with the bus core @@ -201,6 +206,7 @@ EXPORT_SYMBOL(pcmcia_register_driver); void pcmcia_unregister_driver(struct pcmcia_driver *driver) { pr_debug("unregistering driver %s\n", driver->name); + pcmcia_remove_newid_file(driver); driver_unregister(&driver->drv); pcmcia_free_dynids(driver); } diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index 54c493b4226b..4fee024ecc9b 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -129,43 +129,39 @@ store_remove_id(struct device_driver *driver, const char *buf, size_t count) } static DRIVER_ATTR(remove_id, S_IWUSR, NULL, store_remove_id); -static int usb_create_newid_file(struct usb_driver *usb_drv) +static int usb_create_newid_files(struct usb_driver *usb_drv) { int error = 0; if (usb_drv->no_dynamic_id) goto exit; - if (usb_drv->probe != NULL) + if (usb_drv->probe != NULL) { error = driver_create_file(&usb_drv->drvwrap.driver, &driver_attr_new_id); + if (error == 0) { + error = driver_create_file(&usb_drv->drvwrap.driver, + &driver_attr_remove_id); + if (error) + driver_remove_file(&usb_drv->drvwrap.driver, + &driver_attr_new_id); + } + } exit: return error; } -static void usb_remove_newid_file(struct usb_driver *usb_drv) +static void usb_remove_newid_files(struct usb_driver *usb_drv) { if (usb_drv->no_dynamic_id) return; - if (usb_drv->probe != NULL) + if (usb_drv->probe != NULL) { driver_remove_file(&usb_drv->drvwrap.driver, - &driver_attr_new_id); -} - -static int -usb_create_removeid_file(struct usb_driver *drv) -{ - int error = 0; - if (drv->probe != NULL) - error = driver_create_file(&drv->drvwrap.driver, &driver_attr_remove_id); - return error; -} - -static void usb_remove_removeid_file(struct usb_driver *drv) -{ - driver_remove_file(&drv->drvwrap.driver, &driver_attr_remove_id); + driver_remove_file(&usb_drv->drvwrap.driver, + &driver_attr_new_id); + } } static void usb_free_dynids(struct usb_driver *usb_drv) @@ -180,22 +176,12 @@ static void usb_free_dynids(struct usb_driver *usb_drv) spin_unlock(&usb_drv->dynids.lock); } #else -static inline int usb_create_newid_file(struct usb_driver *usb_drv) -{ - return 0; -} - -static void usb_remove_newid_file(struct usb_driver *usb_drv) -{ -} - -static int -usb_create_removeid_file(struct usb_driver *drv) +static inline int usb_create_newid_files(struct usb_driver *usb_drv) { return 0; } -static void usb_remove_removeid_file(struct usb_driver *drv) +static void usb_remove_newid_files(struct usb_driver *usb_drv) { } @@ -872,22 +858,16 @@ int usb_register_driver(struct usb_driver *new_driver, struct module *owner, usbfs_update_special(); - retval = usb_create_newid_file(new_driver); + retval = usb_create_newid_files(new_driver); if (retval) goto out_newid; - retval = usb_create_removeid_file(new_driver); - if (retval) - goto out_removeid; - pr_info("%s: registered new interface driver %s\n", usbcore_name, new_driver->name); out: return retval; -out_removeid: - usb_remove_newid_file(new_driver); out_newid: driver_unregister(&new_driver->drvwrap.driver); @@ -914,10 +894,9 @@ void usb_deregister(struct usb_driver *driver) pr_info("%s: deregistering interface driver %s\n", usbcore_name, driver->name); - usb_remove_removeid_file(driver); - usb_remove_newid_file(driver); - usb_free_dynids(driver); + usb_remove_newid_files(driver); driver_unregister(&driver->drvwrap.driver); + usb_free_dynids(driver); usbfs_update_special(); } -- cgit v1.2.3