call drv->shutdown at rmmod

At the kexec BOF at OSDL there was some discussion on calling the device shutdown method at module remove time, in addition to calling it during reboot. The driver was the observation that the primary source of problems in booting linux from linux are drivers with bad or missing drv->shutdown() routines. The hope is this will increase the testing so people can get it right and kexec can become more useful. In addition to making normal reboots more reliable.

The following patch is an implementation of that idea it calls drv->shutdown() before calling drv->remove(). If drv->shutdown() is implemented.

In addition the driver model documentation in 2.6.0-test3 is badly out of date. So I have attached a minor correction which at least mentions drv->shutdown().

Eric

diff -uNr linux-2.6.0-test3/Documentation/driver-model/driver.txt linux-2.6.0-test3-shutdown_before_remove/Documentation/driver-model/driver.txt
@@ -16,10 +16,10 @@
         int     (*probe)        (struct device * dev);
         int     (*remove)       (struct device * dev);
 
+	void    (*shutdown)     (struct device * dev);
+
         int     (*suspend)      (struct device * dev, u32 state, u32 level);
         int     (*resume)       (struct device * dev, u32 level);
-
-        void    (*release)      (struct device_driver * drv);
 };
 
 

@@ -194,6 +194,18 @@
 
 If the device is still present, it should quiesce the device and place
 it into a supported low-power state.
+
+
+	void    (*shutdown)     (struct device * dev);
+
+shutdown is called to quiescent a device before a reboot, or before
+the device is removed.  A device is quiescent if all on going
+transactions are stopped, and it is not setup to spontaneously
+generate new ones.  In addition the device should be in a state
+that it is reasonable for the drivers initialization code can get it
+working again.  shutdown is a separate case from remove because on a
+reboot the data structures do not need to be freed, and not freeing
+them increases the robustness of a reboot.
 
 	int	(*suspend)	(struct device * dev, u32 state, u32 level);
 

