2.6.17-rc5-git1: regression: resume from suspend(RAM) fails: libata issue

2.6.17-rc5-git1: regression: resume from suspend(RAM) fails: libata issue

[Mark Lord]

My ata_piix based Notebook (Dell i9300) suspends/resumes perfectly (RAM or disk)
with 2.6.16.xx kernels, but fails resume on 2.6.17-rc5-git1 (the first 2.6.17-*
I've attempted on this machine).

On resume from RAM, after a 30-second-ish timeout, the screen comes on
but the hard disk is NOT accessible.  "dmesg" in an already-open window
shows this (typed in from handwritten notes):

sd 0:0:0:0: SCSI error: return code = 0x40000
end_request: I/O error, /dev/sda, sector nnnnnnn
sd 0:0:0:0: SCSI error: return code = 0x40000
end_request: I/O error, /dev/sda, sector nnnnnnn
sd 0:0:0:0: SCSI error: return code = 0x40000
end_request: I/O error, /dev/sda, sector nnnnnnn
sd 0:0:0:0: SCSI error: return code = 0x40000
end_request: I/O error, /dev/sda, sector nnnnnnn
sd 0:0:0:0: SCSI error: return code = 0x40000
end_request: I/O error, /dev/sda, sector nnnnnnn
sd 0:0:0:0: SCSI error: return code = 0x40000
end_request: I/O error, /dev/sda, sector nnnnnnn
sd 0:0:0:0: SCSI error: return code = 0x40000
end_request: I/O error, /dev/sda, sector nnnnnnn

(the nnnnnnn was actually a real number, different on each message).

Is there a fix already floating around for this?

Cheers

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

[Mark Lord]

Mark Lord wrote:
> My ata_piix based Notebook (Dell i9300) suspends/resumes perfectly (RAM 
> or disk)
> with 2.6.16.xx kernels, but fails resume on 2.6.17-rc5-git1 (the first 
> 2.6.17-*
> I've attempted on this machine).
> 
> On resume from RAM, after a 30-second-ish timeout, the screen comes on
> but the hard disk is NOT accessible.  "dmesg" in an already-open window
> shows this (typed in from handwritten notes):
> 
> sd 0:0:0:0: SCSI error: return code = 0x40000
> end_request: I/O error, /dev/sda, sector nnnnnnn
...

Ahh.. the fix for this was posted earlier today by Forrest Zhao.
But his patch is for libata-dev, and doesn't apply as-is on 2.6.17-rc*

Here is a modified version of Forrest's original patch, for 2.6.17-rc5-git1.
It seems to have fixed the resume issue on my machine here,
so that things are now working as they were in the unpatched 2.6.16 kernels.

Can we get (something like) this into 2.6.17, pretty please?

Signed-off-by: Mark Lord <lkml@rtr.ca>
---
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -90,6 +90,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_device.h>
 #include <linux/libata.h>
 
 #define DRV_NAME	"ata_piix"
@@ -151,6 +152,7 @@ static int piix_pata_probe_reset(struct 
 static int piix_sata_probe_reset(struct ata_port *ap, unsigned int *classes);
 static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev);
 static void piix_set_dmamode (struct ata_port *ap, struct ata_device *adev);
+static int piix_scsi_device_resume(struct scsi_device *sdev);
 
 static unsigned int in_module_init = 1;
 
@@ -220,7 +222,7 @@ static struct scsi_host_template piix_sh
 	.dma_boundary		= ATA_DMA_BOUNDARY,
 	.slave_configure	= ata_scsi_slave_config,
 	.bios_param		= ata_std_bios_param,
-	.resume			= ata_scsi_device_resume,
+	.resume			= piix_scsi_device_resume,
 	.suspend		= ata_scsi_device_suspend,
 };
 
@@ -710,6 +712,21 @@ static void piix_set_dmamode (struct ata
 	}
 }
 
