[PATCH alt4] libata resume fixes

[PATCH alt4] libata resume fixes

[Jeff Garzik]

diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 6dc8814..949d496 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -201,7 +201,7 @@ static struct pci_driver piix_pci_driver
 	.probe			= piix_init_one,
 	.remove			= ata_pci_remove_one,
 	.suspend		= ata_pci_device_suspend,
-	.resume			= ata_pci_device_resume,
+	.resume			= pata_pci_device_resume,
 };
 
 static struct scsi_host_template piix_sht = {
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index fa476e7..1d3a6d4 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -4852,6 +4852,25 @@ int ata_pci_device_resume(struct pci_dev
 	pci_set_master(pdev);
 	return 0;
 }
+
+int pata_pci_device_resume(struct pci_dev *pdev)
+{
+	struct device *dev = pci_dev_to_dev(pdev);
+	struct ata_host_set *host_set = dev_get_drvdata(dev);
+	struct ata_port *ap;
+	int i;
+
+	ata_pci_device_resume(pdev);
+
+	for (i = 0; i < host_set->n_ports; i++) {
+		ap = host_set->ports[i];
+		if (ata_bus_probe(ap))
+			printk(KERN_ERR "ata%u: failed to resume\n", ap->id);
+	}
+
+	return 0;
+}
+
 #endif /* CONFIG_PCI */
 
 
@@ -4970,6 +4989,7 @@ EXPORT_SYMBOL_GPL(ata_pci_init_one);
 EXPORT_SYMBOL_GPL(ata_pci_remove_one);
 EXPORT_SYMBOL_GPL(ata_pci_device_suspend);
 EXPORT_SYMBOL_GPL(ata_pci_device_resume);
+EXPORT_SYMBOL_GPL(pata_pci_device_resume);
 EXPORT_SYMBOL_GPL(ata_pci_default_filter);
 EXPORT_SYMBOL_GPL(ata_pci_clear_simplex);
 #endif /* CONFIG_PCI */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b80d2e7..cad531a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -516,6 +516,7 @@ extern int ata_pci_init_one (struct pci_
 extern void ata_pci_remove_one (struct pci_dev *pdev);
 extern int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t state);
 extern int ata_pci_device_resume(struct pci_dev *pdev);
+extern int pata_pci_device_resume(struct pci_dev *pdev);
 extern int ata_pci_clear_simplex(struct pci_dev *pdev);
 #endif /* CONFIG_PCI */
 extern int ata_device_add(const struct ata_probe_ent *ent);

[PATCH alt4 v2] libata resume fixes

[Jeff Garzik]

This fixes a bug in alt4 v1, by eliminating the second ata_set_mode()
call, found in ata_device_resume().


diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 6dc8814..949d496 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -201,7 +201,7 @@ static struct pci_driver piix_pci_driver
 	.probe			= piix_init_one,
 	.remove			= ata_pci_remove_one,
 	.suspend		= ata_pci_device_suspend,
