prevent swsusp from eating disks

prevent swsusp from eating disks

[Pavel Machek]

Hi!

Patrick broke IDE so it wanted to eat data, again. Fixed, please
apply,
								Pavel


--- clean/drivers/ide/ide-disk.c	2002-10-18 23:41:15.000000000 +0200
+++ linux-swsusp/drivers/ide/ide-disk.c	2002-10-21 14:36:36.000000000 +0200
@@ -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);
--- clean/drivers/ide/ide-probe.c	2002-10-20 16:22:39.000000000 +0200
+++ linux-swsusp/drivers/ide/ide-probe.c	2002-10-21 14:45:41.000000000 +0200
@@ -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);
 	}
 

-- 
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?

Re: prevent swsusp from eating disks

[Linus Torvalds]

On Wed, 30 Oct 2002, Pavel Machek wrote:
> 
> Patrick broke IDE so it wanted to eat data, again. Fixed, please
> apply,

This is too ugly for words.

> --- clean/drivers/ide/ide-disk.c	2002-10-18 23:41:15.000000000 +0200
> +++ linux-swsusp/drivers/ide/ide-disk.c	2002-10-21 14:36:36.000000000 +0200
> @@ -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

Re: prevent swsusp from eating disks

[Alan Cox]

> 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.

Re: prevent swsusp from eating disks

[Benjamin Herrenschmidt]

>> 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.

Re: prevent swsusp from eating disks

[Alan Cox]

On Wed, 2002-10-30 at 15:48, Benjamin Herrenschmidt wrote:
> The way it should be done is as follow (propagation of suspend/resume):
> 
> arch->pci_bus->...->pci_driver->ide_subdriver
                                             ----> disk


> 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)


> 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

Re: prevent swsusp from eating disks

[Benjamin Herrenschmidt]

>> 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.

Re: prevent swsusp from eating disks

[Pavel Machek]

Hi!

> >> 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.

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
-- 
When do you have heart between your knees?