HPA Resume patch

HPA Resume patch

[Lee Trager]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

This patch fixes a problem with computers that have HPA on their hard
drive and not being able to come out of resume from RAM or disk. I've
tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
of these. This patch also fixes the bug #6840. This is my first patch to
the kernel and I was told to e-mail the above people to get my patch
into the kernel. If I made a mistake please be gentle and correct me ;)

Lee Trager

[-- Attachment #2: hpa-resume.patch --]
[-- Type: text/plain, Size: 2937 bytes --]

diff -Naur linux-2.6.18-rc4-old/include/linux/ide.h linux-2.6.18-rc4/include/linux/ide.h
--- linux-2.6.18-rc4-old/include/linux/ide.h	2006-08-19 03:49:03.000000000 -0400
+++ linux-2.6.18-rc4/include/linux/ide.h	2006-08-20 19:13:10.000000000 -0400
@@ -1201,6 +1201,17 @@
 void ide_register_subdriver(ide_drive_t *, ide_driver_t *);
 void ide_unregister_subdriver(ide_drive_t *, ide_driver_t *);
 
+/* Bits 10 of command_set_1 and cfs_enable_1 must be equal,
+ * so on non-buggy drives we need test only one.
+ * However, we should also check whether these fields are valid.
+*/
+static inline int idedisk_supports_hpa(const struct hd_driveid *id)
+{
+        return (id->command_set_1 & 0x0400) && (id->cfs_enable_1 & 0x0400);
+}
+
+extern void init_idedisk_capacity (ide_drive_t  *drive);
+
 #define ON_BOARD		1
 #define NEVER_BOARD		0

diff -Naur linux-2.6.18-rc4-old/drivers/ide/ide-disk.c linux-2.6.18-rc4/drivers/ide/ide-disk.c
--- linux-2.6.18-rc4-old/drivers/ide/ide-disk.c	2006-08-19 03:49:03.000000000 -0400
+++ linux-2.6.18-rc4/drivers/ide/ide-disk.c	2006-08-20 19:13:56.000000000 -0400
@@ -464,16 +464,6 @@
 }
 
 /*
- * Bits 10 of command_set_1 and cfs_enable_1 must be equal,
- * so on non-buggy drives we need test only one.
- * However, we should also check whether these fields are valid.
- */
-static inline int idedisk_supports_hpa(const struct hd_driveid *id)
-{
-	return (id->command_set_1 & 0x0400) && (id->cfs_enable_1 & 0x0400);
-}
-
-/*
  * The same here.
  */
 static inline int idedisk_supports_lba48(const struct hd_driveid *id)
@@ -528,7 +518,7 @@
  * in above order (i.e., if value of higher priority is available,
  * reset will be ignored).
  */
-static void init_idedisk_capacity (ide_drive_t  *drive)
+void init_idedisk_capacity (ide_drive_t  *drive)
 {
 	struct hd_driveid *id = drive->id;
 	/*
@@ -555,6 +545,8 @@
 	}
 }
 
+EXPORT_SYMBOL(init_idedisk_capacity);
+
 static sector_t idedisk_capacity (ide_drive_t *drive)
 {
 	return drive->capacity64 - drive->sect0;
diff -Naur linux-2.6.18-rc4-old/drivers/ide/ide.c linux-2.6.18-rc4/drivers/ide/ide.c
--- linux-2.6.18-rc4-old/drivers/ide/ide.c	2006-08-19 03:49:03.000000000 -0400
+++ linux-2.6.18-rc4/drivers/ide/ide.c	2006-08-20 19:12:38.000000000 -0400
@@ -1232,6 +1232,7 @@
 	struct request rq;
 	struct request_pm_state rqpm;
 	ide_task_t args;
+	int ide_cmd;
 
 	memset(&rq, 0, sizeof(rq));
 	memset(&rqpm, 0, sizeof(rqpm));
@@ -1242,7 +1243,15 @@
 	rqpm.pm_step = ide_pm_state_start_resume;
 	rqpm.pm_state = PM_EVENT_ON;
 
-	return ide_do_drive_cmd(drive, &rq, ide_head_wait);
+	ide_cmd = ide_do_drive_cmd(drive, &rq, ide_head_wait);
+
+	/* check to see if this is a hard drive
+	 * if it is then checkhpa needs to be
+	 * disabled */
+	if(drive->media == ide_disk && idedisk_supports_hpa(drive->id))
+		init_idedisk_capacity(drive);
+
+	return ide_cmd;
 }
 
 int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device *bdev,

