[PATCH alt2] libata resume fixes

[PATCH alt2] 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..5b7ab7b 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -4852,6 +4852,30 @@ 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;
+	u8 status;
+	int i;
+
+	ata_pci_device_resume(pdev);
+
+	mdelay(400);
+
+	for (i = 0; i < host_set->n_ports; i++) {
+		ap = host_set->ports[i];
+
+		status = ata_busy_wait(ap, ATA_BUSY, 200000);
+		if (status & ATA_BUSY)
+			printk(KERN_ERR "ata%u: failed to resume\n", ap->id);
+	}
+
+	return 0;
+}
+
 #endif /* CONFIG_PCI */
 
 
@@ -4970,6 +4994,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 alt2] 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;
> +	u8 status;
> +	int i;
> +
> +	ata_pci_device_resume(pdev);
> +
> +	mdelay(400);
> +
> +	for (i = 0; i < host_set->n_ports; i++) {
> +		ap = host_set->ports[i];
> +
> +		status = ata_busy_wait(ap, ATA_BUSY, 200000);
> +		if (status & ATA_BUSY)
> +			printk(KERN_ERR "ata%u: failed to resume\n", ap->id);
> +	}
> +
> +	return 0;
...

Okay, back again after two reboots.  Same failure mode as all of the others.
System freezes for a 30-second timeout with no LCD-backlight on,
then comes up without functioning hard drive access.  An immediate reboot
is necessary to do anything.

Linus's one-liner works perfectly, though.

Cheers

Re: [PATCH alt2] libata resume fixes

[Jeff Garzik]

Mark Lord wrote:
> Okay, back again after two reboots.  Same failure mode as all of the 
> others.
> System freezes for a 30-second timeout with no LCD-backlight on,
> then comes up without functioning hard drive access.  An immediate reboot
> is necessary to do anything.

Thanks for testing.

On the working patch, can you verify that ata_pci_device_resume() is 
called before ata_scsi_device_resume() ?

	Jeff

Re: [PATCH alt2] libata resume fixes

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Okay, back again after two reboots.  Same failure mode as all of the 
>> others.
>> System freezes for a 30-second timeout with no LCD-backlight on,
>> then comes up without functioning hard drive access.  An immediate reboot
>> is necessary to do anything.
> 
> Thanks for testing.
> 
> On the working patch, can you verify that ata_pci_device_resume() is 
> called before ata_scsi_device_resume() ?

You mean on Linus's one-liner patch?
That one didn't change the ordering from current 2.6.17-rc* libata.

I can instrument that, though, and report back if you need.

??

Re: [PATCH alt2] libata resume fixes

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
>>> Okay, back again after two reboots.  Same failure mode as all of the 
>>> others.
>>> System freezes for a 30-second timeout with no LCD-backlight on,
>>> then comes up without functioning hard drive access.  An immediate 
>>> reboot
>>> is necessary to do anything.
>>
>> Thanks for testing.
>>
>> On the working patch, can you verify that ata_pci_device_resume() is 
>> called before ata_scsi_device_resume() ?
> 
> You mean on Linus's one-liner patch?

Yes.


> That one didn't change the ordering from current 2.6.17-rc* libata.

I know.


> I can instrument that, though, and report back if you need.
> 
> ??

Please do...

Thanks,

	Jeff

Re: [PATCH alt2] libata resume fixes

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Jeff Garzik wrote:
>>> On the working patch, can you verify that ata_pci_device_resume() is 
>>> called before ata_scsi_device_resume() ?
>>
>> You mean on Linus's one-liner patch?
> 
> Yes.

Okay, here's the syslog.  Find the lines that begin with ">>>>>  ",
and you'll see ata_pci_device_resume followed by two ata_scsi_device_resume.

...
May 27 15:38:42 localhost kernel: ACPI: PCI Interrupt 0000:00:1e.2[A] -> GSI 16 (level, low) -> IRQ 16
May 27 15:38:42 localhost kernel: PCI: Setting latency timer of device 0000:00:1e.2 to 64
May 27 15:38:42 localhost kernel: >>>>>  ata_pci_device_resume  <<<<<
May 27 15:38:42 localhost kernel: ACPI: PCI Interrupt 0000:00:1f.2[B] -> GSI 17 (level, low) -> IRQ 18
May 27 15:38:42 localhost kernel: PCI: Setting latency timer of device 0000:00:1f.2 to 64
May 27 15:38:42 localhost kernel: ACPI: PCI Interrupt 0000:03:00.0[A] -> GSI 18 (level, low) -> IRQ 19
May 27 15:38:42 localhost kernel: ACPI: PCI Interrupt 0000:03:01.0[A] -> GSI 19 (level, low) -> IRQ 17
May 27 15:38:42 localhost kernel: PCI: Enabling device 0000:03:01.1 (0000 -> 0002)
May 27 15:38:42 localhost kernel: ACPI: PCI Interrupt 0000:03:01.1[B] -> GSI 18 (level, low) -> IRQ 19
May 27 15:38:42 localhost kernel: pnp: Device 00:04 does not support activation.
May 27 15:38:42 localhost kernel: pnp: Device 00:05 does not support activation.
May 27 15:38:42 localhost kernel: >>>>>  ata_scsi_device_resume  <<<<<
May 27 15:38:42 localhost kernel: ata1: dev 0 configured for UDMA/100
May 27 15:38:42 localhost kernel: >>>>>  ata_scsi_device_resume  <<<<<
May 27 15:38:42 localhost kernel: ata2: dev 0 configured for UDMA/33
May 27 15:38:42 localhost kernel: Restarting tasks... done
...

One thought about Linus's one-liner (and the original patch),
is that two seconds may be too short --> I'd suggest a 10-second
timeout there for notebook drive spin-up.  Or one could be very
paranoid and use the standard ATA 31-second timeout.

I wonder if your faster 64-bit machine had that problem?
(two seconds not long enough, whereas my 32-bit machine is slower
gettting to that point, so two-seconds is then enough?).

Cheers

Re: [PATCH alt2] libata resume fixes

[Jeff Garzik]

Mark Lord wrote:
> Okay, here's the syslog.  Find the lines that begin with ">>>>>  ",
> and you'll see ata_pci_device_resume followed by two 
> ata_scsi_device_resume.

Thanks.


> One thought about Linus's one-liner (and the original patch),
> is that two seconds may be too short --> I'd suggest a 10-second
> timeout there for notebook drive spin-up.  Or one could be very
> paranoid and use the standard ATA 31-second timeout.
> 
> I wonder if your faster 64-bit machine had that problem?
> (two seconds not long enough, whereas my 32-bit machine is slower
> gettting to that point, so two-seconds is then enough?).

Well, there's a udelay() in there to guarantee the timing, and 64-bit 
machine is definitely newer and faster, so I doubt that would explain 
the hardlock I see.  I'll test a longer ata_busy_wait() anyway, just to 
be sure.

Overall we _really really_ need to do full controller init.  I'm 
honestly surprised the delay hack works, because the resume skips ALL of 
the controller init in piix_sata_probe().  Since the Linus patch doesn't 
touch PCS at all, it is _luck_ that the controller silicon gives you a 
useful value when it goes to PCI D0 state.

I posted a version that does full bus probe as "[PATCH alt4] libata 
resume fixes" just now.

	Jeff