-	.resume			= ata_pci_device_resume,
+	.resume			= pata_pci_device_resume,
 };
 
 static struct scsi_host_template piix_sht = {
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index fa476e7..ddc2a71 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -4296,10 +4296,8 @@ static int ata_start_drive(struct ata_po
  */
 int ata_device_resume(struct ata_port *ap, struct ata_device *dev)
 {
-	if (ap->flags & ATA_FLAG_SUSPENDED) {
+	if (ap->flags & ATA_FLAG_SUSPENDED)
 		ap->flags &= ~ATA_FLAG_SUSPENDED;
-		ata_set_mode(ap);
-	}
 	if (!ata_dev_present(dev))
 		return 0;
 	if (dev->class == ATA_DEV_ATA)
@@ -4852,6 +4850,25 @@ int ata_pci_device_resume(struct pci_dev
 	pci_set_master(pdev);
 	return 0;
 }
+
+int pata_pci_device_resume(struct pci_dev *pdev)
+{
+	struct device *dev = pci_dev_to_dev(pdev);
+	struct ata_host_set *host_set = dev_get_drvdata(dev);
+	struct ata_port *ap;
+	int i;
+
+	ata_pci_device_resume(pdev);
+
+	for (i = 0; i < host_set->n_ports; i++) {
+		ap = host_set->ports[i];
+		if (ata_bus_probe(ap))
+			printk(KERN_ERR "ata%u: failed to resume\n", ap->id);
+	}
+
+	return 0;
+}
+
 #endif /* CONFIG_PCI */
 
 
@@ -4970,6 +4987,7 @@ EXPORT_SYMBOL_GPL(ata_pci_init_one);
 EXPORT_SYMBOL_GPL(ata_pci_remove_one);
 EXPORT_SYMBOL_GPL(ata_pci_device_suspend);
 EXPORT_SYMBOL_GPL(ata_pci_device_resume);
+EXPORT_SYMBOL_GPL(pata_pci_device_resume);
 EXPORT_SYMBOL_GPL(ata_pci_default_filter);
 EXPORT_SYMBOL_GPL(ata_pci_clear_simplex);
 #endif /* CONFIG_PCI */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b80d2e7..cad531a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -516,6 +516,7 @@ extern int ata_pci_init_one (struct pci_
 extern void ata_pci_remove_one (struct pci_dev *pdev);
 extern int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t state);
 extern int ata_pci_device_resume(struct pci_dev *pdev);
+extern int pata_pci_device_resume(struct pci_dev *pdev);
 extern int ata_pci_clear_simplex(struct pci_dev *pdev);
 #endif /* CONFIG_PCI */
 extern int ata_device_add(const struct ata_probe_ent *ent);

Re: [PATCH alt4 v2] libata resume fixes

[Jeff Garzik]

Jeff Garzik wrote:
> This fixes a bug in alt4 v1, by eliminating the second ata_set_mode()
> call, found in ata_device_resume().

This seems to make my x86-64 box happier...

	Jeff

Re: [PATCH alt4 v2] libata resume fixes

[Mark Lord]

Jeff Garzik wrote:
> Jeff Garzik wrote:
>> This fixes a bug in alt4 v1, by eliminating the second ata_set_mode()
>> call, found in ata_device_resume().
>
> This seems to make my x86-64 box happier...
>
>     Jeff

((second attempt at sending this -- first one seems
to have been blocked by something, probably due to
an attached .jpg screenshot))

With alt4_v2 applied, resume still fails,
but slightly differently from before.

This time, there's the initial blank-screen for 31 seconds,
then the backlight turns on and I can see the syslog scrolling
in the window where I left it, as before.

New, is that the disk activity LED is stuck-on,
and the error-recovery code is doing something
every 30 seconds or so.

Screenshot here temporarily:  http://rtr.ca/libata1.jpg

Other than that, it's still dead -- disk I/O not working.

Can this patch be combined with Linus's one-liner
and still work on your machine?

Cheers 

Re: [PATCH alt4 v2] libata resume fixes

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Jeff Garzik wrote:
>>> This fixes a bug in alt4 v1, by eliminating the second ata_set_mode()
>>> call, found in ata_device_resume().
>>
>> This seems to make my x86-64 box happier...
>>
>>     Jeff
> 
> With alt4_v2 applied, resume still fails,
> but slightly differently from before.
> 
> This time, there's the initial blank-screen for 31 seconds,
> then the backlight turns on and I can see the syslog scrolling
> in the window where I left it, as before.
> 
> New, is that the disk activity LED is stuck-on,
> and the error-recovery code is doing something
> every 30 seconds or so (see attached screen shot).

Yay!  I'm familiar with the 0xD0 status on ata_piix, so this is really 
good progress.


> Other than that, it's still dead -- disk I/O not working.
> 
> Can this patch be combined with Linus's one-liner
> and still work on your machine?

Does alt4v2 plus Linus's patch work on your machine?  (I'm guessing "no")

	Jeff

Re: [PATCH alt4 v2] libata resume fixes

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
..
> Yay!  I'm familiar with the 0xD0 status on ata_piix, so this is really 
> good progress.
> 
> 
>> Other than that, it's still dead -- disk I/O not working.
>>
>> Can this patch be combined with Linus's one-liner
>> and still work on your machine?
> 
> Does alt4v2 plus Linus's patch work on your machine?  (I'm guessing "no")

I haven't tried it here yet.

One other thing:  you patches keep adding a "pata" pci_device_resume handler.

My disk is SATA, or at least libata thinks it is SATA.
Just in case that makes any difference here.

