Matthew Wilcox wrote:
On Thu, Apr 23, 2009 at 07:09:37AM -0400, Jeff Garzik wrote:
Jens Axboe wrote:
+ /* Block Device Characteristics VPD */
+ buffer = scsi_get_vpd_page(sdkp->device, 0xb1);
+
+ if (buffer == NULL)
+ return;
+
+ rot = get_unaligned_be16(&buffer[4]);
Make sure this works for libata as well, and then kill the rotational
check in there instead.
Yep. libata-scsi.c would need to simulate that VPD page.
I already did that. The only problem is that you made me include the stupid:
if (ata_id_major_version(args->id) > 7) {
so of course it doesn't work on any existing hardware. How about
applying this patch:
----
libata: fill in b1 page for all drives, not just ATA-8
Some of the drives on the market fill in the rotational speed and form
factor correctly, even though they claim support for an earlier version
of ATA. The current ata_id_is_ssd() code doesn't check the version
number and doesn't appear to have caused any trouble. Besides, SCSI devices
are also capable of returning garbage in these fields.
Signed-off-by: Matthew Wilcox <willy@xxxxxxxxxxxxxxx>
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 2733b0c..59358ca 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2144,11 +2144,9 @@ static unsigned int ata_scsiop_inq_b1(struct ata_scsi_args *args, u8 *rbuf)
{
rbuf[1] = 0xb1;
rbuf[3] = 0x3c;
- if (ata_id_major_version(args->id) > 7) {
- rbuf[4] = args->id[217] >> 8;
- rbuf[5] = args->id[217];
- rbuf[7] = args->id[168] & 0xf;
- }
+ rbuf[4] = args->id[217] >> 8;
+ rbuf[5] = args->id[217];
+ rbuf[7] = args->id[168] & 0xf;
Thus returning undefined garbage for the vast majority of ATA devices?
Might as well admit that a call to get_random_bytes() is a valid
implementation, at that point.
Linux users deserve more than that :)
If you want to find a better test than "version > 7", that is fine.
Surely a few minutes of thinking and a few minutes of research will
yield a reasonable hueristic, that gives a reasonable estimation of
when/if these fields are valid?
linux/ata.h is filled with examples of proper range checking -- ensuring
that a range of IDENTIFY DEVICE words are valid. There are also typical
tests such as assuming values other than 0x0000 and 0xffff are valid.
Also (to mkp or whoever does the work) -- note Linus's comment, and my
provisional patch[1], about libata potentially wanting to detect NONROT
by looking for "*SSD" from IDENTIFY DEVICE'S model string.
Found it ... and Jens' suggestion that this be done in userspace instead.
It is trivial to do in the kernel, where we already match against model
info for a long list of quirks.
Therefore, I think the Just Works(tm) value to SSD owners is higher.
That way old userlands work with SSDs too.
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
