prevent swsusp from eating disks
Hi!
Patrick broke IDE so it wanted to eat data, again. Fixed, please apply,
Pavel
drivers/ide/ide-disk.c +4 -2
@@ -1891,8 +1891,10 @@static int idedisk_init (void ) { -ide_register_driver (&idedisk_driver); -return 0 ; +int status =ide_register_driver (&idedisk_driver); + idedisk_driver.gen_driver.suspend = idedisk_suspend; + idedisk_driver.gen_driver.resume = idedisk_resume; +return status; }module_init (idedisk_init);
drivers/ide/ide-probe.c +1 -0
@@ -1059,6 +1059,7 @@ "%s","IDE Drive"); drive->gendev.parent = &hwif->gendev; drive->gendev.bus = &ide_bus_type; + drive->gendev.driver_data = drive;if (drive->present)device_register (&drive->gendev); }
Signature · 2 lines
Worst form of spam? Adding advertisment signatures ala sourceforge.net. What goes next? Inserting advertisment *into* email?
Quoted 3 lines · Pavel Machek
Patrick broke IDE so it wanted to eat data, again. Fixed, please apply,
This is too ugly for words.
drivers/ide/ide-disk.c line 1898
Quoted 13 lines
drivers/ide/ide-disk.c +4
-2@@ -1891,8 +1891,10 @@static int idedisk_init (void ) { -ide_register_driver (&idedisk_driver); -return 0 ; +int status =ide_register_driver (&idedisk_driver); + idedisk_driver.gen_driver.suspend = idedisk_suspend; + idedisk_driver.gen_driver.resume = idedisk_resume; +return status; }
Why aren't those fields initialized explicitly in the static declaration of idedisk_driver? And what are the do_idedisk_suspend/do_idedisk_resume functions, that _are_ initialized explicitly?
I want to understand the madness, not add to it.
Linus
Quoted 5 lines
Why aren't those fields initialized explicitly in the static declaration of
idedisk_driver? And what are the do_idedisk_suspend/do_idedisk_resume functions, that _are_ initialized explicitly?I want to understand the madness, not add to it.
The IDE devices are already beginning to use the pci suspend/resume callbacks so I would prefer that we have a very clear model of WTF is going on here.
Quoted 9 lines
Quoted 5 lines
Why aren't those fields initialized explicitly in the static declaration of
idedisk_driver? And what are the do_idedisk_suspend/do_idedisk_resume functions, that _are_ initialized explicitly?I want to understand the madness, not add to it.
The IDE devices are already beginning to use the pci suspend/resume callbacks so I would prefer that we have a very clear model of WTF is going on here.
The way it should be done is as follow (propagation of suspend/resume):
arch->pci_bus->...->pci_driver->ide_subdriver
The details as I see them (and that would match my needs on ppc) are that the suspend request originates from the bus binding of the controller, as any other device. (non PCI hosts will need specific arch tweaks here or whatever parent bus they have in the news model will deal with sending them the suspend call).
The driver (interface driver) is then acting like a bus. So it's it's responsiblity to forward the request to it's sub-drivers (typically ide-disk, ide-cd, whatever is attached to that physical interface). Once the subdrivers are done, the interface driver can proceed to shutting down the actual HW.
However, that leads to some problems regarding the existing bits of code I see in there.
One of them is that the do_idedisk_suspend & friends don't take the proper "level" parameter defined by the device model, which makes it difficult to implement the various suspend/resume steps as defined by the new driver model.
Another is that I feel (and I know Pavel doesn't agree here) that the disk driver should also block further incoming requests (that is leave them in the queue) instead of panic'ing. That is the driver should not rely on not beeing fed any more request, but rather make sure it will leave them in the queue and deal with them when resumed.
I hope I'll finally find enough time to tackle porting of all of the Pmac stuff to the new model, that will give me a chance to implement our power management scheme based on the new semantics and thus validate them (FYI, we've had fairly well working power management on pmac for a couple of years now, based on an arch specific mecanism & driver hooks, and I hope I discussed enough with Patrick & Greg to make sure our experience in this domain ended up in the design of the new model).
Ben.
Quoted 3 lines · Benjamin Herrenschmidt
The way it should be done is as follow (propagation of suspend/resume):
arch->pci_bus->...->pci_driver->ide_subdriver
----> disk
Quoted 5 lines
The details as I see them (and that would match my needs on ppc) are that the suspend request originates from the bus binding of the controller, as any other device. (non PCI hosts will need specific arch tweaks here or whatever parent bus they have in the news model will deal with sending them the suspend call).
We have the hwif structures (ugly I know) which know what the interface is nailed to and what is nailed to each device so it isnt that bad. There are the odd complications of course - chips that appear as two PCI devices and either one of them turns off both for example 8)
Quoted 6 lines
Another is that I feel (and I know Pavel doesn't agree here) that the disk driver should also block further incoming requests (that is leave them in the queue) instead of panic'ing. That is the driver should not rely on not beeing fed any more request, but rather make sure it will leave them in the queue and deal with them when resumed.
It is cleaner if the ordering of power off is right. If the model is right then the first suspend would be the drives. Part of drive suspend ought to be corking the queue.
Alan
Quoted 10 lines
Quoted 6 lines
Another is that I feel (and I know Pavel doesn't agree here) that the disk driver should also block further incoming requests (that is leave them in the queue) instead of panic'ing. That is the driver should not rely on not beeing fed any more request, but rather make sure it will leave them in the queue and deal with them when resumed.
It is cleaner if the ordering of power off is right. If the model is right then the first suspend would be the drives. Part of drive suspend ought to be corking the queue.
Yup.
My point here is that while Pavel approach is to kill/suspend anything that may trigger new IO requests (thus topping all userland, stopping selected kernel threads, etc...), I tend to think we leave all that alive, and just block requests going to suspended devices until those get alive again. That used to work well on pmac and leads to very fast suspend/resume cycles.
Ben.
Hi!
Quoted 11 lines
Quoted 10 lines
Quoted 6 lines
Another is that I feel (and I know Pavel doesn't agree here) that the disk driver should also block further incoming requests (that is leave them in the queue) instead of panic'ing. That is the driver should not rely on not beeing fed any more request, but rather make sure it will leave them in the queue and deal with them when resumed.
It is cleaner if the ordering of power off is right. If the model is right then the first suspend would be the drives. Part of drive suspend ought to be corking the queue.
Quoted 8 lines
Yup.
My point here is that while Pavel approach is to kill/suspend anything that may trigger new IO requests (thus topping all userland, stopping selected kernel threads, etc...), I tend to think we leave all that alive, and just block requests going to suspended devices until those get alive again. That used to work well on pmac and leads to very fast suspend/resume cycles.
I believe it is simpler to do my way ;-). Of couse, adding stopping to drivers will only add robustness and may enable faster suspends in future (I do not think it is going to be significant)....
Pavel
Signature · 1 line
When do you have heart between your knees?