(it really is PATA, but seems to have a SATA bridge
between it and the ICH6M SATA port).

Cheers

Re: [PATCH alt4 v2] libata resume fixes

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
> ..
>> Yay!  I'm familiar with the 0xD0 status on ata_piix, so this is really 
>> good progress.
>>
>>
>>> Other than that, it's still dead -- disk I/O not working.
>>>
>>> Can this patch be combined with Linus's one-liner
>>> and still work on your machine?
>>
>> Does alt4v2 plus Linus's patch work on your machine?  (I'm guessing "no")
> 
> I haven't tried it here yet.
> 
> One other thing:  you patches keep adding a "pata" pci_device_resume 
> handler.
> 
> My disk is SATA, or at least libata thinks it is SATA.
> Just in case that makes any difference here.
> 
> (it really is PATA, but seems to have a SATA bridge
> between it and the ICH6M SATA port).

That's just naming, which indicates it is a PATA-like BMDMA resume 
handler.  Don't worry about it.

As mentioned elsewhere, Linus's patch falls over when we start resuming 
controllers with real SATA phy registers, so that would be a separate 
sata_pci_device_resume() function.

	Jeff

Re: [PATCH alt4 v2] libata resume fixes

[Mark Lord]

Jeff Garzik wrote:
>
> As mentioned elsewhere, Linus's patch falls over when we start resuming 
> controllers with real SATA phy registers, so that would be a separate 
> sata_pci_device_resume() function.

But does it fall over any worse than we already do with the stock kernel?

Re: [PATCH alt4 v2] libata resume fixes

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>>
>> As mentioned elsewhere, Linus's patch falls over when we start 
>> resuming controllers with real SATA phy registers, so that would be a 
>> separate sata_pci_device_resume() function.
> 
> But does it fall over any worse than we already do with the stock kernel?

Are you talking about my x86-64 box + Linus's patch, or the future 
implications?

For the former, the previous behavior was EH spew like what you are 
seeing.  After apply Linus's patch, it hardlocks.  But I don't want that 
to hold up the patch...  libata suspend/resume is one part luck, and one 
part "it's only ata_piix so far."  It has a looooong way to go before it 
is usable outside of that domain.

For the latter, it doesn't make sense to poll BSY on modern SATA 
controllers, particularly (a) FIS-based ones and more importantly (b) 
devices attached behind a Port Multiplier.

	Jeff

Re: [PATCH alt4 v2] libata resume fixes

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>
>> But does it fall over any worse than we already do with the stock kernel?
> 
> Are you talking about my x86-64 box + Linus's patch, or the future 
> implications?
> 
> For the former, the previous behavior was EH spew like what you are 
> seeing.  After apply Linus's patch, it hardlocks.  But I don't want that 
> to hold up the patch...  libata suspend/resume is one part luck, and one 
> part "it's only ata_piix so far."  It has a looooong way to go before it 
> is usable outside of that domain.

I was asking if Linus's one-liner patch makes things any worse
than they already were on your own machine.  It sounds like "Not".

Cheers

Re: [PATCH alt4 v2] libata resume fixes

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
>>
>>> But does it fall over any worse than we already do with the stock 
>>> kernel?
>>
>> Are you talking about my x86-64 box + Linus's patch, or the future 
>> implications?
>>
>> For the former, the previous behavior was EH spew like what you are 
>> seeing.  After apply Linus's patch, it hardlocks.  But I don't want 
>> that to hold up the patch...  libata suspend/resume is one part luck, 
>> and one part "it's only ata_piix so far."  It has a looooong way to go 
>> before it is usable outside of that domain.
> 
> I was asking if Linus's one-liner patch makes things any worse
> than they already were on your own machine.  It sounds like "Not".

The state changes from "kernel alive, but not useful" to "hardlock."

	Jeff

Re: [PATCH alt4] libata resume fixes

[Mark Lord]

Jeff Garzik wrote:
..
> +int pata_pci_device_resume(struct pci_dev *pdev)
> +{
> +	struct device *dev = pci_dev_to_dev(pdev);
> +	struct ata_host_set *host_set = dev_get_drvdata(dev);
> +	struct ata_port *ap;
> +	int i;
> +
> +	ata_pci_device_resume(pdev);
> +
> +	for (i = 0; i < host_set->n_ports; i++) {
> +		ap = host_set->ports[i];
> +		if (ata_bus_probe(ap))
> +			printk(KERN_ERR "ata%u: failed to resume\n", ap->id);
> +	}
> +
> +	return 0;
> +}
..

