Skip to contentSkip to conversation

libata-scsi: hdparm support

PATCH Applied Tobias Lorenz 3 participants 9 messages Oct 22, 2004 – Nov 4, 2004

Subsystems (2)
SCSI libata
lore
Share & tools
lore

Tobias Lorenz

author
#0 lore raw

PATCH 1/1 libata-scsi: hdparm support

Hi,

this patch adds support for transfering drive informations via the hd_driveid structure to the hdparm utility. At the moment, only cylinders, sectors, heads, model and firmware version are transfered.

Our intention was to display the supported and the used UDMA mode(s), but we didn't find the correct structures yet to get these infos...

Signed-off-by Tobias Lorenz <[email protected]>
Quoted 40 lines
drivers/scsi/libata-scsi.c 2004-10-20 19:25:15.000000000 +0200 renamed from drivers/scsi/libata-scsi.c 2004-10-20 22:07:50.000000000 +0200 +23 -0
@@ -215,6 +215,17 @@
        struct ata_port *ap;
        struct ata_device *dev;
        int val = -EINVAL, rc = -EINVAL;
+       struct hd_driveid drv_id = {
+               .cyls           = 0,
+               .sectors        = 0,
+               .heads          = 0,
+               .fw_rev         = "",
+               .model          = "",
+               .cur_cyls       = 0,
+               .cur_heads      = 0,
+               .cur_sectors    = 0,
+       };
+       int geom[3];
 
        ap = (struct ata_port *) &scsidev->host->hostdata[0];
        if (!ap)

@@ -249,7 +260,25 @@
                        return -EACCES;
                return ata_task_ioctl(scsidev, arg);
 
+       case HDIO_GET_IDENTITY:
+               ata_std_bios_param(scsidev, NULL, dev->n_sectors, geom);
+               drv_id.cur_heads        = drv_id.heads          = geom[0];
+               drv_id.cur_sectors      = drv_id.sectors        = geom[1];
+               drv_id.cur_cyls         = drv_id.cyls           = geom[2];
+               strncpy((char *) &drv_id.model, scsidev->model, sizeof(drv_id.model));
+               strncpy((char *) &drv_id.fw_rev, scsidev->rev, sizeof(drv_id.fw_rev));
+               if(copy_to_user((char *) arg, (char *) &drv_id,
+                                sizeof(drv_id)))
+                       return(-EFAULT);
+               return 0;
+
        default:
                rc = -EOPNOTSUPP;
                break;
        }
#1 lore raw

Hi,

Quoted 8 lines · Tobias Lorenz

Hi,

this patch adds support for transfering drive informations via the hd_driveid structure to the hdparm utility. At the moment, only cylinders, sectors, heads, model and firmware version are transfered.

Our intention was to display the supported and the used UDMA mode(s), but we didn't find the correct structures yet to get these infos...

dev->id

HDIO_GET_IDENTITY (as used by 'hdparm -i') should return _original_ drive ID (dev->id should be returned) so currently used mode may not be reported correctly.

'hdparm -I' should be already supported by current libata-dev tree.

Bartlomiej

author
#2 lore raw

Hi,

Quoted 5 lines · Bartlomiej Zolnierkiewicz

dev->id

HDIO_GET_IDENTITY (as used by 'hdparm -i') should return _original_ drive ID (dev->id should be returned) so currently used mode may not be reported correctly.

The used mode isn't reported yet. Only cylinders, sectors, heads, model and firmware version are supported by now.

dev is of type ata_device, so dev->id is
u16 id[ATA_ID_WORDS]; /* IDENTIFY xxx DEVICE data */

Where in dev->id are the model and revision strings encoded ? Here is a comparison:
ata1: dev 0 cfg 49:2f00 82:74eb 83:4bea 84:4000 85:7469 86:0802 87:4003 88:203f
ata_dump_id: 49==0x2f00 53==0x0007 63==0x0007 64==0x0003 75==0x001f
ata_dump_id: 80==0x003c 81==0x0015 82==0x74eb 83==0x4bea 84==0x4000
ata_dump_id: 88==0x203f 93==0x600b