Re: HPA Resume patch

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On Sun, 27 Aug 2006 04:42:03 -0400 Lee Trager wrote:

> This patch fixes a problem with computers that have HPA on their hard
> drive and not being able to come out of resume from RAM or disk. I've
> tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
> of these. This patch also fixes the bug #6840. This is my first patch to
> the kernel and I was told to e-mail the above people to get my patch
> into the kernel. If I made a mistake please be gentle and correct me ;)

The patch adds a call from ide.c to a function inside ide-disk.c - this
won't work when IDE support is built as modules (it will cause a
circular dependency between ide-core and ide-disk modules).

The proper way to do such calls is to add a new method to ide_driver_t
and call it from generic_ide_resume().  Also, if the ide_do_drive_cmd()
call failed, it is probably unsafe to reset HPA, so you need to check
the result and call the resume method only if the low-level resume has
succeeded.

And please and "-p" to diff options.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: HPA Resume patch

[Pavel Machek]

Hi!

> This patch fixes a problem with computers that have HPA on their hard
> drive and not being able to come out of resume from RAM or disk. I've
> tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
> of these. This patch also fixes the bug #6840. This is my first patch to
> the kernel and I was told to e-mail the above people to get my patch
> into the kernel.

Congratulations for a first patch.

> If I made a mistake please be gentle and correct me ;)

We'll need signed-off-by: line next time.

Stefan, can we get this some testing? Or anyone else with thinkpad
with host-protected area still enabled?
							Pavel


> diff -Naur linux-2.6.18-rc4-old/include/linux/ide.h linux-2.6.18-rc4/include/linux/ide.h
> --- linux-2.6.18-rc4-old/include/linux/ide.h	2006-08-19 03:49:03.000000000 -0400
> +++ linux-2.6.18-rc4/include/linux/ide.h	2006-08-20 19:13:10.000000000 -0400
> @@ -1201,6 +1201,17 @@
>  void ide_register_subdriver(ide_drive_t *, ide_driver_t *);
>  void ide_unregister_subdriver(ide_drive_t *, ide_driver_t *);
>  
> +/* Bits 10 of command_set_1 and cfs_enable_1 must be equal,
> + * so on non-buggy drives we need test only one.
> + * However, we should also check whether these fields are valid.
> +*/
> +static inline int idedisk_supports_hpa(const struct hd_driveid *id)
> +{
> +        return (id->command_set_1 & 0x0400) && (id->cfs_enable_1 & 0x0400);
> +}
> +
> +extern void init_idedisk_capacity (ide_drive_t  *drive);
> +
>  #define ON_BOARD		1
>  #define NEVER_BOARD		0
> 
> diff -Naur linux-2.6.18-rc4-old/drivers/ide/ide-disk.c linux-2.6.18-rc4/drivers/ide/ide-disk.c
> --- linux-2.6.18-rc4-old/drivers/ide/ide-disk.c	2006-08-19 03:49:03.000000000 -0400
> +++ linux-2.6.18-rc4/drivers/ide/ide-disk.c	2006-08-20 19:13:56.000000000 -0400
> @@ -464,16 +464,6 @@
>  }
>  
>  /*
> - * Bits 10 of command_set_1 and cfs_enable_1 must be equal,
> - * so on non-buggy drives we need test only one.
> - * However, we should also check whether these fields are valid.
> - */
> -static inline int idedisk_supports_hpa(const struct hd_driveid *id)
> -{
> -	return (id->command_set_1 & 0x0400) && (id->cfs_enable_1 & 0x0400);
> -}
> -
> -/*
>   * The same here.
>   */
>  static inline int idedisk_supports_lba48(const struct hd_driveid *id)
> @@ -528,7 +518,7 @@
>   * in above order (i.e., if value of higher priority is available,
>   * reset will be ignored).
>   */
> -static void init_idedisk_capacity (ide_drive_t  *drive)
> +void init_idedisk_capacity (ide_drive_t  *drive)
>  {
>  	struct hd_driveid *id = drive->id;
>  	/*
> @@ -555,6 +545,8 @@
>  	}
>  }
>  
> +EXPORT_SYMBOL(init_idedisk_capacity);
> +
>  static sector_t idedisk_capacity (ide_drive_t *drive)
>  {
>  	return drive->capacity64 - drive->sect0;
> diff -Naur linux-2.6.18-rc4-old/drivers/ide/ide.c linux-2.6.18-rc4/drivers/ide/ide.c
> --- linux-2.6.18-rc4-old/drivers/ide/ide.c	2006-08-19 03:49:03.000000000 -0400
> +++ linux-2.6.18-rc4/drivers/ide/ide.c	2006-08-20 19:12:38.000000000 -0400
> @@ -1232,6 +1232,7 @@
>  	struct request rq;
>  	struct request_pm_state rqpm;
>  	ide_task_t args;
> +	int ide_cmd;
>  
>  	memset(&rq, 0, sizeof(rq));
>  	memset(&rqpm, 0, sizeof(rqpm));
> @@ -1242,7 +1243,15 @@
>  	rqpm.pm_step = ide_pm_state_start_resume;
>  	rqpm.pm_state = PM_EVENT_ON;
>  
> -	return ide_do_drive_cmd(drive, &rq, ide_head_wait);
> +	ide_cmd = ide_do_drive_cmd(drive, &rq, ide_head_wait);
> +
> +	/* check to see if this is a hard drive
> +	 * if it is then checkhpa needs to be
> +	 * disabled */
> +	if(drive->media == ide_disk && idedisk_supports_hpa(drive->id))
> +		init_idedisk_capacity(drive);
> +
> +	return ide_cmd;
>  }
>  
>  int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device *bdev,


