remove driver model detach_state

PATCH Applied David Brownell 5 participants 6 messages May 12, 2005 – May 13, 2005

Subsystems (5)

Documentation Driver core Suspend to RAM Hibernation +1 more subsystems

lore

Share & tools

lore

David Brownell

author

2005-05-12 19:06 UTC #0 lore raw

PATCH 1/1 remove driver model detach_state

This mechanism has never AFAICT been used, and I asked a while back if anyone knew of uses ... no takers, even though it's been in the kernel for almost two years by now.

Here's a patch to remove the thing. AFAICT, suitable to merge ASAP; it reduces sysfs size impact, and is a net code shrink.

- Dave

The driver model has a "detach_state" mechanism that:

- Has never been used by any in-kernel drive;
- Is superfluous, since driver remove() methods can do the same thing;
- Became buggy when the suspend() parameter changed semantics and type;
- Could self-deadlock when called from certain suspend contexts;
- Is effectively wasted documentation, object code, and headspace.

This removes that "detach_state" mechanism; net code shrink, as well as a per-device saving in the driver model and sysfs.

Signed-off-by David Brownell <[email protected]>

include/linux/device.h +0 -3

@@ -273,9 +273,6 @@
 					   BIOS data relevant to device) */
 	struct dev_pm_info	power;
 
-	u32		detach_state;	/* State to enter when device is
-					   detached from its driver. */
-
 	u64		*dma_mask;	/* dma mask (if dma'able device) */
 	u64		coherent_dma_mask;/* Like dma_mask, but for
 					     alloc_coherent mappings as

drivers/base/power/power.h +0 ~~-11~~

@@ -1,18 +1,7 @@
-
-
-enum {
-	DEVICE_PM_ON,
-	DEVICE_PM1,
-	DEVICE_PM2,
-	DEVICE_PM3,
-	DEVICE_PM_OFF,
-};
-
 /*
  * shutdown.c
  */
 
-extern int device_detach_shutdown(struct device *);
 extern void device_shutdown(void);

drivers/base/power/shutdown.c +0 ~~-14~~

@@ -19,20 +19,6 @@
 extern struct subsystem devices_subsys;
 
 
