[PATCH 1/3] libata: straighten out ATA_ID_* constants

[PATCH 1/3] libata: straighten out ATA_ID_* constants

[Tejun Heo]

* Kill _OFS suffixes in ATA_ID_{SERNO|FW_REV|PROD}_OFS for consistency
  with other ATA_ID_* constants.

* Kill ATA_SERNO_LEN

* Add and use ATA_ID_SERNO_LEN, ATA_ID_FW_REV_LEN and ATA_ID_PROD_LEN.
  This change also makes ata_device_blacklisted() use proper length
  for fwrev.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c      |   21 ++++++++++-----------
 drivers/ata/libata-scsi.c      |   33 ++++++++++++++++-----------------
 drivers/ata/pata_ali.c         |    4 ++--
 drivers/ata/pata_hpt366.c      |    4 ++--
 drivers/ata/pata_hpt37x.c      |    5 ++---
 drivers/ata/pata_it821x.c      |    5 ++---
 drivers/ata/pata_serverworks.c |    4 ++--
 drivers/ata/sata_sil.c         |    4 ++--
 include/linux/ata.h            |   11 +++++++----
 9 files changed, 45 insertions(+), 46 deletions(-)

Index: work/include/linux/ata.h
===================================================================
--- work.orig/include/linux/ata.h
+++ work/include/linux/ata.h
@@ -44,9 +44,9 @@ enum {
 	ATA_MAX_SECTORS_LBA48	= 65535,/* TODO: 65536? */
 
 	ATA_ID_WORDS		= 256,
-	ATA_ID_SERNO_OFS	= 10,
-	ATA_ID_FW_REV_OFS	= 23,
-	ATA_ID_PROD_OFS		= 27,
+	ATA_ID_SERNO		= 10,
+	ATA_ID_FW_REV		= 23,
+	ATA_ID_PROD		= 27,
 	ATA_ID_OLD_PIO_MODES	= 51,
 	ATA_ID_FIELD_VALID	= 53,
 	ATA_ID_MWDMA_MODES	= 63,
@@ -58,8 +58,11 @@ enum {
 	ATA_ID_MAJOR_VER	= 80,
 	ATA_ID_PIO4		= (1 << 1),
 
+	ATA_ID_SERNO_LEN	= 20,
+	ATA_ID_FW_REV_LEN	= 8,
+	ATA_ID_PROD_LEN		= 40,
+
 	ATA_PCI_CTL_OFS		= 2,
-	ATA_SERNO_LEN		= 20,
 	ATA_UDMA0		= (1 << 0),
 	ATA_UDMA1		= ATA_UDMA0 | (1 << 1),
 	ATA_UDMA2		= ATA_UDMA1 | (1 << 2),
Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -3196,7 +3196,8 @@ static int ata_dev_same_device(struct at
 			       const u16 *new_id)
 {
 	const u16 *old_id = dev->id;
-	unsigned char model[2][41], serial[2][21];
+	unsigned char model[2][ATA_ID_PROD_LEN + 1];
+	unsigned char serial[2][ATA_ID_SERNO_LEN + 1];
 	u64 new_n_sectors;
 
 	if (dev->class != new_class) {
@@ -3205,10 +3206,10 @@ static int ata_dev_same_device(struct at
 		return 0;
 	}
 
-	ata_id_c_string(old_id, model[0], ATA_ID_PROD_OFS, sizeof(model[0]));
-	ata_id_c_string(new_id, model[1], ATA_ID_PROD_OFS, sizeof(model[1]));
-	ata_id_c_string(old_id, serial[0], ATA_ID_SERNO_OFS, sizeof(serial[0]));
-	ata_id_c_string(new_id, serial[1], ATA_ID_SERNO_OFS, sizeof(serial[1]));
+	ata_id_c_string(old_id, model[0], ATA_ID_PROD, sizeof(model[0]));
+	ata_id_c_string(new_id, model[1], ATA_ID_PROD, sizeof(model[1]));
+	ata_id_c_string(old_id, serial[0], ATA_ID_SERNO, sizeof(serial[0]));
+	ata_id_c_string(new_id, serial[1], ATA_ID_SERNO, sizeof(serial[1]));
 	new_n_sectors = ata_id_n_sectors(new_id);
 
 	if (strcmp(model[0], model[1])) {
@@ -3347,15 +3348,13 @@ static int ata_strim(char *s, size_t len
 
 unsigned long ata_device_blacklisted(const struct ata_device *dev)
 {
-	unsigned char model_num[40];
-	unsigned char model_rev[16];
+	unsigned char model_num[ATA_ID_PROD_LEN];
+	unsigned char model_rev[ATA_ID_FW_REV_LEN];
 	unsigned int nlen, rlen;
 	const struct ata_blacklist_entry *ad = ata_device_blacklist;
 
-	ata_id_string(dev->id, model_num, ATA_ID_PROD_OFS,
-			  sizeof(model_num));
-	ata_id_string(dev->id, model_rev, ATA_ID_FW_REV_OFS,
-			  sizeof(model_rev));
+	ata_id_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
+	ata_id_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev));
 	nlen = ata_strim(model_num, sizeof(model_num));
 	rlen = ata_strim(model_rev, sizeof(model_rev));
 
Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -1660,8 +1660,8 @@ unsigned int ata_scsiop_inq_std(struct a
 
 	if (buflen > 35) {
 		memcpy(&rbuf[8], "ATA     ", 8);
-		ata_id_string(args->id, &rbuf[16], ATA_ID_PROD_OFS, 16);
-		ata_id_string(args->id, &rbuf[32], ATA_ID_FW_REV_OFS, 4);
+		ata_id_string(args->id, &rbuf[16], ATA_ID_PROD, 16);
+		ata_id_string(args->id, &rbuf[32], ATA_ID_FW_REV, 4);
 		if (rbuf[32] == 0 || rbuf[32] == ' ')
 			memcpy(&rbuf[32], "n/a ", 4);
 	}
@@ -1730,13 +1730,13 @@ unsigned int ata_scsiop_inq_80(struct at
 		0,
 		0x80,			/* this page code */
 		0,
-		ATA_SERNO_LEN,		/* page len */
+		ATA_ID_SERNO_LEN,	/* page len */
 	};
 	memcpy(rbuf, hdr, sizeof(hdr));
 
-	if (buflen > (ATA_SERNO_LEN + 4 - 1))
+	if (buflen > (ATA_ID_SERNO_LEN + 4 - 1))
 		ata_id_string(args->id, (unsigned char *) &rbuf[4],
-			      ATA_ID_SERNO_OFS, ATA_SERNO_LEN);
+			      ATA_ID_SERNO, ATA_ID_SERNO_LEN);
 
 	return 0;
 }
@@ -1761,19 +1761,18 @@ unsigned int ata_scsiop_inq_83(struct at
 {
 	int num;
 	const int sat_model_serial_desc_len = 68;
-	const int ata_model_byte_len = 40;
 
 	rbuf[1] = 0x83;			/* this page code */
 	num = 4;
 
-	if (buflen > (ATA_SERNO_LEN + num + 3)) {
+	if (buflen > (ATA_ID_SERNO_LEN + num + 3)) {
 		/* piv=0, assoc=lu, code_set=ACSII, designator=vendor */
 		rbuf[num + 0] = 2;
-		rbuf[num + 3] = ATA_SERNO_LEN;
+		rbuf[num + 3] = ATA_ID_SERNO_LEN;
 		num += 4;
 		ata_id_string(args->id, (unsigned char *) rbuf + num,
-			      ATA_ID_SERNO_OFS, ATA_SERNO_LEN);
-		num += ATA_SERNO_LEN;
+			      ATA_ID_SERNO, ATA_ID_SERNO_LEN);
+		num += ATA_ID_SERNO_LEN;
 	}
 	if (buflen > (sat_model_serial_desc_len + num + 3)) {
 		/* SAT defined lu model and serial numbers descriptor */
@@ -1785,11 +1784,11 @@ unsigned int ata_scsiop_inq_83(struct at
 		memcpy(rbuf + num, "ATA     ", 8);
 		num += 8;
 		ata_id_string(args->id, (unsigned char *) rbuf + num,
-			      ATA_ID_PROD_OFS, ata_model_byte_len);
-		num += ata_model_byte_len;
+			      ATA_ID_PROD, ATA_ID_PROD_LEN);
+		num += ATA_ID_PROD_LEN;
 		ata_id_string(args->id, (unsigned char *) rbuf + num,
-			      ATA_ID_SERNO_OFS, ATA_SERNO_LEN);
-		num += ATA_SERNO_LEN;
+			      ATA_ID_SERNO, ATA_ID_SERNO_LEN);
+		num += ATA_ID_SERNO_LEN;
 	}
 	rbuf[3] = num - 4;    /* page len (assume less than 256 bytes) */
 	return 0;
@@ -1917,15 +1916,15 @@ static unsigned int ata_msense_rw_recove
  */
 static int ata_dev_supports_fua(u16 *id)
 {
-	unsigned char model[41], fw[9];
+	unsigned char model[ATA_ID_PROD_LEN + 1], fw[ATA_ID_FW_REV_LEN + 1];
 
 	if (!libata_fua)
 		return 0;
 	if (!ata_id_has_fua(id))
 		return 0;
 
-	ata_id_c_string(id, model, ATA_ID_PROD_OFS, sizeof(model));
-	ata_id_c_string(id, fw, ATA_ID_FW_REV_OFS, sizeof(fw));
+	ata_id_c_string(id, model, ATA_ID_PROD, sizeof(model));
+	ata_id_c_string(id, fw, ATA_ID_FW_REV, sizeof(fw));
 
 	if (strcmp(model, "Maxtor"))
 		return 1;
Index: work/drivers/ata/pata_ali.c
===================================================================
--- work.orig/drivers/ata/pata_ali.c
+++ work/drivers/ata/pata_ali.c
@@ -153,11 +153,11 @@ static void ali_early_error_handler(stru
 
 static unsigned long ali_20_filter(const struct ata_port *ap, struct ata_device *adev, unsigned long mask)
 {
-	char model_num[40];
+	char model_num[ATA_ID_PROD_LEN];
 	/* No DMA on anything but a disk for now */
 	if (adev->class != ATA_DEV_ATA)
 		mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
-	ata_id_string(adev->id, model_num, ATA_ID_PROD_OFS, sizeof(model_num));
+	ata_id_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 	if (strstr(model_num, "WDC"))
 		return mask &= ~ATA_MASK_UDMA;
 	return ata_pci_default_filter(ap, adev, mask);
Index: work/drivers/ata/pata_hpt366.c
===================================================================
--- work.orig/drivers/ata/pata_hpt366.c
+++ work/drivers/ata/pata_hpt366.c
@@ -151,12 +151,12 @@ static const char *bad_ata66_3[] = {
 
 static int hpt_dma_blacklisted(const struct ata_device *dev, char *modestr, const char *list[])
 {
-	unsigned char model_num[40];
+	unsigned char model_num[ATA_ID_PROD_LEN];
 	char *s;
 	unsigned int len;
 	int i = 0;
 
-	ata_id_string(dev->id, model_num, ATA_ID_PROD_OFS, sizeof(model_num));
+	ata_id_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 	s = &model_num[0];
 	len = strnlen(s, sizeof(model_num));
 
Index: work/drivers/ata/pata_it821x.c
===================================================================
--- work.orig/drivers/ata/pata_it821x.c
+++ work/drivers/ata/pata_it821x.c
@@ -529,15 +529,14 @@ static void it821x_smart_set_mode(struct
 
 static void it821x_dev_config(struct ata_port *ap, struct ata_device *adev)
 {
-	unsigned char model_num[40];
+	unsigned char model_num[ATA_ID_PROD_LEN];
 	char *s;
 	unsigned int len;
 
 	/* This block ought to be a library routine as it is in several
 	   drivers now */
 
-	ata_id_string(adev->id, model_num, ATA_ID_PROD_OFS,
-			  sizeof(model_num));
+	ata_id_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 	s = &model_num[0];
 	len = strnlen(s, sizeof(model_num));
 
Index: work/drivers/ata/pata_serverworks.c
===================================================================
--- work.orig/drivers/ata/pata_serverworks.c
+++ work/drivers/ata/pata_serverworks.c
@@ -218,7 +218,7 @@ static unsigned long serverworks_osb4_fi
 static unsigned long serverworks_csb_filter(const struct ata_port *ap, struct ata_device *adev, unsigned long mask)
 {
 	const char *p;
-	char model_num[40];
+	char model_num[ATA_ID_PROD_LEN];
 	int len, i;
 
 	/* Disk, UDMA */
@@ -226,7 +226,7 @@ static unsigned long serverworks_csb_fil
 		return ata_pci_default_filter(ap, adev, mask);
 
 	/* Actually do need to check */
-	ata_id_string(adev->id, model_num, ATA_ID_PROD_OFS, sizeof(model_num));
+	ata_id_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 	/* Precuationary - why not do this in the libata core ?? */
 
 	len = strlen(model_num);
Index: work/drivers/ata/sata_sil.c
===================================================================
--- work.orig/drivers/ata/sata_sil.c
+++ work/drivers/ata/sata_sil.c
@@ -541,9 +541,9 @@ static void sil_dev_config(struct ata_po
 {
 	int print_info = ap->eh_context.i.flags & ATA_EHI_PRINTINFO;
 	unsigned int n, quirks = 0;
-	unsigned char model_num[41];
+	unsigned char model_num[ATA_ID_PROD_LEN + 1];
 
-	ata_id_c_string(dev->id, model_num, ATA_ID_PROD_OFS, sizeof(model_num));
+	ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 
 	for (n = 0; sil_blacklist[n].product; n++)
 		if (!strcmp(sil_blacklist[n].product, model_num)) {
Index: work/drivers/ata/pata_hpt37x.c
===================================================================
--- work.orig/drivers/ata/pata_hpt37x.c
+++ work/drivers/ata/pata_hpt37x.c
@@ -349,13 +349,12 @@ static u32 hpt37x_find_mode(struct ata_p
 
 static int hpt_dma_blacklisted(const struct ata_device *dev, char *modestr, const char *list[])
 {
-	unsigned char model_num[40];
+	unsigned char model_num[ATA_ID_PROD_LEN];
 	char *s;
 	unsigned int len;
 	int i = 0;
 
-	ata_id_string(dev->id, model_num, ATA_ID_PROD_OFS,
-			  sizeof(model_num));
+	ata_id_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 	s = &model_num[0];
 	len = strnlen(s, sizeof(model_num));
 

[PATCH 2/3] libata: use ata_id_c_string()

[Tejun Heo]

There were several places where ATA ID strings are manually terminated
and in some places possibly unterminated strings were passed to string
functions which don't limit length like strstr().  This patch converts
all of them over to ata_id_c_string().

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c      |   27 ++++++---------------------
 drivers/ata/pata_ali.c         |    4 ++--
 drivers/ata/pata_hpt366.c      |   18 ++++--------------
 drivers/ata/pata_hpt37x.c      |   18 ++++--------------
 drivers/ata/pata_it821x.c      |   19 +++----------------
 drivers/ata/pata_serverworks.c |   17 +++++------------
 6 files changed, 24 insertions(+), 79 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -3334,35 +3334,20 @@ static const struct ata_blacklist_entry 
 	{ }
 };
 
-static int ata_strim(char *s, size_t len)
-{
-	len = strnlen(s, len);
-
-	/* ATAPI specifies that empty space is blank-filled; remove blanks */
-	while ((len > 0) && (s[len - 1] == ' ')) {
-		len--;
-		s[len] = 0;
-	}
-	return len;
-}
-
 unsigned long ata_device_blacklisted(const struct ata_device *dev)
 {
-	unsigned char model_num[ATA_ID_PROD_LEN];
-	unsigned char model_rev[ATA_ID_FW_REV_LEN];
-	unsigned int nlen, rlen;
+	unsigned char model_num[ATA_ID_PROD_LEN + 1];
+	unsigned char model_rev[ATA_ID_FW_REV_LEN + 1];
 	const struct ata_blacklist_entry *ad = ata_device_blacklist;
 
-	ata_id_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
-	ata_id_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev));
-	nlen = ata_strim(model_num, sizeof(model_num));
-	rlen = ata_strim(model_rev, sizeof(model_rev));
+	ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
+	ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev));
 
 	while (ad->model_num) {
-		if (!strncmp(ad->model_num, model_num, nlen)) {
+		if (!strcmp(ad->model_num, model_num)) {
 			if (ad->model_rev == NULL)
 				return ad->horkage;
-			if (!strncmp(ad->model_rev, model_rev, rlen))
+			if (!strcmp(ad->model_rev, model_rev))
 				return ad->horkage;
 		}
 		ad++;
Index: work/drivers/ata/pata_ali.c
===================================================================
--- work.orig/drivers/ata/pata_ali.c
+++ work/drivers/ata/pata_ali.c
@@ -153,11 +153,11 @@ static void ali_early_error_handler(stru
 
 static unsigned long ali_20_filter(const struct ata_port *ap, struct ata_device *adev, unsigned long mask)
 {
-	char model_num[ATA_ID_PROD_LEN];
+	char model_num[ATA_ID_PROD_LEN + 1];
 	/* No DMA on anything but a disk for now */
 	if (adev->class != ATA_DEV_ATA)
 		mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
-	ata_id_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num));
+	ata_id_c_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 	if (strstr(model_num, "WDC"))
 		return mask &= ~ATA_MASK_UDMA;
 	return ata_pci_default_filter(ap, adev, mask);
Index: work/drivers/ata/pata_hpt366.c
===================================================================
--- work.orig/drivers/ata/pata_hpt366.c
+++ work/drivers/ata/pata_hpt366.c
@@ -151,23 +151,13 @@ static const char *bad_ata66_3[] = {
 
 static int hpt_dma_blacklisted(const struct ata_device *dev, char *modestr, const char *list[])
 {
-	unsigned char model_num[ATA_ID_PROD_LEN];
-	char *s;
-	unsigned int len;
+	unsigned char model_num[ATA_ID_PROD_LEN + 1];
 	int i = 0;
 
-	ata_id_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
-	s = &model_num[0];
-	len = strnlen(s, sizeof(model_num));
-
-	/* ATAPI specifies that empty space is blank-filled; remove blanks */
-	while ((len > 0) && (s[len - 1] == ' ')) {
-		len--;
-		s[len] = 0;
-	}
+	ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 
-	while(list[i] != NULL) {
-		if (!strncmp(list[i], s, len)) {
+	while (list[i] != NULL) {
+		if (!strcmp(list[i], model_num)) {
 			printk(KERN_WARNING DRV_NAME ": %s is not supported for %s.\n",
 				modestr, list[i]);
 			return 1;
Index: work/drivers/ata/pata_it821x.c
===================================================================
--- work.orig/drivers/ata/pata_it821x.c
+++ work/drivers/ata/pata_it821x.c
@@ -529,22 +529,9 @@ static void it821x_smart_set_mode(struct
 
 static void it821x_dev_config(struct ata_port *ap, struct ata_device *adev)
 {
-	unsigned char model_num[ATA_ID_PROD_LEN];
-	char *s;
-	unsigned int len;
-
-	/* This block ought to be a library routine as it is in several
-	   drivers now */
-
-	ata_id_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num));
-	s = &model_num[0];
-	len = strnlen(s, sizeof(model_num));
-
-	/* ATAPI specifies that empty space is blank-filled; remove blanks */
-	while ((len > 0) && (s[len - 1] == ' ')) {
-		len--;
-		s[len] = 0;
-	}
+	unsigned char model_num[ATA_ID_PROD_LEN + 1];
+
+	ata_id_c_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 
 	if (adev->max_sectors > 255)
 		adev->max_sectors = 255;
Index: work/drivers/ata/pata_serverworks.c
===================================================================
--- work.orig/drivers/ata/pata_serverworks.c
+++ work/drivers/ata/pata_serverworks.c
@@ -218,25 +218,18 @@ static unsigned long serverworks_osb4_fi
 static unsigned long serverworks_csb_filter(const struct ata_port *ap, struct ata_device *adev, unsigned long mask)
 {
 	const char *p;
-	char model_num[ATA_ID_PROD_LEN];
-	int len, i;
+	char model_num[ATA_ID_PROD_LEN + 1];
+	int i;
 
 	/* Disk, UDMA */
 	if (adev->class != ATA_DEV_ATA)
 		return ata_pci_default_filter(ap, adev, mask);
 
 	/* Actually do need to check */
-	ata_id_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num));
-	/* Precuationary - why not do this in the libata core ?? */
+	ata_id_c_string(adev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 
-	len = strlen(model_num);
-	while ((len > 0) && (model_num[len - 1] == ' ')) {
-		len--;
-		model_num[len] = 0;
-	}
-
-	for(i = 0; (p = csb_bad_ata100[i]) != NULL; i++) {
-		if (!strncmp(p, model_num, len))
+	for (i = 0; (p = csb_bad_ata100[i]) != NULL; i++) {
+		if (!strcmp(p, model_num))
 			mask &= ~(0x1F << ATA_SHIFT_UDMA);
 	}
 	return ata_pci_default_filter(ap, adev, mask);
Index: work/drivers/ata/pata_hpt37x.c
===================================================================
--- work.orig/drivers/ata/pata_hpt37x.c
+++ work/drivers/ata/pata_hpt37x.c
@@ -349,23 +349,13 @@ static u32 hpt37x_find_mode(struct ata_p
 
 static int hpt_dma_blacklisted(const struct ata_device *dev, char *modestr, const char *list[])
 {
-	unsigned char model_num[ATA_ID_PROD_LEN];
-	char *s;
-	unsigned int len;
+	unsigned char model_num[ATA_ID_PROD_LEN + 1];
 	int i = 0;
 
-	ata_id_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
-	s = &model_num[0];
-	len = strnlen(s, sizeof(model_num));
-
-	/* ATAPI specifies that empty space is blank-filled; remove blanks */
-	while ((len > 0) && (s[len - 1] == ' ')) {
-		len--;
-		s[len] = 0;
-	}
+	ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 
-	while(list[i] != NULL) {
-		if (!strncmp(list[i], s, len)) {
+	while (list[i] != NULL) {
+		if (!strcmp(list[i], model_num)) {
 			printk(KERN_WARNING DRV_NAME ": %s is not supported for %s.\n",
 				modestr, list[i]);
 			return 1;

[PATCH] libata: implement HDIO_GET_IDENTITY

[Tejun Heo]

'hdparm -I' doesn't work with ATAPI devices and sg_sat is not widely
spread yet leaving no easy way to access ATAPI IDENTIFY data.
Implement HDIO_GET_IDENTITY such that at least 'hdparm -i' works.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-scsi.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -149,6 +149,45 @@ int ata_std_bios_param(struct scsi_devic
 }
 
 /**
+ *	ata_get_identity - Handler for HDIO_GET_IDENTITY ioctl
+ *	@sdev: SCSI device to get identify data for
+ *	@arg: User buffer area for identify data
+ *
+ *	LOCKING:
+ *	Defined by the SCSI layer.  We don't really care.
+ *
+ *	RETURNS:
+ *	Zero on success, negative errno on error.
+ */
+static int ata_get_identity(struct scsi_device *sdev, void __user *arg)
+{
+	struct ata_port *ap = ata_shost_to_port(sdev->host);
+	struct ata_device *dev = ata_scsi_find_dev(ap, sdev);
+	u16 __user *dst = arg;
+	char buf[40];
+
+	if (!dev)
+		return -ENOMSG;
+
+	if (copy_to_user(dst, dev->id, ATA_ID_WORDS * sizeof(u16)))
+		return -EFAULT;
+
+	ata_id_string(dev->id, buf, ATA_ID_PROD, ATA_ID_PROD_LEN);
+	if (copy_to_user(dst + ATA_ID_PROD, buf, ATA_ID_PROD_LEN))
+		return -EFAULT;
+
+	ata_id_string(dev->id, buf, ATA_ID_FW_REV, ATA_ID_FW_REV_LEN);
+	if (copy_to_user(dst + ATA_ID_FW_REV, buf, ATA_ID_FW_REV_LEN))
+		return -EFAULT;
+
+	ata_id_string(dev->id, buf, ATA_ID_SERNO, ATA_ID_SERNO_LEN);
+	if (copy_to_user(dst + ATA_ID_SERNO, buf, ATA_ID_SERNO_LEN))
+		return -EFAULT;
+
+	return 0;
+}
+
+/**
  *	ata_cmd_ioctl - Handler for HDIO_DRIVE_CMD ioctl
  *	@scsidev: Device to which we are issuing command
  *	@arg: User provided data for issuing command
@@ -159,7 +198,6 @@ int ata_std_bios_param(struct scsi_devic
  *	RETURNS:
  *	Zero on success, negative errno on error.
  */
-
 int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 {
 	int rc = 0;
@@ -320,6 +358,9 @@ int ata_scsi_ioctl(struct scsi_device *s
 			return -EINVAL;
 		return 0;
 
+	case HDIO_GET_IDENTITY:
+		return ata_get_identity(scsidev, arg);
+
 	case HDIO_DRIVE_CMD:
 		if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
 			return -EACCES;

Re: [PATCH *3/3*] libata: implement HDIO_GET_IDENTITY

[Tejun Heo]

The last one was 3/3 of course.

-- 
tejun

Re: [PATCH] libata: implement HDIO_GET_IDENTITY

[Mark Lord]

Tejun Heo wrote:
> 'hdparm -I' doesn't work with ATAPI devices and sg_sat is not widely
> spread yet leaving no easy way to access ATAPI IDENTIFY data.
> Implement HDIO_GET_IDENTITY such that at least 'hdparm -i' works.

Mmm.. I still think this old ioctl is ugly, and I'd rather
have things fixed so that "hdparm -I" works instead.

Unless having access to the libata cached ID block is useful
beyond just getting ATAPI drives to work with -I.
Is it?  I suppose it might be useful to be able to look at it.

hdparm *does* try to issue the PACKET IDENTIFY whenever
a regular IDENTIFY fails.  Currently these all go through
the HDIO_DRIVE_CMD ioctl().  I don't have a system set up
here that has any ATAPI over libata to test with.
Could you perhaps try and see where that ioctl() is failing?

Thanks

Re: [PATCH] libata: implement HDIO_GET_IDENTITY

[Tejun Heo]

Mark Lord wrote:
> Tejun Heo wrote:
>> 'hdparm -I' doesn't work with ATAPI devices and sg_sat is not widely
>> spread yet leaving no easy way to access ATAPI IDENTIFY data.
>> Implement HDIO_GET_IDENTITY such that at least 'hdparm -i' works.
> 
> Mmm.. I still think this old ioctl is ugly, and I'd rather
> have things fixed so that "hdparm -I" works instead.

Me agrees.  Please drop HDIO_GET_IDENTITY patch.

> Unless having access to the libata cached ID block is useful
> beyond just getting ATAPI drives to work with -I.
> Is it?  I suppose it might be useful to be able to look at it.

No, it's just to allow access to ATAPI IDENTIFY block.  libata does
pretty good job of keeping ID block in sync via revalidating, so they
should be the same.

> hdparm *does* try to issue the PACKET IDENTIFY whenever
> a regular IDENTIFY fails.  Currently these all go through
> the HDIO_DRIVE_CMD ioctl().  I don't have a system set up
> here that has any ATAPI over libata to test with.
> Could you perhaps try and see where that ioctl() is failing?

Ah.. Okay, so that's the second HDIO_DRIVE_CMD.  I'll test why the
second one is failing tomorrow.

-- 
tejun

Re: [PATCH] libata: implement HDIO_GET_IDENTITY

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
..
>> hdparm *does* try to issue the PACKET IDENTIFY whenever
>> a regular IDENTIFY fails.  Currently these all go through
>> the HDIO_DRIVE_CMD ioctl().  I don't have a system set up
>> here that has any ATAPI over libata to test with.
>> Could you perhaps try and see where that ioctl() is failing?
> 
> Ah.. Okay, so that's the second HDIO_DRIVE_CMD.  I'll test why the
> second one is failing tomorrow.
> 

Okay, I suspect it's failing because libata-scsi *always*
forces passthru commands to go via atapi_xlat, even when
the command itself is not a PACKET command.  Doh!

I'll see if there's a sensible easy fix for it, and maybe post
a patch here unless you beat me to it.

Cheers

Re: [PATCH] libata: implement HDIO_GET_IDENTITY

[Mark Lord]

(pardon the duplicate posting -- just trying to get it into the correct thread).

Tejun Heo wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
..
>>> Support for IDENTIFY PACKET DEVICE would be nice too.
>> It already does that, using HDIO_DRIVE_CMD to retrieve it
>> in the same way as for regular IDENTIFY DEVICE commands.
>
> Hmmm... My hdparm doesn't seem to do that.
>
> # hdparm -V
> hdparm v6.9
> # hdparm -I /dev/sr0
>
> /dev/sr0:
>  HDIO_DRIVE_CMD(identify) failed: Input/output error
>
> Am I missing something?

Okay, I've dug through the code and found bugs in SCSI and libata.
After fixing those, "hdparm -I" now works unmodified for libata ATAPI.

I'll post fixes shortly, after some more testing.

Cheers

Re: [PATCH] libata: implement HDIO_GET_IDENTITY

[Jeff Garzik]

Tejun Heo wrote:
> 'hdparm -I' doesn't work with ATAPI devices and sg_sat is not widely
> spread yet leaving no easy way to access ATAPI IDENTIFY data.
> Implement HDIO_GET_IDENTITY such that at least 'hdparm -i' works.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Are you certain this works for both big-endian and little-endian?

A quick "it works" test by somebody on sata_svw (ppc64) or another 
big-endian platform would be nice.

	Jeff

Re: [PATCH] libata: implement HDIO_GET_IDENTITY

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> 'hdparm -I' doesn't work with ATAPI devices and sg_sat is not widely
>> spread yet leaving no easy way to access ATAPI IDENTIFY data.
>> Implement HDIO_GET_IDENTITY such that at least 'hdparm -i' works.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> Are you certain this works for both big-endian and little-endian?
> 
> A quick "it works" test by somebody on sata_svw (ppc64) or another
> big-endian platform would be nice.

I think it does.  What the code does is identical to what drivers/ide
does.  Anyways, it doesn't really matter, I think we should drop this
patch (3/3 only, that is).  The reason why I implemented this was to get
ATAPI ID data using 'hdparm -i' but as Mark Lord pointed out, 'hdparm
-I' can do IDENTIFY PACKET DEVICE if the kernel is fixed.  I think
that's the way to go.

Thanks.

-- 
tejun

Re: [PATCH] libata: implement HDIO_GET_IDENTITY

[Jeff Garzik]

Tejun Heo wrote:
> 'hdparm -I' doesn't work with ATAPI devices and sg_sat is not widely
> spread yet leaving no easy way to access ATAPI IDENTIFY data.
> Implement HDIO_GET_IDENTITY such that at least 'hdparm -i' works.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied

Re: [PATCH] libata: implement HDIO_GET_IDENTITY

[Mark Lord]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> 'hdparm -I' doesn't work with ATAPI devices and sg_sat is not widely
>> spread yet leaving no easy way to access ATAPI IDENTIFY data.
>> Implement HDIO_GET_IDENTITY such that at least 'hdparm -i' works.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> applied

This patch is not necessary -- The SG_IO/ATA16 support patches replace it.

I believe Tejun and I were in agreement that proper
ATA16 support for ATAPI was the better way to go.

I'll regenerate that one last (boot/modparm) patch
that failed on upstream for you -- later this week.

Cheers

Re: [PATCH] libata: implement HDIO_GET_IDENTITY

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> 'hdparm -I' doesn't work with ATAPI devices and sg_sat is not widely
>>> spread yet leaving no easy way to access ATAPI IDENTIFY data.
>>> Implement HDIO_GET_IDENTITY such that at least 'hdparm -i' works.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> applied
> 
> This patch is not necessary -- The SG_IO/ATA16 support patches replace it.
> 
> I believe Tejun and I were in agreement that proper
> ATA16 support for ATAPI was the better way to go.

I like both approaches.  I think direct support of HDIO_GET_IDENTITY can 
reduce the number of failure points, while making old tools work a bit 
better.


> I'll regenerate that one last (boot/modparm) patch
> that failed on upstream for you -- later this week.

Thanks,

	Jeff

Re: [PATCH 1/3] libata: straighten out ATA_ID_* constants

[Jeff Garzik]

Tejun Heo wrote:
> * Kill _OFS suffixes in ATA_ID_{SERNO|FW_REV|PROD}_OFS for consistency
>   with other ATA_ID_* constants.
> 
> * Kill ATA_SERNO_LEN
> 
> * Add and use ATA_ID_SERNO_LEN, ATA_ID_FW_REV_LEN and ATA_ID_PROD_LEN.
>   This change also makes ata_device_blacklisted() use proper length
>   for fwrev.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied patches 1-2 of 3.

patch #3 dropped by your request