-- 
Thanks for all the (sleeping) penguins.

Re: HPA Resume patch

[Jens Axboe]

On Sun, Aug 27 2006, Pavel Machek wrote:
> Hi!
> 
> > This patch fixes a problem with computers that have HPA on their hard
> > drive and not being able to come out of resume from RAM or disk. I've
> > tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
> > of these. This patch also fixes the bug #6840. This is my first patch to
> > the kernel and I was told to e-mail the above people to get my patch
> > into the kernel.
> 
> Congratulations for a first patch.
> 
> > If I made a mistake please be gentle and correct me ;)
> 
> We'll need signed-off-by: line next time.
> 
> Stefan, can we get this some testing? Or anyone else with thinkpad
> with host-protected area still enabled?

It has design issues, at someone else already noticed. hpa restore needs
to be a driver private step, included in the resume state machine. The
current patch is a gross layering violation.

But thanks to Lee for taking a stab at this, I hope he'll continue and
get it polished :-)

-- 
Jens Axboe

Re: HPA Resume patch

[Lee Trager]

Jens Axboe wrote:
> On Sun, Aug 27 2006, Pavel Machek wrote:
>   
>> Hi!
>>
>>     
>>> This patch fixes a problem with computers that have HPA on their hard
>>> drive and not being able to come out of resume from RAM or disk. I've
>>> tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
>>> of these. This patch also fixes the bug #6840. This is my first patch to
>>> the kernel and I was told to e-mail the above people to get my patch
>>> into the kernel.
>>>       
>> Congratulations for a first patch.
>>
>>     
>>> If I made a mistake please be gentle and correct me ;)
>>>       
>> We'll need signed-off-by: line next time.
>>
>> Stefan, can we get this some testing? Or anyone else with thinkpad
>> with host-protected area still enabled?
>>     
>
> It has design issues, at someone else already noticed. hpa restore needs
> to be a driver private step, included in the resume state machine. The
> current patch is a gross layering violation.
>
> But thanks to Lee for taking a stab at this, I hope he'll continue and
> get it polished :-)
>
>   
Ok I redid the patch following exactly what Sergey and Randy said. This
problem happens on any computer that has HPA on their drive when they
come back from resume so I don't think you have to only test this with
Thinkpad users. Anyway my only question is how to I get my patched
signed off by someone?

Thanks for all your help!

--- linux-2.6.18-rc4-old/include/linux/ide.h	2006-08-19 03:49:03.000000000 -0400
+++ linux-2.6.18-rc4/include/linux/ide.h	2006-08-28 05:45:06.000000000 -0400
@@ -987,6 +987,7 @@ typedef struct ide_driver_s {
 	int		(*probe)(ide_drive_t *);
 	void		(*remove)(ide_drive_t *);
 	void		(*shutdown)(ide_drive_t *);
+	void		(*resume)(ide_drive_t *);
 } ide_driver_t;
 
 #define to_ide_driver(drv) container_of(drv, ide_driver_t, gen_driver)