ata_scsiop_inq_std: ENTER
Vendor: ATA Model: IC35L060AVER07-0 Rev: 1.01
Type: Direct-Access ANSI SCSI revision: 05

Quoted 1 line

'hdparm -I' should be already supported by current libata-dev tree.

hdparm -I needs implementations for the HDIO_GET_MULTCOUNT and HDIO_GET_IDENTITY commands in ata_scsi_ioctl to report informations. Both were not implemented.

Toby

#3 lore raw
In reply to Tobias Lorenz
Quoted 35 lines · Tobias Lorenz

Hi,

this patch adds support for transfering drive informations via the hd_driveid structure to the hdparm utility. At the moment, only cylinders, sectors, heads, model and firmware version are transfered.

Our intention was to display the supported and the used UDMA mode(s), but we didn't find the correct structures yet to get these infos...

Signed-off-by Tobias Lorenz <[email protected]>
Quoted 24 lines
drivers/scsi/libata-scsi.c 2004-10-20 19:25:15.000000000 +0200 renamed from drivers/scsi/libata-scsi.c 2004-10-20 22:07:50.000000000 +0200 +11 -0
@@ -215,6 +215,17 @@
       struct ata_port *ap;
       struct ata_device *dev;
       int val = -EINVAL, rc = -EINVAL;
+       struct hd_driveid drv_id = {
+               .cyls           = 0,
+               .sectors        = 0,
+               .heads          = 0,
+               .fw_rev         = "",
+               .model          = "",
+               .cur_cyls       = 0,
+               .cur_heads      = 0,
+               .cur_sectors    = 0,
+       };
+       int geom[3];
 
       ap = (struct ata_port *) &scsidev->host->hostdata[0];
       if (!ap)

@@ -249,7 +260,25 @@
                       return -EACCES;
               return ata_task_ioctl(scsidev, arg);
Quoted 15 lines
Quoted 5 lines
drivers/scsi/libata-scsi.c 2004-10-20 19:25:15.000000000 +0200 renamed from drivers/scsi/libata-scsi.c 2004-10-20 22:07:50.000000000 +0200 +5 -0
+       case HDIO_GET_IDENTITY:
+               ata_std_bios_param(scsidev, NULL, dev->n_sectors, geom);
+               drv_id.cur_heads        = drv_id.heads          = geom[0];
+               drv_id.cur_sectors      = drv_id.sectors        = geom[1];
+               drv_id.cur_cyls         = drv_id.cyls           = geom[2];

drivers/scsi/libata-scsi.c 2004-10-20 19:25:15.000000000 +0200 line 277

Quoted 10 lines
drivers/scsi/libata-scsi.c 2004-10-20 19:25:15.000000000 +0200 renamed from drivers/scsi/libata-scsi.c 2004-10-20 22:07:50.000000000 +0200 +7 -0
+               strncpy((char *) &drv_id.model, scsidev->model, sizeof(drv_id.model));
+               strncpy((char *) &drv_id.fw_rev, scsidev->rev, sizeof(drv_id.fw_rev));
+               if(copy_to_user((char *) arg, (char *) &drv_id,
+                                sizeof(drv_id)))
+                       return(-EFAULT);
+               return 0;
+
       default:
               rc = -EOPNOTSUPP;
               break;

This patch doesn't apply, since it is prefixed with "> " ...

Jeff

author
#4 lore raw

Hi,

Quoted 1 line

This patch doesn't apply, since it is prefixed with "> " ...

You are right... here is a corrected version.

This patch adds support for transfering drive informations via the hd_driveid structure to the hdparm utility. At the moment, only cylinders, sectors, heads, model and firmware version are transfered.

