2.6.22-rc4-mm2: Resume from RAM on HPC nx6325 broken

2.6.22-rc4-mm2: Resume from RAM on HPC nx6325 broken

[Rafael J. Wysocki]

Hi,

Here's the result of the search for the second patch that breaks resuming
from RAM on HPC nx6325 (x86_64):

ide-ide-hpa-detect-from-resume.patch

The symptom is that after the resume there's no backlight and the screen
apparently doesn't work, 100% of the time, although apart from this the system
seems to be functional.

The box doesn't even have an IDE HDD, but the DVD is handled by the atiixp
driver.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: 2.6.22-rc4-mm2: Resume from RAM on HPC nx6325 broken

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday 11 June 2007, Rafael J. Wysocki wrote:
> Hi,
> 
> Here's the result of the search for the second patch that breaks resuming
> from RAM on HPC nx6325 (x86_64):
> 
> ide-ide-hpa-detect-from-resume.patch
> 
> The symptom is that after the resume there's no backlight and the screen
> apparently doesn't work, 100% of the time, although apart from this the system
> seems to be functional.

I find it hard to accept that IDE patch is to blame for that. ;)

> The box doesn't even have an IDE HDD, but the DVD is handled by the atiixp
> driver.

If there are no IDE disks in the system there should be absolutely no
change in the functionality.  Please also take a look at the patch (below)
- it adds support for device driver ->resume method and implements it only
for ide-disk driver.

Either bisection went wrong and this is not the "guilty" patch (could you
please check that applying just this patch to -rc4 breaks resume too?) or
something really weird is going on...

Bart

[PATCH] ide: HPA detect from resume

From: Lee Trager <lt73@cs.drexel.edu>