--- linux-2.6.18-rc4-old/drivers/ide/ide.c	2006-08-19 03:49:03.000000000 -0400
+++ linux-2.6.18-rc4/drivers/ide/ide.c	2006-08-28 21:38:50.000000000 -0400
@@ -1229,9 +1229,11 @@ static int generic_ide_suspend(struct de
 static int generic_ide_resume(struct device *dev)
 {
 	ide_drive_t *drive = dev->driver_data;
+	ide_driver_t *drv = to_ide_driver(dev->driver);
 	struct request rq;
 	struct request_pm_state rqpm;
 	ide_task_t args;
+	int err;
 
 	memset(&rq, 0, sizeof(rq));
 	memset(&rqpm, 0, sizeof(rqpm));
@@ -1242,7 +1244,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->resume)
+		drv->resume(drive);
+
+	return err;
 }
 
 int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device *bdev,
--- linux-2.6.18-rc4-old/drivers/ide/ide-disk.c	2006-08-19 03:49:03.000000000 -0400
+++ linux-2.6.18-rc4/drivers/ide/ide-disk.c	2006-08-28 21:54:17.000000000 -0400
@@ -1024,6 +1024,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
@@ -1067,6 +1078,7 @@ static ide_driver_t idedisk_driver = {
 	.error			= __ide_error,
 	.abort			= __ide_abort,
 	.proc			= idedisk_proc,
+	.resume			= ide_disk_resume,
 };
 
 static int idedisk_open(struct inode *inode, struct file *filp)

Re: HPA Resume patch

[Randy.Dunlap]

On Mon, 28 Aug 2006 22:14:34 -0400 Lee Trager wrote:

> Jens Axboe wrote:
> > On Sun, Aug 27 2006, Pavel Machek wrote:
> >   
> >> Hi!
> >>
> >>     
> >>> This patch fixes a problem with computers that have HPA on their hard
> >>> drive and not being able to come out of resume from RAM or disk. I've
> >>> tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
> >>> of these. This patch also fixes the bug #6840. This is my first patch to
> >>> the kernel and I was told to e-mail the above people to get my patch
> >>> into the kernel.
> >>>       
> >> Congratulations for a first patch.
> >>
> >>     
> >>> If I made a mistake please be gentle and correct me ;)
> >>>       
> >> We'll need signed-off-by: line next time.
> >>
> >> Stefan, can we get this some testing? Or anyone else with thinkpad
> >> with host-protected area still enabled?
> >>     
> >
> > It has design issues, at someone else already noticed. hpa restore needs
> > to be a driver private step, included in the resume state machine. The
> > current patch is a gross layering violation.
> >
> > But thanks to Lee for taking a stab at this, I hope he'll continue and
> > get it polished :-)
> >
> >   
> Ok I redid the patch following exactly what Sergey and Randy said. This
> problem happens on any computer that has HPA on their drive when they
> come back from resume so I don't think you have to only test this with
> Thinkpad users. Anyway my only question is how to I get my patched
> signed off by someone?

You do that :) after you read Documentation/SubmittingPatches and can "sign off"
on the patch.


