RE: hard drive errors on acpi s3 resume

Moore, Robert
2005-11-09T00:06:09Z

Yep on all.
Bob

> + acpi_handle handle, parent_handle;

I don't like multiple declarations in one statement.

Bob

> -----Original Message-----
> From: Pavel Machek [mailto:pavel-+ZI9xUNit7I@public.gmane.org]
> Sent: Tuesday, November 08, 2005 1:12 AM
> To: Moore, Robert
> Cc: ACPI mailing list
> Subject: Re: [ACPI] hard drive errors on acpi s3 resume
>
> 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!