I'm replying in the correct thread this time. ;)

Sorry to say, but no user-visible difference from previous attempts.
Still fails the same way, after a 30-ish second timeout on resume.

Cheers 

[PATCH alt4 v3] libata resume fixes

[Jeff Garzik]

Here's alt4 v3.  Having the host_stat == 0x1 was suspicious.
After digging through the hardware manual, it might be a good idea to
configure BMDMA registers properly, in case that was done by BIOS but
not by D3->D0 silicon reset values.

One core problem with all of libata suspend/resume is that
* we boot from a configuration set up by BIOS
* but going D3->D0, we simply get silicon defaults

Randy Dunlap's ACPI patches will probably help a bit.


diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 6dc8814..949d496 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -201,7 +201,7 @@ static struct pci_driver piix_pci_driver
 	.probe			= piix_init_one,
 	.remove			= ata_pci_remove_one,
 	.suspend		= ata_pci_device_suspend,
-	.resume			= ata_pci_device_resume,
+	.resume			= pata_pci_device_resume,
 };
 
 static struct scsi_host_template piix_sht = {
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index fa476e7..1289e80 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -4296,10 +4296,8 @@ static int ata_start_drive(struct ata_po
  */
 int ata_device_resume(struct ata_port *ap, struct ata_device *dev)
 {
-	if (ap->flags & ATA_FLAG_SUSPENDED) {
+	if (ap->flags & ATA_FLAG_SUSPENDED)
 		ap->flags &= ~ATA_FLAG_SUSPENDED;
-		ata_set_mode(ap);
-	}
 	if (!ata_dev_present(dev))
 		return 0;
 	if (dev->class == ATA_DEV_ATA)
@@ -4852,6 +4850,40 @@ int ata_pci_device_resume(struct pci_dev
 	pci_set_master(pdev);
 	return 0;
 }
+
+int pata_pci_device_resume(struct pci_dev *pdev)
+{
+	struct device *dev = pci_dev_to_dev(pdev);
+	struct ata_host_set *host_set = dev_get_drvdata(dev);
+	struct ata_port *ap;
+	unsigned long bmdma;
+	int i;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+	pci_enable_device(pdev);
+
+	for (i = 0; i < host_set->n_ports; i++) {
+		ap = host_set->ports[i];
+		bmdma = ap->ioaddr.bmdma_addr;
+
+		outb(0, bmdma + ATA_DMA_CMD);
+		ata_altstatus(ap);
+		outb(0xff, bmdma + ATA_DMA_STATUS);
+		ata_altstatus(ap);
+	}
+
+	pci_set_master(pdev);
+
+	for (i = 0; i < host_set->n_ports; i++) {
+		ap = host_set->ports[i];
+		if (ata_bus_probe(ap))
+			printk(KERN_ERR "ata%u: failed to resume\n", ap->id);
+	}
+
+	return 0;
+}
+
 #endif /* CONFIG_PCI */
 
 
@@ -4970,6 +5002,7 @@ EXPORT_SYMBOL_GPL(ata_pci_init_one);
 EXPORT_SYMBOL_GPL(ata_pci_remove_one);
 EXPORT_SYMBOL_GPL(ata_pci_device_suspend);
 EXPORT_SYMBOL_GPL(ata_pci_device_resume);
+EXPORT_SYMBOL_GPL(pata_pci_device_resume);
 EXPORT_SYMBOL_GPL(ata_pci_default_filter);
 EXPORT_SYMBOL_GPL(ata_pci_clear_simplex);
 #endif /* CONFIG_PCI */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b80d2e7..cad531a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -516,6 +516,7 @@ extern int ata_pci_init_one (struct pci_
 extern void ata_pci_remove_one (struct pci_dev *pdev);
 extern int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t state);
 extern int ata_pci_device_resume(struct pci_dev *pdev);
+extern int pata_pci_device_resume(struct pci_dev *pdev);
 extern int ata_pci_clear_simplex(struct pci_dev *pdev);
 #endif /* CONFIG_PCI */
 extern int ata_device_add(const struct ata_probe_ent *ent);

Re: [PATCH alt4 v3] libata resume fixes

[Mark Lord]

I tried merging Linus's one-liner in front of alt4_v2,
and the screen came back right away (instead of stalling 30seconds),
and then the alt4_v2 code killed the interface as before.  :)

Jeff Garzik wrote:
> Here's alt4 v3.  Having the host_stat == 0x1 was suspicious.
> After digging through the hardware manual, it might be a good idea to
> configure BMDMA registers properly, in case that was done by BIOS but
> not by D3->D0 silicon reset values.

I'll try that.  But does it make sense that Linus's one-liner
would also work, if this was really the problem?  (dunno, just asking)

> One core problem with all of libata suspend/resume is that
> * we boot from a configuration set up by BIOS
> * but going D3->D0, we simply get silicon defaults
> 
> Randy Dunlap's ACPI patches will probably help a bit.

The ACPI patches have indeed been working here for over a year now,
on all kernels up to 2.6.15 --> not needed for 2.6.16.

Cheers

Re: [PATCH alt4 v3] libata resume fixes

[Jeff Garzik]

Mark Lord wrote:
> I tried merging Linus's one-liner in front of alt4_v2,
> and the screen came back right away (instead of stalling 30seconds),
> and then the alt4_v2 code killed the interface as before.  :)
> 
> Jeff Garzik wrote:
>> Here's alt4 v3.  Having the host_stat == 0x1 was suspicious.
>> After digging through the hardware manual, it might be a good idea to
>> configure BMDMA registers properly, in case that was done by BIOS but
>> not by D3->D0 silicon reset values.
> 
> I'll try that.  But does it make sense that Linus's one-liner
> would also work, if this was really the problem?  (dunno, just asking)