diff -uNr linux-2.6.0-test3/drivers/base/bus.c linux-2.6.0-test3-shutdown_before_remove/drivers/base/bus.c
@@ -350,6 +350,8 @@
 	if (drv) {
 		sysfs_remove_link(&drv->kobj,dev->kobj.name);
 		list_del_init(&dev->driver_list);
+		if (drv->shutdown)
+			drv->shutdown(dev);
 		if (drv->remove)
 			drv->remove(dev);
 		dev->driver = NULL;

Sounds really confusing. And having shutdown maybe called before remove but not always sounds like a design mistake.

Why do we have shutdown at all? Can't we just call ->remove on shutdown so the device always get's into proper unitialized state on shutdown, too?

That's likely to remove the keyboard driver, and some people like to configure their box so that ctrl-alt-del halts the system, and a further ctrl-alt-del reboots the system once halted.

There are also some bus drivers which want to know the difference between "device (driver) was removed" and "device was shutdown", eg, for setting bus-specific stuff back into a state where the machine can be soft-rebooted.

With the shutdown callback, drivers get the option whether to handle this event or not. Combining it with remove gives them no option what so ever, and bus drivers loose this knowledge.

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

My patch always called shutdown before remove, but only if the methods are implemented. Mandating that shutdown and remove are implemented is not something I am changing with this patch.

Hmm. That is a very weird case. Semantically the keyboard driver and everything else should be removed in any case.

After device_shutdown is called it is improper for any driver to be handling interrupts because the are supposed to be in a quiescent state. And if they are not it is likely to break a soft reboot.

Reasonable. And they still get that knowledge with the way I implemented my patch. They just get to shutdown first.

Russell do you have any objects to always calling shutdown before
remove? I don't think this looses knowledge and in most cases it should
work, but if there are bus devices were we need to do things significantly differently I am open to other solutions.

All I really care about is that we get something that is simple enough that device driver authors can get it right. And according to my limited testing we don't have that right now.

Eric

I don't view it as "really weird" - it's something I've always done with 2.2 and 2.4, and in fact the first thing I do when I get a machine is to modify the inittab to halt the machine on ctrl-alt-del.

It's far safer than hitting ctrl-alt-del and trying to power the machine off at the exact moment it reboots.

However, sometimes you want it to reboot, and in this case its far simpler to wait for the machine to halt, and then use ctrl-alt-del again. It's something that's been supported by both the kernel and init for eons.

I guess then this is another bug we need to add to the list of bugs introduced during 2.5 into 2.6 then...

If it is changing, then someone needs to get that information into davej's document.

The way I'm treating ->shutdown at present is that it is the final call to the device driver. Once this call has been made, the bus driver puts the bus into the correct state for reboot, and the device driver must not attempt to access it.

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

This sounds reasonable. Something that needs to happen is we need to distinguish clearly, between the semantics of a halt and reboot.

For a soft reboot to be safe we need to shutdown all of the drivers.

However what does it mean to call halt?
There are two possibilities.
- A frozen in amber state where the kernel just sits in a busy loop, and possibly the system is powered off, nothing new can be stared but a few remaining things continue on.
- Everything happens like a reboot except we don't transition to any other code.

So to be clear the cases we have to get semantics straight for are.

	case LINUX_REBOOT_CMD_RESTART:
        /* This is a reboot and things are fairly clear */

	case LINUX_REBOOT_CMD_RESTART2:
        /* This is the reboot case and it is fairly clear what needs to
         * happen there.  Of the two this case is biased towards
         * returning to the firmware if there need to be any differences.
         */

	case LINUX_REBOOT_CMD_HALT:
        /* this is the generic halt case and the most confusing.


	case LINUX_REBOOT_CMD_POWER_OFF:
        /* This is the power off case and similarly confusing. */

I think my vote goes for a frozen in amber state where the drivers do nothing except for the little bit of code in machine_halt or machine_power_off, which are practically noops, on x86. And as long as input is event interrupt driven you can do something with it.

I like it because doing absolutely nothing is very much KISS and easy to get right.

If the kernel should just go into some form of busy loop when halt is called, I agree that calling device_shutdown on halt is a bug. If a halt is just a reboot where we don't do anything afterwards then 2.6 is correct, and the init example is unsupportable.

Possibly what is required is another case of sys_reboot so we can have both semantics but that feels like over kill.

I will have to look, I am not up to speed on that one.

I agree with that interpretation of ->shutdown. And that is why I contend it is wrong to access the keyboard controller after
device_shutdown is called. Because device_shutdown calls ->shutdown
on every device. In that case all ->remove could legitimately do in remove is to free the device data structures.

I think calling ->shutdown, ->remove when a device goes away or when we remove the module is compatible with the current semantics of ->shutdown. Although we might need to update a driver or two of the set that has already implemented a ->shutdown method. But I don't think that case is terribly likely to cause problems in practice.

Eric

For a halt quiescing devices is overkill, historically wrong, and error prone when the system is halted. The only drivers that should care are drivers for devices that do the wrong thing when power is removed.

diff -uNr linux-2.6.0-test3/kernel/sys.c linux-2.6.0-test3-no_device_shutdown/kernel/sys.c
@@ -423,7 +423,6 @@
 	case LINUX_REBOOT_CMD_HALT:
 		notifier_call_chain(&reboot_notifier_list, SYS_HALT, NULL);
 		system_running = 0;
-		device_shutdown();
 		printk(KERN_EMERG "System halted.\n");
 		machine_halt();
 		unlock_kernel();

@@ -433,7 +432,6 @@
 	case LINUX_REBOOT_CMD_POWER_OFF:
 		notifier_call_chain(&reboot_notifier_list, SYS_POWER_OFF, NULL);
 		system_running = 0;
-		device_shutdown();
 		printk(KERN_EMERG "Power down.\n");
 		machine_power_off();
 		unlock_kernel();

Several people have asked me how to set this up, so I'll give the info here.

- change inittab to call shutdown with -h instead of -r
- telinit Q

I think there is a catch on x86 though - I think shutdown -h powers the machine down rather than halting it. The redhat initscripts seem to need a /halt file to prevent poweroff. Maybe calling /sbin/halt instead of shutdown from the initscripts would work better in those cases...

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

[ I'm curious as to why you didn't cc me in the first place.. ]

I like the idea behind the patch, but we shouldn't be calling it unconditionally. We're bound to run into some suprises that could really kill us this late in the 2.6 game.

I do think it should go in, as long as there is a flag telling the core whether or not to call shutdown for that particular device. I think it could also be extended to include a power state, so the core could suspend the device before removing the module.

The default would always be 'Do Nothing', but with a per-device sysfs file, a developer/user/gui app could be used to set the policy from user space.

-pat

My apologies, I meant to. Somehow I got Pavel Macheck instead of
Patrick Mochel. The names are only superficially similar but mistakes
happen.

Assuming the device driver can get a device out of the suspend state when the module is loaded. This has been one of the biggest problem areas with the e100 driver. It's init code cannot bring the device out of a low power state.

Possibly. But this is getting complicated. And while I do support things being complicated enough to handle the problem this part of the API feels like it is excessively complicated. Which is why I was trying to simply it by always calling shutdown on a device before we remove it.

Eric

You're assuming that a driver can always bring a device out a shutdown state. That's one of the things we talked about at OLS, and the other half of the justification behind such a feature, not just making sure the device is queisced. Your argument against my suggestion are some of the same arguments for a feature like you're introducing.

We have similar goals - introduce device power state code paths into common operations (e.g module removal). Doing so gets the code paths tested, which will help us ultimately achieve our utopian goals. And, it can be done in one shot.

Eh? This is complicated? especially compared your overall goal, kexec? Let me explain again:

I won't take the patch to unconditionally shutdown devices on module removal, so you need some sort of flag. But, you must have some way to set the flag, which is what sysfs is designed for. While you're at it, we might as well make it an integer value, rather than a pure binary one, and conditionally attempt to set the power state if the user says so.

It's actually pretty simple, and if it is confusing, then sit back and wait, and I'll provide documentation when I submit the patch.

Pat

I am assuming that is a requirement that it is possible for software to bring a device out of the state is place in by the ->shutdown method. ->shutdown is called on the reboot path and if it results in a pure software only reboot (i.e. the reset line on the motherboard is not toggled), then the device driver for the OS loaded after the reboot must reinitialize it.

Yes. My point is that I don't especially like having an additional argument as that introduces more conditional control flow cases. My experience suggests unconditional control flow is more likely to be implemented properly.

If it can be done reasonably I am for it.

Except that kexec suffers from the same class of issues a cooperative multitasking (every component must do the right thing), it is quite simple. All of my code paths are written assuming they are the slow uncommon path, so they place the smallest possible burden on the rest of the kernel.

I think I now see where you are coming from. Instead of changing the production code paths. Introducing a set of testing code paths to assist with driver testing. At least that is the feel I am getting.

Eric

There is a problem of semantics here. Is shutdown() supposed to shutdown the hardware device (ie. low power) or just the driver ? If yes, then it's duplicate of the PM callbacks. My understanding of the shutdown() callback is that it was more than "stop driver activity, put device into idle state" to prepare for a shutdown/reboot (though we do also sleep IDE drives in this case, but this is because of that nasty cache flush issue).

The problem with kexec is just that. What it needs isn't low power devices, it needs device back in "idle" state, but if possible powered up (or at least in whatever state the driver found them on boot). The most important thing is to actually stop pending bus mastering activities.

On PPC, we have a name for that which comes from Open Firmware (since we need to ask the firmware to stop bus mastering & idle devices the same way when we take over it and before we get control of the system memory) and it's called "quiesce".

Ben.

Even if kexec is not brought into the picture the devices need to be quiesed on reboot. On x86 and probably other architectures there are 2 ways a reboot can go.
1) The firmware when it regains control toggles the motherboard reset line resetting all of the devices, so nothing we do really makes a difference.
2) The firmware when it regains control tweaks a few things and pretends it was never out of control, and restarts the boot process.