Currently when system which have HPA require HPA to be detected and
disabled upon resume from RAM or disk. The current IDE drivers do not do
this nor does libata (obviously it since it doesn't support HPA yet).

I have implemented this into the current IDE drivers and it has been
tested by many others since 7/15/2006 in bug number 6840:

	http://bugzilla.kernel.org/show_bug.cgi?id=6840

and it has been confirmed to work fine with no problems.

bart: added drv != NULL check to generic_ide_suspend()

From: Lee Trager <lt73@cs.drexel.edu>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

---
 drivers/ide/ide-disk.c |   12 ++++++++++++
 drivers/ide/ide.c      |    9 ++++++++-
 include/linux/ide.h    |    1 +
 3 files changed, 21 insertions(+), 1 deletion(-)

Index: b/drivers/ide/ide-disk.c
===================================================================
--- a/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -1037,6 +1037,17 @@ static void ide_disk_release(struct kref
 
 static int ide_disk_probe(ide_drive_t *drive);
 
+/*
+ * On HPA drives the capacity needs to be
+ * reinitilized on resume otherwise the disk
+ * can not be used and a hard reset is required
+ */
+static void ide_disk_resume(ide_drive_t *drive)
+{
+	if (idedisk_supports_hpa(drive->id))
+		init_idedisk_capacity(drive);
+}
+
 static void ide_device_shutdown(ide_drive_t *drive)
 {
 #ifdef	CONFIG_ALPHA
@@ -1071,6 +1082,7 @@ static ide_driver_t idedisk_driver = {
 	},
 	.probe			= ide_disk_probe,
 	.remove			= ide_disk_remove,
+	.resume			= ide_disk_resume,
 	.shutdown		= ide_device_shutdown,
 	.version		= IDEDISK_VERSION,
 	.media			= ide_disk,
Index: b/drivers/ide/ide.c
===================================================================
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -1010,9 +1010,11 @@ static int generic_ide_resume(struct dev
 {
 	ide_drive_t *drive = dev->driver_data;
 	ide_hwif_t *hwif = HWIF(drive);
+	ide_driver_t *drv = to_ide_driver(dev->driver);
 	struct request rq;
 	struct request_pm_state rqpm;
 	ide_task_t args;
+	int err;
 
 	/* Call ACPI _STM only once */
 	if (!(drive->dn % 2))
@@ -1029,7 +1031,12 @@ static int generic_ide_resume(struct dev
 	rqpm.pm_step = ide_pm_state_start_resume;
 	rqpm.pm_state = PM_EVENT_ON;
 
-	return ide_do_drive_cmd(drive, &rq, ide_head_wait);
+	err = ide_do_drive_cmd(drive, &rq, ide_head_wait);
+
+	if (err == 0 && drv && drv->resume)
+		drv->resume(drive);
+
+	return err;
 }
 
 int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device *bdev,
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1001,6 +1001,7 @@ struct ide_driver_s {
 	struct device_driver	gen_driver;
 	int		(*probe)(ide_drive_t *);
 	void		(*remove)(ide_drive_t *);
+	void		(*resume)(ide_drive_t *);
 	void		(*shutdown)(ide_drive_t *);
 #ifdef CONFIG_IDE_PROC_FS
 	ide_proc_entry_t	*proc;

Re: 2.6.22-rc4-mm2: Resume from RAM on HPC nx6325 broken

[Rafael J. Wysocki]

On Monday, 11 June 2007 22:03, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Monday 11 June 2007, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > Here's the result of the search for the second patch that breaks resuming
> > from RAM on HPC nx6325 (x86_64):
> > 
> > ide-ide-hpa-detect-from-resume.patch
> > 
> > The symptom is that after the resume there's no backlight and the screen
> > apparently doesn't work, 100% of the time, although apart from this the system
> > seems to be functional.
> 
> I find it hard to accept that IDE patch is to blame for that. ;)

But still (actually, I only don't know why it appeared to be functional :-)).

> > The box doesn't even have an IDE HDD, but the DVD is handled by the atiixp
> > driver.
> 
> If there are no IDE disks in the system there should be absolutely no
> change in the functionality.

Sorry, but the patch is buggy.  Fix appended, details in the changelog.

Greetings,
Rafael


---
From: Rafael J. Wysocki <rjw@sisk.pl>

generic_ide_resume() should check if dev->driver is not NULL before applying
to_ide_driver() to it.  Fix that.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/ide/ide.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: linux-2.6.22-rc4/drivers/ide/ide.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/ide/ide.c
+++ linux-2.6.22-rc4/drivers/ide/ide.c
@@ -1010,7 +1010,6 @@ static int generic_ide_resume(struct dev
 {
 	ide_drive_t *drive = dev->driver_data;
 	ide_hwif_t *hwif = HWIF(drive);
-	ide_driver_t *drv = to_ide_driver(dev->driver);
 	struct request rq;
 	struct request_pm_state rqpm;
 	ide_task_t args;
@@ -1033,8 +1032,12 @@ static int generic_ide_resume(struct dev
 
 	err = ide_do_drive_cmd(drive, &rq, ide_head_wait);
 
-	if (err == 0 && drv && drv->resume)
-		drv->resume(drive);
+	if (err == 0 && dev->driver) {
+		ide_driver_t *drv = to_ide_driver(dev->driver);
+
+		if (drv->resume)
+			drv->resume(drive);
+	}
 
 	return err;
 }

Re: 2.6.22-rc4-mm2: Resume from RAM on HPC nx6325 broken

[Bartlomiej Zolnierkiewicz]

On the second thought...

On Monday 11 June 2007, Bartlomiej Zolnierkiewicz wrote:

> I find it hard to accept that IDE patch is to blame for that. ;)

Arrgghhh :)  It looks more likely now...

> --- a/drivers/ide/ide.c
> +++ b/drivers/ide/ide.c
> @@ -1010,9 +1010,11 @@ static int generic_ide_resume(struct dev
>  {
>  	ide_drive_t *drive = dev->driver_data;
>  	ide_hwif_t *hwif = HWIF(drive);
> +	ide_driver_t *drv = to_ide_driver(dev->driver);
>  	struct request rq;
>  	struct request_pm_state rqpm;
>  	ide_task_t args;
> +	int err;
>  
>  	/* Call ACPI _STM only once */
>  	if (!(drive->dn % 2))
> @@ -1029,7 +1031,12 @@ static int generic_ide_resume(struct dev
>  	rqpm.pm_step = ide_pm_state_start_resume;
>  	rqpm.pm_state = PM_EVENT_ON;
>  
> -	return ide_do_drive_cmd(drive, &rq, ide_head_wait);
> +	err = ide_do_drive_cmd(drive, &rq, ide_head_wait);
> +
> +	if (err == 0 && drv && drv->resume)

Could you try replacing this by

if (err == 0 && dev->driver && drv->resume)

and see if it fixes the problem?

If dev->driver is NULL drv won't be because to_ide_driver() is just
a wrapper for container_of().

Thanks,
Bart

Re: 2.6.22-rc4-mm2: Resume from RAM on HPC nx6325 broken

[Rafael J. Wysocki]

On Tuesday, 12 June 2007 00:01, Bartlomiej Zolnierkiewicz wrote:
> 
> On the second thought...
> 
> On Monday 11 June 2007, Bartlomiej Zolnierkiewicz wrote:
> 
> > I find it hard to accept that IDE patch is to blame for that. ;)
> 
> Arrgghhh :)  It looks more likely now...
> 
> > --- a/drivers/ide/ide.c
> > +++ b/drivers/ide/ide.c
> > @@ -1010,9 +1010,11 @@ static int generic_ide_resume(struct dev
> >  {
> >  	ide_drive_t *drive = dev->driver_data;
> >  	ide_hwif_t *hwif = HWIF(drive);
> > +	ide_driver_t *drv = to_ide_driver(dev->driver);
> >  	struct request rq;
> >  	struct request_pm_state rqpm;
> >  	ide_task_t args;
> > +	int err;
> >  
> >  	/* Call ACPI _STM only once */
> >  	if (!(drive->dn % 2))
> > @@ -1029,7 +1031,12 @@ static int generic_ide_resume(struct dev
> >  	rqpm.pm_step = ide_pm_state_start_resume;
> >  	rqpm.pm_state = PM_EVENT_ON;
> >  
> > -	return ide_do_drive_cmd(drive, &rq, ide_head_wait);
> > +	err = ide_do_drive_cmd(drive, &rq, ide_head_wait);
> > +
> > +	if (err == 0 && drv && drv->resume)
> 
> Could you try replacing this by
> 
> if (err == 0 && dev->driver && drv->resume)
> 
> and see if it fixes the problem?

Yes, that helps.  I've just posted a fix patch. :-)

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: 2.6.22-rc4-mm2: Resume from RAM on HPC nx6325 broken

[Bartlomiej Zolnierkiewicz]

On Monday 11 June 2007, Rafael J. Wysocki wrote:
> On Monday, 11 June 2007 22:03, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Monday 11 June 2007, Rafael J. Wysocki wrote:
> > > Hi,
> > > 
> > > Here's the result of the search for the second patch that breaks resuming
> > > from RAM on HPC nx6325 (x86_64):
> > > 
> > > ide-ide-hpa-detect-from-resume.patch
> > > 
> > > The symptom is that after the resume there's no backlight and the screen
> > > apparently doesn't work, 100% of the time, although apart from this the system
> > > seems to be functional.
> > 
> > I find it hard to accept that IDE patch is to blame for that. ;)
> 
> But still (actually, I only don't know why it appeared to be functional :-)).

Yeah...

> > > The box doesn't even have an IDE HDD, but the DVD is handled by the atiixp
> > > driver.
> > 
> > If there are no IDE disks in the system there should be absolutely no
> > change in the functionality.
> 
> Sorry, but the patch is buggy.  Fix appended, details in the changelog.

Yep, I know already, our emails crossed each other :)

> Greetings,
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> generic_ide_resume() should check if dev->driver is not NULL before applying
> to_ide_driver() to it.  Fix that.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Thanks for fixing this.

> ---
>  drivers/ide/ide.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6.22-rc4/drivers/ide/ide.c
> ===================================================================
> --- linux-2.6.22-rc4.orig/drivers/ide/ide.c
> +++ linux-2.6.22-rc4/drivers/ide/ide.c
> @@ -1010,7 +1010,6 @@ static int generic_ide_resume(struct dev
>  {
>  	ide_drive_t *drive = dev->driver_data;
>  	ide_hwif_t *hwif = HWIF(drive);
> -	ide_driver_t *drv = to_ide_driver(dev->driver);
>  	struct request rq;
>  	struct request_pm_state rqpm;
>  	ide_task_t args;
> @@ -1033,8 +1032,12 @@ static int generic_ide_resume(struct dev
>  
>  	err = ide_do_drive_cmd(drive, &rq, ide_head_wait);
>  
> -	if (err == 0 && drv && drv->resume)
> -		drv->resume(drive);
> +	if (err == 0 && dev->driver) {
> +		ide_driver_t *drv = to_ide_driver(dev->driver);
> +
> +		if (drv->resume)
> +			drv->resume(drive);
> +	}
>  
>  	return err;
>  }