> Thanks for all your help!
> 
> --- linux-2.6.18-rc4-old/include/linux/ide.h	2006-08-19 03:49:03.000000000 -0400
> +++ linux-2.6.18-rc4/include/linux/ide.h	2006-08-28 05:45:06.000000000 -0400
> @@ -987,6 +987,7 @@ typedef struct ide_driver_s {
>  	int		(*probe)(ide_drive_t *);
>  	void		(*remove)(ide_drive_t *);
>  	void		(*shutdown)(ide_drive_t *);
> +	void		(*resume)(ide_drive_t *);
>  } ide_driver_t;
>  
>  #define to_ide_driver(drv) container_of(drv, ide_driver_t, gen_driver)
> --- linux-2.6.18-rc4-old/drivers/ide/ide.c	2006-08-19 03:49:03.000000000 -0400
> +++ linux-2.6.18-rc4/drivers/ide/ide.c	2006-08-28 21:38:50.000000000 -0400
> @@ -1229,9 +1229,11 @@ static int generic_ide_suspend(struct de
>  static int generic_ide_resume(struct device *dev)
>  {
>  	ide_drive_t *drive = dev->driver_data;
> +	ide_driver_t *drv = to_ide_driver(dev->driver);
>  	struct request rq;
>  	struct request_pm_state rqpm;
>  	ide_task_t args;
> +	int err;
>  
>  	memset(&rq, 0, sizeof(rq));
>  	memset(&rqpm, 0, sizeof(rqpm));
> @@ -1242,7 +1244,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->resume)
> +		drv->resume(drive);
> +
> +	return err;
>  }
>  
>  int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device *bdev,
> --- linux-2.6.18-rc4-old/drivers/ide/ide-disk.c	2006-08-19 03:49:03.000000000 -0400
> +++ linux-2.6.18-rc4/drivers/ide/ide-disk.c	2006-08-28 21:54:17.000000000 -0400
> @@ -1024,6 +1024,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
> @@ -1067,6 +1078,7 @@ static ide_driver_t idedisk_driver = {
>  	.error			= __ide_error,
>  	.abort			= __ide_abort,
>  	.proc			= idedisk_proc,
> +	.resume			= ide_disk_resume,
>  };
>  
>  static int idedisk_open(struct inode *inode, struct file *filp)


---
~Randy

Re: HPA Resume patch

[Lee Trager]

Randy.Dunlap wrote:
> On Mon, 28 Aug 2006 22:14:34 -0400 Lee Trager wrote:
>
>   
>> Jens Axboe wrote:
>>     
>>> On Sun, Aug 27 2006, Pavel Machek wrote:
>>>   
>>>       
>>>> Hi!
>>>>
>>>>     
>>>>         
>>>>> This patch fixes a problem with computers that have HPA on their hard
>>>>> drive and not being able to come out of resume from RAM or disk. I've
>>>>> tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
>>>>> of these. This patch also fixes the bug #6840. This is my first patch to
>>>>> the kernel and I was told to e-mail the above people to get my patch
>>>>> into the kernel.
>>>>>       
>>>>>           
>>>> Congratulations for a first patch.
>>>>
>>>>     
>>>>         
>>>>> If I made a mistake please be gentle and correct me ;)
>>>>>       
>>>>>           
>>>> We'll need signed-off-by: line next time.
>>>>
>>>> Stefan, can we get this some testing? Or anyone else with thinkpad
>>>> with host-protected area still enabled?
>>>>     
>>>>         
>>> It has design issues, at someone else already noticed. hpa restore needs
>>> to be a driver private step, included in the resume state machine. The
>>> current patch is a gross layering violation.
>>>
>>> But thanks to Lee for taking a stab at this, I hope he'll continue and
>>> get it polished :-)
>>>
>>>   
>>>       
>> Ok I redid the patch following exactly what Sergey and Randy said. This
>> problem happens on any computer that has HPA on their drive when they
>> come back from resume so I don't think you have to only test this with
>> Thinkpad users. Anyway my only question is how to I get my patched
>> signed off by someone?
>>     
>
> You do that :) after you read Documentation/SubmittingPatches and can "sign off"
> on the patch.
>
>
>   
>> Thanks for all your help!
>>
>> --- linux-2.6.18-rc4-old/include/linux/ide.h	2006-08-19 03:49:03.000000000 -0400
>> +++ linux-2.6.18-rc4/include/linux/ide.h	2006-08-28 05:45:06.000000000 -0400
>> @@ -987,6 +987,7 @@ typedef struct ide_driver_s {
>>  	int		(*probe)(ide_drive_t *);
>>  	void		(*remove)(ide_drive_t *);
>>  	void		(*shutdown)(ide_drive_t *);
>> +	void		(*resume)(ide_drive_t *);
>>  } ide_driver_t;
>>  
>>  #define to_ide_driver(drv) container_of(drv, ide_driver_t, gen_driver)
>> --- linux-2.6.18-rc4-old/drivers/ide/ide.c	2006-08-19 03:49:03.000000000 -0400
>> +++ linux-2.6.18-rc4/drivers/ide/ide.c	2006-08-28 21:38:50.000000000 -0400
>> @@ -1229,9 +1229,11 @@ static int generic_ide_suspend(struct de
>>  static int generic_ide_resume(struct device *dev)
>>  {
>>  	ide_drive_t *drive = dev->driver_data;
>> +	ide_driver_t *drv = to_ide_driver(dev->driver);
>>  	struct request rq;
>>  	struct request_pm_state rqpm;
>>  	ide_task_t args;
>> +	int err;
>>  
>>  	memset(&rq, 0, sizeof(rq));
>>  	memset(&rqpm, 0, sizeof(rqpm));
>> @@ -1242,7 +1244,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->resume)
>> +		drv->resume(drive);
>> +
>> +	return err;
>>  }
>>  
>>  int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device *bdev,
>> --- linux-2.6.18-rc4-old/drivers/ide/ide-disk.c	2006-08-19 03:49:03.000000000 -0400
>> +++ linux-2.6.18-rc4/drivers/ide/ide-disk.c	2006-08-28 21:54:17.000000000 -0400
>> @@ -1024,6 +1024,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
>> @@ -1067,6 +1078,7 @@ static ide_driver_t idedisk_driver = {
>>  	.error			= __ide_error,
>>  	.abort			= __ide_abort,
>>  	.proc			= idedisk_proc,
>> +	.resume			= ide_disk_resume,
>>  };
>>  
>>  static int idedisk_open(struct inode *inode, struct file *filp)
>>     
>
>
> ---
> ~Randy
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>   
Ok I read the docs on submitting patches and coding style, I'll resubmit.

