From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=44446 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PnFwp-0005GE-6z for qemu-devel@nongnu.org; Wed, 09 Feb 2011 14:43:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PnFwm-00019k-OQ for qemu-devel@nongnu.org; Wed, 09 Feb 2011 14:43:30 -0500 Received: from e31.co.us.ibm.com ([32.97.110.149]:50906) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PnFwm-00017r-4v for qemu-devel@nongnu.org; Wed, 09 Feb 2011 14:43:28 -0500 Received: from d03relay05.boulder.ibm.com (d03relay05.boulder.ibm.com [9.17.195.107]) by e31.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p19JSWNP002141 for ; Wed, 9 Feb 2011 12:28:32 -0700 Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p19JhAqe091320 for ; Wed, 9 Feb 2011 12:43:10 -0700 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p19Jekdj030268 for ; Wed, 9 Feb 2011 12:40:46 -0700 Date: Wed, 9 Feb 2011 13:43:06 -0600 From: Ryan Harper Subject: Re: [Qemu-devel] [PATCH] Fix ATA SMART and CHECK POWER MODE Message-ID: <20110209194306.GS4914@us.ibm.com> References: <1297114303.7349.17.camel@nibbler.dlib.indiana.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1297114303.7349.17.camel@nibbler.dlib.indiana.edu> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Brian Wheeler Cc: qemu-devel@nongnu.org * Brian Wheeler [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 > --- > 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 > > -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 ryanh@us.ibm.com