LinuxLists.cc - [RFC]add ACPI hooks for IDE suspend/resume

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Tue, Dec 06, 2005 at 02:10:04PM +0800, Shaohua Li wrote:

> TODO: invoking ATA commands.
>
> Though we didn't invoke ATA commands, this patch fixes the bug at
> http://bugzilla.kernel.org/show_bug.cgi?id=5604. And Matthew said this
> actually fixes a lot of systems in his test.
> I'm not familiar with IDE, so comments/suggestions are welcome.

Of the DSDTs I've looked at, most seem to provide taskfile commands
concerned about doing things like setting up drive security. I haven't
yet seen an IDE failure on resume when using this patch, so the _GTF
stuff doesn't seem terribly important. The reason for it not currently
being implemented is that the IDE queues haven't been restarted at the
point where this code is called, so there's no obvious way of submitting
them to the drive yet.

--
Matthew Garrett | [email protected]

2005-12-06 22:52:22

by Alan

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Maw, 2005-12-06 at 22:20 +0000, Matthew Garrett wrote:
> stuff doesn't seem terribly important. The reason for it not currently
> being implemented is that the IDE queues haven't been restarted at the
> point where this code is called, so there's no obvious way of submitting
> them to the drive yet.

Issue the task files in sequence to the drive directly with nIEN set and
poll them. In the libata case there is a clean API to do this but even
the old IDE has enough order and logic in this area it would not be hard
to do nicely.

2005-12-07 00:11:29

by Randy Dunlap

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Tue, 6 Dec 2005, Shaohua Li wrote:

> Hi,
> Adding ACPI IDE hook in IDE suspend/resume. The ACPI spec
> explicitly says we must call some ACPI methods to restore IDE drives.
> The sequences defined by ACPI spec are:
> suspend:
> 1. Get the DMA and PIO info from IDE channel's _GTM method.
>
> resume:
> 1. Calling IDE channel's _STM to set the transfer timing setting.
> 2. For each drive on the IDE channel, running drive's _GTF to get the
> ATA commands required to reinitialize each drive.
> 3. Sending the ATA commands gotton from step 2 to drives.
>
> TODO: invoking ATA commands.
>
> Though we didn't invoke ATA commands, this patch fixes the bug at
> http://bugzilla.kernel.org/show_bug.cgi?id=5604. And Matthew said this
> actually fixes a lot of systems in his test.
> I'm not familiar with IDE, so comments/suggestions are welcome.
>
> ---
>
> linux-2.6.15-rc5-root/drivers/ide/ide.c | 282 ++++++++++++++++++++++++++++++++
> 1 files changed, 282 insertions(+)
>
> diff -puN drivers/ide/ide.c~acpi-ide drivers/ide/ide.c
> --- linux-2.6.15-rc5/drivers/ide/ide.c~acpi-ide 2005-12-07 03:01:36.000000000 +0800
> +++ linux-2.6.15-rc5-root/drivers/ide/ide.c 2005-12-07 03:01:36.000000000 +0800
> @@ -155,6 +155,10 @@
> #include <linux/device.h>
> #include <linux/bitops.h>
>
> +#ifdef CONFIG_ACPI
> +#include <linux/acpi.h>
> +#endif

Shouldn't need or use ifdef/endif for #includes.

> #include <asm/byteorder.h>
> #include <asm/irq.h>
> #include <asm/uaccess.h>
> @@ -1214,6 +1218,279 @@ int system_bus_clock (void)
>
> EXPORT_SYMBOL(system_bus_clock);
>
> +#ifdef CONFIG_ACPI
> +static int ide_acpi_find_device(struct device *dev, acpi_handle *handle)
> +{
> + int i, tmp;
> + acpi_integer addr;
> +
> + if (sscanf(dev->bus_id, "%u.%u", &tmp, &i) != 2)
> + return -ENODEV;
> +
> + addr = (acpi_integer)i;
> + *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr);
> + if (!*handle)
> + return -ENODEV;
> + return 0;
> +}
> +
> +/* This assumes the ide controller is a PCI device */
> +static int ide_acpi_find_channel(struct device *dev, acpi_handle *handle)
> +{
> + int num;
> + int channel;
> + acpi_integer addr;
> +
> + num = sscanf(dev->bus_id, "ide%x", &channel);
> +
> + if (num != 1 || !dev->parent)
> + return -ENODEV;
> + addr = (acpi_integer)channel;
> + *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr);
> + if (!*handle)
> + return -ENODEV;
> + return 0;
> +}
> +
> +static struct acpi_bus_type ide_acpi_bus = {
> + .bus = &ide_bus_type,
> + .find_device = ide_acpi_find_device,
> + .find_bridge = ide_acpi_find_channel,
> +};
> +
> +static int __init ide_acpi_init(void)
> +{
> + return register_acpi_bus_type(&ide_acpi_bus);
> +}
> +
> +/* The _GTM return package length is 5 dwords */
> +#define GTM_LEN (sizeof(u32) * 5)
> +struct acpi_ide_state {
> + acpi_handle handle; /* channel device's handle */
> + u32 gtm[GTM_LEN/sizeof(u32)]; /* info from _GTM */
> + struct hd_driveid id_buff[2]; /* one chanel has two drives */

s/2/MAX_DRIVES/
s/chanel/channel/

> + int suspend_drives;
> + int resume_drives;
> +};
> +
> +static void acpi_ide_data_handler(acpi_handle handle,
> + u32 function, void *context)
> +{
> + /* nothing to do */
> +}
> +
> +/* acpi data for a chanel */