Signed-off-by Tobias Lorenz <[email protected]>
drivers/scsi/libata-scsi.c +22 -0
@@ -215,6 +215,17 @@
 	struct ata_port *ap;
 	struct ata_device *dev;
 	int val = -EINVAL, rc = -EINVAL;
+	struct hd_driveid drv_id = {
+		.cyls		= 0,
+		.sectors	= 0,
+		.heads		= 0,
+		.fw_rev		= "",
+		.model		= "",
+		.cur_cyls	= 0,
+		.cur_heads	= 0,
+		.cur_sectors	= 0,
+	};
+	int geom[3];
 
 	ap = (struct ata_port *) &scsidev->host->hostdata[0];
 	if (!ap)

@@ -249,6 +260,17 @@
 			return -EACCES;
 		return ata_task_ioctl(scsidev, arg);
 
+	case HDIO_GET_IDENTITY:
+		ata_std_bios_param(scsidev, NULL, dev->n_sectors, geom);
+		drv_id.cur_heads	= drv_id.heads		= geom[0];
+		drv_id.cur_sectors	= drv_id.sectors	= geom[1];
+		drv_id.cur_cyls		= drv_id.cyls		= geom[2];
+		strncpy((char *) &drv_id.model, scsidev->model, sizeof(drv_id.model));
+		strncpy((char *) &drv_id.fw_rev, scsidev->rev, sizeof(drv_id.fw_rev));
+		if(copy_to_user((char *) arg, (char *) &drv_id, sizeof(drv_id)))
+			return(-EFAULT);
+		return 0;
+
 	default:
 		rc = -EOPNOTSUPP;
 		break;
Applied
#5 lore raw
Quoted 10 lines · Tobias Lorenz

Hi,

Quoted 1 line

This patch doesn't apply, since it is prefixed with "> " ...

You are right... here is a corrected version.

This patch adds support for transfering drive informations via the hd_driveid structure to the hdparm utility. At the moment, only cylinders, sectors, heads, model and firmware version are transfered.

Applied to libata-dev... I'll need to ponder the correctness a bit more, before pushing upstream.

Jeff

#6 lore raw
Quoted 6 lines · Jeff Garzik
Quoted 5 lines · Tobias Lorenz

Hi,

Quoted 1 line

This patch doesn't apply, since it is prefixed with "> " ...

Quoted 8 lines
Quoted 5 lines

You are right... here is a corrected version.

This patch adds support for transfering drive informations via the hd_driveid structure to the hdparm utility. At the moment, only cylinders, sectors, heads, model and firmware version are transfered.

Applied to libata-dev... I'll need to ponder the correctness a bit more, before pushing upstream.

Speaking about correctness...

HDIO_GET_IDENTIFY returns ID block in CPU order (not LE).

If you decide to add it sooner or later you will need something ala ide_fix_driveid() from ide-iops.c (bloat).

Bartlomiej

author
#7 lore raw

Hi,

Quoted 6 lines · Bartlomiej Zolnierkiewicz

Speaking about correctness...

HDIO_GET_IDENTIFY returns ID block in CPU order (not LE).

If you decide to add it sooner or later you will need something ala ide_fix_driveid() from ide-iops.c (bloat).

Okay, I am wondering, who defined, that hd_driveid has to be little endian and not cpu order. Does that really has to be platform independent ? But anyway...

There are two possible solutions:

The quick way is attached and just adds the missing conversions by copy&paste from ide-iops:ide_fix_driveid.

The better and less redundant way would be to use the original function:
- by setting a Kconfig dependency on CONFIG_IDE.
- or by moving ide_fix_driveid to a seperate file (header file ?).

What do you mean ?
Toby

Signed-off-by Tobias Lorenz <[email protected]>
drivers/scsi/libata-scsi.c +46 -0
@@ -31,6 +31,7 @@
 #include <linux/libata.h>
 #include <linux/hdreg.h>
 #include <asm/uaccess.h>
+#include <asm/byteorder.h>
 
 #include "libata.h"
 