<shrug>  ata_piix is probably going through its emulated-PATA sequence, 
so I was just trying different things.


>> One core problem with all of libata suspend/resume is that
>> * we boot from a configuration set up by BIOS
>> * but going D3->D0, we simply get silicon defaults
>>
>> Randy Dunlap's ACPI patches will probably help a bit.
> 
> The ACPI patches have indeed been working here for over a year now,
> on all kernels up to 2.6.15 --> not needed for 2.6.16.

Long term they are definitely needed, if only for the case where a drive 
password has been set in BIOS.  Otherwise when you resume, you won't be 
able to talk to your disk.  Additionally the preferences (such drive 
acoustic settings) aren't reprogrammed, but that is of lesser importance.

On laptops, the ACPI tables sometimes also hold special vendor-specific 
taskfiles to work around device-specific or drive-firmware-specific 
problems.

	Jeff

Re: [PATCH alt4 v3] libata resume fixes

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>
>> The ACPI patches have indeed been working here for over a year now,
>> on all kernels up to 2.6.15 --> not needed for 2.6.16.
> 
> Long term they are definitely needed, if only for the case where a drive 
> password has been set in BIOS.  Otherwise when you resume, you won't be 
> able to talk to your disk.  Additionally the preferences (such drive 
> acoustic settings) aren't reprogrammed, but that is of lesser importance.
> 
> On laptops, the ACPI tables sometimes also hold special vendor-specific 
> taskfiles to work around device-specific or drive-firmware-specific 
> problems.

Agreed, 100%.

So why aren't they in yet?  ;)

Re: [PATCH alt4 v3] libata resume fixes

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
>>
>>> The ACPI patches have indeed been working here for over a year now,
>>> on all kernels up to 2.6.15 --> not needed for 2.6.16.
>>
>> Long term they are definitely needed, if only for the case where a 
>> drive password has been set in BIOS.  Otherwise when you resume, you 
>> won't be able to talk to your disk.  Additionally the preferences 
>> (such drive acoustic settings) aren't reprogrammed, but that is of 
>> lesser importance.
>>
>> On laptops, the ACPI tables sometimes also hold special 
>> vendor-specific taskfiles to work around device-specific or 
>> drive-firmware-specific problems.
> 
> Agreed, 100%.
> 
> So why aren't they in yet?  ;)

Hey, I'm ready for them...

Now that Tejun's EH work is done, the floodgates are open...

	Jeff

