Re: [PATCH] Re: 2.6.17-rc5-git1: regression: resume from suspend(RAM) fails: libata issue

[Jeff Garzik <jeff@garzik.org>]

Linus Torvalds wrote:
> 
> On Sat, 27 May 2006, Jens Axboe wrote:
>> @@ -4846,6 +4847,7 @@ int ata_pci_device_suspend(struct pci_de
>>  
>>  int ata_pci_device_resume(struct pci_dev *pdev)
>>  {
>> +	msleep(500);
>>  	pci_set_power_state(pdev, PCI_D0);
> 
> That's just insane. Why would we need a delay before going to D0? 
> Especially a long one like half a second? 
> 
> There's something else going on here, and that delay must be just hiding 
> the real issue.

It is.  But I thought you wanted something that works?  :)

Your patch + Jens' patch [with the delay MOVED to the end] would get my 
ACK for 2.6.17, and we already have infrastructure queued for 2.6.18 to 
do a better job of kicking the controller and bus.


> Looking at ata_device_resume(), I think the whole thing is pretty broken.
> 
> Just as an example, it calls "ata_set_mode()", but that sounds pretty damn 
> broken, and it should probably do
> 
> 	if (ap->ops->set_mode)
> 		ap->ops->set_mode(ap);
> 	else
> 		ata_set_mode(ap);

Correct.

Furthermore, we probably need to issue IDLE IMMEDIATE command ("wake 
up") before SET FEATURES - XFER MODE.


> like ata_bus_probe() does. Similarly, why shouldn't it do the 
> probe_reset/phy_reset that is also done in ata_bus_probe()? If it was 

It should, most definitely.  Otherwise we skip initializing the 
controller and phy (a key objection of mine all along).


> required in ata_bus_probe(), it sounds like it would damn well be required 
> at resume time too - from a hw perspective, there should really be _no_ 
> difference between the initial bootup, and a resume event. The hardware is 
> in the same state.

Correct, correct, correct :)


> (From a _software_ perspective, it's different, of course - one does 
> discovery, while the other might instead try to see if the state _matches_ 
> what it already knows. But the point is that coming back from power-off 
> after a resume should really not be any different than coming back from 
> power-off after a bootup)

Key difference:  we have no BIOS to give us sane hardware state, so the 
controller is in a different state from bootup.

Lacking controller init (you mention this above), we basically have raw 
silicon defaults.

We are really debugging from scratch here, and there's a lot of work to do.

Just apply the delay patch for 2.6.17, Mr. Get-It-Working ;-)  Else dive 
into the big can of worms and give us time to fix it right...

	Jeff