[PATCHSET] libata: reorganize ata_dev_identify()

[PATCHSET] libata: reorganize ata_dev_identify()

[Tejun Heo]

Hello, Jeff.  Hello, Albert.

This patchset is the third part of divided 'reorganize configuration
and implement revalidation' patchset[1].  The first[2] and second[3]
parts made into #upstream lately.

This patchset is against
  the current upstream (254a5c45f8e53775a1e1dd7498a8dcd665341f1e)
  + rename ata_dev_id_[c_]string() patch [4]

Note: Jeff, I generated patches over the rename patch as the patch
      make naming not only shorter but also more consistent.  If you
      don't like the rename, I'll regenerate this patchset without it.

This patchset concentrates on reorganizing ata_dev_identify() into
ata_dev_read_id() and ata_dev_configure() such that those functions
can be also be used for revalidation and other purposes.

Also, both functions don't directly take devices or ports offline.
They just report failures to upper layers and determining whether to
take devices/ports offline is the responsibility of upper layer
driving logic.

Other than failure case handling, reorganized configuration code
should be functionality-wise identical to ata_dev_identify().

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/7632
[2] http://article.gmane.org/gmane.linux.ide/7977
[3] http://article.gmane.org/gmane.linux.ide/7981
[4] http://article.gmane.org/gmane.linux.ide/7997

[PATCH 2/7] 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 |   11 +++++++++--
 include/linux/libata.h     |    2 +-
 2 files changed, 10 insertions(+), 3 deletions(-)

437d923fd2bb8020b4a5cdaa8080bcf9f20698cd
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index f97ead8..a435454 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -953,6 +953,10 @@ static void ata_dev_identify(struct ata_
 
 	ata_dev_select(ap, device, 1, 1); /* select device 0/1 */
 
+	dev->id = kmalloc(sizeof(dev->id[0]) * ATA_ID_WORDS, GFP_KERNEL);
+	if (dev->id == NULL)
+		goto err_out;
+
 retry:
 	ata_tf_init(ap, &tf, device);
 
@@ -967,7 +971,7 @@ retry:
 	tf.protocol = ATA_PROT_PIO;
 
 	err_mask = ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
-				     dev->id, sizeof(dev->id));
+				     dev->id, sizeof(dev->id[0]) * ATA_ID_WORDS);
 
 	if (err_mask) {
 		if (err_mask & ~AC_ERR_DEV)
@@ -2515,7 +2519,7 @@ static void ata_dev_reread_id(struct ata
 	tf.protocol = ATA_PROT_PIO;
 
 	if (ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
-			      dev->id, sizeof(dev->id)))
+			      dev->id, sizeof(dev->id[0]) * ATA_ID_WORDS))
 		goto err_out;
 
 	swap_buf_le16(dev->id, ATA_ID_WORDS);
@@ -4722,11 +4726,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 0d6bf50..1f15666 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.1.5

[PATCH 6/7] 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(-)

74bf76a028e19e725d788e332486ce0b2dfcca36
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index bb575d2..761186d 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
@@ -1261,7 +1249,6 @@ static int ata_bus_probe(struct ata_port
 			continue;
 		}
 
-		ata_dev_config(ap, i);
 		found = 1;
 	}
 
@@ -4959,7 +4946,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 1f15666..e8e02a0 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.1.5

Re: [PATCH 6/7] libata: fold ata_dev_config() into ata_dev_configure()

[Jeff Garzik]

Tejun Heo wrote:
> 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>

ACK

[PATCH 4/7] libata: kill ata_dev_reread_id()

[Tejun Heo]

Kill now-unused ata_dev_reread_id().

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

---

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

4436e50d21e0dcfaa26f54dc798b9d23c4d71d11
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 6de78d1..0119e15 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -61,7 +61,6 @@
 
 #include "libata.h"
 
-static void ata_dev_reread_id(struct ata_port *ap, struct ata_device *dev);
 static unsigned int ata_dev_init_params(struct ata_port *ap,
 					struct ata_device *dev);
 static void ata_set_mode(struct ata_port *ap);