+int piix_scsi_device_resume(struct scsi_device *sdev)
+{
+	struct ata_port *ap = (struct ata_port *) &sdev->host->hostdata[0];
+	struct ata_device *dev = &ap->device[sdev->id];
+	u8 status;
+
+	status = ata_busy_wait(ap, ATA_BUSY, 200000);
+	if (status & ATA_BUSY) {
+		printk(KERN_ERR "libata port failed to resume "
+				"in 2 secs)\n");
+		return 1;
+	}
+	return ata_device_resume(ap, dev);
+}
+
 #define AHCI_PCI_BAR 5
 #define AHCI_GLOBAL_CTL 0x04
 #define AHCI_ENABLE (1 << 31)

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

[Jeff Garzik]

Mark Lord wrote:
> Mark Lord wrote:
>> My ata_piix based Notebook (Dell i9300) suspends/resumes perfectly (RAM 
>> or disk)
>> with 2.6.16.xx kernels, but fails resume on 2.6.17-rc5-git1 (the first 
>> 2.6.17-*
>> I've attempted on this machine).
>>
>> On resume from RAM, after a 30-second-ish timeout, the screen comes on
>> but the hard disk is NOT accessible.  "dmesg" in an already-open window
>> shows this (typed in from handwritten notes):
>>
>> sd 0:0:0:0: SCSI error: return code = 0x40000
>> end_request: I/O error, /dev/sda, sector nnnnnnn
> ...
> 
> Ahh.. the fix for this was posted earlier today by Forrest Zhao.
> But his patch is for libata-dev, and doesn't apply as-is on 2.6.17-rc*
> 
> Here is a modified version of Forrest's original patch, for 2.6.17-rc5-git1.
> It seems to have fixed the resume issue on my machine here,
> so that things are now working as they were in the unpatched 2.6.16 kernels.
> 
> Can we get (something like) this into 2.6.17, pretty please?

Definitely not.  I've repeatedly explained (and just done so again) why 
this is very wrong.  And you should know why, too, Mark ;-)

The controller resume (ata_pci_device_resume) does nothing 
controller-specific.  More importantly, the controller does not resume 
the ATA bus, and bring the ATA bus to bus-idle state.

	Jeff

[PATCH] Add ata_piix's own resume function

[zhao, forrest]

For ata_piix resume operation, it first waits for BUSY bit clear,
then calls ata_device_resume().


The patch is against #upstream 957d2df1801865eb1e63864bc63b970aa9c460ba

Thanks,
Forrest

Signed-off-by: Forrest Zhao <forrest.zhao@intel.com>
---

 drivers/scsi/ata_piix.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

55116b42caac4c48b2b85228e5b961d71266de22
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 1db007f..405f039 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -90,6 +90,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_device.h>
 #include <linux/libata.h>
 
 #define DRV_NAME	"ata_piix"
@@ -151,6 +152,7 @@ static int piix_pata_probe_reset(struct 
 static int piix_sata_probe_reset(struct ata_port *ap, unsigned int *classes);
 static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev);
 static void piix_set_dmamode (struct ata_port *ap, struct ata_device *adev);
+static int piix_scsi_device_resume(struct scsi_device *sdev);
 
 static unsigned int in_module_init = 1;
 
@@ -220,7 +222,7 @@ static struct scsi_host_template piix_sh
 	.dma_boundary		= ATA_DMA_BOUNDARY,
 	.slave_configure	= ata_scsi_slave_config,
 	.bios_param		= ata_std_bios_param,
-	.resume			= ata_scsi_device_resume,
+	.resume			= piix_scsi_device_resume,
 	.suspend		= ata_scsi_device_suspend,
 };
 
@@ -710,6 +712,21 @@ static void piix_set_dmamode (struct ata
 	}
 }
 
