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,
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.
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.
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
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
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)
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
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
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 [email protected]
> 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!
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)
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