s/chanel/channel/

> +static struct acpi_ide_state *ide_alloc_acpi_state(acpi_handle handle)
> +{
> + struct acpi_ide_state * state;
> + acpi_status status;
> +
> + state = kzalloc(sizeof(struct acpi_ide_state), GFP_KERNEL);
> + if (!state)
> + return NULL;
> + status = acpi_attach_data(handle, acpi_ide_data_handler, state);
> + if (ACPI_FAILURE(status))
> + return NULL;
> + return state;
> +}
> +
> +static struct acpi_ide_state *ide_get_acpi_state(acpi_handle handle)
> +{
> + struct acpi_ide_state * state;
> + acpi_status status;
> +
> + status = acpi_get_data(handle, acpi_ide_data_handler, (void **)&state);
> + if (ACPI_FAILURE(status))
> + return NULL;
> + return state;
> +}
> +
> +static void ide_free_acpi_state(acpi_handle handle)
> +{
> + struct acpi_ide_state *state;
> +
> + state = ide_get_acpi_state(handle);
> + acpi_detach_data(handle, acpi_ide_data_handler);
> + kfree(state);
> +}
> +
> +static int acpi_ide_suspend(struct device *dev)
> +{
> + acpi_handle handle, parent_handle;
> + struct acpi_ide_state *state;
> + acpi_status status;
> + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> + union acpi_object *package;
> + ide_drive_t *drive = dev->driver_data;
> + int drive_id = 0;
> +
> + handle = DEVICE_ACPI_HANDLE(dev);
> + if (!handle) {
> + printk(KERN_DEBUG "IDE device's ACPI handler is NULL\n");

s/handler/handle/ ??

> + return -ENODEV;
> + }
> + if (ACPI_FAILURE(acpi_get_parent(handle, &parent_handle))) {
> + printk(KERN_ERR "ACPI get parent handler error\n");

s/handler/handle/ ??

> + return -ENODEV;
> + }
> + state = ide_get_acpi_state(parent_handle);
> + if (!state) {
> + state = ide_alloc_acpi_state(parent_handle);
> + if (!state)
> + return -ENODEV;
> + }
> +
> + /* invoke _GTM only once */
> + state->suspend_drives++;
> + if (state->suspend_drives > 1) {
> + drive_id = 1;
> + goto id;
> + }
> +
> + status = acpi_evaluate_object(parent_handle, "_GTM", NULL, &buffer);
> + if (ACPI_FAILURE(status)) {
> + printk(KERN_ERR "Error evaluating _GTM\n");

I don't read the ACPI spec. as saying that _GTM is required, (?)
so I would make this a KERN_DEBUG instead of KERN_ERR.

> + return -ENODEV;
> + }
> + package = (union acpi_object *) buffer.pointer;
> + if (package->buffer.length != GTM_LEN) {
> + printk(KERN_ERR "Buffer length returned by _GTM is wrong\n");
> + acpi_os_free(buffer.pointer);
> + return -ENODEV;
> + }
> + memcpy(state->gtm, package->buffer.pointer, GTM_LEN);
> + state->handle = parent_handle;
> + acpi_os_free(buffer.pointer);
> +id:
> + taskfile_lib_get_identify(drive, (u8*)&state->id_buff[drive_id]);
> + return 0;
> +}
> +
> +static int acpi_ide_invoke_stm(struct acpi_ide_state *state)
> +{
> + struct acpi_object_list input;
> + union acpi_object params[3];
> + acpi_status status;
> +
> + input.count = 3;
> + input.pointer = params;
> + params[0].type = ACPI_TYPE_BUFFER;
> + params[0].buffer.length = sizeof(state->gtm);
> + params[0].buffer.pointer = (char*)state->gtm;
> +
> + params[1].type = ACPI_TYPE_BUFFER;
> + params[1].buffer.length = sizeof(state->id_buff[0]);
> + params[1].buffer.pointer = (char *)&state->id_buff[0];
> +
> + params[2].type = ACPI_TYPE_BUFFER;
> + params[2].buffer.length = sizeof(state->id_buff[1]);
> + params[2].buffer.pointer = (char *)&state->id_buff[1];
> +
> + status = acpi_evaluate_object(state->handle, "_STM", &input, NULL);
> + if (ACPI_FAILURE(status)) {
> + printk(KERN_ERR "Evaluating _STM error\n");

Same as for _GTM, KERN_DEBUG instead of KERN_ERR.

> + return -ENODEV;
> + }
> + return 0;
> +}
> +
> +static int acpi_ide_invoke_gtf(acpi_handle handle, ide_drive_t *drive)
> +{
> + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> +#if 0
> + ide_task_t args;
> + int index = 0;
> + unsigned char *data;
> +#endif
> + union acpi_object *package;
> + acpi_status status;
> +
> + status = acpi_evaluate_object(handle, "_GTF", NULL, &output);
> + if (ACPI_FAILURE(status)) {
> + printk(KERN_ERR "evaluate _GTF error\n");

KERN_DEBUG if not present since it's not required AFAIK.

> + return -ENODEV;
> + }
> +
> + package = (union acpi_object *) output.pointer;
> + if (package->type != ACPI_TYPE_BUFFER
> + || (package->buffer.length % 7) != 0) {
> + acpi_os_free(output.pointer);
> + printk(KERN_ERR "_GTF returned value is wrong\n");
> + return -ENODEV;
> + }
> +#if 0
> + printk(KERN_DEBUG "Start invoking _GTF commands\n");
> +
> + data = package->buffer.pointer;
> + /* sumbit ATA commands */
> + while (index < package->buffer.length) {
> + memset(&args, 0, sizeof(ide_task_t));
> + args.tfRegister[IDE_ERROR_OFFSET] = data[index];
> + args.tfRegister[IDE_NSECTOR_OFFSET] = data[index + 1];
> + args.tfRegister[IDE_SECTOR_OFFSET] = data[index + 2];
> + args.tfRegister[IDE_LCYL_OFFSET] = data[index + 3];
> + args.tfRegister[IDE_HCYL_OFFSET] = data[index + 4];
> + args.tfRegister[IDE_SELECT_OFFSET] = data[index + 5];
> + args.tfRegister[IDE_STATUS_OFFSET] = data[index + 6];
> + args.command_type = IDE_DRIVE_TASK_NO_DATA;
> + args.handler = &task_no_data_intr;
> + /* submit command */
> + index += 7;
> + }
> +#endif
> + acpi_os_free(output.pointer);
> + return 0;
> +}
> +
> +static int acpi_ide_resume(struct device *dev)
> +{
> + acpi_handle handle, parent_handle;
> + struct acpi_ide_state *state;
> + ide_drive_t *drive = dev->driver_data;
> +
> + handle = DEVICE_ACPI_HANDLE(dev);
> + if (!handle) {
> + printk(KERN_DEBUG "IDE device's ACPI handler is NULL\n");

s/handler/handle/ ??

> + return -ENODEV;
> + }
> + if (ACPI_FAILURE(acpi_get_parent(handle, &parent_handle))) {
> + printk(KERN_ERR "ACPI get parent handler error\n");

s/handler/handle/ ??

> + return -ENODEV;
> + }
> + state = ide_get_acpi_state(parent_handle);
> + if (state == NULL)
> + return -ENODEV;
> +
> + /* invoke _STM only once */
> + state->resume_drives++;
> + if (state->resume_drives == 1) {
> + printk(KERN_DEBUG "Start invoking _STM\n");
> + if (acpi_ide_invoke_stm(state))
> + return -ENODEV;
> + }
> +
> + if (state->resume_drives == state->suspend_drives)
> + ide_free_acpi_state(parent_handle);
> + return acpi_ide_invoke_gtf(handle, drive);
> +}
> +
> +#else
> +static int __init ide_acpi_init(void)
> +{
> + return 0;
> +}
> +
> +static int acpi_ide_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static int acpi_ide_resume(struct device *dev)
> +{
> + return 0;
> +}
> +#endif
> +
> static int generic_ide_suspend(struct device *dev, pm_message_t state)
> {
> ide_drive_t *drive = dev->driver_data;
> @@ -1221,6 +1498,8 @@ static int generic_ide_suspend(struct de
> struct request_pm_state rqpm;
> ide_task_t args;
>
> + acpi_ide_suspend(dev);
> +
> memset(&rq, 0, sizeof(rq));
> memset(&rqpm, 0, sizeof(rqpm));
> memset(&args, 0, sizeof(args));
> @@ -1240,6 +1519,8 @@ static int generic_ide_resume(struct dev
> struct request_pm_state rqpm;
> ide_task_t args;
>
> + acpi_ide_resume(dev);
> +
> memset(&rq, 0, sizeof(rq));
> memset(&rqpm, 0, sizeof(rqpm));
> memset(&args, 0, sizeof(args));
> @@ -1923,6 +2204,7 @@ static int __init ide_init(void)
> system_bus_speed = ide_system_bus_speed();
>
> bus_register(&ide_bus_type);
> + ide_acpi_init();
>
> init_ide_data();
>
> _

--
~Randy

2005-12-07 08:17:34

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On 12/6/05, Matthew Garrett <[email protected]> wrote:
> On Tue, Dec 06, 2005 at 02:10:04PM +0800, Shaohua Li wrote:
>
> > TODO: invoking ATA commands.
> >
> > Though we didn't invoke ATA commands, this patch fixes the bug at
> > http://bugzilla.kernel.org/show_bug.cgi?id=5604. And Matthew said this
> > actually fixes a lot of systems in his test.
> > I'm not familiar with IDE, so comments/suggestions are welcome.
>
> Of the DSDTs I've looked at, most seem to provide taskfile commands
> concerned about doing things like setting up drive security. I haven't
> yet seen an IDE failure on resume when using this patch, so the _GTF
> stuff doesn't seem terribly important. The reason for it not currently
> being implemented is that the IDE queues haven't been restarted at the
> point where this code is called, so there's no obvious way of submitting
> them to the drive yet.

Isn't ide-io.c:ide_{start,complete}_power_step() enough?

BTW:

bugzilla bug #5604 - user is using 'ide-generic' host driver instead
of 'piix' so no wonder that suspend/resume doesn't work

bugzilla bug #2039 - is probably fixed by 2.6.12 (contains bugfix for
via82cxxx host driver not bugfix for not restoring transfer mode)

"From Matthew Garrett 2005-03-30 17:17 UTC [reply] -------

Linux currently has no real support for setting up IDE interfaces on resume.
Some machines are kind enough to set the IDE interface up themselves, but on
others we're doomed to failure. I'm looking into implementing this, but it won't
happen until some time after Hoary."

ide_{start,complete}_power_step() was there since the early 2.6.x days

Bartlomiej

2005-12-07 13:15:04

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Wed, Dec 07, 2005 at 09:17:31AM +0100, Bartlomiej Zolnierkiewicz wrote:

> Isn't ide-io.c:ide_{start,complete}_power_step() enough?

No.

--
Matthew Garrett | [email protected]

2005-12-07 14:19:13

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On 12/7/05, Matthew Garrett <[email protected]> wrote:
> On Wed, Dec 07, 2005 at 09:17:31AM +0100, Bartlomiej Zolnierkiewicz wrote:
>
> > Isn't ide-io.c:ide_{start,complete}_power_step() enough?
>
> No.

Why? :)

2005-12-07 14:26:47

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On 12/7/05, Bartlomiej Zolnierkiewicz <[email protected]> wrote:
> On 12/7/05, Matthew Garrett <[email protected]> wrote:
> > On Wed, Dec 07, 2005 at 09:17:31AM +0100, Bartlomiej Zolnierkiewicz wrote:
> >
> > > Isn't ide-io.c:ide_{start,complete}_power_step() enough?

Why feeding device with ACPI taskfile(s) can't be added to
the existing suspend/resume state machine (ide_*_power_step)?

> > No.
>
> Why? :)