Thanks!

Re: HPA Resume patch

[Jens Axboe]

On Mon, Aug 28 2006, Lee Trager wrote:
> Jens Axboe wrote:
> > On Sun, Aug 27 2006, Pavel Machek wrote:
> >   
> >> Hi!
> >>
> >>     
> >>> This patch fixes a problem with computers that have HPA on their hard
> >>> drive and not being able to come out of resume from RAM or disk. I've
> >>> tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
> >>> of these. This patch also fixes the bug #6840. This is my first patch to
> >>> the kernel and I was told to e-mail the above people to get my patch
> >>> into the kernel.
> >>>       
> >> Congratulations for a first patch.
> >>
> >>     
> >>> If I made a mistake please be gentle and correct me ;)
> >>>       
> >> We'll need signed-off-by: line next time.
> >>
> >> Stefan, can we get this some testing? Or anyone else with thinkpad
> >> with host-protected area still enabled?
> >>     
> >
> > It has design issues, at someone else already noticed. hpa restore needs
> > to be a driver private step, included in the resume state machine. The
> > current patch is a gross layering violation.
> >
> > But thanks to Lee for taking a stab at this, I hope he'll continue and
> > get it polished :-)
> >
> >   
> Ok I redid the patch following exactly what Sergey and Randy said. This
> problem happens on any computer that has HPA on their drive when they
> come back from resume so I don't think you have to only test this with
> Thinkpad users. Anyway my only question is how to I get my patched
> signed off by someone?
> 
> Thanks for all your help!

While this is a _lot_ better than your previous patch, I don't think you
quite understood my suggestion. Which is probably fault, so I'll try to
be a little more verbose.

If you look at the suspend state machine, it goes through a (small)
sequence of steps (see drivers/ide/ide-io.c:ide_complete_power_step())
and get the device suspended. Same thing for resume, just not that many
steps there. My suggestion was to continue using this infrastructure and
just add the HPA restore as a resume state. Right now it does:

        ide_pm_state_start_resume (== idedisk_pm_idle)
                complete that
        ide_pm_restore_dma
                complete that

and we are done. Your patch basically puts more actions into a single
resume state switch, not ideal. What you want to do is have the HPA
restore as an additional state.

Is that clearer? If not, let me know...

-- 
Jens Axboe

Re: HPA Resume patch

[Lee Trager]

