[PATCHSET] libata: reorganize ata_dev_identify(), take 3

[PATCHSET] libata: reorganize ata_dev_identify(), take 3

[Tejun Heo]

Hello, Jeff.

This is the third take of reorganize-ata_dev_identify patchset.  In
the last take[1], two patches (%1 and %2) made into #upstream out of
six patches.  %2 contained an illegal kfree in ata_dev_read_id() which
is left over of patch reordering.  The bug was spotted by Randy Dunlap
and patch was submitted[2].

This patchset contains the remaining four patches regenerated against
the current #upstream[3] + fix-illegal-kfree patch[2].

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/8176
[2] http://article.gmane.org/gmane.linux.ide/8324
[3] cccc65a3b60edaf721cdee5a14f68ba009341822

[PATCH 1/4] libata: convert dev->id to pointer

[Tejun Heo]

Convert dev->id from array to pointer.  This is to accomodate
revalidation.  During revalidation, both old and new IDENTIFY pages
should be accessible and single ->id array doesn't cut it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   20 +++++++++++++++++---
 include/linux/libata.h     |    2 +-
 2 files changed, 18 insertions(+), 4 deletions(-)

6ae458d0e6b0f04d5092e79a8e45a4c187037d27
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 436520e..a630cda 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -907,7 +907,7 @@ unsigned int ata_pio_need_iordy(const st
  *	@dev: target device
  *	@p_class: pointer to class of the target device (may be changed)
  *	@post_reset: is this read ID post-reset?
- *	@id: buffer to fill IDENTIFY page into
+ *	@p_id: read IDENTIFY page (newly allocated)
  *
  *	Read ID data from the specified device.  ATA_CMD_ID_ATA is
  *	performed on ATA devices and ATA_CMD_ID_ATAPI on ATAPI
@@ -922,12 +922,13 @@ unsigned int ata_pio_need_iordy(const st
  *	0 on success, -errno otherwise.
  */
 static int ata_dev_read_id(struct ata_port *ap, struct ata_device *dev,
-			   unsigned int *p_class, int post_reset, u16 *id)
+			   unsigned int *p_class, int post_reset, u16 **p_id)
 {
 	unsigned int class = *p_class;
 	unsigned int using_edd;
 	struct ata_taskfile tf;
 	unsigned int err_mask = 0;
+	u16 *id;
 	const char *reason;
 	int rc;
 
@@ -941,6 +942,13 @@ static int ata_dev_read_id(struct ata_po
 
 	ata_dev_select(ap, dev->devno, 1, 1); /* select device 0/1 */
 
+	id = kmalloc(sizeof(id[0]) * ATA_ID_WORDS, GFP_KERNEL);
+	if (id == NULL) {
+		rc = -ENOMEM;
+		reason = "out of memory";
+		goto err_out;
+	}
+
  retry:
 	ata_tf_init(ap, &tf, dev->devno);
 
@@ -1031,11 +1039,13 @@ static int ata_dev_read_id(struct ata_po
 	}
 
 	*p_class = class;
+	*p_id = id;
 	return 0;
 
  err_out:
 	printk(KERN_WARNING "ata%u: dev %u failed to IDENTIFY (%s)\n",
 	       ap->id, dev->devno, reason);
+	kfree(id);
 	return rc;
 }
 
@@ -1075,7 +1085,8 @@ static void ata_dev_identify(struct ata_
 
 	DPRINTK("ENTER, host %u, dev %u\n", ap->id, device);
 
-	rc = ata_dev_read_id(ap, dev, &dev->class, 1, dev->id);
+	WARN_ON(dev->id != NULL);
+	rc = ata_dev_read_id(ap, dev, &dev->class, 1, &dev->id);
 	if (rc)
 		goto err_out;
 
@@ -4728,11 +4739,14 @@ void ata_host_set_remove(struct ata_host
 int ata_scsi_release(struct Scsi_Host *host)
 {
 	struct ata_port *ap = (struct ata_port *) &host->hostdata[0];
+	int i;
 
 	DPRINTK("ENTER\n");
 
 	ap->ops->port_disable(ap);
 	ata_host_remove(ap, 0);
+	for (i = 0; i < ATA_MAX_DEVICES; i++)
+		kfree(ap->device[i].id);
 
 	DPRINTK("EXIT\n");
 	return 1;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 66b6847..68666b9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -339,7 +339,7 @@ struct ata_device {
 	unsigned long		flags;		/* ATA_DFLAG_xxx */
 	unsigned int		class;		/* ATA_DEV_xxx */
 	unsigned int		devno;		/* 0 or 1 */
-	u16			id[ATA_ID_WORDS]; /* IDENTIFY xxx DEVICE data */
+	u16			*id;		/* IDENTIFY xxx DEVICE data */
 	u8			pio_mode;
 	u8			dma_mode;
 	u8			xfer_mode;
-- 
1.2.1

[PATCH 2/4] libata: separate out ata_dev_configure()

[Tejun Heo]

Separate out ata_dev_configure() from ata_dev_identify() such that
ata_dev_configure() only configures @dev according to passed in @id.
The function now does not disable device on failure, it just returns
appropirate error code.

As this change leaves ata_dev_identify() with only reading ID, calling
configure and disabling devices according to the results, this patch
also kills ata_dev_identify() and inlines the logic into
ata_bus_probe().

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   78 ++++++++++++++++++++++----------------------
 1 files changed, 39 insertions(+), 39 deletions(-)

9e8f99c18ddb9615403cdeca438a2d3c26b012fb
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index a630cda..dcc6b06 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1050,45 +1050,31 @@ static int ata_dev_read_id(struct ata_po
 }
 
 /**
- *	ata_dev_identify - obtain IDENTIFY x DEVICE page
- *	@ap: port on which device we wish to probe resides
- *	@device: device bus address, starting at zero
+ *	ata_dev_configure - Configure the specified ATA/ATAPI device
+ *	@ap: Port on which target device resides
+ *	@dev: Target device to configure
  *
- *	Following bus reset, we issue the IDENTIFY [PACKET] DEVICE
- *	command, and read back the 512-byte device information page.
- *	The device information page is fed to us via the standard
- *	PIO-IN protocol, but we hand-code it here. (TODO: investigate
- *	using standard PIO-IN paths)
- *
- *	After reading the device information page, we use several
- *	bits of information from it to initialize data structures
- *	that will be used during the lifetime of the ata_device.
- *	Other data from the info page is used to disqualify certain
- *	older ATA devices we do not wish to support.
+ *	Configure @dev according to @dev->id.  Generic and low-level
+ *	driver specific fixups are also applied.
  *
  *	LOCKING:
- *	Inherited from caller.  Some functions called by this function
- *	obtain the host_set lock.
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise
  */
-
-static void ata_dev_identify(struct ata_port *ap, unsigned int device)
+static int ata_dev_configure(struct ata_port *ap, struct ata_device *dev)
 {
-	struct ata_device *dev = &ap->device[device];
 	unsigned long xfer_modes;
 	int i, rc;
 
 	if (!ata_dev_present(dev)) {
 		DPRINTK("ENTER/EXIT (host %u, dev %u) -- nodev\n",
-			ap->id, device);
-		return;
+			ap->id, dev->devno);
+		return 0;
 	}
 
-	DPRINTK("ENTER, host %u, dev %u\n", ap->id, device);
-
-	WARN_ON(dev->id != NULL);
-	rc = ata_dev_read_id(ap, dev, &dev->class, 1, &dev->id);
-	if (rc)
-		goto err_out;
+	DPRINTK("ENTER, host %u, dev %u\n", ap->id, dev->devno);
 
 	/*
 	 * common ATA, ATAPI feature tests
@@ -1097,6 +1083,7 @@ static void ata_dev_identify(struct ata_
 	/* we require DMA support (bits 8 of word 49) */
 	if (!ata_id_has_dma(dev->id)) {
 		printk(KERN_DEBUG "ata%u: no dma\n", ap->id);
+		rc = -EINVAL;
 		goto err_out_nosup;
 	}
 
@@ -1121,12 +1108,12 @@ static void ata_dev_identify(struct ata_
 
 			/* print device info to dmesg */
 			printk(KERN_INFO "ata%u: dev %u ATA-%d, max %s, %Lu sectors:%s\n",
-			       ap->id, device,
+			       ap->id, dev->devno,
 			       ata_id_major_version(dev->id),
 			       ata_mode_string(xfer_modes),
 			       (unsigned long long)dev->n_sectors,
 			       dev->flags & ATA_DFLAG_LBA48 ? " LBA48" : " LBA");
-		} else { 
+		} else {
 			/* CHS */
 
 			/* Default translation */
@@ -1143,7 +1130,7 @@ static void ata_dev_identify(struct ata_
 
 			/* print device info to dmesg */
 			printk(KERN_INFO "ata%u: dev %u ATA-%d, max %s, %Lu sectors: CHS %d/%d/%d\n",
-			       ap->id, device,
+			       ap->id, dev->devno,
 			       ata_id_major_version(dev->id),
 			       ata_mode_string(xfer_modes),
 			       (unsigned long long)dev->n_sectors,
@@ -1159,13 +1146,14 @@ static void ata_dev_identify(struct ata_
 		rc = atapi_cdb_len(dev->id);
 		if ((rc < 12) || (rc > ATAPI_CDB_LEN)) {
 			printk(KERN_WARNING "ata%u: unsupported CDB len\n", ap->id);
+			rc = -EINVAL;
 			goto err_out_nosup;
 		}
 		dev->cdb_len = (unsigned int) rc;
 
 		/* print device info to dmesg */
 		printk(KERN_INFO "ata%u: dev %u ATAPI, max %s\n",
-		       ap->id, device,
+		       ap->id, dev->devno,
 		       ata_mode_string(xfer_modes));
 	}
 
@@ -1176,14 +1164,13 @@ static void ata_dev_identify(struct ata_
 					      ap->device[i].cdb_len);
 
 	DPRINTK("EXIT, drv_stat = 0x%x\n", ata_chk_status(ap));
-	return;
+	return 0;
 
 err_out_nosup:
 	printk(KERN_WARNING "ata%u: dev %u not supported, ignoring\n",
-	       ap->id, device);
-err_out:
-	dev->class++;	/* converts ATA_DEV_xxx into ATA_DEV_xxx_UNSUP */
+	       ap->id, dev->devno);
 	DPRINTK("EXIT, err\n");
+	return rc;
 }
 
 
@@ -1259,11 +1246,24 @@ static int ata_bus_probe(struct ata_port
 		goto err_out;
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
-		ata_dev_identify(ap, i);
-		if (ata_dev_present(&ap->device[i])) {
-			found = 1;
-			ata_dev_config(ap,i);
+		struct ata_device *dev = &ap->device[i];
+
+		if (!ata_dev_present(dev))
+			continue;
+
+		WARN_ON(dev->id != NULL);
+		if (ata_dev_read_id(ap, dev, &dev->class, 1, &dev->id)) {
+			dev->class = ATA_DEV_NONE;
+			continue;
 		}
+
+		if (ata_dev_configure(ap, dev)) {
+			dev->class++;	/* disable device */
+			continue;
+		}
+
+		ata_dev_config(ap, i);
+		found = 1;
 	}
 
 	if ((!found) || (ap->flags & ATA_FLAG_PORT_DISABLED))
-- 
1.2.1

[PATCH 4/4] libata: reorganize ata_bus_probe()

[Tejun Heo]

Now that reset and configure are converted such that they don't modify
or disable libata core data structures, reorganize ata_bus_probe() to
reflect this change.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   46 ++++++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 21 deletions(-)

fd8657c1907afa3fc13b55ef4c274580d413527a
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index f9ab563..63ba1aa 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1207,35 +1207,40 @@ err_out_nosup:
 
 static int ata_bus_probe(struct ata_port *ap)
 {
-	unsigned int i, found = 0;
+	unsigned int classes[ATA_MAX_DEVICES];
+	unsigned int i, rc, found = 0;
 
-	if (ap->ops->probe_reset) {
-		unsigned int classes[ATA_MAX_DEVICES];
-		int rc;
-
-		ata_port_probe(ap);
+	ata_port_probe(ap);
 
+	/* reset */
+	if (ap->ops->probe_reset) {
 		rc = ap->ops->probe_reset(ap, classes);
-		if (rc == 0) {
-			for (i = 0; i < ATA_MAX_DEVICES; i++) {
-				if (classes[i] == ATA_DEV_UNKNOWN)
-					classes[i] = ATA_DEV_NONE;
-				ap->device[i].class = classes[i];
-			}
-		} else {
-			printk(KERN_ERR "ata%u: probe reset failed, "
-			       "disabling port\n", ap->id);
-			ata_port_disable(ap);
+		if (rc) {
+			printk("ata%u: reset failed (errno=%d)\n", ap->id, rc);
+			return rc;
 		}
-	} else
+
+		for (i = 0; i < ATA_MAX_DEVICES; i++)
+			if (classes[i] == ATA_DEV_UNKNOWN)
+				classes[i] = ATA_DEV_NONE;
+	} else {
 		ap->ops->phy_reset(ap);
 
-	if (ap->flags & ATA_FLAG_PORT_DISABLED)
-		goto err_out;
+		for (i = 0; i < ATA_MAX_DEVICES; i++) {
+			if (!(ap->flags & ATA_FLAG_PORT_DISABLED))
+				classes[i] = ap->device[i].class;
+			else
+				ap->device[i].class = ATA_DEV_UNKNOWN;
+		}
+		ata_port_probe(ap);
+	}
 
+	/* read IDENTIFY page and configure devices */
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
 
+		dev->class = classes[i];
+
 		if (!ata_dev_present(dev))
 			continue;
 
@@ -1253,7 +1258,7 @@ static int ata_bus_probe(struct ata_port
 		found = 1;
 	}
 
-	if ((!found) || (ap->flags & ATA_FLAG_PORT_DISABLED))
+	if (!found)
 		goto err_out_disable;
 
 	ata_set_mode(ap);
@@ -1264,7 +1269,6 @@ static int ata_bus_probe(struct ata_port
 
 err_out_disable:
 	ap->ops->port_disable(ap);
-err_out:
 	return -1;
 }
 
-- 
1.2.1

[PATCH 3/4] libata: fold ata_dev_config() into ata_dev_configure()

[Tejun Heo]

ata_dev_config() needs to be done everytime a device is configured.
Fold it into ata_dev_configure().

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   48 ++++++++++++++++----------------------------
 include/linux/libata.h     |    1 -
 2 files changed, 17 insertions(+), 32 deletions(-)

8686b79624ba861834b6648a043c1ca95ea806dd
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index dcc6b06..f9ab563 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1049,6 +1049,12 @@ static int ata_dev_read_id(struct ata_po
 	return rc;
 }
 
+static inline u8 ata_dev_knobble(const struct ata_port *ap,
+				 struct ata_device *dev)
+{
+	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
+}
+
 /**
  *	ata_dev_configure - Configure the specified ATA/ATAPI device
  *	@ap: Port on which target device resides
@@ -1163,6 +1169,17 @@ static int ata_dev_configure(struct ata_
 					      ap->host->max_cmd_len,
 					      ap->device[i].cdb_len);
 
+	/* limit bridge transfers to udma5, 200 sectors */
+	if (ata_dev_knobble(ap, dev)) {
+		printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
+		       ap->id, dev->devno);
+		ap->udma_mask &= ATA_UDMA5;
+		dev->max_sectors = ATA_MAX_SECTORS;
+	}
+
+	if (ap->ops->dev_config)
+		ap->ops->dev_config(ap, dev);
+
 	DPRINTK("EXIT, drv_stat = 0x%x\n", ata_chk_status(ap));
 	return 0;
 
@@ -1173,35 +1190,6 @@ err_out_nosup:
 	return rc;
 }
 
-
-static inline u8 ata_dev_knobble(const struct ata_port *ap,
-				 struct ata_device *dev)
-{
-	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
-}
-
-/**
- * ata_dev_config - Run device specific handlers & check for SATA->PATA bridges
- * @ap: Bus
- * @i:  Device
- *
- * LOCKING:
- */
-
-void ata_dev_config(struct ata_port *ap, unsigned int i)
-{
-	/* limit bridge transfers to udma5, 200 sectors */
-	if (ata_dev_knobble(ap, &ap->device[i])) {
-		printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
-		       ap->id, i);
-		ap->udma_mask &= ATA_UDMA5;
-		ap->device[i].max_sectors = ATA_MAX_SECTORS;
-	}
-
-	if (ap->ops->dev_config)
-		ap->ops->dev_config(ap, &ap->device[i]);
-}
-
 /**
  *	ata_bus_probe - Reset and probe ATA bus
  *	@ap: Bus to probe
@@ -1262,7 +1250,6 @@ static int ata_bus_probe(struct ata_port
 			continue;
 		}
 
-		ata_dev_config(ap, i);
 		found = 1;
 	}
 
@@ -4963,7 +4950,6 @@ EXPORT_SYMBOL_GPL(ata_host_intr);
 EXPORT_SYMBOL_GPL(ata_dev_classify);
 EXPORT_SYMBOL_GPL(ata_id_string);
 EXPORT_SYMBOL_GPL(ata_id_c_string);
-EXPORT_SYMBOL_GPL(ata_dev_config);
 EXPORT_SYMBOL_GPL(ata_scsi_simulate);
 EXPORT_SYMBOL_GPL(ata_eh_qc_complete);
 EXPORT_SYMBOL_GPL(ata_eh_qc_retry);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 68666b9..1aab218 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -542,7 +542,6 @@ extern void ata_id_string(const u16 *id,
 			  unsigned int ofs, unsigned int len);
 extern void ata_id_c_string(const u16 *id, unsigned char *s,
 			    unsigned int ofs, unsigned int len);
-extern void ata_dev_config(struct ata_port *ap, unsigned int i);
 extern void ata_bmdma_setup (struct ata_queued_cmd *qc);
 extern void ata_bmdma_start (struct ata_queued_cmd *qc);
 extern void ata_bmdma_stop(struct ata_queued_cmd *qc);
-- 
1.2.1

Re: [PATCH 1/4] libata: convert dev->id to pointer

[Jeff Garzik]

Tejun Heo wrote:
> Convert dev->id from array to pointer.  This is to accomodate
> revalidation.  During revalidation, both old and new IDENTIFY pages
> should be accessible and single ->id array doesn't cut it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied 1-4