2005-12-07 14:33:43

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Wed, Dec 07, 2005 at 03:19:07PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On 12/7/05, Matthew Garrett <[email protected]> wrote:
> > On Wed, Dec 07, 2005 at 09:17:31AM +0100, Bartlomiej Zolnierkiewicz wrote:
> >
> > > Isn't ide-io.c:ide_{start,complete}_power_step() enough?
> >
> > No.
>
> Why? :)

Heh :) The failure is (IIRC, I don't have the serial logs to hand)
before ide_start_power_step() is called. We get to start_request(), and
the system then fails with "bus not ready on wakeup" and "drive not
ready on wakeup". Calling the _GTM and _STM ACPI methods before then
lets the system come back up normally.

--
Matthew Garrett | [email protected]

2005-12-07 14:34:57

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Wed, Dec 07, 2005 at 03:26:45PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On 12/7/05, Bartlomiej Zolnierkiewicz <[email protected]> wrote:
> > On 12/7/05, Matthew Garrett <[email protected]> wrote:
> > > On Wed, Dec 07, 2005 at 09:17:31AM +0100, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > > Isn't ide-io.c:ide_{start,complete}_power_step() enough?
>
> Why feeding device with ACPI taskfile(s) can't be added to
> the existing suspend/resume state machine (ide_*_power_step)?

Oh, I see what you mean. Yes, I guess it ought to be ok at that stage.
--
Matthew Garrett | [email protected]

2005-12-07 14:45:22

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On 12/7/05, Matthew Garrett <[email protected]> wrote:
> On Wed, Dec 07, 2005 at 03:19:07PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > On 12/7/05, Matthew Garrett <[email protected]> wrote:
> > > On Wed, Dec 07, 2005 at 09:17:31AM +0100, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > > Isn't ide-io.c:ide_{start,complete}_power_step() enough?
> > >
> > > No.
> >
> > Why? :)
>
> Heh :) The failure is (IIRC, I don't have the serial logs to hand)
> before ide_start_power_step() is called. We get to start_request(), and
> the system then fails with "bus not ready on wakeup" and "drive not
> ready on wakeup". Calling the _GTM and _STM ACPI methods before then
> lets the system come back up normally.

OK, I understand it now - when using 'ide-generic' host driver for IDE
PCI device, resume fails (for obvious reason - IDE PCI device is not
re-configured) and this patch fixes it through using ACPI methods.

Thanks,
Bartlomiej

2005-12-07 14:58:19

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Wed, Dec 07, 2005 at 03:45:19PM +0100, Bartlomiej Zolnierkiewicz wrote:

> OK, I understand it now - when using 'ide-generic' host driver for IDE
> PCI device, resume fails (for obvious reason - IDE PCI device is not
> re-configured) and this patch fixes it through using ACPI methods.

Unfortunately not - you get the same failure with piix (am I right in
thinking that piix doesn't have any suspend/resume methods beyond the
generic PCI ones? I'm afraid I don't have enough knowledge of the IDE
layer to know if there's some other magic that calls an ide-specific
resume function in it...)

--
Matthew Garrett | [email protected]

2005-12-07 15:43:06

by Alan

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Mer, 2005-12-07 at 15:45 +0100, Bartlomiej Zolnierkiewicz wrote:
> OK, I understand it now - when using 'ide-generic' host driver for IDE
> PCI device, resume fails (for obvious reason - IDE PCI device is not
> re-configured) and this patch fixes it through using ACPI methods.

Even in the piix case some devices need it because the bios wants to
issue commands such as password control if the laptop is set up in
secure modes.

2005-12-07 15:44:56

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On 12/7/05, Matthew Garrett <[email protected]> wrote:
> On Wed, Dec 07, 2005 at 03:45:19PM +0100, Bartlomiej Zolnierkiewicz wrote:
>
> > OK, I understand it now - when using 'ide-generic' host driver for IDE
> > PCI device, resume fails (for obvious reason - IDE PCI device is not
> > re-configured) and this patch fixes it through using ACPI methods.
>
> Unfortunately not - you get the same failure with piix (am I right in
> thinking that piix doesn't have any suspend/resume methods beyond the
> generic PCI ones? I'm afraid I don't have enough knowledge of the IDE
> layer to know if there's some other magic that calls an ide-specific
> resume function in it...)

PCI device will get re-configured indirectly by ide_complete_power_step()
which is calling hwif->ide_dma_check() (piix_config_drive_xfer_rate).

Bartlomiej

2005-12-07 15:49:46

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On 12/7/05, Alan Cox <[email protected]> wrote:
> On Mer, 2005-12-07 at 15:45 +0100, Bartlomiej Zolnierkiewicz wrote:
> > OK, I understand it now - when using 'ide-generic' host driver for IDE
> > PCI device, resume fails (for obvious reason - IDE PCI device is not
> > re-configured) and this patch fixes it through using ACPI methods.

I was talking about bugzilla bug #5604.

> Even in the piix case some devices need it because the bios wants to
> issue commands such as password control if the laptop is set up in
> secure modes.

I completely agree. However at the moment this patch doesn't seem
to issue any ATA commands (code is commented out in _GTF) so
this is not a case for bugzilla bug #5604.

Bartlomiej

2005-12-07 15:53:53

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Wed, Dec 07, 2005 at 04:44:39PM +0100, Bartlomiej Zolnierkiewicz wrote:

> PCI device will get re-configured indirectly by ide_complete_power_step()
> which is calling hwif->ide_dma_check() (piix_config_drive_xfer_rate).

Ah, right - which is /after/ the failure I see, so it's not surprising
that it doesn't work :)

--
Matthew Garrett | [email protected]

2005-12-07 17:16:16

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Tue, 2005-12-06 at 16:11 -0800, Randy.Dunlap wrote:
> On Tue, 6 Dec 2005, Shaohua Li wrote:
>
> > Hi,
> > Adding ACPI IDE hook in IDE suspend/resume. The ACPI spec
> > explicitly says we must call some ACPI methods to restore IDE drives.
> > The sequences defined by ACPI spec are:
> > suspend:
> > 1. Get the DMA and PIO info from IDE channel's _GTM method.
> >
> > resume:
> > 1. Calling IDE channel's _STM to set the transfer timing setting.
> > 2. For each drive on the IDE channel, running drive's _GTF to get the
> > ATA commands required to reinitialize each drive.
> > 3. Sending the ATA commands gotton from step 2 to drives.
> >
> > TODO: invoking ATA commands.
> >
> > Though we didn't invoke ATA commands, this patch fixes the bug at
> > http://bugzilla.kernel.org/show_bug.cgi?id=5604. And Matthew said this
> > actually fixes a lot of systems in his test.
> > I'm not familiar with IDE, so comments/suggestions are welcome.
> >
> > ---
> >
> > linux-2.6.15-rc5-root/drivers/ide/ide.c | 282 ++++++++++++++++++++++++++++++++
> > 1 files changed, 282 insertions(+)
> >
> > diff -puN drivers/ide/ide.c~acpi-ide drivers/ide/ide.c
> > --- linux-2.6.15-rc5/drivers/ide/ide.c~acpi-ide 2005-12-07 03:01:36.000000000 +0800
> > +++ linux-2.6.15-rc5-root/drivers/ide/ide.c 2005-12-07 03:01:36.000000000 +0800
> > @@ -155,6 +155,10 @@
> > #include <linux/device.h>
> > #include <linux/bitops.h>
> >
> > +#ifdef CONFIG_ACPI
> > +#include <linux/acpi.h>
> > +#endif
>
> Shouldn't need or use ifdef/endif for #includes.
Ok.

> > +
> > +/* The _GTM return package length is 5 dwords */
> > +#define GTM_LEN (sizeof(u32) * 5)
> > +struct acpi_ide_state {
> > + acpi_handle handle; /* channel device's handle */
> > + u32 gtm[GTM_LEN/sizeof(u32)]; /* info from _GTM */
> > + struct hd_driveid id_buff[2]; /* one chanel has two drives */
>
> s/2/MAX_DRIVES/
> s/chanel/channel/
:), thanks!

> > + if (!handle) {
> > + printk(KERN_DEBUG "IDE device's ACPI handler is NULL\n");
>
> s/handler/handle/ ??
A typo.

> > + status = acpi_evaluate_object(parent_handle, "_GTM", NULL, &buffer);
> > + if (ACPI_FAILURE(status)) {
> > + printk(KERN_ERR "Error evaluating _GTM\n");
>
> I don't read the ACPI spec. as saying that _GTM is required, (?)
> so I would make this a KERN_DEBUG instead of KERN_ERR.
Yes, _GTM is required.

> > + status = acpi_evaluate_object(state->handle, "_STM", &input, NULL);
> > + if (ACPI_FAILURE(status)) {
> > + printk(KERN_ERR "Evaluating _STM error\n");
>
> Same as for _GTM, KERN_DEBUG instead of KERN_ERR.
_STM also is required.

> > + acpi_status status;
> > +
> > + status = acpi_evaluate_object(handle, "_GTF", NULL, &output);
> > + if (ACPI_FAILURE(status)) {
> > + printk(KERN_ERR "evaluate _GTF error\n");
>
> KERN_DEBUG if not present since it's not required AFAIK.
Actually it's required, but I have no idea how to invoke the ata
commands gotten from _GTF.

Thanks for your time. I'll update this patch as you suggested after the
IDE gurus think it's ok.

Thanks,
Shaohua

2005-12-07 17:22:23

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Wed, 2005-12-07 at 16:49 +0100, Bartlomiej Zolnierkiewicz wrote:
> On 12/7/05, Alan Cox <[email protected]> wrote:
> > On Mer, 2005-12-07 at 15:45 +0100, Bartlomiej Zolnierkiewicz wrote:
> > > OK, I understand it now - when using 'ide-generic' host driver for IDE
> > > PCI device, resume fails (for obvious reason - IDE PCI device is not
> > > re-configured) and this patch fixes it through using ACPI methods.
>
> I was talking about bugzilla bug #5604.
Sorry for my ignorance in IDE side. From the ACPI spec, there isn't a
generic way to save/restore IDE's configuration. That's why ACPI
provides such methods. I suppose all IDE drivers need call the methods,
wrong?

> > Even in the piix case some devices need it because the bios wants to
> > issue commands such as password control if the laptop is set up in
> > secure modes.
>
> I completely agree. However at the moment this patch doesn't seem
> to issue any ATA commands (code is commented out in _GTF) so
> this is not a case for bugzilla bug #5604.
I actually tried to invoke ATA commands using IDE APIs, but can't find
any available one. I'd be very happy if you can give me any hint how to
do it or even you can fix it.

Thanks,
Shaohua

2005-12-07 19:15:11

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On 12/7/05, Shaohua Li <[email protected]> wrote:
> On Wed, 2005-12-07 at 16:49 +0100, Bartlomiej Zolnierkiewicz wrote:
> > On 12/7/05, Alan Cox <[email protected]> wrote:
> > > On Mer, 2005-12-07 at 15:45 +0100, Bartlomiej Zolnierkiewicz wrote:
> > > > OK, I understand it now - when using 'ide-generic' host driver for IDE
> > > > PCI device, resume fails (for obvious reason - IDE PCI device is not
> > > > re-configured) and this patch fixes it through using ACPI methods.
> >
> > I was talking about bugzilla bug #5604.
> Sorry for my ignorance in IDE side. From the ACPI spec, there isn't a
> generic way to save/restore IDE's configuration. That's why ACPI
> provides such methods. I suppose all IDE drivers need call the methods,
> wrong?

>From the hardware POV:
* there is generic way to save/restores IDE device's configuration
* there is no generic way to save/restore IDE controller's configuration

>From the software POV what we only do currently is setting controller
and drive for a correct transfer mode by using host driver specific callback
(in case of using 'ide-generic' there is no such callback).

> > > Even in the piix case some devices need it because the bios wants to
> > > issue commands such as password control if the laptop is set up in
> > > secure modes.
> >
> > I completely agree. However at the moment this patch doesn't seem
> > to issue any ATA commands (code is commented out in _GTF) so
> > this is not a case for bugzilla bug #5604.
> I actually tried to invoke ATA commands using IDE APIs, but can't find
> any available one. I'd be very happy if you can give me any hint how to
> do it or even you can fix it.

Probably do_rw_taskfile() is the method you want to use, you also need
to place invoking of ACPI provided ATA commands in the right place in
the IDE PM state machine [ ide_{start,complete}_power_step() ].

PS1 Please don't use taskfile_lib_get_identify(), drive->id
should contain valid ID - if it doesn't it is a BUG.

PS2 Have you seen libata ACPI patches by Randy?
Maybe some of the code dealing with ACPI can be put to
<linux/ata.h> and be shared between IDE and libata drivers?

Thanks,
Bartlomiej

2005-12-07 21:58:38

[permalink] [raw]

Subject: RE: [RFC]add ACPI hooks for IDE suspend/resume

Hi,
>
>On 12/7/05, Shaohua Li <[email protected]> wrote:
>> On Wed, 2005-12-07 at 16:49 +0100, Bartlomiej Zolnierkiewicz wrote:
>> > On 12/7/05, Alan Cox <[email protected]> wrote:
>> > > On Mer, 2005-12-07 at 15:45 +0100, Bartlomiej Zolnierkiewicz
wrote:
>> > > > OK, I understand it now - when using 'ide-generic' host driver
for
>IDE
>> > > > PCI device, resume fails (for obvious reason - IDE PCI device
is
>not
>> > > > re-configured) and this patch fixes it through using ACPI
methods.
>> >
>> > I was talking about bugzilla bug #5604.
>> Sorry for my ignorance in IDE side. From the ACPI spec, there isn't a
>> generic way to save/restore IDE's configuration. That's why ACPI
>> provides such methods. I suppose all IDE drivers need call the
methods,
>> wrong?
>
>From the hardware POV:
>* there is generic way to save/restores IDE device's configuration
>* there is no generic way to save/restore IDE controller's
configuration
>
>From the software POV what we only do currently is setting controller
>and drive for a correct transfer mode by using host driver specific
>callback
>(in case of using 'ide-generic' there is no such callback).
>
>> > > Even in the piix case some devices need it because the bios wants
to
>> > > issue commands such as password control if the laptop is set up
in
>> > > secure modes.
>> >
>> > I completely agree. However at the moment this patch doesn't seem
>> > to issue any ATA commands (code is commented out in _GTF) so
>> > this is not a case for bugzilla bug #5604.
>> I actually tried to invoke ATA commands using IDE APIs, but can't
find
>> any available one. I'd be very happy if you can give me any hint how
to
>> do it or even you can fix it.
>
>Probably do_rw_taskfile() is the method you want to use, you also need
>to place invoking of ACPI provided ATA commands in the right place in
>the IDE PM state machine [ ide_{start,complete}_power_step() ].
>
>PS1 Please don't use taskfile_lib_get_identify(), drive->id
>should contain valid ID - if it doesn't it is a BUG.
>
>PS2 Have you seen libata ACPI patches by Randy?
>Maybe some of the code dealing with ACPI can be put to
><linux/ata.h> and be shared between IDE and libata drivers?
Thanks a lot! I'll try your suggestions after my travel. Hopefully I can
understand the IDE staffs you mentioned then :).

Thanks,
Shaohua

2005-12-07 22:36:55

by Alan

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Mer, 2005-12-07 at 20:15 +0100, Bartlomiej Zolnierkiewicz wrote:
> PS1 Please don't use taskfile_lib_get_identify(), drive->id
> should contain valid ID - if it doesn't it is a BUG.

If someone swapped the drive while suspended that isnt true. OTOH I'm
not sure what the hell you'd do if that was the case and you are using
drivers/ide right now.

> PS2 Have you seen libata ACPI patches by Randy?
> Maybe some of the code dealing with ACPI can be put to
> <linux/ata.h> and be shared between IDE and libata drivers?

Definitely a good idea. Also exposing the methods will be useful for
producing a proper libata 'ata_acpi' driver that does the best it can
using ACPI methods for tuning unknown hardware.

2005-12-07 22:42:16

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On 12/7/05, Alan Cox <[email protected]> wrote:
> On Mer, 2005-12-07 at 20:15 +0100, Bartlomiej Zolnierkiewicz wrote:
> > PS1 Please don't use taskfile_lib_get_identify(), drive->id
> > should contain valid ID - if it doesn't it is a BUG.
>
> If someone swapped the drive while suspended that isnt true. OTOH I'm
> not sure what the hell you'd do if that was the case and you are using
> drivers/ide right now.

This is unsupported anyway so doesn't matter for IDE ACPI support...

2005-12-07 22:45:46

by Pavel Machek

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

Hi!

> > PS1 Please don't use taskfile_lib_get_identify(), drive->id
> > should contain valid ID - if it doesn't it is a BUG.
>
> If someone swapped the drive while suspended that isnt true. OTOH I'm
> not sure what the hell you'd do if that was the case and you are using
> drivers/ide right now.

If someone swapped the drive while runtime, it would not be true, too,
I guess. But that would be stupid thing to do. Swapping drive during
suspend-to-RAM would be similary stupid. During suspend-to-disk, it
might theoretically work, but we have big warnings saying "don't do
that".

Pavel
--
Thanks, Sharp!

2005-12-07 22:49:22

[permalink] [raw]

Subject: Re: [RFC]add ACPI hooks for IDE suspend/resume

On Wed, Dec 07, 2005 at 11:43:23PM +0100, Pavel Machek wrote:

> If someone swapped the drive while runtime, it would not be true, too,
> I guess. But that would be stupid thing to do. Swapping drive during
> suspend-to-RAM would be similary stupid. During suspend-to-disk, it
> might theoretically work, but we have big warnings saying "don't do
> that".

ACPI systems tend to fire ACPI notifications when a drive is ejected
(and sometimes when you're preparing to eject them, depending on the
system), which ought to make hotswap possible. I'm looking at
integrating support for that into libata right now.

--
Matthew Garrett | [email protected]

2005-12-07 23:19:38