@@ -215,6 +216,17 @@
 	struct ata_port *ap;
 	struct ata_device *dev;
 	int val = -EINVAL, rc = -EINVAL;
+	struct hd_driveid drv_id = {
+		.cyls		= 0,
+		.sectors	= 0,
+		.heads		= 0,
+		.fw_rev		= "",
+		.model		= "",
+		.cur_cyls	= 0,
+		.cur_heads	= 0,
+		.cur_sectors	= 0,
+	};
+	int geom[3];
 
 	ap = (struct ata_port *) &scsidev->host->hostdata[0];
 	if (!ap)

@@ -249,6 +261,40 @@
 			return -EACCES;
 		return ata_task_ioctl(scsidev, arg);
 
+	case HDIO_GET_IDENTITY:
+		ata_std_bios_param(scsidev, NULL, dev->n_sectors, geom);
+		drv_id.cur_heads	= drv_id.heads		= geom[0];
+		drv_id.cur_sectors	= drv_id.sectors	= geom[1];
+		drv_id.cur_cyls		= drv_id.cyls		= geom[2];
+		strncpy((char *) &drv_id.model, scsidev->model, sizeof(drv_id.model));
+		strncpy((char *) &drv_id.fw_rev, scsidev->rev, sizeof(drv_id.fw_rev));
+#ifndef __LITTLE_ENDIAN
+# ifdef __BIG_ENDIAN
+		{
+			int i;
+			u16 *stringcast;
+
+			drv_id.cyls           = __le16_to_cpu(drv_id.cyls);
+			drv_id.heads          = __le16_to_cpu(drv_id.heads);
+			drv_id.sectors        = __le16_to_cpu(drv_id.sectors);
+			stringcast = (u16 *)&drv_id.fw_rev[0];
+			for (i = 0; i < (8/2); i++)
+				stringcast[i] = __le16_to_cpu(stringcast[i]);
+			stringcast = (u16 *)&drv_id.model[0];
+			for (i = 0; i < (40/2); i++)
+				stringcast[i] = __le16_to_cpu(stringcast[i]);
+			drv_id.cur_cyls       = __le16_to_cpu(drv_id.cur_cyls);
+			drv_id.cur_heads      = __le16_to_cpu(drv_id.cur_heads);
+			drv_id.cur_sectors    = __le16_to_cpu(drv_id.cur_sectors);
+		}
+# else
+#  error "Please fix <asm/byteorder.h>"
+# endif
+#endif
+		if(copy_to_user((char *) arg, (char *) &drv_id, sizeof(drv_id)))
+			return(-EFAULT);
+		return 0;
+
 	default:
 		rc = -EOPNOTSUPP;
 		break;
#8 lore raw

Hi!

Quoted 5 lines · Tobias Lorenz

Hi,

Quoted 2 lines · Bartlomiej Zolnierkiewicz

Speaking about correctness...

Quoted 7 lines
Quoted 4 lines

HDIO_GET_IDENTIFY returns ID block in CPU order (not LE).

If you decide to add it sooner or later you will need something ala ide_fix_driveid() from ide-iops.c (bloat).

Okay, I am wondering, who defined, that hd_driveid has to be little endian and not cpu order. Does that really has to be platform

ATA spec ;-)

HDIO_DRIVE_CMD: LE order
HDIO_GET_IDENTIFY : CPU order

Quoted 1 line

independent ? But anyway...

HDIO_GET_IDENTIFY - yes

Quoted 4 lines

There are two possible solutions:

The quick way is attached and just adds the missing conversions by copy&paste from ide-iops:ide_fix_driveid.

This is not needed (yet)...

ata_std_bios_params() returns things in CPU order arleady, model and fw_rev are also in CPU order (libata driver handles them).

Quoted 3 lines

The better and less redundant way would be to use the original function:
- by setting a Kconfig dependency on CONFIG_IDE.
- or by moving ide_fix_driveid to a seperate file (header file ?).