When the firmware does not toggle the motherboard reset line during a reboot the firmware case is exactly equivalent to the kexec one.

So shutdown needs to quiese things.

Eric

Yup, my point was mostly to say that quiescing for shutdown and putting in low power state are 2 different things, and that the "shutdown" name for the callback is a bit misleading...

Ben.

You have it right - ->shutdown() is only supposed to queisce the device.

Pat

On 14 Aug 2003 10:26:47 -0600 [email protected] (Eric W. Biederman) wrote:

| Patrick Mochel <[email protected]> writes:
| 
| > > At the kexec BOF at OSDL there was some discussion on calling the
| > > device shutdown method at module remove time, in addition to calling
| > > it during reboot.  The driver was the observation that the primary
| > > source of problems in booting linux from linux are drivers with bad
| > > or missing drv->shutdown() routines.  The hope is this will increase
| > > the testing so people can get it right and kexec can become more
| > > useful.  In addition to making normal reboots more reliable.
| > > 
| > > The following patch is an implementation of that idea it calls drv->shutdown()
| > 
| > > before calling drv->remove().  If drv->shutdown() is implemented.
| > 
| > I like the idea behind the patch, but we shouldn't be calling it 
| > unconditionally. We're bound to run into some suprises that could really 
| > kill us this late in the 2.6 game. 
| > 
| > I do think it should go in, as long as there is a flag telling the core 
| > whether or not to call shutdown for that particular device. I think it 
| > could also be extended to include a power state, so the core could suspend 
| > the device before removing the module. 
| 
| Assuming the device driver can get a device out of the suspend state
| when the module is loaded.  This has been one of the biggest problem areas
| with the e100 driver.  It's init code cannot bring the device out of a low
| power state.
| 
| > The default would always be 'Do Nothing', but with a per-device sysfs 
| > file, a developer/user/gui app could be used to set the policy from user 
| > space. 
| 
| Possibly.  But this is getting complicated.  And while I do support things
| being complicated enough to handle the problem this part of the API feels
| like it is excessively complicated.  Which is why I was trying to simply
| it by always calling shutdown on a device before we remove it.

Can it just be a (kernel hacking/debug) CONFIG_DRIVER_SHUTDOWN build option?

--
~Randy