Jens Axboe wrote:
> On Mon, Aug 28 2006, Lee Trager wrote:
>   
>> Jens Axboe wrote:
>>     
>>> On Sun, Aug 27 2006, Pavel Machek wrote:
>>>   
>>>       
>>>> Hi!
>>>>
>>>>     
>>>>         
>>>>> This patch fixes a problem with computers that have HPA on their hard
>>>>> drive and not being able to come out of resume from RAM or disk. I've
>>>>> tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
>>>>> of these. This patch also fixes the bug #6840. This is my first patch to
>>>>> the kernel and I was told to e-mail the above people to get my patch
>>>>> into the kernel.
>>>>>       
>>>>>           
>>>> Congratulations for a first patch.
>>>>
>>>>     
>>>>         
>>>>> If I made a mistake please be gentle and correct me ;)
>>>>>       
>>>>>           
>>>> We'll need signed-off-by: line next time.
>>>>
>>>> Stefan, can we get this some testing? Or anyone else with thinkpad
>>>> with host-protected area still enabled?
>>>>     
>>>>         
>>> It has design issues, at someone else already noticed. hpa restore needs
>>> to be a driver private step, included in the resume state machine. The
>>> current patch is a gross layering violation.
>>>
>>> But thanks to Lee for taking a stab at this, I hope he'll continue and
>>> get it polished :-)
>>>
>>>   
>>>       
>> Ok I redid the patch following exactly what Sergey and Randy said. This
>> problem happens on any computer that has HPA on their drive when they
>> come back from resume so I don't think you have to only test this with
>> Thinkpad users. Anyway my only question is how to I get my patched
>> signed off by someone?
>>
>> Thanks for all your help!
>>     
>
> While this is a _lot_ better than your previous patch, I don't think you
> quite understood my suggestion. Which is probably fault, so I'll try to
> be a little more verbose.
>
> If you look at the suspend state machine, it goes through a (small)
> sequence of steps (see drivers/ide/ide-io.c:ide_complete_power_step())
> and get the device suspended. Same thing for resume, just not that many
> steps there. My suggestion was to continue using this infrastructure and
> just add the HPA restore as a resume state. Right now it does:
>
>         ide_pm_state_start_resume (== idedisk_pm_idle)
>                 complete that
>         ide_pm_restore_dma
>                 complete that
>
> and we are done. Your patch basically puts more actions into a single
> resume state switch, not ideal. What you want to do is have the HPA
> restore as an additional state.
>
> Is that clearer? If not, let me know...
>
>   
Ok I've been looking through the source to see how to do this properly.
I can't figure out where ide_generic_resume() is called from. It doesn't
look like its called from ide_complete_power_step() I want to know
because I want the HPA stuff to be done right after ide_generic_resume()
is called. I'll do some testing to see if it works to get called before
and after the DMA stuff.

Thanks,

Lee

-- 
VGER BF report: U 0.5

Re: HPA Resume patch

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 427 bytes --]

On Mon, 28 Aug 2006 22:14:34 EDT, Lee Trager said:

> Thinkpad users. Anyway my only question is how to I get my patched
> signed off by someone?

You read Documentation/SubmittingPatches, and follow the directions there,
and remember to merge in any comments people might have...

> +	err = ide_do_drive_cmd(drive, &rq, ide_head_wait);
> +
> +	if (err == 0 && drv->resume)

Often better written as:

	if (!err && drv->resume)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: HPA Resume patch

[Randy.Dunlap]

On Sun, 27 Aug 2006 04:42:03 -0400 Lee Trager wrote:

> This patch fixes a problem with computers that have HPA on their hard
> drive and not being able to come out of resume from RAM or disk. I've
> tested this patch on 2.6.17.x and 2.6.18-rc4 and it works great on both
> of these. This patch also fixes the bug #6840. This is my first patch to
> the kernel and I was told to e-mail the above people to get my patch
> into the kernel. If I made a mistake please be gentle and correct me ;)

Please use inline patches if your mail system supports/allows it.
Attachments make it difficult to review a patch.
(now do some cp-n-paste for comments)

+/* Bits 10 of command_set_1 and cfs_enable_1 must be equal,
+ * so on non-buggy drives we need test only one.
+ * However, we should also check whether these fields are valid.
+*/

Long comment style in Linux is:
/*
 * foo bar
 * comments
 */

+static inline int idedisk_supports_hpa(const struct hd_driveid *id)
+{
+        return (id->command_set_1 & 0x0400) && (id->cfs_enable_1 & 0x0400);
+}

Please use a #defined value for the 0x400.
(Yes, you just moved it from somewhere else.)

+	/* check to see if this is a hard drive
+	 * if it is then checkhpa needs to be
+	 * disabled */

Comment style again.

+	if(drive->media == ide_disk && idedisk_supports_hpa(drive->id))

space after "if".

+		init_idedisk_capacity(drive);


---
~Randy