+int piix_scsi_device_resume(struct scsi_device *sdev)
+{
+	struct ata_port *ap = ata_shost_to_port(sdev->host);
+	struct ata_device *dev = &ap->device[sdev->id];
+	u8 status;
+
+	status = ata_busy_wait(ap, ATA_BUSY, 200000);
+	if (status & ATA_BUSY) {
+		ata_port_printk(ap, KERN_ERR, "port failed to resume "
+				"in 2 secs)\n");
+		return 1;
+	}
+	return ata_device_resume(dev);
+}
+
 #define AHCI_PCI_BAR 5
 #define AHCI_GLOBAL_CTL 0x04
 #define AHCI_ENABLE (1 << 31)
-- 
1.2.6

Re: [PATCH] Add ata_piix's own resume function

[Jens Axboe]

On Fri, May 26 2006, zhao, forrest wrote:
> For ata_piix resume operation, it first waits for BUSY bit clear,
> then calls ata_device_resume().

This has the problem that it introduces scsi specific knowledge into
ata_piix, something that is both a violation and a problem because we
are moving libata away from scsi. I think the best way to currently do
this is to introduce a ata_port_ops hook (pre_resume()?) that waits for
busy clear and gets called in ata_device_resume is probably the way to
go.

-- 
Jens Axboe

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

[Mark Lord]

Mark Lord wrote:
> Mark Lord wrote:
>> My ata_piix based Notebook (Dell i9300) suspends/resumes perfectly (RAM 
>> or disk)
>> with 2.6.16.xx kernels, but fails resume on 2.6.17-rc5-git1 (the first 
>> 2.6.17-*
>> I've attempted on this machine).
>>
>> On resume from RAM, after a 30-second-ish timeout, the screen comes on
>> but the hard disk is NOT accessible.  "dmesg" in an already-open window
>> shows this (typed in from handwritten notes):
>>
>> sd 0:0:0:0: SCSI error: return code = 0x40000
>> end_request: I/O error, /dev/sda, sector nnnnnnn
> ...
> 
> Ahh.. the fix for this was posted earlier today by Forrest Zhao.
..
> Here is a modified version of Forrest's original patch, for 2.6.17-rc5-git1.
> It seems to have fixed the resume issue on my machine here,
> so that things are now working as they were in the unpatched 2.6.16 kernels.

> Jens Axboe wrote:
> This has the problem that it introduces scsi specific knowledge into
> ata_piix, something that is both a violation and a problem because we
> are moving libata away from scsi. I think the best way to currently do
> this is to introduce a ata_port_ops hook (pre_resume()?) that waits for
> busy clear and gets called in ata_device_resume is probably the way to go.

Well, this problem has been with us all for a year now,
and at this point it impacts practically *every* new "centrino"
notebook out there.

We have a very simple workaround (previous post) that addresses it
for 2.6.17, and it's about damn time it got fixed.

If there's a better solution for *2.6.17*, then *please* post it.
Otherwise, we have a fix.  Maybe Linus or Andrew should just apply it?

Cheers

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

[Linus Torvalds]

On Fri, 26 May 2006, Mark Lord wrote:
> 
> Well, this problem has been with us all for a year now,
> and at this point it impacts practically *every* new "centrino"
> notebook out there.
> 
> We have a very simple workaround (previous post) that addresses it
> for 2.6.17, and it's about damn time it got fixed.
> 
> If there's a better solution for *2.6.17*, then *please* post it.
> Otherwise, we have a fix.  Maybe Linus or Andrew should just apply it?

I'm definitely in the "at some point, protesting a patch that works 
becomes an untenably position to take, no matter _how_ ugly the patch is" 
camp.

If the people who complain that it is ugly cannot come up with an 
alternate solution that works and isn't ugly, at some point the "ugly" 
complaint just becomes totally pointless. 

Of course, I'm not on linux-ide, and I didn't see this particular 
discussion from the start (or even the alledged simple workaround in the 
"previous post"), but can people please fill me in? And if the choice is 
not between "ugly" vs "pretty", but between "ugly" vs "nonworking", I 
think we know what the answer should be.

			Linus

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

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Fri, 26 May 2006, Mark Lord wrote:
>> Well, this problem has been with us all for a year now,
>> and at this point it impacts practically *every* new "centrino"
>> notebook out there.
>>
>> We have a very simple workaround (previous post) that addresses it
>> for 2.6.17, and it's about damn time it got fixed.
>>
>> If there's a better solution for *2.6.17*, then *please* post it.
>> Otherwise, we have a fix.  Maybe Linus or Andrew should just apply it?
> 
> I'm definitely in the "at some point, protesting a patch that works 
> becomes an untenably position to take, no matter _how_ ugly the patch is" 
> camp.
> 
> If the people who complain that it is ugly cannot come up with an 
> alternate solution that works and isn't ugly, at some point the "ugly" 
> complaint just becomes totally pointless. 
> 
> Of course, I'm not on linux-ide, and I didn't see this particular 
> discussion from the start (or even the alledged simple workaround in the 
> "previous post"), but can people please fill me in? And if the choice is 
> not between "ugly" vs "pretty", but between "ugly" vs "nonworking", I 
> think we know what the answer should be.

Mark is just a slacker, like the rest of us ;-)