-int device_detach_shutdown(struct device * dev)
-{
-	if (!dev->detach_state)
-		return 0;
-
-	if (dev->detach_state == DEVICE_PM_OFF) {
-		if (dev->driver && dev->driver->shutdown)
-			dev->driver->shutdown(dev);
-		return 0;
-	}
-	return dpm_runtime_suspend(dev, dev->detach_state);
-}
-
-
 /**
  * We handle system devices differently - we suspend and shut them
  * down last and resume them first. That way, we don't do anything stupid like

drivers/base/bus.c +0 -1

@@ -390,7 +390,6 @@
 		sysfs_remove_link(&drv->kobj, kobject_name(&dev->kobj));
 		sysfs_remove_link(&dev->kobj, "driver");
 		list_del_init(&dev->driver_list);
-		device_detach_shutdown(dev);
 		if (drv->remove)
 			drv->remove(dev);
 		dev->driver = NULL;

drivers/base/Makefile +1 -1

@@ -1,6 +1,6 @@
 # Makefile for the Linux device tree
 
-obj-y			:= core.o sys.o interface.o bus.o \
+obj-y			:= core.o sys.o bus.o \
 			   driver.o class.o class_simple.o platform.o \
 			   cpu.o firmware.o init.o map.o dmapool.o \
 			   attribute_container.o transport_class.o

drivers/base/interface.c deleted +0 ~~-51~~

51 lines not on this page · raw message

drivers/base/core.c +0 -3

@@ -31,8 +31,6 @@
 #define to_dev(obj) container_of(obj, struct device, kobj)
 #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
 
-extern struct attribute * dev_default_attrs[];
-
 static ssize_t
 dev_attr_show(struct kobject * kobj, struct attribute * attr, char * buf)
 {

@@ -89,7 +87,6 @@
 static struct kobj_type ktype_device = {
 	.release	= device_release,
 	.sysfs_ops	= &dev_sysfs_ops,
-	.default_attrs	= dev_default_attrs,
 };

Documentation/filesystems/sysfs-pci.txt +2 -4

@@ -7,7 +7,6 @@
      |-- 0000:17:00.0
      |   |-- class
      |   |-- config
-     |   |-- detach_state
      |   |-- device
      |   |-- irq
      |   |-- local_cpus

@@ -19,7 +18,7 @@
      |   |-- subsystem_device
      |   |-- subsystem_vendor
      |   `-- vendor
-     `-- detach_state
+     `-- ...
 
 The topmost element describes the PCI domain and bus number.  In this case,
 the domain number is 0000 and the bus number is 17 (both values are in hex).

@@ -31,7 +30,6 @@
        ----		   --------
        class		   PCI class (ascii, ro)
        config		   PCI config space (binary, rw)
-       detach_state	   connection status (bool, rw)
        device		   PCI device (ascii, ro)
        irq		   IRQ number (ascii, ro)
        local_cpus	   nearby CPU mask (cpumask, ro)

@@ -85,4 +83,4 @@
 
 Legacy resources are protected by the HAVE_PCI_LEGACY define.  Platforms
 wishing to support legacy functionality should define it and provide
-pci_legacy_read, pci_legacy_write and pci_mmap_legacy_page_range functions.
\ No newline at end of file
+pci_legacy_read, pci_legacy_write and pci_mmap_legacy_page_range functions.

Documentation/power/devices.txt +0 ~~-21~~

@@ -207,27 +207,6 @@
 #READY_AFTER_RESUME
 #
 
-Driver Detach Power Management
-
-The kernel now supports the ability to place a device in a low-power
-state when it is detached from its driver, which happens when its
-module is removed. 
-
-Each device contains a 'detach_state' file in its sysfs directory
-which can be used to control this state. Reading from this file
-displays what the current detach state is set to. This is 0 (On) by
-default. A user may write a positive integer value to this file in the
-range of 1-4 inclusive. 
-
-A value of 1-3 will indicate the device should be placed in that
-low-power state, which will cause ->suspend() to be called for that
-device. A value of 4 indicates that the device should be shutdown, so
-->shutdown() will be called for that device. 
-
-The driver is responsible for reinitializing the device when the
-module is re-inserted during it's ->probe() (or equivalent) method. 
-The driver core will not call any extra functions when binding the
-device to the driver. 
 
 pm_message_t meaning

Documentation/powerpc/hvcs.txt +2 -2

@@ -347,8 +347,8 @@
 looks like the following:
 
 	Pow5:/sys/bus/vio/drivers/hvcs/30000004 # ls
-	.   current_vty   devspec  name          partner_vtys
-	..  detach_state  index    partner_clcs  vterm_state
+	.   current_vty   devspec       name          partner_vtys
+	..  index         partner_clcs  vterm_state
 
 Each entry is provided, by default with a "name" attribute.  Reading the
 "name" attribute will reveal the device type as shown in the following

Alan Stern

2005-05-12 19:31 UTC #1 lore raw

Quoted 6 lines · David Brownell

This mechanism has never AFAICT been used, and I asked a while back if anyone knew of uses ... no takers, even though it's been in the kernel for almost two years by now.
Here's a patch to remove the thing. AFAICT, suitable to merge ASAP; it reduces sysfs size impact, and is a net code shrink.

Note that for Greg's "driver" tree, some of the changes you made to bus.c will have to apply to dd.c instead. Maybe Greg will fix this up by himself; I still don't know how he manages to juggle multiple conflicting source trees...

On the whole this looks like a good thing to do.

Alan Stern

Greg KH

2005-05-12 20:37 UTC #2 lore raw

Quoted 6 lines · Alan Stern

Quoted 4 lines · David Brownell
This mechanism has never AFAICT been used, and I asked a while back if anyone knew of uses ... no takers, even though it's been in the kernel for almost two years by now.

Quoted 7 lines

Quoted 2 lines
Here's a patch to remove the thing. AFAICT, suitable to merge ASAP; it reduces sysfs size impact, and is a net code shrink.
Note that for Greg's "driver" tree, some of the changes you made to bus.c will have to apply to dd.c instead. Maybe Greg will fix this up by himself; I still don't know how he manages to juggle multiple conflicting source trees...

Don't worry, I can handle it :)

Quoted 1 line

On the whole this looks like a good thing to do.

I agree. Will apply it later this evening.

greg k-h

Adam Belay

2005-05-12 22:06 UTC #3 lore raw

Quoted 15 lines · Greg KH

Quoted 6 lines · Alan Stern
Quoted 4 lines · David Brownell
This mechanism has never AFAICT been used, and I asked a while back if anyone knew of uses ... no takers, even though it's been in the kernel for almost two years by now.
Quoted 7 lines
Quoted 2 lines
Here's a patch to remove the thing. AFAICT, suitable to merge ASAP; it reduces sysfs size impact, and is a net code shrink.
Note that for Greg's "driver" tree, some of the changes you made to bus.c will have to apply to dd.c instead. Maybe Greg will fix this up by himself; I still don't know how he manages to juggle multiple conflicting source trees...

Quoted 7 lines

Don't worry, I can handle it :)
Quoted 1 line
On the whole this looks like a good thing to do.
I agree. Will apply it later this evening.
greg k-h

Although I agree with this patch, I think it's important that we don't forget about the code's original intentions. When manual driver binding and unbinding is supported, it would be nice if we could specify a detach state. My guess is that this should only include "on" or "off", but not any intermediate states.

->remove isn't good enough, as the driver developer makes the decision "let's turn off the device before detaching" or "let's leave it on". This will vary between implementations. I think it may be better to allow the user to specify this.

Finally, if the device is disabled, the configuration (e.g. PCI config space) may need to be restored for the device to work on a future driver. This seems like a tricky problem if the driver is removed.

Selective suspend may be a better alternative, in which case, we need to decide if it will unregister class devices etc. (as I suggested earlier) or just do a somewhat regular device_suspend().

Driver binding/unbinding and requests to disable a device play an interesting role in power management. It would be nice if we could define some goals for this.

Thanks,
Adam

David Brownell

author

2005-05-13 07:31 UTC #4 lore raw

Quoted 11 lines · Adam Belay

Although I agree with this patch, I think it's important that we don't forget about the code's original intentions. When manual driver binding and unbinding is supported, it would be nice if we could specify a detach state. My guess is that this should only include "on" or "off", but not any intermediate states.
->remove isn't good enough, as the driver developer makes the decision "let's turn off the device before detaching" or "let's leave it on". This will vary between implementations. I think it may be better to allow the user to specify this.

Users haven't yet needed it ... or sysadmins either. I don't see why this should need any policy decisions at all. If unused/idle devices would just stay in low power modes all the time, then the only issue would be buggy hardware ... and buggy hardware calls for hardware-specific workarounds, always. (Either in infrastructure quirk code, or in the devices' drivers.)

For example, lots of ARM chips (in embedded hardware) leave most things unclocked, and where relevant also powered down, until they're needed. That's just the way things are ... reset powers things down, the bootloader (maybe something like U-Boot) turns on enough to get Linux started, and then Linux turns on other things as needed.

Many PCI devices could do the same thing, and stay in D3hot state until some more active mode is needed.

Quoted 3 lines

Finally, if the device is disabled, the configuration (e.g. PCI config space) may need to be restored for the device to work on a future driver. This seems like a tricky problem if the driver is removed.

Drivers are expected to initialize using the current hardware state. If they can't, it's a driver bug. They normally leave PCI config space alone.

- Dave

[email protected]

Applied

2005-05-13 18:37 UTC #5 lore raw

patch driver-remove-detach_state.patch added to gregkh-2.6 tree

In reply to David Brownell

This is a note to let you know that I've just added the patch titled

Subject: Driver core: remove driver model detach_state

to my gregkh-2.6 tree. Its filename is

driver-remove-detach_state.patch

This tree can be found at
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/

Patches currently in gregkh-2.6 which might be from [email protected] are

driver/driver-pm-diag-update.patch driver/driver-remove-detach_state.patch usb/usb-omap_udc_cleanups.patch usb/usb-ethernet_gadget_cleanups.patch usb/usb-rndis_cleanups.patch usb/usb-gadget-kconfig.patch usb/usb-dummy_hcd-otg.patch usb/usb-ohci_reboot_notifier.patch usb/usb-usbtest.patch usb/usb-urb_documentation.patch usb/usb-ehci-minor-updates.patch usb/usb-isp116x-hcd-add.patch usb/usb-usbnet-fixes.patch usb/usb-omap_udc_update.patch usb/usb-turn-a-user-mode-driver-error-into-a-hard-error.patch usb/usb-gadget-pxa2xx_udc-updates.patch usb/usb-gadget-setup-api-change-goku_udc.patch usb/usb-gadget-setup-api-change-net2280.patch usb/usb-gadget-setup-api-change.patch usb/usb-ehci-suspend-stop-timer.patch