Re: hard drive errors on acpi s3 resume

Pavel Machek
2005-11-08T09:11:44Z

Hi!

Please don't top post.

> Just out of curiosity, what issues do you find with the coding
> style?

I hope that means you want to clean it up and apply it ;-).

+#ifdef CONFIG_ACPI
+#include <linux/acpi.h>
+#define DBG(x...) printk(x)

Include in the middle of file? Renaming of printk to confuse reader?

+ int i, tmp;
+ acpi_integer addr;
...
+ addr = i;

...oops, something is very wrong in acpi land.

+#define GTM_LEN (sizeof(u32) * 5)
...
+ u32 gtm[GTM_LEN/sizeof(u32)]; /* info from _GTM */

Eh? Better names would be nice, too.

+ for (i = 0; i < MAX_DEVICES; i ++)

Please no space between i and ++.

+static struct acpi_ide_stat *ide_get_acpi_state(acpi_handle handle)

The function is strange, btw. Linear search, and it basically returns
pointer to zeros. I'd expect zeroing to be in caller or something like that.

+int acpi_ide_suspend(struct device *dev)
+{
+ acpi_handle handle, parent_handle;
+ struct acpi_ide_stat *stat;
+ acpi_status status;

At least alignment should be consistent locally.

+static int acpi_ide_gtf(acpi_handle handle, ide_drive_t *drive)
+{
+ struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+ ide_task_t args;
+ int index = 0;
+ unsigned char *data;
+ union acpi_object *package = NULL;
+ acpi_status status;

...even more horible example of above.

+ /* submit command request */
+// printk("return value %d\n", ide_raw_taskfile(drive, &args, NULL));
+ index += 7;

Someone should learn to use delete key and version control...

+ if (stat->channel_handled == 0) {
+ stat->handle = NULL;
+ goto gtf;
+ }
+DBG("Start STM\n");
+ if (acpi_ide_stm(stat))
+ return -ENODEV;
+ stat->channel_handled = 0;
+gtf:
+ return acpi_ide_gtf(handle, drive);
+}

What about "else" instead of ugly goto. And that DBG should really go
away.

-
+#ifdef CONFIG_ACPI
+ acpi_ide_suspend(dev);
+#endif

-
+#ifdef CONFIG_ACPI
+ acpi_ide_resume(dev);
+#endif
memset(&rq, 0, sizeof(rq));
memset(&rqpm, 0, sizeof(rqpm));
memset(&args, 0, sizeof(args));

We normally allways provide acpi_ide_resume, but make them nop in
!ACPI case. That means #ifdefs are avoided in callers.

...should be enough...
Pavel
--
Thanks, Sharp!