The solution, described in [1], is basically "move the delay from <here> 
to <there>."

The current code does
	resume PCI device
	kick the ATA device
when it should do
	resume PCI device
	bring up the ATA bus
	kick the ATA device

Regards,

	Jeff


[1] http://marc.theaimsgroup.com/?l=linux-ide&m=114868613527204&w=2

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

[Jens Axboe]

On Fri, May 26 2006, Linus Torvalds wrote:
> 
> 
> On Fri, 26 May 2006, Mark Lord wrote:
> > 
> > Well, this problem has been with us all for a year now,
> > and at this point it impacts practically *every* new "centrino"
> > notebook out there.
> > 
> > We have a very simple workaround (previous post) that addresses it
> > for 2.6.17, and it's about damn time it got fixed.
> > 
> > If there's a better solution for *2.6.17*, then *please* post it.
> > Otherwise, we have a fix.  Maybe Linus or Andrew should just apply it?
> 
> I'm definitely in the "at some point, protesting a patch that works 
> becomes an untenably position to take, no matter _how_ ugly the patch is" 
> camp.
> 
> If the people who complain that it is ugly cannot come up with an 
> alternate solution that works and isn't ugly, at some point the "ugly" 
> complaint just becomes totally pointless. 

If that wasn't the case, then we wouldn't even have basic sata suspend
support in the Linus kernels right now... So agreed.

> Of course, I'm not on linux-ide, and I didn't see this particular 
> discussion from the start (or even the alledged simple workaround in the 
> "previous post"), but can people please fill me in? And if the choice is 
> not between "ugly" vs "pretty", but between "ugly" vs "nonworking", I 
> think we know what the answer should be.

There was no real discussion on this issue yet. I think we all agree
that the functionality of the patch (waiting for BSY clear on resume) is
the right thing to do. This posted patch moved SCSI stuff into ata_piix,
which isn't really very nice. Jeff wants to do it from the pci resume,
which just seems wrong to me since it's a device (disk) condition not a
"host" condition. FWIW, here's what I had in mind as suggested in the
original reply. Not tested at all for functionality, but it compiles.

diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 6dc8814..103afc3 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -151,6 +151,7 @@ static int piix_pata_probe_reset(struct 
 static int piix_sata_probe_reset(struct ata_port *ap, unsigned int *classes);
 static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev);
 static void piix_set_dmamode (struct ata_port *ap, struct ata_device *adev);
+static int piix_port_resume(struct ata_port *ap);
 
 static unsigned int in_module_init = 1;
 
@@ -250,6 +251,7 @@ static const struct ata_port_operations 
 
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
+	.port_resume		= piix_port_resume,
 	.host_stop		= ata_host_stop,
 };
 
@@ -738,6 +740,16 @@ static int piix_disable_ahci(struct pci_
 	return rc;
 }
 