There was recent discussion about this wrt to isd200.c driver. IMHO the correct solution is to not need ide_fix_driveid() et all.

Quoted 1 line

What do you mean ?

My point is that once you decide to support HDIO_GET_IDENTIFY in libata you will have to take care of this issue (in the future)...

The result will be the same crap as ide_fix_driveid() etc.

I think that is much easier to teach blktool about SG_IO ioctl and do the stuff right than to try to be "smart" and support all badly designed IDE driver ioctls used by hdparm...

Bartlomiej

Quoted 82 lines

Toby

Signed-off-by Tobias Lorenz <[email protected]>
drivers/scsi/libata-scsi.c 2004-11-04 17:39:58.000000000 +0100 renamed from drivers/scsi/libata-scsi.c 2004-10-30 16:31:01.000000000 +0200 +12 -0
@@ -31,6 +31,7 @@
 #include <linux/libata.h>
 #include <linux/hdreg.h>
 #include <asm/uaccess.h>
+#include <asm/byteorder.h>
 
 #include "libata.h"
 

@@ -215,6 +216,17 @@
 
 
        struct ata_port *ap;
        struct ata_device *dev;
        int val = -EINVAL, rc = -EINVAL;
+       struct hd_driveid drv_id = {
+               .cyls           = 0,
+               .sectors        = 0,
+               .heads          = 0,
+               .fw_rev         = "",
+               .model          = "",
+               .cur_cyls       = 0,
+               .cur_heads      = 0,
+               .cur_sectors    = 0,
+       };
+       int geom[3];
 

        ap = (struct ata_port *) &scsidev->host->hostdata[0];
        if (!ap)
drivers/scsi/libata-scsi.c 2004-11-04 17:39:58.000000000 +0100 renamed from drivers/scsi/libata-scsi.c 2004-10-30 16:31:01.000000000 +0200 +34 -0
@@ -249,6 +261,40 @@
 
 
                        return -EACCES;
                return ata_task_ioctl(scsidev, arg);
 
+       case HDIO_GET_IDENTITY:
+               ata_std_bios_param(scsidev, NULL, dev->n_sectors, geom);
+               drv_id.cur_heads        = drv_id.heads          = geom[0];
+               drv_id.cur_sectors      = drv_id.sectors        = geom[1];
+               drv_id.cur_cyls         = drv_id.cyls           = geom[2];
+               strncpy((char *) &drv_id.model, scsidev->model, sizeof(drv_id.model));
+               strncpy((char *) &drv_id.fw_rev, scsidev->rev, sizeof(drv_id.fw_rev));
+#ifndef __LITTLE_ENDIAN
+# ifdef __BIG_ENDIAN
+               {
+                       int i;
+                       u16 *stringcast;
+
+                       drv_id.cyls           = __le16_to_cpu(drv_id.cyls);
+                       drv_id.heads          = __le16_to_cpu(drv_id.heads);
+                       drv_id.sectors        = __le16_to_cpu(drv_id.sectors);
+                       stringcast = (u16 *)&drv_id.fw_rev[0];
+                       for (i = 0; i < (8/2); i++)
+                               stringcast[i] = __le16_to_cpu(stringcast[i]);
+                       stringcast = (u16 *)&drv_id.model[0];
+                       for (i = 0; i < (40/2); i++)
+                               stringcast[i] = __le16_to_cpu(stringcast[i]);
+                       drv_id.cur_cyls       = __le16_to_cpu(drv_id.cur_cyls);
+                       drv_id.cur_heads      = __le16_to_cpu(drv_id.cur_heads);
+                       drv_id.cur_sectors    = __le16_to_cpu(drv_id.cur_sectors);
+               }
+# else
+#  error "Please fix <asm/byteorder.h>"
+# endif
+#endif
 
 
+               if(copy_to_user((char *) arg, (char *) &drv_id, sizeof(drv_id)))
+                       return(-EFAULT);
+               return 0;
+
        default:
                rc = -EOPNOTSUPP;
                break;