Re: [PATCH alt4 v3] libata resume fixes

[Mark Lord]

The alt4_v3 patch from Jeff also still fails here.

The screen stays off for 30 seconds, then comes on,
but the error-recovery attempts do NOT appear in the syslog
as they did with the alt4_v2 patch.  The disk LED is still
stuck on, and the syslog shows only this, from early on:

qc timeout (cmd 0xe1)
do_simple_cmd: ata command failed: 4

I've got an errand to run now, but I'll be back again later.

Cheers

Re: [PATCH alt4 v3] libata resume fixes

[Jeff Garzik]

Mark Lord wrote:
> The alt4_v3 patch from Jeff also still fails here.
> 
> The screen stays off for 30 seconds, then comes on,
> but the error-recovery attempts do NOT appear in the syslog
> as they did with the alt4_v2 patch.  The disk LED is still
> stuck on, and the syslog shows only this, from early on:
> 
> qc timeout (cmd 0xe1)
> do_simple_cmd: ata command failed: 4

0xE1 is IDLE IMMEDIATE, so it looks like it is making progress.

This makes me realize that ata_bus_probe() won't get the job done alone, 
we must do:

* host init
* phy init
* idle immediate
* the rest of bus probe

Good, good data points here...

	Jeff

Re: [PATCH alt4 v3] libata resume fixes

[zhao, forrest]

On Sat, 2006-05-27 at 17:21 -0400, Jeff Garzik wrote:
> o_simple_cmd: ata command failed: 4
> 
> 0xE1 is IDLE IMMEDIATE, so it looks like it is making progress.
> 
> This makes me realize that ata_bus_probe() won't get the job done
> alone, 
> we must do:
> 
> * host init
> * phy init
> * idle immediate
> * the rest of bus probe
> 
> Good, good data points here...

Jeff,

This observation is very valuable. I'll write the patch for this
together with the AHCI suspend/resume support for 2.6.1[8/9] soon.

BTW. Have we achieved consensus that the bus resume operation should be
done in [s/p]ata_pci_device_resume() in 2.6.1[8/9]?

Thanks,
Forrest

Re: [PATCH alt4 v3] libata resume fixes

[Jeff Garzik]

zhao, forrest wrote:
> On Sat, 2006-05-27 at 17:21 -0400, Jeff Garzik wrote:
>> o_simple_cmd: ata command failed: 4
>>
>> 0xE1 is IDLE IMMEDIATE, so it looks like it is making progress.
>>
>> This makes me realize that ata_bus_probe() won't get the job done
>> alone, 
>> we must do:
>>
>> * host init
>> * phy init
>> * idle immediate
>> * the rest of bus probe
>>
>> Good, good data points here...
> 
> Jeff,
> 
> This observation is very valuable. I'll write the patch for this
> together with the AHCI suspend/resume support for 2.6.1[8/9] soon.

Thanks for your continued work on AHCI.

BTW, you/Hannes' AHCI suspend/resume patch needs to be split up into 
multiple steps, for easier reviewing and applying.  Something like:
* update ahci_{start,stop}_engine, and update all it users
* split out start/stop FIS RX into separate functions, no code flow changes
* use ahci_{start,stop}_fis_rx in appropriate places
* add suspend/resume support

The current AHCI suspend/resume patch does a lot more than just add 
suspend/resume... it modifies the current AHCI probe code and 
operational code in several areas.  We need to be able to evaluate these 
changes outside the context of suspend/resume, because the changes 
affect more than just suspend/resume.

The analogy I use [stolen from Al Viro] is mathematic, for illustrating 
a progression of patches:  when working a mathematical proof, you supply 
the progression leading to the answer, in addition to the answer itself.


> BTW. Have we achieved consensus that the bus resume operation should be
> done in [s/p]ata_pci_device_resume() in 2.6.1[8/9]?

Its pretty much a requirement for FIS-based controllers and Port 
Multipliers.  Since the bus is not an independent object from the device 
model perspective, we must manage it as part of the controller (which 
indeed it is -- a wire and phy managed by the controller hardware).

Long term, I hope that we will have a device model with proper 
dependencies.  That means an object for the bus in addition to objects 
for controller and device.  Once that happens, we can do not only proper 
suspend/resume, but also runtime power management.

	Jeff