+static int piix_port_resume(struct ata_port *ap)
+{
+	u8 status = ata_busy_wait(ap, ATA_BUSY, 200000);
+
+	if (status & ATA_BUSY)
+		return 1;
+
+	return 0;
+}
+
 /**
  *	piix_check_450nx_errata	-	Check for problem 450NX setup
  *	@ata_dev: the PCI device to check
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index fa476e7..ae7fac1 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -4298,6 +4298,8 @@ int ata_device_resume(struct ata_port *a
 {
 	if (ap->flags & ATA_FLAG_SUSPENDED) {
 		ap->flags &= ~ATA_FLAG_SUSPENDED;
+		if (ap->ops->port_resume)
+			ap->ops->port_resume(ap);
 		ata_set_mode(ap);
 	}
 	if (!ata_dev_present(dev))
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b80d2e7..0be5d02 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -461,6 +461,7 @@ struct ata_port_operations {
 
 	int (*port_start) (struct ata_port *ap);
 	void (*port_stop) (struct ata_port *ap);
+	int (*port_resume) (struct ata_port *ap);
 
 	void (*host_stop) (struct ata_host_set *host_set);
 

-- 
Jens Axboe

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

[Jeff Garzik]

Jens Axboe wrote:
> There was no real discussion on this issue yet. I think we all agree
> that the functionality of the patch (waiting for BSY clear on resume) is
> the right thing to do. This posted patch moved SCSI stuff into ata_piix,
> which isn't really very nice. Jeff wants to do it from the pci resume,
> which just seems wrong to me since it's a device (disk) condition not a

Wrong.  It is a _bus_ condition, not a device condition.


> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index fa476e7..ae7fac1 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -4298,6 +4298,8 @@ int ata_device_resume(struct ata_port *a
>  {
>  	if (ap->flags & ATA_FLAG_SUSPENDED) {
>  		ap->flags &= ~ATA_FLAG_SUSPENDED;
> +		if (ap->ops->port_resume)
> +			ap->ops->port_resume(ap);

This is even MORE broken!

A port can have multiple devices hanging off of it.  With this 
silliness, you will be calling ->port_resume() for both master and slave 
devices...  or all devices attached to a port multiplier.

Control flow is top-down, not bottom-up.

	Jeff

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

[Jens Axboe]

On Sat, May 27 2006, Jeff Garzik wrote:
> Jens Axboe wrote:
> >There was no real discussion on this issue yet. I think we all agree
> >that the functionality of the patch (waiting for BSY clear on resume) is
> >the right thing to do. This posted patch moved SCSI stuff into ata_piix,
> >which isn't really very nice. Jeff wants to do it from the pci resume,
> >which just seems wrong to me since it's a device (disk) condition not a
> 
> Wrong.  It is a _bus_ condition, not a device condition.

See my other mail.

> >diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> >index fa476e7..ae7fac1 100644
> >--- a/drivers/scsi/libata-core.c
> >+++ b/drivers/scsi/libata-core.c
> >@@ -4298,6 +4298,8 @@ int ata_device_resume(struct ata_port *a
> > {
> > 	if (ap->flags & ATA_FLAG_SUSPENDED) {
> > 		ap->flags &= ~ATA_FLAG_SUSPENDED;
> >+		if (ap->ops->port_resume)
> >+			ap->ops->port_resume(ap);
> 
> This is even MORE broken!
> 
> A port can have multiple devices hanging off of it.  With this 
> silliness, you will be calling ->port_resume() for both master and slave 
> devices...  or all devices attached to a port multiplier.

Worst case is the N-1 invocations basically being noops. Since
2.6.17-rc5 iirc doesn't even support port multipliers, I'd say this is a
pretty weak case.

What I care about is getting libata suspend/resume working, and so do a
lot of people. So far you've done nothing but whine at the posted
patches from the beginning and absolutely zero work on helping us get
there in _your_ driver/sub system. If you think your posted patch is the
best solution, by all means just put in there. Just make sure that we at
least get _something_ in there that does for 2.6.17.

I'll pick this up again tomorrow and actually test the patches so at
least I can say that 2.6.17 will suspend/resume for me.

-- 
Jens Axboe

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

[Jeff Garzik]

Jens Axboe wrote:
> On Sat, May 27 2006, Jeff Garzik wrote:
>> Jens Axboe wrote:
>>> There was no real discussion on this issue yet. I think we all agree
>>> that the functionality of the patch (waiting for BSY clear on resume) is
>>> the right thing to do. This posted patch moved SCSI stuff into ata_piix,
>>> which isn't really very nice. Jeff wants to do it from the pci resume,
>>> which just seems wrong to me since it's a device (disk) condition not a
>> Wrong.  It is a _bus_ condition, not a device condition.
> 
> See my other mail.
> 
>>> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
>>> index fa476e7..ae7fac1 100644
>>> --- a/drivers/scsi/libata-core.c
>>> +++ b/drivers/scsi/libata-core.c
>>> @@ -4298,6 +4298,8 @@ int ata_device_resume(struct ata_port *a
>>> {
>>> 	if (ap->flags & ATA_FLAG_SUSPENDED) {
>>> 		ap->flags &= ~ATA_FLAG_SUSPENDED;
>>> +		if (ap->ops->port_resume)
>>> +			ap->ops->port_resume(ap);
>> This is even MORE broken!
>>
>> A port can have multiple devices hanging off of it.  With this 
>> silliness, you will be calling ->port_resume() for both master and slave 
>> devices...  or all devices attached to a port multiplier.
> 
> Worst case is the N-1 invocations basically being noops. Since
> 2.6.17-rc5 iirc doesn't even support port multipliers, I'd say this is a
> pretty weak case.

If N-1 invocations are no-ops, that is a clear sign you got the layering 
very wrong.  Backwards, in fact.

And if you had to do something other than test for BSY -- say for 
example powering the bus on -- then you would be doing N-1 needless 
resets and power-ons.


> What I care about is getting libata suspend/resume working, and so do a
> lot of people. So far you've done nothing but whine at the posted
> patches from the beginning and absolutely zero work on helping us get
> there in _your_ driver/sub system. If you think your posted patch is the
> best solution, by all means just put in there. Just make sure that we at
> least get _something_ in there that does for 2.6.17.

After describing what to do innumerable times to people actively working 
in the area, I did indeed write the simple and obvious patch.  As soon 
as positive results appear, its going in.  The relevant subject is 
"[PATCH] libata resume improvements"

	Jeff

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

[Mark Lord]

Jens Axboe wrote:
..
> There was no real discussion on this issue yet. I think we all agree
> that the functionality of the patch (waiting for BSY clear on resume) is
> the right thing to do. This posted patch moved SCSI stuff into ata_piix,
> which isn't really very nice. Jeff wants to do it from the pci resume,
> which just seems wrong to me since it's a device (disk) condition not a
> "host" condition. FWIW, here's what I had in mind as suggested in the
> original reply. Not tested at all for functionality, but it compiles.
> 
> diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
> index 6dc8814..103afc3 100644
> --- a/drivers/scsi/ata_piix.c
> +++ b/drivers/scsi/ata_piix.c
> @@ -151,6 +151,7 @@ static int piix_pata_probe_reset(struct 
>  static int piix_sata_probe_reset(struct ata_port *ap, unsigned int *classes);
...

That patch, like Jeff's, does nothing (good) on my system.
It still pauses for a longish timeout on resume, and then fails
with a zillion "read errors", and leaves the drive inaccessible.

I'm will y'all on the poor layering, but non-functionality is worse.

Cheers

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

[Jeff Garzik]

Mark Lord wrote:
> Mark Lord wrote:
>> Mark Lord wrote:
>>> My ata_piix based Notebook (Dell i9300) suspends/resumes perfectly 
>>> (RAM or disk)
>>> with 2.6.16.xx kernels, but fails resume on 2.6.17-rc5-git1 (the 
>>> first 2.6.17-*
>>> I've attempted on this machine).
>>>
>>> On resume from RAM, after a 30-second-ish timeout, the screen comes on
>>> but the hard disk is NOT accessible.  "dmesg" in an already-open window
>>> shows this (typed in from handwritten notes):
>>>
>>> sd 0:0:0:0: SCSI error: return code = 0x40000
>>> end_request: I/O error, /dev/sda, sector nnnnnnn
>> ...
>>
>> Ahh.. the fix for this was posted earlier today by Forrest Zhao.
> ..
>> Here is a modified version of Forrest's original patch, for 
>> 2.6.17-rc5-git1.
>> It seems to have fixed the resume issue on my machine here,
>> so that things are now working as they were in the unpatched 2.6.16 
>> kernels.
> 
>> Jens Axboe wrote:
>> This has the problem that it introduces scsi specific knowledge into
>> ata_piix, something that is both a violation and a problem because we
>> are moving libata away from scsi. I think the best way to currently do
>> this is to introduce a ata_port_ops hook (pre_resume()?) that waits for
>> busy clear and gets called in ata_device_resume is probably the way to 
>> go.
> 
> Well, this problem has been with us all for a year now,
> and at this point it impacts practically *every* new "centrino"
> notebook out there.
> 
> We have a very simple workaround (previous post) that addresses it
> for 2.6.17, and it's about damn time it got fixed.
> 
> If there's a better solution for *2.6.17*, then *please* post it.
> Otherwise, we have a fix.  Maybe Linus or Andrew should just apply it?

Having been around for a year+, there is no reason to rush a really bad 
fix into the kernel.

Especially since I told you guys the proper place to put basically the 
exact same fix.  You probably could have created the proper fix in the 
time it took to get annoyed at my email, and type the reply...

	Jeff

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

[Jens Axboe]

On Fri, May 26 2006, Mark Lord wrote:
> Mark Lord wrote:
> >Mark Lord wrote:
> >>My ata_piix based Notebook (Dell i9300) suspends/resumes perfectly (RAM 
> >>or disk)
> >>with 2.6.16.xx kernels, but fails resume on 2.6.17-rc5-git1 (the first 
> >>2.6.17-*
> >>I've attempted on this machine).
> >>
> >>On resume from RAM, after a 30-second-ish timeout, the screen comes on
> >>but the hard disk is NOT accessible.  "dmesg" in an already-open window
> >>shows this (typed in from handwritten notes):
> >>
> >>sd 0:0:0:0: SCSI error: return code = 0x40000
> >>end_request: I/O error, /dev/sda, sector nnnnnnn
> >...
> >
> >Ahh.. the fix for this was posted earlier today by Forrest Zhao.
> ..
> >Here is a modified version of Forrest's original patch, for 
> >2.6.17-rc5-git1.
> >It seems to have fixed the resume issue on my machine here,
> >so that things are now working as they were in the unpatched 2.6.16 
> >kernels.
> 
> >Jens Axboe wrote:
> >This has the problem that it introduces scsi specific knowledge into
> >ata_piix, something that is both a violation and a problem because we
> >are moving libata away from scsi. I think the best way to currently do
> >this is to introduce a ata_port_ops hook (pre_resume()?) that waits for
> >busy clear and gets called in ata_device_resume is probably the way to go.
> 
> Well, this problem has been with us all for a year now,
> and at this point it impacts practically *every* new "centrino"
> notebook out there.
> 
> We have a very simple workaround (previous post) that addresses it
> for 2.6.17, and it's about damn time it got fixed.
> 
> If there's a better solution for *2.6.17*, then *please* post it.
> Otherwise, we have a fix.  Maybe Linus or Andrew should just apply it?

Don't get me wrong, I could not agree more. I rely on suspend/resume all
the time, and the fact that we didn't get this fully working _in kernel_
years ago is really embarassing. So I fully want the ata_piix busy clear
patch to be in 2.6.17, my objection was merely that shoving scsi
knowledge into ata_piix is not the way to do it.

-- 
Jens Axboe