[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Fix ATA SMART and CHECK POWER MODE
|
From: |
Ryan Harper |
|
Subject: |
Re: [Qemu-devel] [PATCH] Fix ATA SMART and CHECK POWER MODE |
|
Date: |
Wed, 9 Feb 2011 13:43:06 -0600 |
|
User-agent: |
Mutt/1.5.6+20040907i |
* Brian Wheeler <address@hidden> [2011-02-07 16:48]:
> This patch fixes two things:
>
> 1) CHECK POWER MODE
>
> The error return value wasn't always zero, so it would show up as
> offline. Error is now explicitly set to zero.
>
> 2) SMART
>
> The smart values that were returned were invalid and tools like skdump
> would not recognize that the smart data was actually valid and would
> dump weird output. The data has been fixed up and raw value support was
> added. Tools like skdump and palimpsest work as expected.
>
> Signed-of-by: Brian Wheeler <address@hidden>
> ---
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index dd63664..4d4ccfa 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -34,15 +34,37 @@
>
> #include <hw/ide/internal.h>
>
> -static const int smart_attributes[][5] = {
> - /* id, flags, val, wrst, thrsh */
> - { 0x01, 0x03, 0x64, 0x64, 0x06}, /* raw read */
> - { 0x03, 0x03, 0x64, 0x64, 0x46}, /* spin up */
> - { 0x04, 0x02, 0x64, 0x64, 0x14}, /* start stop count */
> - { 0x05, 0x03, 0x64, 0x64, 0x36}, /* remapped sectors */
> - { 0x00, 0x00, 0x00, 0x00, 0x00}
> +/* These values were taking from a running system. */
> +static const int smart_attributes[][12] = {
> + /* id, flags, hflags, val, wrst, raw (up to 6 bytes) */
> + /* raw read error rate*/
> + { 0x01, 0x03, 0x00, 0x74, 0x63, 0x31, 0x6d, 0x3f, 0x0d},
> + /* spin up */
> + { 0x03, 0x03, 0x00, 0x61, 0x61},
> + /* start stop count */
> + { 0x04, 0x02, 0x00, 0x64, 0x64, 0x64},
> + /* remapped sectors */
> + { 0x05, 0x03, 0x00, 0x64, 0x64},
> + /* power on hours */
> + { 0x09, 0x03, 0x00, 0x61, 0x61, 0x68, 0x0a},
> + /* power cycle count */
> + { 0x0c, 0x03, 0x00, 0x64, 0x64, 0x32},
> + /* airflow-temperature-celsius */
> + { 190, 0x03, 0x00, 0x64, 0x64, 0x1f, 0x00, 0x16, 0x22},
> + { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
> };
>
> +static const int smart_thresholds[][2] = {
> + { 0x01, 0x06},
> + { 0x03, 0x00},
> + { 0x04, 0x14},
> + { 0x05, 0x24},
> + { 0x09, 0x00},
> + { 190, 0x32},
> + { 0x00, 0x00}
> +};
Are these values from a specification or what? WOuld be good to
document where we get the values that are being used. If you are
selecting some defaults, what those values are and why.
> +
> +
> /* XXX: DVDs that could fit on a CD will be reported as a CD */
> static inline int media_present(IDEState *s)
> {
> @@ -1843,6 +1865,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
> break;
> case WIN_CHECKPOWERMODE1:
G> case WIN_CHECKPOWERMODE2:
> + s->error = 0;
> s->nsector = 0xff; /* device active or idle */
> s->status = READY_STAT | SEEK_STAT;
> ide_set_irq(s->bus);
> @@ -2068,24 +2091,24 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
> case SMART_ATTR_AUTOSAVE:
> switch (s->sector) {
> case 0x00:
> - s->smart_autosave = 0;
> - break;
> + s->smart_autosave = 0;
> + break;
> case 0xf1:
> - s->smart_autosave = 1;
> - break;
> + s->smart_autosave = 1;
> + break;
> default:
> - goto abort_cmd;
> + goto abort_cmd;
Did you edit fix these up automatically? tabs/spaces?
> }
> s->status = READY_STAT | SEEK_STAT;
> ide_set_irq(s->bus);
> break;
> case SMART_STATUS:
> if (!s->smart_errors) {
> - s->hcyl = 0xc2;
> - s->lcyl = 0x4f;
> + s->hcyl = 0xc2;
> + s->lcyl = 0x4f;
> } else {
> - s->hcyl = 0x2c;
> - s->lcyl = 0xf4;
> + s->hcyl = 0x2c;
> + s->lcyl = 0xf4;
same
> }
> s->status = READY_STAT | SEEK_STAT;
> ide_set_irq(s->bus);
> @@ -2094,13 +2117,13 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
> memset(s->io_buffer, 0, 0x200);
> s->io_buffer[0] = 0x01; /* smart struct version */
> for (n=0; n<30; n++) {
> - if (smart_attributes[n][0] == 0)
> + if (smart_attributes[n][0] == 0)
> break;
If you are going to touch the if, then CODING_STYLE wants braced ifs.
if () {
}
> - s->io_buffer[2+0+(n*12)] = smart_attributes[n][0];
> - s->io_buffer[2+1+(n*12)] = smart_attributes[n][4];
> + s->io_buffer[2+0+(n*12)] = smart_thresholds[n][0];
> + s->io_buffer[2+1+(n*12)] = smart_thresholds[n][1];
> }
> for (n=0; n<511; n++) /* checksum */
> - s->io_buffer[511] += s->io_buffer[n];
> + s->io_buffer[511] += s->io_buffer[n];
> s->io_buffer[511] = 0x100 - s->io_buffer[511];
> s->status = READY_STAT | SEEK_STAT;
> ide_transfer_start(s, s->io_buffer, 0x200, ide_transfer_stop);
> @@ -2110,21 +2133,22 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
> memset(s->io_buffer, 0, 0x200);
> s->io_buffer[0] = 0x01; /* smart struct version */
> for (n=0; n<30; n++) {
> - if (smart_attributes[n][0] == 0)
> + if (smart_attributes[n][0] == 0)
CODING_STYLE if()
> break;
> - s->io_buffer[2+0+(n*12)] = smart_attributes[n][0];
> - s->io_buffer[2+1+(n*12)] = smart_attributes[n][1];
> - s->io_buffer[2+3+(n*12)] = smart_attributes[n][2];
> - s->io_buffer[2+4+(n*12)] = smart_attributes[n][3];
> + int i;
> + for(i = 0; i < 12; i++) {
> + s->io_buffer[2+i+(n*12)] = smart_attributes[n][i];
> + }
> }
> +
whitespace
> s->io_buffer[362] = 0x02 | (s->smart_autosave?0x80:0x00);
> if (s->smart_selftest_count == 0) {
> - s->io_buffer[363] = 0;
> + s->io_buffer[363] = 0;
> } else {
> - s->io_buffer[363] =
> + s->io_buffer[363] =
> s->smart_selftest_data[3 +
> - (s->smart_selftest_count - 1) *
> - 24];
> + (s->smart_selftest_count - 1) *
> + 24];
> }
> s->io_buffer[364] = 0x20;
> s->io_buffer[365] = 0x01;
> @@ -2136,9 +2160,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
> s->io_buffer[372] = 0x02; /* minutes for poll short test */
> s->io_buffer[373] = 0x36; /* minutes for poll ext test */
> s->io_buffer[374] = 0x01; /* minutes for poll conveyance */
> -
> - for (n=0; n<511; n++)
> - s->io_buffer[511] += s->io_buffer[n];
> + for (n=0; n<511; n++) { /* checksum */
> + s->io_buffer[511] += s->io_buffer[n];
> + }
> s->io_buffer[511] = 0x100 - s->io_buffer[511];
> s->status = READY_STAT | SEEK_STAT;
> ide_transfer_start(s, s->io_buffer, 0x200, ide_transfer_stop);
> @@ -2147,32 +2171,31 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
> case SMART_READ_LOG:
> switch (s->sector) {
> case 0x01: /* summary smart error log */
> - memset(s->io_buffer, 0, 0x200);
> - s->io_buffer[0] = 0x01;
> - s->io_buffer[1] = 0x00; /* no error entries */
> - s->io_buffer[452] = s->smart_errors & 0xff;
> - s->io_buffer[453] = (s->smart_errors & 0xff00) >> 8;
> -
> - for (n=0; n<511; n++)
> + memset(s->io_buffer, 0, 0x200);
> + s->io_buffer[0] = 0x01;
> + s->io_buffer[1] = 0x00; /* no error entries */
> + s->io_buffer[452] = s->smart_errors & 0xff;
> + s->io_buffer[453] = (s->smart_errors & 0xff00) >> 8;
> + for (n=0; n<511; n++)
brace
> s->io_buffer[511] += s->io_buffer[n];
> - s->io_buffer[511] = 0x100 - s->io_buffer[511];
> - break;
> + s->io_buffer[511] = 0x100 - s->io_buffer[511];
> + break;
> case 0x06: /* smart self test log */
> - memset(s->io_buffer, 0, 0x200);
> - s->io_buffer[0] = 0x01;
> - if (s->smart_selftest_count == 0) {
> + memset(s->io_buffer, 0, 0x200);
> + s->io_buffer[0] = 0x01;
> + if (s->smart_selftest_count == 0) {
> s->io_buffer[508] = 0;
> - } else {
> + } else {
> s->io_buffer[508] = s->smart_selftest_count;
> for (n=2; n<506; n++)
> - s->io_buffer[n] = s->smart_selftest_data[n];
> - }
> - for (n=0; n<511; n++)
> + s->io_buffer[n] = s->smart_selftest_data[n];
> + }
> + for (n=0; n<511; n++)
brace
> s->io_buffer[511] += s->io_buffer[n];
> - s->io_buffer[511] = 0x100 - s->io_buffer[511];
> - break;
> + s->io_buffer[511] = 0x100 - s->io_buffer[511];
> + break;
> default:
> - goto abort_cmd;
> + goto abort_cmd;
> }
> s->status = READY_STAT | SEEK_STAT;
> ide_transfer_start(s, s->io_buffer, 0x200, ide_transfer_stop);
>
This patchset looks a lot larger than it should since there is a lot of
indentation movement, it would be good to see a version that just
implemented the changes needed, which AFAICT are mainly the additional
attributes and limits.
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
address@hidden