libata support for rotation speed

Version 2, Jeff pointed out I was missing a check for a sufficiently recent version of the specification.

Add support for VPD page b1 to libata

SCSI VPD page b1 reports the nominal rotation speed of the device. ATA devices return this information in word 217 of the identify data. b1 can also report the physical size of the device (5.25", 3.5", etc) but that doesn't currently seem to be defined in the ATA specification.

Signed-off-by Matthew Wilcox <[email protected]>

drivers/ata/libata-scsi.c +3 -1

index 2e6e162..07512d4 100644
@@ -1778,7 +1778,9 @@ static unsigned int ata_scsiop_inq_00(struct ata_scsi_args *args, u8 *rbuf)
 	const u8 pages[] = {
 		0x00,	/* page 0x00, this page */
 		0x80,	/* page 0x80, unit serial no page */
-		0x83	/* page 0x83, device ident page */
+		0x83,	/* page 0x83, device ident page */
+		0x89,	/* page 0x89, ata info page */
+		0xb1,	/* page 0xb1, block device characteristics page */
 	};
 
 	rbuf[3] = sizeof(pages);	/* number of supported VPD pages */

Quoted 20 lines · Matthew Wilcox

Version 2, Jeff pointed out I was missing a check for a sufficiently recent version of the specification.

Add support for VPD page b1 to libata

SCSI VPD page b1 reports the nominal rotation speed of the device. ATA devices return this information in word 217 of the identify data. b1 can also report the physical size of the device (5.25", 3.5", etc) but that doesn't currently seem to be defined in the ATA specification.

Signed-off-by Matthew Wilcox <[email protected]>

drivers/ata/libata-scsi.c +0 -0

index 2e6e162..07512d4 100644
@@ -1899,6 +1901,18 @@ static unsigned int ata_scsiop_inq_89(struct ata_scsi_args *args, u8 *rbuf)
 	return 0;
 }
 

drivers/ata/libata-scsi.c static unsigned int ata_scsiop_inq_89(struct ata_scsi_args *args, u8 *rbuf) · line 1906

Quoted 3 lines

drivers/ata/libata-scsi.c +3 -0

+static unsigned int ata_scsiop_inq_b1(struct ata_scsi_args *args, u8 *rbuf)
+{
+	rbuf[1] = 0xb1;

This looks like it can be called for ATA_DEV_ATAPI as well, in which case it's an illegal page to ask for.

Also, shouldn't this be added to
VPD page 0 (which shows the list of available VPD pages ... although it looks like the supported 0x89 is missing too)?

Quoted 10 lines

drivers/ata/libata-scsi.c +3 -1

@@ -1778,7 +1778,9 @@ static unsigned int ata_scsiop_inq_00(struct ata_scsi_args *args, u8 *rbuf)
 	const u8 pages[] = {
 		0x00,	/* page 0x00, this page */
 		0x80,	/* page 0x80, unit serial no page */
-		0x83	/* page 0x83, device ident page */
+		0x83,	/* page 0x83, device ident page */
+		0x89,	/* page 0x89, ata info page */
+		0xb1,	/* page 0xb1, block device characteristics page */
 	};
 

drivers/ata/libata-scsi.c static unsigned int ata_scsiop_inq_00(struct ata_scsi_args *args, u8 *rbuf) · line 1786

Quoted 1 line

drivers/ata/libata-scsi.c +0 -0

 	rbuf[3] = sizeof(pages);	/* number of supported VPD pages */

This would currently set device nominal form factor to unreported. It's actually available in identify word 168 (bits 0:3) in an identical format to the one the VPD inquiry is expecting, so why not use it?

Quoted 2 lines · James Bottomley

This looks like it can be called for ATA_DEV_ATAPI as well, in which case it's an illegal page to ask for.

I was under the impression that this entire file is only used for non-atapi devices.

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines · James Bottomley

This looks like it can be called for ATA_DEV_ATAPI as well, in which case it's an illegal page to ask for.

I was under the impression that this entire file is only used for non-atapi devices.

Correct. That code path is not travelled by ATAPI.

Quoted 7 lines · Jeff Garzik

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · James Bottomley

This looks like it can be called for ATA_DEV_ATAPI as well, in which case it's an illegal page to ask for.

I was under the impression that this entire file is only used for non-atapi devices.

Correct. That code path is not travelled by ATAPI.

Thanks, Jeff. Here's version three, including the form factor from word 168:

Add support for VPD page b1 to libata

SCSI VPD page b1 reports the nominal rotation speed and physical size of the device. Devices that conform to ATA-8 can return this information in words 217 and 168 of the identify data.

Signed-off-by Matthew Wilcox <[email protected]>

drivers/ata/libata-scsi.c +3 -1

index 2e6e162..c08c3b9 100644
@@ -1778,7 +1778,9 @@ static unsigned int ata_scsiop_inq_00(struct ata_scsi_args *args, u8 *rbuf)
 	const u8 pages[] = {
 		0x00,	/* page 0x00, this page */
 		0x80,	/* page 0x80, unit serial no page */
-		0x83	/* page 0x83, device ident page */
+		0x83,	/* page 0x83, device ident page */
+		0x89,	/* page 0x89, ata info page */
+		0xb1,	/* page 0xb1, block device characteristics page */
 	};
 
 	rbuf[3] = sizeof(pages);	/* number of supported VPD pages */

drivers/ata/libata-scsi.c +6 -0

+		rbuf[7] = args->id[168] & 0xf;
+	}
+
+	return 0;
+}
+
 /**
  *	ata_scsiop_noop - Command handler that simply returns success.
  *	@args: device IDENTIFY data / SCSI command of interest.

drivers/ata/libata-scsi.c static unsigned int ata_scsiop_inq_89(struct ata_scsi_args *args, u8 *rbuf) · line 1910

Quoted 2 lines

drivers/ata/libata-scsi.c +2 -0

+		rbuf[4] = args->id[217] >> 8;
+		rbuf[5] = args->id[217];

args->id of struct ata_scsi_args are defined as u16. Are they actually SWABed at this point? Are they LE? BE?

if args->id are actually __be16 then above should be
+ put_unaligned(args->id[217], &rbuf[4]);
else
+ put_unaligned_be16(args->id[217], &rbuf[4]);

Quoted 5 lines · Boaz Harrosh

drivers/ata/libata-scsi.c static unsigned int ata_scsiop_inq_89(struct ata_scsi_args *args, u8 *rbuf) · line 1910

Quoted 2 lines

drivers/ata/libata-scsi.c +2 -0

+		rbuf[4] = args->id[217] >> 8;
+		rbuf[5] = args->id[217];

args->id of struct ata_scsi_args are defined as u16. Are they actually SWABed at this point? Are they LE? BE?

They're cpu-endian at this point, I believe. See ata_dev_read_id() in libata-core.c where it calls swap_buf_le16().

Quoted 4 lines

if args->id are actually __be16 then above should be
+ put_unaligned(args->id[217], &rbuf[4]);
else
+ put_unaligned_be16(args->id[217], &rbuf[4]);

But the buf isn't unaligned, nor the offset within it. And I get confused by all these macros anyway. If we had something like store_scsi_u16(unsigned char *, u16), I'd use that, but put_unaligned_be16 just doesn't make sense.

I actually wrote my own accessors for scsi_ram. If they were to be more generic, they ought to be renamed, but this is what I found useful:

/*
* SCSI requires quantities to be written MSB. They're frequently misaligned,
* so don't mess about with cpu_to_beN, just access it byte-wise */
static void scsi_ram_put_u32(unsigned char *addr, unsigned int data)