@@ -2545,47 +2544,6 @@ static void ata_dev_set_xfermode(struct 
 }
 
 /**
- *	ata_dev_reread_id - Reread the device identify device info
- *	@ap: port where the device is
- *	@dev: device to reread the identify device info
- *
- *	LOCKING:
- */
-
-static void ata_dev_reread_id(struct ata_port *ap, struct ata_device *dev)
-{
-	struct ata_taskfile tf;
-
-	ata_tf_init(ap, &tf, dev->devno);
-
-	if (dev->class == ATA_DEV_ATA) {
-		tf.command = ATA_CMD_ID_ATA;
-		DPRINTK("do ATA identify\n");
-	} else {
-		tf.command = ATA_CMD_ID_ATAPI;
-		DPRINTK("do ATAPI identify\n");
-	}
-
-	tf.flags |= ATA_TFLAG_DEVICE;
-	tf.protocol = ATA_PROT_PIO;
-
-	if (ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
-			      dev->id, sizeof(dev->id[0]) * ATA_ID_WORDS))
-		goto err_out;
-
-	swap_buf_le16(dev->id, ATA_ID_WORDS);
-
-	ata_dump_id(dev->id);
-
-	DPRINTK("EXIT\n");
-
-	return;
-err_out:
-	printk(KERN_ERR "ata%u: failed to reread ID, disabled\n", ap->id);
-	ata_port_disable(ap);
-}
-
-/**
  *	ata_dev_init_params - Issue INIT DEV PARAMS command
  *	@ap: Port associated with device @dev
  *	@dev: Device to which command will be sent
-- 
1.1.5

Re: [PATCH 4/7] libata: kill ata_dev_reread_id()

[Jeff Garzik]

Tejun Heo wrote:
> Kill now-unused ata_dev_reread_id().
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK

[PATCH 5/7] 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 |   76 ++++++++++++++++++++++----------------------
 1 files changed, 38 insertions(+), 38 deletions(-)

8d98269f37ee0ef92dc4a0822d250acdf75a4cf3
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 0119e15..bb575d2 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1050,44 +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);
-
-	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
@@ -1096,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;
 	}
 
@@ -1120,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 */
@@ -1142,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,
@@ -1158,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));
 	}
 
@@ -1175,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;
 }
 
 
@@ -1258,11 +1246,23 @@ 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;
+
+		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.1.5

Re: [PATCH 5/7] libata: separate out ata_dev_configure()

[Jeff Garzik]

Tejun Heo wrote:
> 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>

ACK

[PATCH 7/7] 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(-)

04a9356ddbbfafba7ec9b6b71db30ccad2190e02
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 761186d..8c0d48f 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;
 
@@ -1252,7 +1257,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);
@@ -1263,7 +1268,6 @@ static int ata_bus_probe(struct ata_port
 
 err_out_disable:
 	ap->ops->port_disable(ap);
-err_out:
 	return -1;
 }
 
-- 
1.1.5

Re: [PATCH 7/7] libata: reorganize ata_bus_probe()

[Jeff Garzik]

Tejun Heo wrote:
> 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(-)
> 
> 04a9356ddbbfafba7ec9b6b71db30ccad2190e02
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index 761186d..8c0d48f 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);
> +	}

the legacy phy_reset code path appears to call ata_port_probe() a second 
time.  otherwise, seems OK.

	Jeff

Re: [PATCH 7/7] libata: reorganize ata_bus_probe()

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> 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>
>>
> 
> the legacy phy_reset code path appears to call ata_port_probe() a second 
> time.  otherwise, seems OK.
> 

Hello, Jeff.

That is intentional. ->phy_reset disables port if something goes wrong 
during reset while the new model just offlines affected devices. The 
code there emulates new behavior by turning off all devices and enabling 
the port if the port gets disabled during ->phy_reset.

-- 
tejun