{
        addr[0] = data >> 24;
        addr[1] = data >> 16;
        addr[2] = data >> 8;
        addr[3] = data;
}

static unsigned int scsi_ram_get_u16(unsigned char *addr)

{
        unsigned int data;
        data = addr[0] << 8;
        data |= addr[1];

        return data;
}

static unsigned int scsi_ram_get_u24(unsigned char *addr)

{
        unsigned int data;
        data = addr[0] << 16;
        data |= addr[1] << 8;
        data |= addr[2];

        return data;
}

static unsigned int scsi_ram_get_u32(unsigned char *addr)

{
        unsigned int data;
        data = addr[0] << 24;
        data |= addr[1] << 16;
        data |= addr[2] << 8;
        data |= addr[3];

        return data;
}

Quoted 4 lines · Boaz Harrosh

drivers/ata/libata-scsi.c static unsigned int ata_scsiop_inq_89(struct ata_scsi_args *args, u8 *rbuf) · line 1910

Quoted 2 lines

drivers/ata/libata-scsi.c +2 -0

+		rbuf[4] = args->id[217] >> 8;
+		rbuf[5] = args->id[217];

args->id of struct ata_scsi_args are defined as u16. Are they actually SWABed at this point? Are they LE? BE?

They're cpu-endian at this point, I believe. See ata_dev_read_id() in libata-core.c where it calls swap_buf_le16().

Quoted 4 lines

if args->id are actually __be16 then above should be
+ put_unaligned(args->id[217], &rbuf[4]);
else
+ put_unaligned_be16(args->id[217], &rbuf[4]);

But the buf isn't unaligned, nor the offset within it. And I get confused by all these macros anyway. If we had something like store_scsi_u16(unsigned char *, u16), I'd use that, but put_unaligned_be16 just doesn't make sense.

I actually wrote my own accessors for scsi_ram. If they were to be more generic, they ought to be renamed, but this is what I found useful:

/*
* SCSI requires quantities to be written MSB. They're frequently misaligned,
* so don't mess about with cpu_to_beN, just access it byte-wise */
static void scsi_ram_put_u32(unsigned char *addr, unsigned int data)

{
        addr[0] = data >> 24;
        addr[1] = data >> 16;
        addr[2] = data >> 8;
        addr[3] = data;
}

static unsigned int scsi_ram_get_u16(unsigned char *addr)

{
        unsigned int data;
        data = addr[0] << 8;
        data |= addr[1];

        return data;
}

static unsigned int scsi_ram_get_u24(unsigned char *addr)

{
        unsigned int data;
        data = addr[0] << 16;
        data |= addr[1] << 8;
        data |= addr[2];

        return data;
}

static unsigned int scsi_ram_get_u32(unsigned char *addr)

{
        unsigned int data;
        data = addr[0] << 24;
        data |= addr[1] << 16;
        data |= addr[2] << 8;
        data |= addr[3];

        return data;
}

I'm sure a more fully-featured set would include put_u24 and put_u16 too. For the curious, u24 is useful for implementing read6/write6.

Quoted 7 lines · Jeff Garzik

Quoted 5 lines · Matthew Wilcox

Quoted 2 lines · James Bottomley

This looks like it can be called for ATA_DEV_ATAPI as well, in which case it's an illegal page to ask for.

I was under the impression that this entire file is only used for non-atapi devices.

Correct. That code path is not travelled by ATAPI.

Thanks, Jeff. Here's version three, including the form factor from word 168:

Add support for VPD page b1 to libata

SCSI VPD page b1 reports the nominal rotation speed and physical size of the device. Devices that conform to ATA-8 can return this information in words 217 and 168 of the identify data.

Signed-off-by Matthew Wilcox <[email protected]>