[PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

There were several places where only enabled devices should be
iterated over but device enabledness wasn't checked.

* IDENTIFY data 40 wire check in cable_is_40wire()
* xfer_mode/ncq_enabled saving in ata_scsi_error()
* DUBIOUS_XFER handling in ata_set_mode()

While at it, reformat comment in cable_is_40wire().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
Jeff, this four patch series fixes iteration bugs and improves
iterators and fix ata_port_detach() related bugs.  The iterator change
is a bit big for #upstream-fixes but it's fairly straight forward and
I verified them in most combinations, so it should be pretty safe.

Thanks.

 drivers/ata/libata-core.c |   18 ++++++++----------
 drivers/ata/libata-eh.c   |    9 +++++++++
 2 files changed, 17 insertions(+), 10 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -4169,18 +4169,16 @@ static int cable_is_40wire(struct ata_po
 	if (ap->cbl == ATA_CBL_PATA40_SHORT)
 		return 0;
 	/* If the controller doesn't know we scan
-
-	   - Note: We look for all 40 wire detects at this point.
-	     Any 80 wire detect is taken to be 80 wire cable
-	     because
-	     - In many setups only the one drive (slave if present)
-               will give a valid detect
-             - If you have a non detect capable drive you don't
-               want it to colour the choice
-        */
+	 *
+	 * - Note: We look for all 40 wire detects at this point.  Any
+	 *   80 wire detect is taken to be 80 wire cable because - In
+	 *   many setups only the one drive (slave if present) will
+	 *   give a valid detect - If you have a non detect capable
+	 *   drive you don't want it to colour the choice
+	 */
 	ata_port_for_each_link(link, ap) {
 		ata_link_for_each_dev(dev, link) {
-			if (!ata_is_40wire(dev))
+			if (ata_dev_enabled(dev) && !ata_is_40wire(dev))
 				return 0;
 		}
 	}
Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -603,6 +603,9 @@ void ata_scsi_error(struct Scsi_Host *ho
 			ata_link_for_each_dev(dev, link) {
 				int devno = dev->devno;
 
+				if (!ata_dev_enabled(dev))
+					continue;
+
 				ehc->saved_xfer_mode[devno] = dev->xfer_mode;
 				if (ata_ncq_enabled(dev))
 					ehc->saved_ncq_enabled |= 1 << devno;
@@ -2790,6 +2793,9 @@ int ata_set_mode(struct ata_link *link,
 
 	/* if data transfer is verified, clear DUBIOUS_XFER on ering top */
 	ata_link_for_each_dev(dev, link) {
+		if (!ata_dev_enabled(dev))
+			continue;
+
 		if (!(dev->flags & ATA_DFLAG_DUBIOUS_XFER)) {
 			struct ata_ering_entry *ent;
 
@@ -2811,6 +2817,9 @@ int ata_set_mode(struct ata_link *link,
 		u8 saved_xfer_mode = ehc->saved_xfer_mode[dev->devno];
 		u8 saved_ncq = !!(ehc->saved_ncq_enabled & (1 << dev->devno));
 
+		if (!ata_dev_enabled(dev))
+			continue;
+
 		if (dev->xfer_mode != saved_xfer_mode ||
 		    ata_ncq_enabled(dev) != saved_ncq)
 			dev->flags |= ATA_DFLAG_DUBIOUS_XFER;

[PATCH #upstream-fixes 2/4] libata: beef up iterators

[Tejun Heo]

There currently are the following loop constructs.

* __ata_port_for_each_link() for all available links
* ata_port_for_each_link() for edge links
* ata_link_for_each_dev() for all devices
* ata_link_for_each_dev_reverse() for all devices in reverse order

Now there's a need for loop construct which is similar to
__ata_port_for_each_link() but iterates over PMP links before the host
link.  Instead of adding another one with long name, do the following
cleanup.

* Implement and export ata_link_next() and ata_dev_next() which take
  @mode parameter and can be used to build custom loop.
* Implement ata_for_each_link() and ata_for_each_dev() which take
  looping mode explicitly.

The following iteration modes are implemented.

* ATA_LITER_EDGE		: loop over edge links
* ATA_LITER_HOST_FIRST		: loop over all links, host link first
* ATA_LITER_PMP_FIRST		: loop over all links, PMP links first

* ATA_DITER_ENABLED		: loop over enabled devices
* ATA_DITER_ENABLED_REVERSE	: loop over enabled devices in reverse order
* ATA_DITER_ALL			: loop over all devices
* ATA_DITER_ALL_REVERSE		: loop over all devices in reverse order

This change removes exlicit device enabledness checks from many loops
and makes it clear which ones are iterated over in which direction.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/ahci.c           |    8 -
 drivers/ata/ata_generic.c    |    5 -
 drivers/ata/libata-acpi.c    |   19 +---
 drivers/ata/libata-core.c    |  183 ++++++++++++++++++++++++++++---------------
 drivers/ata/libata-eh.c      |   84 ++++++++-----------
 drivers/ata/libata-pmp.c     |   22 ++---
 drivers/ata/libata-scsi.c    |   22 ++---
 drivers/ata/pata_it821x.c    |   34 +++----
 drivers/ata/pata_ixp4xx_cf.c |   14 +--
 drivers/ata/pata_legacy.c    |   17 +--
 drivers/ata/pata_pdc2027x.c  |   27 ++----
 drivers/ata/pata_platform.c  |   14 +--
 drivers/ata/pata_rz1000.c    |   16 +--
 drivers/ata/sata_sil.c       |    2 
 include/linux/libata.h       |   50 +++++++----
 15 files changed, 279 insertions(+), 238 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -163,43 +163,119 @@ MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
 
-/*
- * Iterator helpers.  Don't use directly.
+/**
+ *	ata_link_next - link iteration helper
+ *	@link: the previous link, NULL to start
+ *	@ap: ATA port containing links to iterate
+ *	@mode: iteration mode, one of ATA_LITER_*
+ *
+ *	LOCKING:
+ *	Host lock or EH context.
  *
- * LOCKING:
- * Host lock or EH context.
+ *	RETURNS:
+ *	Pointer to the next link.
  */
-struct ata_link *__ata_port_next_link(struct ata_port *ap,
-				      struct ata_link *link, bool dev_only)
+struct ata_link *ata_link_next(struct ata_link *link, struct ata_port *ap,
+			       enum ata_link_iter_mode mode)
 {
+	BUG_ON(mode != ATA_LITER_EDGE &&
+	       mode != ATA_LITER_PMP_FIRST && mode != ATA_LITER_HOST_FIRST);
+
 	/* NULL link indicates start of iteration */
-	if (!link) {
-		if (dev_only && sata_pmp_attached(ap))
-			return ap->pmp_link;
-		return &ap->link;
-	}
+	if (!link)
+		switch (mode) {
+		case ATA_LITER_EDGE:
+		case ATA_LITER_PMP_FIRST:
+			if (sata_pmp_attached(ap))
+				return ap->pmp_link;
+			/* fall through */
+		case ATA_LITER_HOST_FIRST:
+			return &ap->link;
+		}
 
-	/* we just iterated over the host master link, what's next? */
-	if (link == &ap->link) {
-		if (!sata_pmp_attached(ap)) {
-			if (unlikely(ap->slave_link) && !dev_only)
+	/* we just iterated over the host link, what's next? */
+	if (link == &ap->link)
+		switch (mode) {
+		case ATA_LITER_HOST_FIRST:
+			if (sata_pmp_attached(ap))
+				return ap->pmp_link;
+			/* fall through */
+		case ATA_LITER_PMP_FIRST:
+			if (unlikely(ap->slave_link))
 				return ap->slave_link;
+			/* fall through */
+		case ATA_LITER_EDGE:
 			return NULL;
 		}
-		return ap->pmp_link;
-	}
 
 	/* slave_link excludes PMP */
 	if (unlikely(link == ap->slave_link))
 		return NULL;
 
-	/* iterate to the next PMP link */
+	/* we were over a PMP link */
 	if (++link < ap->pmp_link + ap->nr_pmp_links)
 		return link;
+
+	if (mode == ATA_LITER_PMP_FIRST)
+		return &ap->link;
+
 	return NULL;
 }
 
 /**
+ *	ata_dev_next - device iteration helper
+ *	@dev: the previous device, NULL to start
+ *	@link: ATA link containing devices to iterate
+ *	@mode: iteration mode, one of ATA_DITER_*
+ *
+ *	LOCKING:
+ *	Host lock or EH context.
+ *
+ *	RETURNS:
+ *	Pointer to the next device.
+ */
+struct ata_device *ata_dev_next(struct ata_device *dev, struct ata_link *link,
+				enum ata_dev_iter_mode mode)
+{
+	BUG_ON(mode != ATA_DITER_ENABLED && mode != ATA_DITER_ENABLED_REVERSE &&
+	       mode != ATA_DITER_ALL && mode != ATA_DITER_ALL_REVERSE);
+
+	/* NULL dev indicates start of iteration */
+	if (!dev)
+		switch (mode) {
+		case ATA_DITER_ENABLED:
+		case ATA_DITER_ALL:
+			dev = link->device;
+			goto check;
+		case ATA_DITER_ENABLED_REVERSE:
+		case ATA_DITER_ALL_REVERSE:
+			dev = link->device + ata_link_max_devices(link) - 1;
+			goto check;
+		}
+
+ next:
+	/* move to the next one */
+	switch (mode) {
+	case ATA_DITER_ENABLED:
+	case ATA_DITER_ALL:
+		if (++dev < link->device + ata_link_max_devices(link))
+			goto check;
+		return NULL;
+	case ATA_DITER_ENABLED_REVERSE:
+	case ATA_DITER_ALL_REVERSE:
+		if (--dev >= link->device)
+			goto check;
+		return NULL;
+	}
+
+ check:
+	if ((mode == ATA_DITER_ENABLED || mode == ATA_DITER_ENABLED_REVERSE) &&
+	    !ata_dev_enabled(dev))
+		goto next;
+	return dev;
+}
+
+/**
  *	ata_dev_phys_link - find physical link for a device
  *	@dev: ATA device to look up physical link for
  *
@@ -1107,8 +1183,8 @@ static void ata_lpm_enable(struct ata_ho
 
 	for (i = 0; i < host->n_ports; i++) {
 		ap = host->ports[i];
-		ata_port_for_each_link(link, ap) {
-			ata_link_for_each_dev(dev, link)
+		ata_for_each_link(link, ap, EDGE) {
+			ata_for_each_dev(dev, link, ALL)
 				ata_dev_disable_pm(dev);
 		}
 	}
@@ -2584,11 +2660,11 @@ int ata_bus_probe(struct ata_port *ap)
 
 	ata_port_probe(ap);
 
-	ata_link_for_each_dev(dev, &ap->link)
+	ata_for_each_dev(dev, &ap->link, ALL)
 		tries[dev->devno] = ATA_PROBE_MAX_TRIES;
 
  retry:
-	ata_link_for_each_dev(dev, &ap->link) {
+	ata_for_each_dev(dev, &ap->link, ALL) {
 		/* If we issue an SRST then an ATA drive (not ATAPI)
 		 * may change configuration and be in PIO0 timing. If
 		 * we do a hard reset (or are coming from power on)
@@ -2610,7 +2686,7 @@ int ata_bus_probe(struct ata_port *ap)
 	/* reset and determine device classes */
 	ap->ops->phy_reset(ap);
 
-	ata_link_for_each_dev(dev, &ap->link) {
+	ata_for_each_dev(dev, &ap->link, ALL) {
 		if (!(ap->flags & ATA_FLAG_DISABLED) &&
 		    dev->class != ATA_DEV_UNKNOWN)
 			classes[dev->devno] = dev->class;
@@ -2626,7 +2702,7 @@ int ata_bus_probe(struct ata_port *ap)
 	   specific sequence bass-ackwards so that PDIAG- is released by
 	   the slave device */
 
-	ata_link_for_each_dev_reverse(dev, &ap->link) {
+	ata_for_each_dev(dev, &ap->link, ALL_REVERSE) {
 		if (tries[dev->devno])
 			dev->class = classes[dev->devno];
 
@@ -2643,24 +2719,19 @@ int ata_bus_probe(struct ata_port *ap)
 	if (ap->ops->cable_detect)
 		ap->cbl = ap->ops->cable_detect(ap);
 
-	/* We may have SATA bridge glue hiding here irrespective of the
-	   reported cable types and sensed types */
-	ata_link_for_each_dev(dev, &ap->link) {
-		if (!ata_dev_enabled(dev))
-			continue;
-		/* SATA drives indicate we have a bridge. We don't know which
-		   end of the link the bridge is which is a problem */
+	/* We may have SATA bridge glue hiding here irrespective of
+	 * the reported cable types and sensed types.  When SATA
+	 * drives indicate we have a bridge, we don't know which end
+	 * of the link the bridge is which is a problem.
+	 */
+	ata_for_each_dev(dev, &ap->link, ENABLED)
 		if (ata_id_is_sata(dev->id))
 			ap->cbl = ATA_CBL_SATA;
-	}
 
 	/* After the identify sequence we can now set up the devices. We do
 	   this in the normal order so that the user doesn't get confused */
 
-	ata_link_for_each_dev(dev, &ap->link) {
-		if (!ata_dev_enabled(dev))
-			continue;
-
+	ata_for_each_dev(dev, &ap->link, ENABLED) {
 		ap->link.eh_context.i.flags |= ATA_EHI_PRINTINFO;
 		rc = ata_dev_configure(dev);
 		ap->link.eh_context.i.flags &= ~ATA_EHI_PRINTINFO;
@@ -2673,9 +2744,8 @@ int ata_bus_probe(struct ata_port *ap)
 	if (rc)
 		goto fail;
 
-	ata_link_for_each_dev(dev, &ap->link)
-		if (ata_dev_enabled(dev))
-			return 0;
+	ata_for_each_dev(dev, &ap->link, ENABLED)
+		return 0;
 
 	/* no device present, disable port */
 	ata_port_disable(ap);
@@ -3321,13 +3391,10 @@ int ata_do_set_mode(struct ata_link *lin
 	int rc = 0, used_dma = 0, found = 0;
 
 	/* step 1: calculate xfer_mask */
-	ata_link_for_each_dev(dev, link) {
+	ata_for_each_dev(dev, link, ENABLED) {
 		unsigned long pio_mask, dma_mask;
 		unsigned int mode_mask;
 
-		if (!ata_dev_enabled(dev))
-			continue;
-
 		mode_mask = ATA_DMA_MASK_ATA;
 		if (dev->class == ATA_DEV_ATAPI)
 			mode_mask = ATA_DMA_MASK_ATAPI;
@@ -3356,10 +3423,7 @@ int ata_do_set_mode(struct ata_link *lin
 		goto out;
 
 	/* step 2: always set host PIO timings */
-	ata_link_for_each_dev(dev, link) {
-		if (!ata_dev_enabled(dev))
-			continue;
-
+	ata_for_each_dev(dev, link, ENABLED) {
 		if (dev->pio_mode == 0xff) {
 			ata_dev_printk(dev, KERN_WARNING, "no PIO support\n");
 			rc = -EINVAL;
@@ -3373,8 +3437,8 @@ int ata_do_set_mode(struct ata_link *lin
 	}
 
 	/* step 3: set host DMA timings */
-	ata_link_for_each_dev(dev, link) {
-		if (!ata_dev_enabled(dev) || !ata_dma_enabled(dev))
+	ata_for_each_dev(dev, link, ENABLED) {
+		if (!ata_dma_enabled(dev))
 			continue;
 
 		dev->xfer_mode = dev->dma_mode;
@@ -3384,11 +3448,7 @@ int ata_do_set_mode(struct ata_link *lin
 	}
 
 	/* step 4: update devices' xfer mode */
-	ata_link_for_each_dev(dev, link) {
-		/* don't update suspended devices' xfer mode */
-		if (!ata_dev_enabled(dev))
-			continue;
-
+	ata_for_each_dev(dev, link, ENABLED) {
 		rc = ata_dev_set_mode(dev);
 		if (rc)
 			goto out;
@@ -4176,9 +4236,9 @@ static int cable_is_40wire(struct ata_po
 	 *   give a valid detect - If you have a non detect capable
 	 *   drive you don't want it to colour the choice
 	 */
-	ata_port_for_each_link(link, ap) {
-		ata_link_for_each_dev(dev, link) {
-			if (ata_dev_enabled(dev) && !ata_is_40wire(dev))
+	ata_for_each_link(link, ap, EDGE) {
+		ata_for_each_dev(dev, link, ENABLED) {
+			if (!ata_is_40wire(dev))
 				return 0;
 		}
 	}
@@ -5130,7 +5190,7 @@ static int ata_host_request_pm(struct at
 		}
 
 		ap->pflags |= ATA_PFLAG_PM_PENDING;
-		__ata_port_for_each_link(link, ap) {
+		ata_for_each_link(link, ap, HOST_FIRST) {
 			link->eh_info.action |= action;
 			link->eh_info.flags |= ehi_flags;
 		}
@@ -5975,9 +6035,9 @@ static void ata_port_detach(struct ata_p
 	/* EH is now guaranteed to see UNLOADING - EH context belongs
 	 * to us.  Restore SControl and disable all existing devices.
 	 */
-	__ata_port_for_each_link(link, ap) {
+	ata_for_each_link(link, ap, HOST_FIRST) {
 		sata_scr_write(link, SCR_CONTROL, link->saved_scontrol);
-		ata_link_for_each_dev(dev, link)
+		ata_for_each_dev(dev, link, ALL)
 			ata_dev_disable(dev);
 	}
 
@@ -6440,7 +6500,8 @@ EXPORT_SYMBOL_GPL(ata_base_port_ops);
 EXPORT_SYMBOL_GPL(sata_port_ops);
 EXPORT_SYMBOL_GPL(ata_dummy_port_ops);
 EXPORT_SYMBOL_GPL(ata_dummy_port_info);
-EXPORT_SYMBOL_GPL(__ata_port_next_link);
+EXPORT_SYMBOL_GPL(ata_link_next);
+EXPORT_SYMBOL_GPL(ata_dev_next);
 EXPORT_SYMBOL_GPL(ata_std_bios_param);
 EXPORT_SYMBOL_GPL(ata_host_init);
 EXPORT_SYMBOL_GPL(ata_host_alloc);
Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -422,7 +422,7 @@ static void ata_eh_clear_action(struct a
 
 	if (!dev) {
 		ehi->action &= ~action;
-		ata_link_for_each_dev(tdev, link)
+		ata_for_each_dev(tdev, link, ALL)
 			ehi->dev_action[tdev->devno] &= ~action;
 	} else {
 		/* doesn't make sense for port-wide EH actions */
@@ -430,7 +430,7 @@ static void ata_eh_clear_action(struct a
 
 		/* break ehi->action into ehi->dev_action */
 		if (ehi->action & action) {
-			ata_link_for_each_dev(tdev, link)
+			ata_for_each_dev(tdev, link, ALL)
 				ehi->dev_action[tdev->devno] |=
 					ehi->action & action;
 			ehi->action &= ~action;
@@ -592,7 +592,7 @@ void ata_scsi_error(struct Scsi_Host *ho
 		/* fetch & clear EH info */
 		spin_lock_irqsave(ap->lock, flags);
 
-		__ata_port_for_each_link(link, ap) {
+		ata_for_each_link(link, ap, HOST_FIRST) {
 			struct ata_eh_context *ehc = &link->eh_context;
 			struct ata_device *dev;
 
@@ -600,12 +600,9 @@ void ata_scsi_error(struct Scsi_Host *ho
 			link->eh_context.i = link->eh_info;
 			memset(&link->eh_info, 0, sizeof(link->eh_info));
 
-			ata_link_for_each_dev(dev, link) {
+			ata_for_each_dev(dev, link, ENABLED) {
 				int devno = dev->devno;
 
-				if (!ata_dev_enabled(dev))
-					continue;
-
 				ehc->saved_xfer_mode[devno] = dev->xfer_mode;
 				if (ata_ncq_enabled(dev))
 					ehc->saved_ncq_enabled |= 1 << devno;
@@ -647,7 +644,7 @@ void ata_scsi_error(struct Scsi_Host *ho
 		}
 
 		/* this run is complete, make sure EH info is clear */
-		__ata_port_for_each_link(link, ap)
+		ata_for_each_link(link, ap, HOST_FIRST)
 			memset(&link->eh_info, 0, sizeof(link->eh_info));
 
 		/* Clear host_eh_scheduled while holding ap->lock such
@@ -1028,7 +1025,7 @@ int sata_async_notification(struct ata_p
 		struct ata_link *link;
 
 		/* check and notify ATAPI AN */
-		ata_port_for_each_link(link, ap) {
+		ata_for_each_link(link, ap, EDGE) {
 			if (!(sntf & (1 << link->pmp)))
 				continue;
 
@@ -2008,7 +2005,7 @@ void ata_eh_autopsy(struct ata_port *ap)
 {
 	struct ata_link *link;
 
-	ata_port_for_each_link(link, ap)
+	ata_for_each_link(link, ap, EDGE)
 		ata_eh_link_autopsy(link);
 
 	/* Handle the frigging slave link.  Autopsy is done similarly
@@ -2222,7 +2219,7 @@ void ata_eh_report(struct ata_port *ap)
 {
 	struct ata_link *link;
 
-	__ata_port_for_each_link(link, ap)
+	ata_for_each_link(link, ap, HOST_FIRST)
 		ata_eh_link_report(link);
 }
 
@@ -2233,7 +2230,7 @@ static int ata_do_reset(struct ata_link
 	struct ata_device *dev;
 
 	if (clear_classes)
-		ata_link_for_each_dev(dev, link)
+		ata_for_each_dev(dev, link, ALL)
 			classes[dev->devno] = ATA_DEV_UNKNOWN;
 
 	return reset(link, classes, deadline);
@@ -2293,7 +2290,7 @@ int ata_eh_reset(struct ata_link *link,
 	ata_eh_about_to_do(link, NULL, ATA_EH_RESET);
 	ehc->last_reset = jiffies;
 
-	ata_link_for_each_dev(dev, link) {
+	ata_for_each_dev(dev, link, ALL) {
 		/* If we issue an SRST then an ATA drive (not ATAPI)
 		 * may change configuration and be in PIO0 timing. If
 		 * we do a hard reset (or are coming from power on)
@@ -2354,7 +2351,7 @@ int ata_eh_reset(struct ata_link *link,
 						"port disabled. ignoring.\n");
 				ehc->i.action &= ~ATA_EH_RESET;
 
-				ata_link_for_each_dev(dev, link)
+				ata_for_each_dev(dev, link, ALL)
 					classes[dev->devno] = ATA_DEV_NONE;
 
 				rc = 0;
@@ -2368,7 +2365,7 @@ int ata_eh_reset(struct ata_link *link,
 		 * bang classes and return.
 		 */
 		if (reset && !(ehc->i.action & ATA_EH_RESET)) {
-			ata_link_for_each_dev(dev, link)
+			ata_for_each_dev(dev, link, ALL)
 				classes[dev->devno] = ATA_DEV_NONE;
 			rc = 0;
 			goto out;
@@ -2453,7 +2450,7 @@ int ata_eh_reset(struct ata_link *link,
 	/*
 	 * Post-reset processing
 	 */
-	ata_link_for_each_dev(dev, link) {
+	ata_for_each_dev(dev, link, ALL) {
 		/* After the reset, the device state is PIO 0 and the
 		 * controller state is undefined.  Reset also wakes up
 		 * drives from sleeping mode.
@@ -2509,7 +2506,7 @@ int ata_eh_reset(struct ata_link *link,
 	 * can be reliably detected and retried.
 	 */
 	nr_unknown = 0;
-	ata_link_for_each_dev(dev, link) {
+	ata_for_each_dev(dev, link, ALL) {
 		/* convert all ATA_DEV_UNKNOWN to ATA_DEV_NONE */
 		if (classes[dev->devno] == ATA_DEV_UNKNOWN) {
 			classes[dev->devno] = ATA_DEV_NONE;
@@ -2618,8 +2615,8 @@ static inline void ata_eh_pull_park_acti
 
 	spin_lock_irqsave(ap->lock, flags);
 	INIT_COMPLETION(ap->park_req_pending);
-	ata_port_for_each_link(link, ap) {
-		ata_link_for_each_dev(dev, link) {
+	ata_for_each_link(link, ap, EDGE) {
+		ata_for_each_dev(dev, link, ALL) {
 			struct ata_eh_info *ehi = &link->eh_info;
 
 			link->eh_context.i.dev_action[dev->devno] |=
@@ -2674,7 +2671,7 @@ static int ata_eh_revalidate_and_attach(
 	 * be done backwards such that PDIAG- is released by the slave
 	 * device before the master device is identified.
 	 */
-	ata_link_for_each_dev_reverse(dev, link) {
+	ata_for_each_dev(dev, link, ALL_REVERSE) {
 		unsigned int action = ata_eh_dev_action(dev);
 		unsigned int readid_flags = 0;
 
@@ -2743,7 +2740,7 @@ static int ata_eh_revalidate_and_attach(
 	/* Configure new devices forward such that user doesn't see
 	 * device detection messages backwards.
 	 */
-	ata_link_for_each_dev(dev, link) {
+	ata_for_each_dev(dev, link, ALL) {
 		if (!(new_mask & (1 << dev->devno)) ||
 		    dev->class == ATA_DEV_PMP)
 			continue;
@@ -2792,10 +2789,7 @@ int ata_set_mode(struct ata_link *link,
 	int rc;
 
 	/* if data transfer is verified, clear DUBIOUS_XFER on ering top */
-	ata_link_for_each_dev(dev, link) {
-		if (!ata_dev_enabled(dev))
-			continue;
-
+	ata_for_each_dev(dev, link, ENABLED) {
 		if (!(dev->flags & ATA_DFLAG_DUBIOUS_XFER)) {
 			struct ata_ering_entry *ent;
 
@@ -2812,14 +2806,11 @@ int ata_set_mode(struct ata_link *link,
 		rc = ata_do_set_mode(link, r_failed_dev);
 
 	/* if transfer mode has changed, set DUBIOUS_XFER on device */
-	ata_link_for_each_dev(dev, link) {
+	ata_for_each_dev(dev, link, ENABLED) {
 		struct ata_eh_context *ehc = &link->eh_context;
 		u8 saved_xfer_mode = ehc->saved_xfer_mode[dev->devno];
 		u8 saved_ncq = !!(ehc->saved_ncq_enabled & (1 << dev->devno));
 
-		if (!ata_dev_enabled(dev))
-			continue;
-
 		if (dev->xfer_mode != saved_xfer_mode ||
 		    ata_ncq_enabled(dev) != saved_ncq)
 			dev->flags |= ATA_DFLAG_DUBIOUS_XFER;
@@ -2880,9 +2871,8 @@ static int ata_link_nr_enabled(struct at
 	struct ata_device *dev;
 	int cnt = 0;
 
-	ata_link_for_each_dev(dev, link)
-		if (ata_dev_enabled(dev))
-			cnt++;
+	ata_for_each_dev(dev, link, ENABLED)
+		cnt++;
 	return cnt;
 }
 
@@ -2891,7 +2881,7 @@ static int ata_link_nr_vacant(struct ata
 	struct ata_device *dev;
 	int cnt = 0;
 
-	ata_link_for_each_dev(dev, link)
+	ata_for_each_dev(dev, link, ALL)
 		if (dev->class == ATA_DEV_UNKNOWN)
 			cnt++;
 	return cnt;
@@ -2917,7 +2907,7 @@ static int ata_eh_skip_recovery(struct a
 		return 0;
 
 	/* skip if class codes for all vacant slots are ATA_DEV_NONE */
-	ata_link_for_each_dev(dev, link) {
+	ata_for_each_dev(dev, link, ALL) {
 		if (dev->class == ATA_DEV_UNKNOWN &&
 		    ehc->classes[dev->devno] != ATA_DEV_NONE)
 			return 0;
@@ -3025,7 +3015,7 @@ int ata_eh_recover(struct ata_port *ap,
 	DPRINTK("ENTER\n");
 
 	/* prep for recovery */
-	ata_port_for_each_link(link, ap) {
+	ata_for_each_link(link, ap, EDGE) {
 		struct ata_eh_context *ehc = &link->eh_context;
 
 		/* re-enable link? */
@@ -3037,7 +3027,7 @@ int ata_eh_recover(struct ata_port *ap,
 			ata_eh_done(link, NULL, ATA_EH_ENABLE_LINK);
 		}
 
-		ata_link_for_each_dev(dev, link) {
+		ata_for_each_dev(dev, link, ALL) {
 			if (link->flags & ATA_LFLAG_NO_RETRY)
 				ehc->tries[dev->devno] = 1;
 			else
@@ -3067,19 +3057,19 @@ int ata_eh_recover(struct ata_port *ap,
 		goto out;
 
 	/* prep for EH */
-	ata_port_for_each_link(link, ap) {
+	ata_for_each_link(link, ap, EDGE) {
 		struct ata_eh_context *ehc = &link->eh_context;
 
 		/* skip EH if possible. */
 		if (ata_eh_skip_recovery(link))
 			ehc->i.action = 0;
 
-		ata_link_for_each_dev(dev, link)
+		ata_for_each_dev(dev, link, ALL)
 			ehc->classes[dev->devno] = ATA_DEV_UNKNOWN;
 	}
 
 	/* reset */
-	ata_port_for_each_link(link, ap) {
+	ata_for_each_link(link, ap, EDGE) {
 		struct ata_eh_context *ehc = &link->eh_context;
 
 		if (!(ehc->i.action & ATA_EH_RESET))
@@ -3104,8 +3094,8 @@ int ata_eh_recover(struct ata_port *ap,
 		ata_eh_pull_park_action(ap);
 
 		deadline = jiffies;
-		ata_port_for_each_link(link, ap) {
-			ata_link_for_each_dev(dev, link) {
+		ata_for_each_link(link, ap, EDGE) {
+			ata_for_each_dev(dev, link, ALL) {
 				struct ata_eh_context *ehc = &link->eh_context;
 				unsigned long tmp;
 
@@ -3133,8 +3123,8 @@ int ata_eh_recover(struct ata_port *ap,
 		deadline = wait_for_completion_timeout(&ap->park_req_pending,
 						       deadline - now);
 	} while (deadline);
-	ata_port_for_each_link(link, ap) {
-		ata_link_for_each_dev(dev, link) {
+	ata_for_each_link(link, ap, EDGE) {
+		ata_for_each_dev(dev, link, ALL) {
 			if (!(link->eh_context.unloaded_mask &
 			      (1 << dev->devno)))
 				continue;
@@ -3145,7 +3135,7 @@ int ata_eh_recover(struct ata_port *ap,
 	}
 
 	/* the rest */
-	ata_port_for_each_link(link, ap) {
+	ata_for_each_link(link, ap, EDGE) {
 		struct ata_eh_context *ehc = &link->eh_context;
 
 		/* revalidate existing devices and attach new ones */
@@ -3171,7 +3161,7 @@ int ata_eh_recover(struct ata_port *ap,
 		 * disrupting the current users of the device.
 		 */
 		if (ehc->i.flags & ATA_EHI_DID_RESET) {
-			ata_link_for_each_dev(dev, link) {
+			ata_for_each_dev(dev, link, ALL) {
 				if (dev->class != ATA_DEV_ATAPI)
 					continue;
 				rc = atapi_eh_clear_ua(dev);
@@ -3182,7 +3172,7 @@ int ata_eh_recover(struct ata_port *ap,
 
 		/* configure link power saving */
 		if (ehc->i.action & ATA_EH_LPM)
-			ata_link_for_each_dev(dev, link)
+			ata_for_each_dev(dev, link, ALL)
 				ata_dev_enable_pm(dev, ap->pm_policy);
 
 		/* this link is okay now */
@@ -3287,7 +3277,7 @@ void ata_do_eh(struct ata_port *ap, ata_
 	rc = ata_eh_recover(ap, prereset, softreset, hardreset, postreset,
 			    NULL);
 	if (rc) {
-		ata_link_for_each_dev(dev, &ap->link)
+		ata_for_each_dev(dev, &ap->link, ALL)
 			ata_dev_disable(dev);
 	}
 
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -1281,26 +1281,36 @@ static inline int ata_link_active(struct
 	return ata_tag_valid(link->active_tag) || link->sactive;
 }
 
-extern struct ata_link *__ata_port_next_link(struct ata_port *ap,
-					     struct ata_link *link,
-					     bool dev_only);
-
-#define __ata_port_for_each_link(link, ap) \
-	for ((link) = __ata_port_next_link((ap), NULL, false); (link); \
-	     (link) = __ata_port_next_link((ap), (link), false))
-
-#define ata_port_for_each_link(link, ap) \
-	for ((link) = __ata_port_next_link((ap), NULL, true); (link); \
-	     (link) = __ata_port_next_link((ap), (link), true))
-
-#define ata_link_for_each_dev(dev, link) \
-	for ((dev) = (link)->device; \
-	     (dev) < (link)->device + ata_link_max_devices(link) || ((dev) = NULL); \
-	     (dev)++)
-
-#define ata_link_for_each_dev_reverse(dev, link) \
-	for ((dev) = (link)->device + ata_link_max_devices(link) - 1; \
-	     (dev) >= (link)->device || ((dev) = NULL); (dev)--)
+enum ata_link_iter_mode {
+	ATA_LITER_EDGE,		/* if present, PMP links only; otherwise,
+				 * host link.  no slave link */
+	ATA_LITER_HOST_FIRST,	/* host link followed by PMP or slave links */
+	ATA_LITER_PMP_FIRST,	/* PMP links followed by host link,
+				 * slave link still comes after host link */
+};
+
+enum ata_dev_iter_mode {
+	ATA_DITER_ENABLED,
+	ATA_DITER_ENABLED_REVERSE,
+	ATA_DITER_ALL,
+	ATA_DITER_ALL_REVERSE,
+};
+
+extern struct ata_link *ata_link_next(struct ata_link *link,
+				      struct ata_port *ap,
+				      enum ata_link_iter_mode mode);
+
+extern struct ata_device *ata_dev_next(struct ata_device *dev,
+				       struct ata_link *link,
+				       enum ata_dev_iter_mode mode);
+
+#define ata_for_each_link(link, ap, mode) \
+	for ((link) = ata_link_next(NULL, (ap), ATA_LITER_##mode); (link); \
+	     (link) = ata_link_next((link), (ap), ATA_LITER_##mode))
+
+#define ata_for_each_dev(dev, link, mode) \
+	for ((dev) = ata_dev_next(NULL, (link), ATA_DITER_##mode); (dev); \
+	     (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))
 
 /**
  *	ata_ncq_enabled - Test whether NCQ is enabled
Index: work/drivers/ata/ahci.c
===================================================================
--- work.orig/drivers/ata/ahci.c
+++ work/drivers/ata/ahci.c
@@ -1105,14 +1105,14 @@ static void ahci_start_port(struct ata_p
 
 	/* turn on LEDs */
 	if (ap->flags & ATA_FLAG_EM) {
-		ata_port_for_each_link(link, ap) {
+		ata_for_each_link(link, ap, EDGE) {
 			emp = &pp->em_priv[link->pmp];
 			ahci_transmit_led_message(ap, emp->led_state, 4);
 		}
 	}
 
 	if (ap->flags & ATA_FLAG_SW_ACTIVITY)
-		ata_port_for_each_link(link, ap)
+		ata_for_each_link(link, ap, EDGE)
 			ahci_init_sw_activity(link);
 
 }
@@ -1344,7 +1344,7 @@ static ssize_t ahci_led_show(struct ata_
 	struct ahci_em_priv *emp;
 	int rc = 0;
 
-	ata_port_for_each_link(link, ap) {
+	ata_for_each_link(link, ap, EDGE) {
 		emp = &pp->em_priv[link->pmp];
 		rc += sprintf(buf, "%lx\n", emp->led_state);
 	}
@@ -1924,7 +1924,7 @@ static void ahci_error_intr(struct ata_p
 	u32 serror;
 
 	/* determine active link */
-	ata_port_for_each_link(link, ap)
+	ata_for_each_link(link, ap, EDGE)
 		if (ata_link_active(link))
 			break;
 	if (!link)
Index: work/drivers/ata/ata_generic.c
===================================================================
--- work.orig/drivers/ata/ata_generic.c
+++ work/drivers/ata/ata_generic.c
@@ -57,10 +57,7 @@ static int generic_set_mode(struct ata_l
 	if (pdev->vendor == PCI_VENDOR_ID_CENATEK)
 		dma_enabled = 0xFF;
 
-	ata_link_for_each_dev(dev, link) {
-		if (!ata_dev_enabled(dev))
-			continue;
-
+	ata_for_each_dev(dev, link, ENABLED) {
 		/* We don't really care */
 		dev->pio_mode = XFER_PIO_0;
 		dev->dma_mode = XFER_MW_DMA_0;
Index: work/drivers/ata/libata-acpi.c
===================================================================
--- work.orig/drivers/ata/libata-acpi.c
+++ work/drivers/ata/libata-acpi.c
@@ -89,7 +89,7 @@ void ata_acpi_associate_sata_port(struct
 
 		ap->link.device->acpi_handle = NULL;
 
-		ata_port_for_each_link(link, ap) {
+		ata_for_each_link(link, ap, EDGE) {
 			acpi_integer adr = SATA_ADR(ap->port_no, link->pmp);
 
 			link->device->acpi_handle =
@@ -144,8 +144,8 @@ static void ata_acpi_detach_device(struc
 		struct ata_link *tlink;
 		struct ata_device *tdev;
 
-		ata_port_for_each_link(tlink, ap)
-			ata_link_for_each_dev(tdev, tlink)
+		ata_for_each_link(tlink, ap, EDGE)
+			ata_for_each_dev(tdev, tlink, ALL)
 				tdev->flags |= ATA_DFLAG_DETACH;
 	}
 
@@ -627,12 +627,9 @@ int ata_acpi_cbl_80wire(struct ata_port
 {
 	struct ata_device *dev;
 
-	ata_link_for_each_dev(dev, &ap->link) {
+	ata_for_each_dev(dev, &ap->link, ENABLED) {
 		unsigned long xfer_mask, udma_mask;
 
-		if (!ata_dev_enabled(dev))
-			continue;
-
 		xfer_mask = ata_acpi_gtm_xfermask(dev, gtm);
 		ata_unpack_xfermask(xfer_mask, NULL, NULL, &udma_mask);
 
@@ -932,7 +929,7 @@ void ata_acpi_on_resume(struct ata_port
 		 * use values set by _STM.  Cache _GTF result and
 		 * schedule _GTF.
 		 */
-		ata_link_for_each_dev(dev, &ap->link) {
+		ata_for_each_dev(dev, &ap->link, ALL) {
 			ata_acpi_clear_gtf(dev);
 			if (ata_dev_enabled(dev) &&
 			    ata_dev_get_GTF(dev, NULL) >= 0)
@@ -943,7 +940,7 @@ void ata_acpi_on_resume(struct ata_port
 		 * there's no reason to evaluate IDE _GTF early
 		 * without _STM.  Clear cache and schedule _GTF.
 		 */
-		ata_link_for_each_dev(dev, &ap->link) {
+		ata_for_each_dev(dev, &ap->link, ALL) {
 			ata_acpi_clear_gtf(dev);
 			if (ata_dev_enabled(dev))
 				dev->flags |= ATA_DFLAG_ACPI_PENDING;
@@ -971,8 +968,8 @@ void ata_acpi_set_state(struct ata_port
 	if (state.event == PM_EVENT_ON)
 		acpi_bus_set_power(ap->acpi_handle, ACPI_STATE_D0);
 
-	ata_link_for_each_dev(dev, &ap->link) {
-		if (dev->acpi_handle && ata_dev_enabled(dev))
+	ata_for_each_dev(dev, &ap->link, ENABLED) {
+		if (dev->acpi_handle)
 			acpi_bus_set_power(dev->acpi_handle,
 				state.event == PM_EVENT_ON ?
 					ACPI_STATE_D0 : ACPI_STATE_D3);
Index: work/drivers/ata/libata-pmp.c
===================================================================
--- work.orig/drivers/ata/libata-pmp.c
+++ work/drivers/ata/libata-pmp.c
@@ -321,7 +321,7 @@ static void sata_pmp_quirks(struct ata_p
 
 	if (vendor == 0x1095 && devid == 0x3726) {
 		/* sil3726 quirks */
-		ata_port_for_each_link(link, ap) {
+		ata_for_each_link(link, ap, EDGE) {
 			/* Class code report is unreliable and SRST
 			 * times out under certain configurations.
 			 */
@@ -336,7 +336,7 @@ static void sata_pmp_quirks(struct ata_p
 		}
 	} else if (vendor == 0x1095 && devid == 0x4723) {
 		/* sil4723 quirks */
-		ata_port_for_each_link(link, ap) {
+		ata_for_each_link(link, ap, EDGE) {
 			/* class code report is unreliable */
 			if (link->pmp < 2)
 				link->flags |= ATA_LFLAG_ASSUME_ATA;
@@ -348,7 +348,7 @@ static void sata_pmp_quirks(struct ata_p
 		}
 	} else if (vendor == 0x1095 && devid == 0x4726) {
 		/* sil4726 quirks */
-		ata_port_for_each_link(link, ap) {
+		ata_for_each_link(link, ap, EDGE) {
 			/* Class code report is unreliable and SRST
 			 * times out under certain configurations.
 			 * Config device can be at port 0 or 5 and
@@ -450,7 +450,7 @@ int sata_pmp_attach(struct ata_device *d
 	if (ap->ops->pmp_attach)
 		ap->ops->pmp_attach(ap);
 
-	ata_port_for_each_link(tlink, ap)
+	ata_for_each_link(tlink, ap, EDGE)
 		sata_link_init_spd(tlink);
 
 	ata_acpi_associate_sata_port(ap);
@@ -487,7 +487,7 @@ static void sata_pmp_detach(struct ata_d
 	if (ap->ops->pmp_detach)
 		ap->ops->pmp_detach(ap);
 
-	ata_port_for_each_link(tlink, ap)
+	ata_for_each_link(tlink, ap, EDGE)
 		ata_eh_detach_dev(tlink->device);
 
 	spin_lock_irqsave(ap->lock, flags);
@@ -700,7 +700,7 @@ static int sata_pmp_eh_recover_pmp(struc
 		}
 
 		/* PMP is reset, SErrors cannot be trusted, scan all */
-		ata_port_for_each_link(tlink, ap) {
+		ata_for_each_link(tlink, ap, EDGE) {
 			struct ata_eh_context *ehc = &tlink->eh_context;
 
 			ehc->i.probe_mask |= ATA_ALL_DEVICES;
@@ -768,7 +768,7 @@ static int sata_pmp_eh_handle_disabled_l
 
 	spin_lock_irqsave(ap->lock, flags);
 
-	ata_port_for_each_link(link, ap) {
+	ata_for_each_link(link, ap, EDGE) {
 		if (!(link->flags & ATA_LFLAG_DISABLED))
 			continue;
 
@@ -852,7 +852,7 @@ static int sata_pmp_eh_recover(struct at
 	int cnt, rc;
 
 	pmp_tries = ATA_EH_PMP_TRIES;
-	ata_port_for_each_link(link, ap)
+	ata_for_each_link(link, ap, EDGE)
 		link_tries[link->pmp] = ATA_EH_PMP_LINK_TRIES;
 
  retry:
@@ -861,7 +861,7 @@ static int sata_pmp_eh_recover(struct at
 		rc = ata_eh_recover(ap, ops->prereset, ops->softreset,
 				    ops->hardreset, ops->postreset, NULL);
 		if (rc) {
-			ata_link_for_each_dev(dev, &ap->link)
+			ata_for_each_dev(dev, &ap->link, ALL)
 				ata_dev_disable(dev);
 			return rc;
 		}
@@ -870,7 +870,7 @@ static int sata_pmp_eh_recover(struct at
 			return 0;
 
 		/* new PMP online */
-		ata_port_for_each_link(link, ap)
+		ata_for_each_link(link, ap, EDGE)
 			link_tries[link->pmp] = ATA_EH_PMP_LINK_TRIES;
 
 		/* fall through */
@@ -942,7 +942,7 @@ static int sata_pmp_eh_recover(struct at
 	}
 
 	cnt = 0;
-	ata_port_for_each_link(link, ap) {
+	ata_for_each_link(link, ap, EDGE) {
 		if (!(gscr_error & (1 << link->pmp)))
 			continue;
 
Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -3228,12 +3228,12 @@ void ata_scsi_scan_host(struct ata_port
 		return;
 
  repeat:
-	ata_port_for_each_link(link, ap) {
-		ata_link_for_each_dev(dev, link) {
+	ata_for_each_link(link, ap, EDGE) {
+		ata_for_each_dev(dev, link, ENABLED) {
 			struct scsi_device *sdev;
 			int channel = 0, id = 0;
 
-			if (!ata_dev_enabled(dev) || dev->sdev)
+			if (dev->sdev)
 				continue;
 
 			if (ata_is_host_link(link))
@@ -3254,9 +3254,9 @@ void ata_scsi_scan_host(struct ata_port
 	 * failure occurred, scan would have failed silently.  Check
 	 * whether all devices are attached.
 	 */
-	ata_port_for_each_link(link, ap) {
-		ata_link_for_each_dev(dev, link) {
-			if (ata_dev_enabled(dev) && !dev->sdev)
+	ata_for_each_link(link, ap, EDGE) {
+		ata_for_each_dev(dev, link, ENABLED) {
+			if (!dev->sdev)
 				goto exit_loop;
 		}
 	}
@@ -3380,7 +3380,7 @@ static void ata_scsi_handle_link_detach(
 	struct ata_port *ap = link->ap;
 	struct ata_device *dev;
 
-	ata_link_for_each_dev(dev, link) {
+	ata_for_each_dev(dev, link, ALL) {
 		unsigned long flags;
 
 		if (!(dev->flags & ATA_DFLAG_DETACHED))
@@ -3495,7 +3495,7 @@ static int ata_scsi_user_scan(struct Scs
 	if (devno == SCAN_WILD_CARD) {
 		struct ata_link *link;
 
-		ata_port_for_each_link(link, ap) {
+		ata_for_each_link(link, ap, EDGE) {
 			struct ata_eh_info *ehi = &link->eh_info;
 			ehi->probe_mask |= ATA_ALL_DEVICES;
 			ehi->action |= ATA_EH_RESET;
@@ -3543,11 +3543,11 @@ void ata_scsi_dev_rescan(struct work_str
 
 	spin_lock_irqsave(ap->lock, flags);
 
-	ata_port_for_each_link(link, ap) {
-		ata_link_for_each_dev(dev, link) {
+	ata_for_each_link(link, ap, EDGE) {
+		ata_for_each_dev(dev, link, ENABLED) {
 			struct scsi_device *sdev = dev->sdev;
 
-			if (!ata_dev_enabled(dev) || !sdev)
+			if (!sdev)
 				continue;
 			if (scsi_device_get(sdev))
 				continue;
Index: work/drivers/ata/pata_it821x.c
===================================================================
--- work.orig/drivers/ata/pata_it821x.c
+++ work/drivers/ata/pata_it821x.c
@@ -465,24 +465,22 @@ static int it821x_smart_set_mode(struct
 {
 	struct ata_device *dev;
 
-	ata_link_for_each_dev(dev, link) {
-		if (ata_dev_enabled(dev)) {
-			/* We don't really care */
-			dev->pio_mode = XFER_PIO_0;
-			dev->dma_mode = XFER_MW_DMA_0;
-			/* We do need the right mode information for DMA or PIO
-			   and this comes from the current configuration flags */
-			if (ata_id_has_dma(dev->id)) {
-				ata_dev_printk(dev, KERN_INFO, "configured for DMA\n");
-				dev->xfer_mode = XFER_MW_DMA_0;
-				dev->xfer_shift = ATA_SHIFT_MWDMA;
-				dev->flags &= ~ATA_DFLAG_PIO;
-			} else {
-				ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
-				dev->xfer_mode = XFER_PIO_0;
-				dev->xfer_shift = ATA_SHIFT_PIO;
-				dev->flags |= ATA_DFLAG_PIO;
-			}
+	ata_for_each_dev(dev, link, ENABLED) {
+		/* We don't really care */
+		dev->pio_mode = XFER_PIO_0;
+		dev->dma_mode = XFER_MW_DMA_0;
+		/* We do need the right mode information for DMA or PIO
+		   and this comes from the current configuration flags */
+		if (ata_id_has_dma(dev->id)) {
+			ata_dev_printk(dev, KERN_INFO, "configured for DMA\n");
+			dev->xfer_mode = XFER_MW_DMA_0;
+			dev->xfer_shift = ATA_SHIFT_MWDMA;
+			dev->flags &= ~ATA_DFLAG_PIO;
+		} else {
+			ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
+			dev->xfer_mode = XFER_PIO_0;
+			dev->xfer_shift = ATA_SHIFT_PIO;
+			dev->flags |= ATA_DFLAG_PIO;
 		}
 	}
 	return 0;
Index: work/drivers/ata/pata_ixp4xx_cf.c
===================================================================
--- work.orig/drivers/ata/pata_ixp4xx_cf.c
+++ work/drivers/ata/pata_ixp4xx_cf.c
@@ -30,14 +30,12 @@ static int ixp4xx_set_mode(struct ata_li
 {
 	struct ata_device *dev;
 
-	ata_link_for_each_dev(dev, link) {
-		if (ata_dev_enabled(dev)) {
-			ata_dev_printk(dev, KERN_INFO, "configured for PIO0\n");
-			dev->pio_mode = XFER_PIO_0;
-			dev->xfer_mode = XFER_PIO_0;
-			dev->xfer_shift = ATA_SHIFT_PIO;
-			dev->flags |= ATA_DFLAG_PIO;
-		}
+	ata_for_each_dev(dev, link, ENABLED) {
+		ata_dev_printk(dev, KERN_INFO, "configured for PIO0\n");
+		dev->pio_mode = XFER_PIO_0;
+		dev->xfer_mode = XFER_PIO_0;
+		dev->xfer_shift = ATA_SHIFT_PIO;
+		dev->flags |= ATA_DFLAG_PIO;
 	}
 	return 0;
 }
Index: work/drivers/ata/pata_legacy.c
===================================================================
--- work.orig/drivers/ata/pata_legacy.c
+++ work/drivers/ata/pata_legacy.c
@@ -194,15 +194,12 @@ static int legacy_set_mode(struct ata_li
 {
 	struct ata_device *dev;
 
-	ata_link_for_each_dev(dev, link) {
-		if (ata_dev_enabled(dev)) {
-			ata_dev_printk(dev, KERN_INFO,
-						"configured for PIO\n");
-			dev->pio_mode = XFER_PIO_0;
-			dev->xfer_mode = XFER_PIO_0;
-			dev->xfer_shift = ATA_SHIFT_PIO;
-			dev->flags |= ATA_DFLAG_PIO;
-		}
+	ata_for_each_dev(dev, link, ENABLED) {
+		ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
+		dev->pio_mode = XFER_PIO_0;
+		dev->xfer_mode = XFER_PIO_0;
+		dev->xfer_shift = ATA_SHIFT_PIO;
+		dev->flags |= ATA_DFLAG_PIO;
 	}
 	return 0;
 }
@@ -1028,7 +1025,7 @@ static __init int legacy_init_one(struct
 	/* Nothing found means we drop the port as its probably not there */
 
 	ret = -ENODEV;
-	ata_link_for_each_dev(dev, &ap->link) {
+	ata_for_each_dev(dev, &ap->link, ALL) {
 		if (!ata_dev_absent(dev)) {
 			legacy_host[probe->slot] = host;
 			ld->platform_dev = pdev;
Index: work/drivers/ata/pata_pdc2027x.c
===================================================================
--- work.orig/drivers/ata/pata_pdc2027x.c
+++ work/drivers/ata/pata_pdc2027x.c
@@ -406,23 +406,20 @@ static int pdc2027x_set_mode(struct ata_
 	if (rc < 0)
 		return rc;
 
-	ata_link_for_each_dev(dev, link) {
-		if (ata_dev_enabled(dev)) {
+	ata_for_each_dev(dev, link, ENABLED) {
+		pdc2027x_set_piomode(ap, dev);
 
-			pdc2027x_set_piomode(ap, dev);
+		/*
+		 * Enable prefetch if the device support PIO only.
+		 */
+		if (dev->xfer_shift == ATA_SHIFT_PIO) {
+			u32 ctcr1 = ioread32(dev_mmio(ap, dev, PDC_CTCR1));
+			ctcr1 |= (1 << 25);
+			iowrite32(ctcr1, dev_mmio(ap, dev, PDC_CTCR1));
 
-			/*
-			 * Enable prefetch if the device support PIO only.
-			 */
-			if (dev->xfer_shift == ATA_SHIFT_PIO) {
-				u32 ctcr1 = ioread32(dev_mmio(ap, dev, PDC_CTCR1));
-				ctcr1 |= (1 << 25);
-				iowrite32(ctcr1, dev_mmio(ap, dev, PDC_CTCR1));
-
-				PDPRINTK("Turn on prefetch\n");
-			} else {
-				pdc2027x_set_dmamode(ap, dev);
-			}
+			PDPRINTK("Turn on prefetch\n");
+		} else {
+			pdc2027x_set_dmamode(ap, dev);
 		}
 	}
 	return 0;
Index: work/drivers/ata/pata_platform.c
===================================================================
--- work.orig/drivers/ata/pata_platform.c
+++ work/drivers/ata/pata_platform.c
@@ -34,14 +34,12 @@ static int pata_platform_set_mode(struct
 {
 	struct ata_device *dev;
 
-	ata_link_for_each_dev(dev, link) {
-		if (ata_dev_enabled(dev)) {
-			/* We don't really care */
-			dev->pio_mode = dev->xfer_mode = XFER_PIO_0;
-			dev->xfer_shift = ATA_SHIFT_PIO;
-			dev->flags |= ATA_DFLAG_PIO;
-			ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
-		}
+	ata_for_each_dev(dev, link, ENABLED) {
+		/* We don't really care */
+		dev->pio_mode = dev->xfer_mode = XFER_PIO_0;
+		dev->xfer_shift = ATA_SHIFT_PIO;
+		dev->flags |= ATA_DFLAG_PIO;
+		ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
 	}
 	return 0;
 }
Index: work/drivers/ata/pata_rz1000.c
===================================================================
--- work.orig/drivers/ata/pata_rz1000.c
+++ work/drivers/ata/pata_rz1000.c
@@ -38,15 +38,13 @@ static int rz1000_set_mode(struct ata_li
 {
 	struct ata_device *dev;
 
-	ata_link_for_each_dev(dev, link) {
-		if (ata_dev_enabled(dev)) {
-			/* We don't really care */
-			dev->pio_mode = XFER_PIO_0;
-			dev->xfer_mode = XFER_PIO_0;
-			dev->xfer_shift = ATA_SHIFT_PIO;
-			dev->flags |= ATA_DFLAG_PIO;
-			ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
-		}
+	ata_for_each_dev(dev, link, ENABLED) {
+		/* We don't really care */
+		dev->pio_mode = XFER_PIO_0;
+		dev->xfer_mode = XFER_PIO_0;
+		dev->xfer_shift = ATA_SHIFT_PIO;
+		dev->flags |= ATA_DFLAG_PIO;
+		ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
 	}
 	return 0;
 }
Index: work/drivers/ata/sata_sil.c
===================================================================
--- work.orig/drivers/ata/sata_sil.c
+++ work/drivers/ata/sata_sil.c
@@ -278,7 +278,7 @@ static int sil_set_mode(struct ata_link
 	if (rc)
 		return rc;
 
-	ata_link_for_each_dev(dev, link) {
+	ata_for_each_dev(dev, link, ALL) {
 		if (!ata_dev_enabled(dev))
 			dev_mode[dev->devno] = 0;	/* PIO0/1/2 */
 		else if (dev->flags & ATA_DFLAG_PIO)

[PATCH #upstream-fixes 3/4] libata: when restoring SControl during detach do the PMP links first

[Tejun Heo]

When restoring SControl during detach, PMP links should be handled
first as changing SControl of the host link can affect SCR access of
PMP links.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -6035,7 +6035,7 @@ static void ata_port_detach(struct ata_p
 	/* EH is now guaranteed to see UNLOADING - EH context belongs
 	 * to us.  Restore SControl and disable all existing devices.
 	 */
-	ata_for_each_link(link, ap, HOST_FIRST) {
+	ata_for_each_link(link, ap, PMP_FIRST) {
 		sata_scr_write(link, SCR_CONTROL, link->saved_scontrol);
 		ata_for_each_dev(dev, link, ALL)
 			ata_dev_disable(dev);

[PATCH #upstream-fixes 4/4] libata: perform port detach in EH

[Tejun Heo]

ata_port_detach() first made sure EH saw ATA_PFLAG_UNLOADING and then
assumed EH context belongs to it and performed detach operation
itself.  However, UNLOADING doesn't disable all of EH and this could
lead to problems including triggering WARN_ON()'s in EH path.

This patch makes port detach behave more like other EH actions such
that ata_port_detach() requests EH to detach and waits for completion.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-core.c |   23 ++++-------------------
 drivers/ata/libata-eh.c   |   32 +++++++++++++++++++++++++++++++-
 include/linux/libata.h    |    3 ++-
 3 files changed, 37 insertions(+), 21 deletions(-)

Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -491,6 +491,31 @@ enum blk_eh_timer_return ata_scsi_timed_
 	return ret;
 }
 
+static void ata_eh_unload(struct ata_port *ap)
+{
+	struct ata_link *link;
+	struct ata_device *dev;
+	unsigned long flags;
+
+	/* Restore SControl IPM and SPD for the next driver and
+	 * disable attached devices.
+	 */
+	ata_for_each_link(link, ap, PMP_FIRST) {
+		sata_scr_write(link, SCR_CONTROL, link->saved_scontrol & 0xff0);
+		ata_for_each_dev(dev, link, ALL)
+			ata_dev_disable(dev);
+	}
+
+	/* freeze and set UNLOADED */
+	spin_lock_irqsave(ap->lock, flags);
+
+	ata_port_freeze(ap);			/* won't be thawed */
+	ap->pflags &= ~ATA_PFLAG_EH_PENDING;	/* clear pending from freeze */
+	ap->pflags |= ATA_PFLAG_UNLOADED;
+
+	spin_unlock_irqrestore(ap->lock, flags);
+}
+
 /**
  *	ata_scsi_error - SCSI layer error handler callback
  *	@host: SCSI host on which error occurred
@@ -621,8 +646,13 @@ void ata_scsi_error(struct Scsi_Host *ho
 		/* invoke EH, skip if unloading or suspended */
 		if (!(ap->pflags & (ATA_PFLAG_UNLOADING | ATA_PFLAG_SUSPENDED)))
 			ap->ops->error_handler(ap);
-		else
+		else {
+			/* if unloading, commence suicide */
+			if ((ap->pflags & ATA_PFLAG_UNLOADING) &&
+			    !(ap->pflags & ATA_PFLAG_UNLOADED))
+				ata_eh_unload(ap);
 			ata_eh_finish(ap);
+		}
 
 		/* process port suspend request */
 		ata_eh_handle_port_suspend(ap);
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -213,10 +213,11 @@ enum {
 	ATA_PFLAG_FROZEN	= (1 << 2), /* port is frozen */
 	ATA_PFLAG_RECOVERED	= (1 << 3), /* recovery action performed */
 	ATA_PFLAG_LOADING	= (1 << 4), /* boot/loading probe */
-	ATA_PFLAG_UNLOADING	= (1 << 5), /* module is unloading */
 	ATA_PFLAG_SCSI_HOTPLUG	= (1 << 6), /* SCSI hotplug scheduled */
 	ATA_PFLAG_INITIALIZING	= (1 << 7), /* being initialized, don't touch */
 	ATA_PFLAG_RESETTING	= (1 << 8), /* reset in progress */
+	ATA_PFLAG_UNLOADING	= (1 << 9), /* driver is being unloaded */
+	ATA_PFLAG_UNLOADED	= (1 << 10), /* driver is unloaded */
 
 	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -6019,8 +6019,6 @@ int ata_host_activate(struct ata_host *h
 static void ata_port_detach(struct ata_port *ap)
 {
 	unsigned long flags;
-	struct ata_link *link;
-	struct ata_device *dev;
 
 	if (!ap->ops->error_handler)
 		goto skip_eh;
@@ -6028,28 +6026,15 @@ static void ata_port_detach(struct ata_p
 	/* tell EH we're leaving & flush EH */
 	spin_lock_irqsave(ap->lock, flags);
 	ap->pflags |= ATA_PFLAG_UNLOADING;
+	ata_port_schedule_eh(ap);
 	spin_unlock_irqrestore(ap->lock, flags);
 
+	/* wait till EH commits suicide */
 	ata_port_wait_eh(ap);
 
-	/* EH is now guaranteed to see UNLOADING - EH context belongs
-	 * to us.  Restore SControl and disable all existing devices.
-	 */
-	ata_for_each_link(link, ap, PMP_FIRST) {
-		sata_scr_write(link, SCR_CONTROL, link->saved_scontrol);
-		ata_for_each_dev(dev, link, ALL)
-			ata_dev_disable(dev);
-	}
-
-	/* Final freeze & EH.  All in-flight commands are aborted.  EH
-	 * will be skipped and retrials will be terminated with bad
-	 * target.
-	 */
-	spin_lock_irqsave(ap->lock, flags);
-	ata_port_freeze(ap);	/* won't be thawed */
-	spin_unlock_irqrestore(ap->lock, flags);
+	/* it better be dead now */
+	WARN_ON(!(ap->pflags & ATA_PFLAG_UNLOADED));
 
-	ata_port_wait_eh(ap);
 	cancel_rearming_delayed_work(&ap->hotplug_task);
 
  skip_eh:

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:

> There were several places where only enabled devices should be
> iterated over but device enabledness wasn't checked.
>
> * IDENTIFY data 40 wire check in cable_is_40wire()
> * xfer_mode/ncq_enabled saving in ata_scsi_error()
> * DUBIOUS_XFER handling in ata_set_mode()
>
> While at it, reformat comment in cable_is_40wire().
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
>   

   More nitpicking... :-)

> Index: work/drivers/ata/libata-core.c
> ===================================================================
> --- work.orig/drivers/ata/libata-core.c
> +++ work/drivers/ata/libata-core.c
> @@ -4169,18 +4169,16 @@ static int cable_is_40wire(struct ata_po
>  	if (ap->cbl == ATA_CBL_PATA40_SHORT)
>  		return 0;
>  	/* If the controller doesn't know we scan
> -
> -	   - Note: We look for all 40 wire detects at this point.
> -	     Any 80 wire detect is taken to be 80 wire cable
> -	     because
> -	     - In many setups only the one drive (slave if present)
> -               will give a valid detect
> -             - If you have a non detect capable drive you don't
> -               want it to colour the choice
> -        */
> +	 *
> +	 * - Note: We look for all 40 wire detects at this point.  Any
> +	 *   80 wire detect is taken to be 80 wire cable because - In
> +	 *   many setups only the one drive (slave if present) will
> +	 *   give a valid detect - If you have a non detect capable
> +	 *   drive you don't want it to colour the choice
> +	 */
>   

   The comment formatting  now is worse than it was... I'd drop hyphen 
before "Note:" and align the bullets under this note (also doing s/If/if/).

MBR, Sergei

Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators

[Elias Oltmanns]

Tejun Heo <tj@kernel.org> wrote:
> There currently are the following loop constructs.
>
> * __ata_port_for_each_link() for all available links
> * ata_port_for_each_link() for edge links
> * ata_link_for_each_dev() for all devices
> * ata_link_for_each_dev_reverse() for all devices in reverse order
>
> Now there's a need for loop construct which is similar to
> __ata_port_for_each_link() but iterates over PMP links before the host
> link.  Instead of adding another one with long name, do the following
> cleanup.
>
> * Implement and export ata_link_next() and ata_dev_next() which take
>   @mode parameter and can be used to build custom loop.
> * Implement ata_for_each_link() and ata_for_each_dev() which take
>   looping mode explicitly.
>
> The following iteration modes are implemented.
>
> * ATA_LITER_EDGE		: loop over edge links
> * ATA_LITER_HOST_FIRST		: loop over all links, host link first
> * ATA_LITER_PMP_FIRST		: loop over all links, PMP links first
>
> * ATA_DITER_ENABLED		: loop over enabled devices
> * ATA_DITER_ENABLED_REVERSE	: loop over enabled devices in reverse order
> * ATA_DITER_ALL			: loop over all devices
> * ATA_DITER_ALL_REVERSE		: loop over all devices in reverse order
>
> This change removes exlicit device enabledness checks from many loops
> and makes it clear which ones are iterated over in which direction.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
[...]
> Index: work/drivers/ata/libata-core.c
> ===================================================================
> --- work.orig/drivers/ata/libata-core.c
> +++ work/drivers/ata/libata-core.c
[...]
> @@ -1107,8 +1183,8 @@ static void ata_lpm_enable(struct ata_ho
>  
>  	for (i = 0; i < host->n_ports; i++) {
>  		ap = host->ports[i];
> -		ata_port_for_each_link(link, ap) {
> -			ata_link_for_each_dev(dev, link)
> +		ata_for_each_link(link, ap, EDGE) {
> +			ata_for_each_dev(dev, link, ALL)

Where did these short forms (EDGE, ALL) spring from? Does this code even
compile?

[...]
> Index: work/drivers/ata/libata-eh.c
> ===================================================================
> --- work.orig/drivers/ata/libata-eh.c
> +++ work/drivers/ata/libata-eh.c
[...]
> @@ -2880,9 +2871,8 @@ static int ata_link_nr_enabled(struct at
>  	struct ata_device *dev;
>  	int cnt = 0;
>  
> -	ata_link_for_each_dev(dev, link)
> -		if (ata_dev_enabled(dev))
> -			cnt++;
> +	ata_for_each_dev(dev, link, ENABLED)
> +		cnt++;
>  	return cnt;
>  }
>  
> @@ -2891,7 +2881,7 @@ static int ata_link_nr_vacant(struct ata
>  	struct ata_device *dev;
>  	int cnt = 0;
>  
> -	ata_link_for_each_dev(dev, link)
> +	ata_for_each_dev(dev, link, ALL)
>  		if (dev->class == ATA_DEV_UNKNOWN)
>  			cnt++;
>  	return cnt;

What about making the two above (ata_link_nr_*()) static inline while
you are at it? Or is this another one of those cases where the compiler
knows best anyway?

[...]
> Index: work/drivers/ata/libata-scsi.c
> ===================================================================
> --- work.orig/drivers/ata/libata-scsi.c
> +++ work/drivers/ata/libata-scsi.c
[...]
> @@ -3254,9 +3254,9 @@ void ata_scsi_scan_host(struct ata_port
>  	 * failure occurred, scan would have failed silently.  Check
>  	 * whether all devices are attached.
>  	 */
> -	ata_port_for_each_link(link, ap) {
> -		ata_link_for_each_dev(dev, link) {
> -			if (ata_dev_enabled(dev) && !dev->sdev)
> +	ata_for_each_link(link, ap, EDGE) {
> +		ata_for_each_dev(dev, link, ENABLED) {
> +			if (!dev->sdev)
>  				goto exit_loop;
>  		}
>  	}

Getting rid of those braces would make things even cleaner in my
opinion. (That's my nit picking since Sergei picked on that comment
formatting issue shortly before I was going to ;-) ).

Regards,

Elias

Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators

[Tejun Heo]

Hello,

Elias Oltmanns wrote:
>> -		ata_port_for_each_link(link, ap) {
>> -			ata_link_for_each_dev(dev, link)
>> +		ata_for_each_link(link, ap, EDGE) {
>> +			ata_for_each_dev(dev, link, ALL)
> 
> Where did these short forms (EDGE, ALL) spring from? Does this code even
> compile?

+#define ata_for_each_link(link, ap, mode) \
+	for ((link) = ata_link_next(NULL, (ap), ATA_LITER_##mode); (link); \
+	     (link) = ata_link_next((link), (ap), ATA_LITER_##mode))
+
+#define ata_for_each_dev(dev, link, mode) \
+	for ((dev) = ata_dev_next(NULL, (link), ATA_DITER_##mode); (dev); \
+	     (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))

>> @@ -2891,7 +2881,7 @@ static int ata_link_nr_vacant(struct ata
>>  	struct ata_device *dev;
>>  	int cnt = 0;
>>  
>> -	ata_link_for_each_dev(dev, link)
>> +	ata_for_each_dev(dev, link, ALL)
>>  		if (dev->class == ATA_DEV_UNKNOWN)
>>  			cnt++;
>>  	return cnt;
> 
> What about making the two above (ata_link_nr_*()) static inline while
> you are at it? Or is this another one of those cases where the compiler
> knows best anyway?

Yeah, it's a static function used only in single place.  Should be
pretty straight forward for the compiler and even if it fails to
notice it, it doesn't hurt at all.

> [...]
>> Index: work/drivers/ata/libata-scsi.c
>> ===================================================================
>> --- work.orig/drivers/ata/libata-scsi.c
>> +++ work/drivers/ata/libata-scsi.c
> [...]
>> @@ -3254,9 +3254,9 @@ void ata_scsi_scan_host(struct ata_port
>>  	 * failure occurred, scan would have failed silently.  Check
>>  	 * whether all devices are attached.
>>  	 */
>> -	ata_port_for_each_link(link, ap) {
>> -		ata_link_for_each_dev(dev, link) {
>> -			if (ata_dev_enabled(dev) && !dev->sdev)
>> +	ata_for_each_link(link, ap, EDGE) {
>> +		ata_for_each_dev(dev, link, ENABLED) {
>> +			if (!dev->sdev)
>>  				goto exit_loop;
>>  		}
>>  	}
> 
> Getting rid of those braces would make things even cleaner in my
> opinion. (That's my nit picking since Sergei picked on that comment
> formatting issue shortly before I was going to ;-) ).

Yeah, maybe.  I don't know.  The reason why I added braces there in
the first place was because emacs has some problem dealing with nested
non-standard looping constructs.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

Sergei Shtylyov wrote:
> Hello.
> 
> Tejun Heo wrote:
> 
>> There were several places where only enabled devices should be
>> iterated over but device enabledness wasn't checked.
>>
>> * IDENTIFY data 40 wire check in cable_is_40wire()
>> * xfer_mode/ncq_enabled saving in ata_scsi_error()
>> * DUBIOUS_XFER handling in ata_set_mode()
>>
>> While at it, reformat comment in cable_is_40wire().
>>
>> Signed-off-by: Tejun Heo <tj@kernel.org>
>>   
> 
>   More nitpicking... :-)
> 
>> Index: work/drivers/ata/libata-core.c
>> ===================================================================
>> --- work.orig/drivers/ata/libata-core.c
>> +++ work/drivers/ata/libata-core.c
>> @@ -4169,18 +4169,16 @@ static int cable_is_40wire(struct ata_po
>>      if (ap->cbl == ATA_CBL_PATA40_SHORT)
>>          return 0;
>>      /* If the controller doesn't know we scan
>> -
>> -       - Note: We look for all 40 wire detects at this point.
>> -         Any 80 wire detect is taken to be 80 wire cable
>> -         because
>> -         - In many setups only the one drive (slave if present)
>> -               will give a valid detect
>> -             - If you have a non detect capable drive you don't
>> -               want it to colour the choice
>> -        */
>> +     *
>> +     * - Note: We look for all 40 wire detects at this point.  Any
>> +     *   80 wire detect is taken to be 80 wire cable because - In
>> +     *   many setups only the one drive (slave if present) will
>> +     *   give a valid detect - If you have a non detect capable
>> +     *   drive you don't want it to colour the choice
>> +     */
>>   
> 
>   The comment formatting  now is worse than it was... I'd drop hyphen
> before "Note:" and align the bullets under this note (also doing s/If/if/).

Yeah, somehow I missed those bullets altogether.  Will regenerate the
patch.  Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators

[Elias Oltmanns]

Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> Elias Oltmanns wrote:
>>> -		ata_port_for_each_link(link, ap) {
>>> -			ata_link_for_each_dev(dev, link)
>>> +		ata_for_each_link(link, ap, EDGE) {
>>> +			ata_for_each_dev(dev, link, ALL)
>> 
>> Where did these short forms (EDGE, ALL) spring from? Does this code even
>> compile?
>
> +#define ata_for_each_link(link, ap, mode) \
> +	for ((link) = ata_link_next(NULL, (ap), ATA_LITER_##mode); (link); \
> +	     (link) = ata_link_next((link), (ap), ATA_LITER_##mode))
> +
> +#define ata_for_each_dev(dev, link, mode) \
> +	for ((dev) = ata_dev_next(NULL, (link), ATA_DITER_##mode); (dev); \
> +	     (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))

Sorry, I should have been more explicit. I was referring to EDGE and ALL
as opposed to ATA_LITER_EDGE and ATA_DITER_ALL. Unless I've missed
something, the former aren't defined anywhere in your patch.

>
>>> @@ -2891,7 +2881,7 @@ static int ata_link_nr_vacant(struct ata
>>>  	struct ata_device *dev;
>>>  	int cnt = 0;
>>>  
>>> -	ata_link_for_each_dev(dev, link)
>>> +	ata_for_each_dev(dev, link, ALL)
>>>  		if (dev->class == ATA_DEV_UNKNOWN)
>>>  			cnt++;
>>>  	return cnt;
>> 
>> What about making the two above (ata_link_nr_*()) static inline while
>> you are at it? Or is this another one of those cases where the compiler
>> knows best anyway?
>
> Yeah, it's a static function used only in single place.  Should be
> pretty straight forward for the compiler and even if it fails to
> notice it, it doesn't hurt at all.

I should have guessed, I suppose. Probably just was too lazy to check,
sorry.

>
>> [...]
>>> Index: work/drivers/ata/libata-scsi.c
>>> ===================================================================
>>> --- work.orig/drivers/ata/libata-scsi.c
>>> +++ work/drivers/ata/libata-scsi.c
>> [...]
>>> @@ -3254,9 +3254,9 @@ void ata_scsi_scan_host(struct ata_port
>>>  	 * failure occurred, scan would have failed silently.  Check
>>>  	 * whether all devices are attached.
>>>  	 */
>>> -	ata_port_for_each_link(link, ap) {
>>> -		ata_link_for_each_dev(dev, link) {
>>> -			if (ata_dev_enabled(dev) && !dev->sdev)
>>> +	ata_for_each_link(link, ap, EDGE) {
>>> +		ata_for_each_dev(dev, link, ENABLED) {
>>> +			if (!dev->sdev)
>>>  				goto exit_loop;
>>>  		}
>>>  	}
>> 
>> Getting rid of those braces would make things even cleaner in my
>> opinion. (That's my nit picking since Sergei picked on that comment
>> formatting issue shortly before I was going to ;-) ).
>
> Yeah, maybe.  I don't know.  The reason why I added braces there in
> the first place was because emacs has some problem dealing with nested
> non-standard looping constructs.

Right, I seem to remember having trouble with that myself. So I'm all in
favour of keeping them after all ;-).

Regards,

Elias

Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators

[Tejun Heo]

Elias Oltmanns wrote:
> Tejun Heo <tj@kernel.org> wrote:
>> Hello,
>>
>> Elias Oltmanns wrote:
>>>> -		ata_port_for_each_link(link, ap) {
>>>> -			ata_link_for_each_dev(dev, link)
>>>> +		ata_for_each_link(link, ap, EDGE) {
>>>> +			ata_for_each_dev(dev, link, ALL)
>>> Where did these short forms (EDGE, ALL) spring from? Does this code even
>>> compile?
>> +#define ata_for_each_link(link, ap, mode) \
>> +	for ((link) = ata_link_next(NULL, (ap), ATA_LITER_##mode); (link); \
>> +	     (link) = ata_link_next((link), (ap), ATA_LITER_##mode))
>> +
>> +#define ata_for_each_dev(dev, link, mode) \
>> +	for ((dev) = ata_dev_next(NULL, (link), ATA_DITER_##mode); (dev); \
>> +	     (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))
> 
> Sorry, I should have been more explicit. I was referring to EDGE and ALL
> as opposed to ATA_LITER_EDGE and ATA_DITER_ALL. Unless I've missed
> something, the former aren't defined anywhere in your patch.

I seriously can't be more explicit.  Please take a shower and read my
reply again, especially, the "ATA_[LD]ITER_##mode" part.  :-)

-- 
tejun

[PATCH #upstream-fixes 1/4 UPDATED] libata: fix device iteration bugs

[Tejun Heo]

There were several places where only enabled devices should be
iterated over but device enabledness wasn't checked.

* IDENTIFY data 40 wire check in cable_is_40wire()
* xfer_mode/ncq_enabled saving in ata_scsi_error()
* DUBIOUS_XFER handling in ata_set_mode()

While at it, reformat comments in cable_is_40wire().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
Comments reformatted as per Sergei's comment.

 drivers/ata/libata-core.c |   32 ++++++++++++++++++--------------
 drivers/ata/libata-eh.c   |    9 +++++++++
 2 files changed, 27 insertions(+), 14 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -4158,29 +4158,33 @@ static int cable_is_40wire(struct ata_po
 	struct ata_link *link;
 	struct ata_device *dev;
 
-	/* If the controller thinks we are 40 wire, we are */
+	/* If the controller thinks we are 40 wire, we are. */
 	if (ap->cbl == ATA_CBL_PATA40)
 		return 1;
-	/* If the controller thinks we are 80 wire, we are */
+
+	/* If the controller thinks we are 80 wire, we are. */
 	if (ap->cbl == ATA_CBL_PATA80 || ap->cbl == ATA_CBL_SATA)
 		return 0;
-	/* If the system is known to be 40 wire short cable (eg laptop),
-	   then we allow 80 wire modes even if the drive isn't sure */
+
+	/* If the system is known to be 40 wire short cable (eg
+	 * laptop), then we allow 80 wire modes even if the drive
+	 * isn't sure.
+	 */
 	if (ap->cbl == ATA_CBL_PATA40_SHORT)
 		return 0;
-	/* If the controller doesn't know we scan
 
-	   - Note: We look for all 40 wire detects at this point.
-	     Any 80 wire detect is taken to be 80 wire cable
-	     because
-	     - In many setups only the one drive (slave if present)
-               will give a valid detect
-             - If you have a non detect capable drive you don't
-               want it to colour the choice
-        */
+	/* If the controller doesn't know, we scan.
+	 *
+	 * Note: We look for all 40 wire detects at this point.  Any
+	 *       80 wire detect is taken to be 80 wire cable because
+	 * - in many setups only the one drive (slave if present) will
+	 *   give a valid detect
+	 * - if you have a non detect capable drive you don't want it
+	 *   to colour the choice
+	 */
 	ata_port_for_each_link(link, ap) {
 		ata_link_for_each_dev(dev, link) {
-			if (!ata_is_40wire(dev))
+			if (ata_dev_enabled(dev) && !ata_is_40wire(dev))
 				return 0;
 		}
 	}
Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -603,6 +603,9 @@ void ata_scsi_error(struct Scsi_Host *ho
 			ata_link_for_each_dev(dev, link) {
 				int devno = dev->devno;
 
+				if (!ata_dev_enabled(dev))
+					continue;
+
 				ehc->saved_xfer_mode[devno] = dev->xfer_mode;
 				if (ata_ncq_enabled(dev))
 					ehc->saved_ncq_enabled |= 1 << devno;
@@ -2790,6 +2793,9 @@ int ata_set_mode(struct ata_link *link,
 
 	/* if data transfer is verified, clear DUBIOUS_XFER on ering top */
 	ata_link_for_each_dev(dev, link) {
+		if (!ata_dev_enabled(dev))
+			continue;
+
 		if (!(dev->flags & ATA_DFLAG_DUBIOUS_XFER)) {
 			struct ata_ering_entry *ent;
 
@@ -2811,6 +2817,9 @@ int ata_set_mode(struct ata_link *link,
 		u8 saved_xfer_mode = ehc->saved_xfer_mode[dev->devno];
 		u8 saved_ncq = !!(ehc->saved_ncq_enabled & (1 << dev->devno));
 
+		if (!ata_dev_enabled(dev))
+			continue;
+
 		if (dev->xfer_mode != saved_xfer_mode ||
 		    ata_ncq_enabled(dev) != saved_ncq)
 			dev->flags |= ATA_DFLAG_DUBIOUS_XFER;

Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators

[Elias Oltmanns]

Tejun Heo <tj@kernel.org> wrote:
> Elias Oltmanns wrote:
>> Tejun Heo <tj@kernel.org> wrote:
>
>>> Hello,
>>>
>>> Elias Oltmanns wrote:
>>>>> -		ata_port_for_each_link(link, ap) {
>>>>> -			ata_link_for_each_dev(dev, link)
>>>>> +		ata_for_each_link(link, ap, EDGE) {
>>>>> +			ata_for_each_dev(dev, link, ALL)
>>>> Where did these short forms (EDGE, ALL) spring from? Does this code even
>>>> compile?
>>> +#define ata_for_each_link(link, ap, mode) \
>>> +	for ((link) = ata_link_next(NULL, (ap), ATA_LITER_##mode); (link); \
>>> +	     (link) = ata_link_next((link), (ap), ATA_LITER_##mode))
>>> +
>>> +#define ata_for_each_dev(dev, link, mode) \
>>> +	for ((dev) = ata_dev_next(NULL, (link), ATA_DITER_##mode); (dev); \
>>> +	     (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))
>> 
>> Sorry, I should have been more explicit. I was referring to EDGE and ALL
>> as opposed to ATA_LITER_EDGE and ATA_DITER_ALL. Unless I've missed
>> something, the former aren't defined anywhere in your patch.
>
> I seriously can't be more explicit.  Please take a shower and read my
> reply again, especially, the "ATA_[LD]ITER_##mode" part.  :-)

Oh dear, I really need to drop that idea that a macro does what I think
it does. Sorry for the noise.

Regards,

Elias

Re: [PATCH #upstream-fixes 1/4 UPDATED] libata: fix device iteration bugs

[Jeff Garzik]

Tejun Heo wrote:
> There were several places where only enabled devices should be
> iterated over but device enabledness wasn't checked.
> 
> * IDENTIFY data 40 wire check in cable_is_40wire()
> * xfer_mode/ncq_enabled saving in ata_scsi_error()
> * DUBIOUS_XFER handling in ata_set_mode()
> 
> While at it, reformat comments in cable_is_40wire().
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
> Comments reformatted as per Sergei's comment.
> 
>  drivers/ata/libata-core.c |   32 ++++++++++++++++++--------------
>  drivers/ata/libata-eh.c   |    9 +++++++++
>  2 files changed, 27 insertions(+), 14 deletions(-)

applied

Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators

[Jeff Garzik]

Tejun Heo wrote:
> There currently are the following loop constructs.
> 
> * __ata_port_for_each_link() for all available links
> * ata_port_for_each_link() for edge links
> * ata_link_for_each_dev() for all devices
> * ata_link_for_each_dev_reverse() for all devices in reverse order
> 
> Now there's a need for loop construct which is similar to
> __ata_port_for_each_link() but iterates over PMP links before the host
> link.  Instead of adding another one with long name, do the following
> cleanup.
> 
> * Implement and export ata_link_next() and ata_dev_next() which take
>   @mode parameter and can be used to build custom loop.
> * Implement ata_for_each_link() and ata_for_each_dev() which take
>   looping mode explicitly.
> 
> The following iteration modes are implemented.
> 
> * ATA_LITER_EDGE		: loop over edge links
> * ATA_LITER_HOST_FIRST		: loop over all links, host link first
> * ATA_LITER_PMP_FIRST		: loop over all links, PMP links first
> 
> * ATA_DITER_ENABLED		: loop over enabled devices
> * ATA_DITER_ENABLED_REVERSE	: loop over enabled devices in reverse order
> * ATA_DITER_ALL			: loop over all devices
> * ATA_DITER_ALL_REVERSE		: loop over all devices in reverse order
> 
> This change removes exlicit device enabledness checks from many loops
> and makes it clear which ones are iterated over in which direction.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/ata/ahci.c           |    8 -
>  drivers/ata/ata_generic.c    |    5 -
>  drivers/ata/libata-acpi.c    |   19 +---
>  drivers/ata/libata-core.c    |  183 ++++++++++++++++++++++++++++---------------
>  drivers/ata/libata-eh.c      |   84 ++++++++-----------
>  drivers/ata/libata-pmp.c     |   22 ++---
>  drivers/ata/libata-scsi.c    |   22 ++---
>  drivers/ata/pata_it821x.c    |   34 +++----
>  drivers/ata/pata_ixp4xx_cf.c |   14 +--
>  drivers/ata/pata_legacy.c    |   17 +--
>  drivers/ata/pata_pdc2027x.c  |   27 ++----
>  drivers/ata/pata_platform.c  |   14 +--
>  drivers/ata/pata_rz1000.c    |   16 +--
>  drivers/ata/sata_sil.c       |    2 
>  include/linux/libata.h       |   50 +++++++----
>  15 files changed, 279 insertions(+), 238 deletions(-)

ACK... for 2.6.29.  I think this is too much change for -rc2.  It's a 
good idea though.

Also, it could use more documentation than just the commit message... 
something that programmers can easily refer to (header file comment?)

	Jeff

Re: [PATCH #upstream-fixes 4/4] libata: perform port detach in EH

[Jeff Garzik]

Tejun Heo wrote:
> ata_port_detach() first made sure EH saw ATA_PFLAG_UNLOADING and then
> assumed EH context belongs to it and performed detach operation
> itself.  However, UNLOADING doesn't disable all of EH and this could
> lead to problems including triggering WARN_ON()'s in EH path.
> 
> This patch makes port detach behave more like other EH actions such
> that ata_port_detach() requests EH to detach and waits for completion.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/ata/libata-core.c |   23 ++++-------------------
>  drivers/ata/libata-eh.c   |   32 +++++++++++++++++++++++++++++++-
>  include/linux/libata.h    |    3 ++-
>  3 files changed, 37 insertions(+), 21 deletions(-)

dropped patches 3-4 as well, due to dropping patch #2

Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators

[Mark Lord]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> There currently are the following loop constructs.
>>
>> * __ata_port_for_each_link() for all available links
>> * ata_port_for_each_link() for edge links
>> * ata_link_for_each_dev() for all devices
>> * ata_link_for_each_dev_reverse() for all devices in reverse order
>>
>> Now there's a need for loop construct which is similar to
>> __ata_port_for_each_link() but iterates over PMP links before the host
>> link.  Instead of adding another one with long name, do the following
>> cleanup.
>>
>> * Implement and export ata_link_next() and ata_dev_next() which take
>>   @mode parameter and can be used to build custom loop.
>> * Implement ata_for_each_link() and ata_for_each_dev() which take
>>   looping mode explicitly.
>>
>> The following iteration modes are implemented.
>>
>> * ATA_LITER_EDGE        : loop over edge links
>> * ATA_LITER_HOST_FIRST        : loop over all links, host link first
>> * ATA_LITER_PMP_FIRST        : loop over all links, PMP links first
>>
>> * ATA_DITER_ENABLED        : loop over enabled devices
>> * ATA_DITER_ENABLED_REVERSE    : loop over enabled devices in reverse 
>> order
>> * ATA_DITER_ALL            : loop over all devices
>> * ATA_DITER_ALL_REVERSE        : loop over all devices in reverse order
>>
>> This change removes exlicit device enabledness checks from many loops
>> and makes it clear which ones are iterated over in which direction.
>>
>> Signed-off-by: Tejun Heo <tj@kernel.org>
>> ---
>>  drivers/ata/ahci.c           |    8 -
>>  drivers/ata/ata_generic.c    |    5 -
>>  drivers/ata/libata-acpi.c    |   19 +---
>>  drivers/ata/libata-core.c    |  183 
>> ++++++++++++++++++++++++++++---------------
>>  drivers/ata/libata-eh.c      |   84 ++++++++-----------
>>  drivers/ata/libata-pmp.c     |   22 ++---
>>  drivers/ata/libata-scsi.c    |   22 ++---
>>  drivers/ata/pata_it821x.c    |   34 +++----
>>  drivers/ata/pata_ixp4xx_cf.c |   14 +--
>>  drivers/ata/pata_legacy.c    |   17 +--
>>  drivers/ata/pata_pdc2027x.c  |   27 ++----
>>  drivers/ata/pata_platform.c  |   14 +--
>>  drivers/ata/pata_rz1000.c    |   16 +--
>>  drivers/ata/sata_sil.c       |    2  include/linux/libata.h       |   
>> 50 +++++++----
>>  15 files changed, 279 insertions(+), 238 deletions(-)
> 
> ACK... for 2.6.29.  I think this is too much change for -rc2.  It's a 
> good idea though.
> 
> Also, it could use more documentation than just the commit message... 
> something that programmers can easily refer to (header file comment?)
..

Yeah.  Especially a reminder of what "LITER" and "DITER" stand for!  :)

Cheers

Re: [PATCH #upstream-fixes 2/4] libata: beef up iterators

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> There currently are the following loop constructs.
>>>
>>> * __ata_port_for_each_link() for all available links
>>> * ata_port_for_each_link() for edge links
>>> * ata_link_for_each_dev() for all devices
>>> * ata_link_for_each_dev_reverse() for all devices in reverse order
>>>
>>> Now there's a need for loop construct which is similar to
>>> __ata_port_for_each_link() but iterates over PMP links before the host
>>> link.  Instead of adding another one with long name, do the following
>>> cleanup.
>>>
>>> * Implement and export ata_link_next() and ata_dev_next() which take
>>>   @mode parameter and can be used to build custom loop.
>>> * Implement ata_for_each_link() and ata_for_each_dev() which take
>>>   looping mode explicitly.
>>>
>>> The following iteration modes are implemented.
>>>
>>> * ATA_LITER_EDGE        : loop over edge links
>>> * ATA_LITER_HOST_FIRST        : loop over all links, host link first
>>> * ATA_LITER_PMP_FIRST        : loop over all links, PMP links first
>>>
>>> * ATA_DITER_ENABLED        : loop over enabled devices
>>> * ATA_DITER_ENABLED_REVERSE    : loop over enabled devices in reverse 
>>> order
>>> * ATA_DITER_ALL            : loop over all devices
>>> * ATA_DITER_ALL_REVERSE        : loop over all devices in reverse order
>>>
>>> This change removes exlicit device enabledness checks from many loops
>>> and makes it clear which ones are iterated over in which direction.
>>>
>>> Signed-off-by: Tejun Heo <tj@kernel.org>
>>> ---
>>>  drivers/ata/ahci.c           |    8 -
>>>  drivers/ata/ata_generic.c    |    5 -
>>>  drivers/ata/libata-acpi.c    |   19 +---
>>>  drivers/ata/libata-core.c    |  183 
>>> ++++++++++++++++++++++++++++---------------
>>>  drivers/ata/libata-eh.c      |   84 ++++++++-----------
>>>  drivers/ata/libata-pmp.c     |   22 ++---
>>>  drivers/ata/libata-scsi.c    |   22 ++---
>>>  drivers/ata/pata_it821x.c    |   34 +++----
>>>  drivers/ata/pata_ixp4xx_cf.c |   14 +--
>>>  drivers/ata/pata_legacy.c    |   17 +--
>>>  drivers/ata/pata_pdc2027x.c  |   27 ++----
>>>  drivers/ata/pata_platform.c  |   14 +--
>>>  drivers/ata/pata_rz1000.c    |   16 +--
>>>  drivers/ata/sata_sil.c       |    2  include/linux/libata.h       
>>> |   50 +++++++----
>>>  15 files changed, 279 insertions(+), 238 deletions(-)
>>
>> ACK... for 2.6.29.  I think this is too much change for -rc2.  It's a 
>> good idea though.
>>
>> Also, it could use more documentation than just the commit message... 
>> something that programmers can easily refer to (header file comment?)
> ..
> 
> Yeah.  Especially a reminder of what "LITER" and "DITER" stand for!  :)

Well, DITER stands for device iteration, and LITER stands for an amount 
of petrol... <grin>

	Jeff

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Guntsche Michael]

On Nov 11, 2008, at 15:29, Mark Lord wrote:
> .
>
> Yeah, that's the right one.  But if you are technically apt,
> then perhaps you could try a brief experiment for us:
>
> Use drivers/ide instead, and see what it reports in the boot log.
>
> One simple way to do this, is to boot a Knoppix CD, say v5.01 or so
> (but not the newest version, which probably uses libata instead).
>
> Then just grab the boot log with dmesg, and post it here.

Ok, recompiled the kernel with the old driver, here the relevant dmesg  
output.

Uniform Multi-Platform E-IDE driver
piix 0000:00:07.1: IDE controller (0x8086:0x7010 rev 0x00)
piix 0000:00:07.1: not 100% native mode: will probe irqs later
     ide0: BM-DMA at 0xe800-0xe807
     ide1: BM-DMA at 0xe808-0xe80f
Probing IDE interface ide0...
hda: IC35L040AVER07-0, ATA DISK drive
hda: host max PIO4 wanted PIO255(auto-tune) selected PIO4
hda: MWDMA2 mode selected
Probing IDE interface ide1...
hdc: SAMSUNG CD-R/RW SW-408B, ATAPI CD/DVD-ROM drive
hdc: host max PIO4 wanted PIO255(auto-tune) selected PIO4
hdc: MWDMA2 mode selected
ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
ide1 at 0x170-0x177,0x376 on irq 15

Hope that helps,
Mike

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Guntsche Michael wrote:
> 
> On Nov 11, 2008, at 15:29, Mark Lord wrote:
>> .
>>
>> Yeah, that's the right one.  But if you are technically apt,
>> then perhaps you could try a brief experiment for us:
>>
>> Use drivers/ide instead, and see what it reports in the boot log.
>>
>> One simple way to do this, is to boot a Knoppix CD, say v5.01 or so
>> (but not the newest version, which probably uses libata instead).
>>
>> Then just grab the boot log with dmesg, and post it here.
> 
> Ok, recompiled the kernel with the old driver, here the relevant dmesg 
> output.
> 
> Uniform Multi-Platform E-IDE driver
> piix 0000:00:07.1: IDE controller (0x8086:0x7010 rev 0x00)
> piix 0000:00:07.1: not 100% native mode: will probe irqs later
>     ide0: BM-DMA at 0xe800-0xe807
>     ide1: BM-DMA at 0xe808-0xe80f
> Probing IDE interface ide0...
> hda: IC35L040AVER07-0, ATA DISK drive
> hda: host max PIO4 wanted PIO255(auto-tune) selected PIO4
> hda: MWDMA2 mode selected
> Probing IDE interface ide1...
> hdc: SAMSUNG CD-R/RW SW-408B, ATAPI CD/DVD-ROM drive
> hdc: host max PIO4 wanted PIO255(auto-tune) selected PIO4
> hdc: MWDMA2 mode selected
> ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> ide1 at 0x170-0x177,0x376 on irq 15
..

Mmm.. no phantom device showing up there,
so I suppose what's left of my ancient device probing
is still mostly working (does any of it remain now?).

Tejun?

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

Mark Lord wrote:
> Guntsche Michael wrote:
>>
>> On Nov 11, 2008, at 15:29, Mark Lord wrote:
>>> .
>>>
>>> Yeah, that's the right one.  But if you are technically apt,
>>> then perhaps you could try a brief experiment for us:
>>>
>>> Use drivers/ide instead, and see what it reports in the boot log.
>>>
>>> One simple way to do this, is to boot a Knoppix CD, say v5.01 or so
>>> (but not the newest version, which probably uses libata instead).
>>>
>>> Then just grab the boot log with dmesg, and post it here.
>>
>> Ok, recompiled the kernel with the old driver, here the relevant dmesg
>> output.
>>
>> Uniform Multi-Platform E-IDE driver
>> piix 0000:00:07.1: IDE controller (0x8086:0x7010 rev 0x00)
>> piix 0000:00:07.1: not 100% native mode: will probe irqs later
>>     ide0: BM-DMA at 0xe800-0xe807
>>     ide1: BM-DMA at 0xe808-0xe80f
>> Probing IDE interface ide0...
>> hda: IC35L040AVER07-0, ATA DISK drive
>> hda: host max PIO4 wanted PIO255(auto-tune) selected PIO4
>> hda: MWDMA2 mode selected
>> Probing IDE interface ide1...
>> hdc: SAMSUNG CD-R/RW SW-408B, ATAPI CD/DVD-ROM drive
>> hdc: host max PIO4 wanted PIO255(auto-tune) selected PIO4
>> hdc: MWDMA2 mode selected
>> ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
>> ide1 at 0x170-0x177,0x376 on irq 15
> ..
> 
> Mmm.. no phantom device showing up there,
> so I suppose what's left of my ancient device probing
> is still mostly working (does any of it remain now?).

Looks like we screwed up phantom device detection somewhere.  Michael,
can you please apply the attached patch and report kernel boot log?

Thanks.

-- 
tejun

[-- Attachment #2: phantom-debug.patch --]
[-- Type: text/x-patch, Size: 1639 bytes --]

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 4b47394..abbb5f0 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1226,11 +1226,16 @@ fsm_start:
 		} else {
 			/* ATA PIO protocol */
 			if (unlikely((status & ATA_DRQ) == 0)) {
+				ata_dev_printk(qc->dev, KERN_INFO, "XXX status=0x%x hdiag=%d\n",
+					       status, qc->dev->horkage & ATA_HORKAGE_DIAGNOSTIC);
+
 				/* handle BSY=0, DRQ=0 as error */
-				if (likely(status & (ATA_ERR | ATA_DF)))
+				if (likely(status & (ATA_ERR | ATA_DF))) {
 					/* device stops HSM for abort/error */
 					qc->err_mask |= AC_ERR_DEV;
-				else {
+					if (qc->dev->horkage & ATA_HORKAGE_DIAGNOSTIC)
+						qc->err_mask |= AC_ERR_NODEV_HINT;
+				} else {
 					/* HSM violation. Let EH handle this.
 					 * Phantom devices also trigger this
 					 * condition.  Mark hint.
@@ -1838,6 +1843,8 @@ unsigned int ata_sff_dev_classify(struct ata_device *dev, int present,
 
 	/* determine if device is ATA or ATAPI */
 	class = ata_dev_classify(&tf);
+	ata_dev_printk(dev, KERN_INFO, "XXX sff_dev_classify present=%d hdiag=%d tf=%02x:%02x:%02x class=%d\n",
+		       present, dev->horkage & ATA_HORKAGE_DIAGNOSTIC, tf.lbal, tf.lbam, tf.lbah, class);
 
 	if (class == ATA_DEV_UNKNOWN) {
 		/* If the device failed diagnostic, it's likely to
@@ -1981,6 +1988,7 @@ int ata_sff_softreset(struct ata_link *link, unsigned int *classes,
 		devmask |= (1 << 0);
 	if (slave_possible && ata_devchk(ap, 1))
 		devmask |= (1 << 1);
+	ata_link_printk(link, KERN_INFO, "XXX devmask=%x\n", devmask);
 
 	/* select device 0 again */
 	ap->ops->sff_dev_select(ap, 0);

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Wed, 12 Nov 2008 11:34:19 +0900, Tejun Heo <tj@kernel.org> wrote:

> Looks like we screwed up phantom device detection somewhere.  Michael,
> can you please apply the attached patch and report kernel boot log?

Here the relevant dmesg output

ata_piix 0000:00:07.1: version 2.12
scsi0 : ata_piix
scsi1 : ata_piix
ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
ata1: XXX devmask=3
ata1.00: XXX sff_dev_classify present=1 hdiag=0 tf=01:00:00 class=1
ata1.01: XXX sff_dev_classify present=2 hdiag=0 tf=01:00:00 class=1
ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
ata1.00: 66055248 sectors, multi 16: LBA
ata1.00: configured for MWDMA2
ata2: XXX devmask=1
ata2.00: XXX sff_dev_classify present=1 hdiag=0 tf=01:14:eb class=3
ata2.01: XXX sff_dev_classify present=0 hdiag=1 tf=ff:00:00 class=1
ata2.01: XXX status=0x1 hdiag=1
ata2.01: NODEV after polling detection
ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
ata2.00: configured for MWDMA2

Hope this helps,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

Michael Guntsche wrote:
> On Wed, 12 Nov 2008 11:34:19 +0900, Tejun Heo <tj@kernel.org> wrote:
> 
>> Looks like we screwed up phantom device detection somewhere.  Michael,
>> can you please apply the attached patch and report kernel boot log?
> 
> Here the relevant dmesg output
> 
> ata_piix 0000:00:07.1: version 2.12
> scsi0 : ata_piix
> scsi1 : ata_piix
> ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
> ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
> ata1: XXX devmask=3
> ata1.00: XXX sff_dev_classify present=1 hdiag=0 tf=01:00:00 class=1
> ata1.01: XXX sff_dev_classify present=2 hdiag=0 tf=01:00:00 class=1
> ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
> ata1.00: 66055248 sectors, multi 16: LBA
> ata1.00: configured for MWDMA2
> ata2: XXX devmask=1
> ata2.00: XXX sff_dev_classify present=1 hdiag=0 tf=01:14:eb class=3
> ata2.01: XXX sff_dev_classify present=0 hdiag=1 tf=ff:00:00 class=1
> ata2.01: XXX status=0x1 hdiag=1
> ata2.01: NODEV after polling detection
> ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
> ata2.00: configured for MWDMA2

Can you please test the attached patch?

Thanks.

-- 
tejun

[-- Attachment #2: phantom-fix.patch --]
[-- Type: text/x-patch, Size: 5782 bytes --]

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 4b47394..9859705 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1083,7 +1083,6 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
 
 /**
  *	ata_sff_hsm_move - move the HSM to the next state.
- *	@ap: the target ata_port
  *	@qc: qc on going
  *	@status: current device status
  *	@in_wq: 1 if called from workqueue, 0 otherwise
@@ -1091,9 +1090,11 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
  *	RETURNS:
  *	1 when poll next status needed, 0 otherwise.
  */
-int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
-		     u8 status, int in_wq)
+int ata_sff_hsm_move(struct ata_queued_cmd *qc, u8 status, int in_wq)
 {
+	struct ata_port *ap = qc->ap;
+	struct ata_device *dev = qc->dev;
+	struct ata_eh_context *ehc = &ap->link.eh_context;
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	unsigned long flags = 0;
 	int poll_next;
@@ -1227,10 +1228,15 @@ fsm_start:
 			/* ATA PIO protocol */
 			if (unlikely((status & ATA_DRQ) == 0)) {
 				/* handle BSY=0, DRQ=0 as error */
-				if (likely(status & (ATA_ERR | ATA_DF)))
+				if (likely(status & (ATA_ERR | ATA_DF))) {
 					/* device stops HSM for abort/error */
 					qc->err_mask |= AC_ERR_DEV;
-				else {
+
+					if (!(ehc->sff_devmask &
+					      (1 << dev->devno)))
+						qc->err_mask |=
+							AC_ERR_NODEV_HINT;
+				} else {
 					/* HSM violation. Let EH handle this.
 					 * Phantom devices also trigger this
 					 * condition.  Mark hint.
@@ -1359,7 +1365,7 @@ fsm_start:
 	}
 
 	/* move the HSM */
-	poll_next = ata_sff_hsm_move(ap, qc, status, 1);
+	poll_next = ata_sff_hsm_move(qc, status, 1);
 
 	/* another command or interrupt handler
 	 * may be running at this point.
@@ -1593,7 +1599,7 @@ inline unsigned int ata_sff_host_intr(struct ata_port *ap,
 	/* ack bmdma irq events */
 	ap->ops->sff_irq_clear(ap);
 
-	ata_sff_hsm_move(ap, qc, status, 0);
+	ata_sff_hsm_move(qc, status, 0);
 
 	if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
 				       qc->tf.protocol == ATAPI_PROT_DMA))
@@ -1969,25 +1975,26 @@ int ata_sff_softreset(struct ata_link *link, unsigned int *classes,
 		      unsigned long deadline)
 {
 	struct ata_port *ap = link->ap;
+	struct ata_eh_context *ehc = &link->eh_context;
 	unsigned int slave_possible = ap->flags & ATA_FLAG_SLAVE_POSS;
-	unsigned int devmask = 0;
 	int rc;
 	u8 err;
 
 	DPRINTK("ENTER\n");
 
 	/* determine if device 0/1 are present */
+	ehc->sff_devmask = 0;
 	if (ata_devchk(ap, 0))
-		devmask |= (1 << 0);
+		ehc->sff_devmask |= (1 << 0);
 	if (slave_possible && ata_devchk(ap, 1))
-		devmask |= (1 << 1);
+		ehc->sff_devmask |= (1 << 1);
 
 	/* select device 0 again */
 	ap->ops->sff_dev_select(ap, 0);
 
 	/* issue bus reset */
-	DPRINTK("about to softreset, devmask=%x\n", devmask);
-	rc = ata_bus_softreset(ap, devmask, deadline);
+	DPRINTK("about to softreset, devmask=%x\n", ehc->sff_devmask);
+	rc = ata_bus_softreset(ap, ehc->sff_devmask, deadline);
 	/* if link is occupied, -ENODEV too is an error */
 	if (rc && (rc != -ENODEV || sata_scr_valid(link))) {
 		ata_link_printk(link, KERN_ERR, "SRST failed (errno=%d)\n", rc);
@@ -1996,10 +2003,10 @@ int ata_sff_softreset(struct ata_link *link, unsigned int *classes,
 
 	/* determine by signature whether we have ATA or ATAPI devices */
 	classes[0] = ata_sff_dev_classify(&link->device[0],
-					  devmask & (1 << 0), &err);
+					  ehc->sff_devmask & (1 << 0), &err);
 	if (slave_possible && err != 0x81)
 		classes[1] = ata_sff_dev_classify(&link->device[1],
-						  devmask & (1 << 1), &err);
+					ehc->sff_devmask & (1 << 1), &err);
 
 	DPRINTK("EXIT, classes[0]=%u [1]=%u\n", classes[0], classes[1]);
 	return 0;
diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
index 1266924..1458ce8 100644
--- a/drivers/ata/pata_bf54x.c
+++ b/drivers/ata/pata_bf54x.c
@@ -1406,7 +1406,7 @@ static unsigned int bfin_ata_host_intr(struct ata_port *ap,
 	/* ack bmdma irq events */
 	ap->ops->sff_irq_clear(ap);
 
-	ata_sff_hsm_move(ap, qc, status, 0);
+	ata_sff_hsm_move(qc, status, 0);
 
 	if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
 				       qc->tf.protocol == ATAPI_PROT_DMA))
diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index 031d7b7..061f471 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -413,7 +413,7 @@ static void sil_host_intr(struct ata_port *ap, u32 bmdma2)
 	ata_sff_irq_clear(ap);
 
 	/* kick HSM in the ass */
-	ata_sff_hsm_move(ap, qc, status, 0);
+	ata_sff_hsm_move(qc, status, 0);
 
 	if (unlikely(qc->err_mask) && ata_is_dma(qc->tf.protocol))
 		ata_ehi_push_desc(ehi, "BMDMA2 stat 0x%x", bmdma2);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 59b0f1c..0a14022 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -639,6 +639,9 @@ struct ata_eh_context {
 	u8			saved_xfer_mode[ATA_MAX_DEVICES];
 	/* timestamp for the last reset attempt or success */
 	unsigned long		last_reset;
+#ifdef CONFIG_ATA_SFF
+	unsigned int		sff_devmask;
+#endif
 };
 
 struct ata_acpi_drive
@@ -1511,8 +1514,7 @@ extern unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev,
 			unsigned char *buf, unsigned int buflen, int rw);
 extern u8 ata_sff_irq_on(struct ata_port *ap);
 extern void ata_sff_irq_clear(struct ata_port *ap);
-extern int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
-			    u8 status, int in_wq);
+extern int ata_sff_hsm_move(struct ata_queued_cmd *qc, u8 status, int in_wq);
 extern unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc);
 extern bool ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc);
 extern unsigned int ata_sff_host_intr(struct ata_port *ap,

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Wed, 12 Nov 2008 17:15:58 +0900, Tejun Heo <tj@kernel.org> wrote:

> Can you please test the attached patch?


ata_piix 0000:00:07.1: version 2.12
scsi0 : ata_piix
scsi1 : ata_piix
ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
ata1.00: 66055248 sectors, multi 16: LBA
ata1.00: configured for MWDMA2
ata2.01: NODEV after polling detection
ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
ata2.00: configured for MWDMA2

This is the output I get. I do not know if I have any stalls there since I
am not in front of the machine right now.
But if I understand "NODEV after polling detection" correctly, the code is
no able to see that there is nothing attached there.
I will give it another reboot when I am back home to see if the stall is
still happening.

On a side note, since we are already talking about old chipsets and mobos.
Since this is a rather old hardware it was not possible to get the 40GB IBM
disk running in the first place. 
What I did back then (2.4 kernel days) was to use "ibmsetmax" to clamp the
disk to a smaller size and enabled the relevent option in the kernel. This
way I got past the BIOS (it stopped if the harddisk was saying it was >
32GB??) but the linux kernel correctly saw the right size.
Looking ad the dmesg output right now I think that it is only seeing the
smaller size.

sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
support DPO

ibmsetmax output:
 ./ibmsetmax /dev/sda
Using device /dev/sda
native max address: 66055247
that is 33820286976 bytes, 33.8 GB
lba capacity: 66055248 sectors (33820286976 bytes)

Is the clamping feature present in 2.6 by default? I cannot test this with
a 2.4 kernel right now, since the libc no longer supports it. :)

Anyway thank you all very much for your time and support,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

Hello,

Michael Guntsche wrote:
> ata_piix 0000:00:07.1: version 2.12
> scsi0 : ata_piix
> scsi1 : ata_piix
> ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
> ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
> ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
> ata1.00: 66055248 sectors, multi 16: LBA
> ata1.00: configured for MWDMA2
> ata2.01: NODEV after polling detection
> ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
> ata2.00: configured for MWDMA2
> 
> This is the output I get. I do not know if I have any stalls there since I
> am not in front of the machine right now.
> But if I understand "NODEV after polling detection" correctly, the code is
> no able to see that there is nothing attached there.
> I will give it another reboot when I am back home to see if the stall is
> still happening.

Yeap, it's working correctly now.

> On a side note, since we are already talking about old chipsets and mobos.
> Since this is a rather old hardware it was not possible to get the 40GB IBM
> disk running in the first place. 
> What I did back then (2.4 kernel days) was to use "ibmsetmax" to clamp the
> disk to a smaller size and enabled the relevent option in the kernel. This
> way I got past the BIOS (it stopped if the harddisk was saying it was >
> 32GB??) but the linux kernel correctly saw the right size.
> Looking ad the dmesg output right now I think that it is only seeing the
> smaller size.
> 
> sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
> support DPO
> 
> ibmsetmax output:
>  ./ibmsetmax /dev/sda
> Using device /dev/sda
> native max address: 66055247
> that is 33820286976 bytes, 33.8 GB
> lba capacity: 66055248 sectors (33820286976 bytes)
> 
> Is the clamping feature present in 2.6 by default? I cannot test this with
> a 2.4 kernel right now, since the libc no longer supports it. :)

Does libata.ignore_hpa=1 help?

-- 
tejun

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Wed, 12 Nov 2008 18:27:38 +0900, Tejun Heo <tj@kernel.org> wrote:
> Hello,
> Yeap, it's working correctly now.

Good to know

> Does libata.ignore_hpa=1 help?

No it does not.

/proc/cmdline:
==============

auto BOOT_IMAGE=Linux ro root=802 libata.ignore_hpa=1

dmesg:
======

ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
ata1.00: 66055248 sectors, multi 16: LBA
ata1.00: configured for MWDMA2
ata2.01: NODEV after polling detection
ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
ata2.00: configured for MWDMA2
scsi 0:0:0:0: Direct-Access     ATA      IC35L040AVER07-0 ER4O PQ: 0 ANSI:
5
sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
support DPO or FUA
sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
support DPO or FUA

ibmsetmax:
==========

Using device /dev/sda
native max address: 66055247
that is 33820286976 bytes, 33.8 GB
lba capacity: 66055248 sectors (33820286976 bytes)

Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

Michael Guntsche wrote:
> On Wed, 12 Nov 2008 18:27:38 +0900, Tejun Heo <tj@kernel.org> wrote:
>> Hello,
>> Yeap, it's working correctly now.
> 
> Good to know
> 
>> Does libata.ignore_hpa=1 help?
> 
> No it does not.
> 
> /proc/cmdline:
> ==============
> 
> auto BOOT_IMAGE=Linux ro root=802 libata.ignore_hpa=1
> 
> dmesg:
> ======
> 
> ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
> ata1.00: 66055248 sectors, multi 16: LBA
> ata1.00: configured for MWDMA2
> ata2.01: NODEV after polling detection
> ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
> ata2.00: configured for MWDMA2
> scsi 0:0:0:0: Direct-Access     ATA      IC35L040AVER07-0 ER4O PQ: 0 ANSI:
> 5
> sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
> support DPO or FUA
> sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
> support DPO or FUA
> 
> ibmsetmax:
> ==========
> 
> Using device /dev/sda
> native max address: 66055247
> that is 33820286976 bytes, 33.8 GB
> lba capacity: 66055248 sectors (33820286976 bytes)

Care to post "hdparm -I /dev/sda"?

-- 
tejun

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Wed, 12 Nov 2008 18:48:50 +0900, Tejun Heo <tj@kernel.org> wrote:

> Care to post "hdparm -I /dev/sda"?

Here it is.

/dev/sda:

ATA device, with non-removable media
powers-up in standby; SET FEATURES subcmd spins-up.
        Model Number:       IC35L040AVER07-0
        Serial Number:      SXPTXGK0136
        Firmware Revision:  ER4OA45A
Standards:
        Used: ATA/ATAPI-5 T13 1321D revision 1
        Supported: 5 4 3 & some of 6
Configuration:
        Logical         max     current
        cylinders       16383   16383
        heads           16      16
        sectors/track   63      63
        --
        CHS current addressable sectors:   16514064
        LBA    user addressable sectors:   66055248
        device size with M = 1024*1024:       32253 MBytes
        device size with M = 1000*1000:       33820 MBytes (33 GB)
Capabilities:
        LBA, IORDY(can be disabled)
        bytes avail on r/w long: 40     Queue depth: 32
        Standby timer values: spec'd by Standard, no device specific
minimum
        R/W multiple sector transfer: Max = 16  Current = 16
        Advanced power management level: disabled
        Recommended acoustic management value: 128, current value: 254
        DMA: mdma0 mdma1 *mdma2 udma0 udma1 udma2 udma3 udma4 udma5
             Cycle time: min=120ns recommended=120ns
        PIO: pio0 pio1 pio2 pio3 pio4
             Cycle time: no flow control=240ns  IORDY flow control=120ns
Commands/features:
        Enabled Supported:
           *    SMART feature set
                Security Mode feature set
           *    Power Management feature set
           *    Write cache
           *    Look-ahead
                Release interrupt
           *    Host Protected Area feature set
           *    WRITE_BUFFER command
           *    READ_BUFFER command
           *    NOP cmd
           *    READ/WRITE_DMA_QUEUED
                Advanced Power Management feature set
                Power-Up In Standby feature set
                SET_FEATURES required to spinup after power up
                Address Offset Reserved Area Boot
                SET_MAX security extension
           *    Automatic Acoustic Management feature set
           *    Device Configuration Overlay feature set
Security:
        Master password revision code = 65534
                supported
        not     enabled
        not     locked
        not     frozen
        not     expired: security count
        not     supported: enhanced erase
        30min for SECURITY ERASE UNIT.
HW reset results:
        CBLID- above Vih
        Device num = 0 determined by the jumper
Checksum: correct


/Mike

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Michael Guntsche wrote:
> On Wed, 12 Nov 2008 18:48:50 +0900, Tejun Heo <tj@kernel.org> wrote:
> 
>> Care to post "hdparm -I /dev/sda"?
> 
> Here it is.
> 
> /dev/sda:
> 
> ATA device, with non-removable media
> powers-up in standby; SET FEATURES subcmd spins-up.
>         Model Number:       IC35L040AVER07-0
>         Serial Number:      SXPTXGK0136
>         Firmware Revision:  ER4OA45A
> Standards:
>         Used: ATA/ATAPI-5 T13 1321D revision 1
>         Supported: 5 4 3 & some of 6
> Configuration:
>         Logical         max     current
>         cylinders       16383   16383
>         heads           16      16
>         sectors/track   63      63
>         --
>         CHS current addressable sectors:   16514064
>         LBA    user addressable sectors:   66055248
>         device size with M = 1024*1024:       32253 MBytes
>         device size with M = 1000*1000:       33820 MBytes (33 GB)
> Capabilities:
>         LBA, IORDY(can be disabled)
>         bytes avail on r/w long: 40     Queue depth: 32
>         Standby timer values: spec'd by Standard, no device specific
> minimum
>         R/W multiple sector transfer: Max = 16  Current = 16
>         Advanced power management level: disabled
>         Recommended acoustic management value: 128, current value: 254
>         DMA: mdma0 mdma1 *mdma2 udma0 udma1 udma2 udma3 udma4 udma5
>              Cycle time: min=120ns recommended=120ns
>         PIO: pio0 pio1 pio2 pio3 pio4
>              Cycle time: no flow control=240ns  IORDY flow control=120ns
> Commands/features:
>         Enabled Supported:
>            *    SMART feature set
>                 Security Mode feature set
>            *    Power Management feature set
>            *    Write cache
>            *    Look-ahead
>                 Release interrupt
>            *    Host Protected Area feature set
>            *    WRITE_BUFFER command
>            *    READ_BUFFER command
>            *    NOP cmd
>            *    READ/WRITE_DMA_QUEUED
>                 Advanced Power Management feature set
>                 Power-Up In Standby feature set
>                 SET_FEATURES required to spinup after power up
>                 Address Offset Reserved Area Boot
>                 SET_MAX security extension
>            *    Automatic Acoustic Management feature set
>            *    Device Configuration Overlay feature set
> Security:
>         Master password revision code = 65534
>                 supported
>         not     enabled
>         not     locked
>         not     frozen
>         not     expired: security count
>         not     supported: enhanced erase
>         30min for SECURITY ERASE UNIT.
> HW reset results:
>         CBLID- above Vih
>         Device num = 0 determined by the jumper
> Checksum: correct
..

One more:  Let's see "hdparm -N" for that drive
(note to self: add -N functionality to -I someday..).

Thanks

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Thu, 13 Nov 2008 21:38:30 -0500, Mark Lord <liml@rtr.ca> wrote:

> One more:  Let's see "hdparm -N" for that drive
> (note to self: add -N functionality to -I someday..).

/dev/sda:
 max sectors   = 66055248/15723600, HPA setting seems invalid

Hmm, is the "HPA setting seems invalid" an indication that this drive is
clamped?

Hope that helps,
Mike

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Michael Guntsche wrote:
> On Thu, 13 Nov 2008 21:38:30 -0500, Mark Lord <liml@rtr.ca> wrote:
> 
>> One more:  Let's see "hdparm -N" for that drive
>> (note to self: add -N functionality to -I someday..).
> 
> /dev/sda:
>  max sectors   = 66055248/15723600, HPA setting seems invalid
> 
> Hmm, is the "HPA setting seems invalid" an indication that this drive is clamped?
..

It's definitely clamped, but that output is a sign that you
may need a newer version of hdparm -- could you try that again
using hdparm-9.3 from sourceforge?

Thanks

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Mark Lord wrote:
> Michael Guntsche wrote:
>> On Thu, 13 Nov 2008 21:38:30 -0500, Mark Lord <liml@rtr.ca> wrote:
>>
>>> One more:  Let's see "hdparm -N" for that drive
>>> (note to self: add -N functionality to -I someday..).
>>
>> /dev/sda:
>>  max sectors   = 66055248/15723600, HPA setting seems invalid
>>
>> Hmm, is the "HPA setting seems invalid" an indication that this drive 
>> is clamped?
> ..
> 
> It's definitely clamped, but that output is a sign that you
> may need a newer version of hdparm -- could you try that again
> using hdparm-9.3 from sourceforge?
..

To clarify, the second number reported should NEVER be less than
the first number.  If it is, then there's a bug in the low-level
libata driver for your SATA/IDE controller -- it's not returning
the high-order-bytes from the SAT command.  Several drivers had
this bug until 2.6.27 or so, and some may still misbehave.

A newer version of hdparm will verify that, and also show the correct
value (hopefully) obtained via a completely different mechanism.

Thanks

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Guntsche Michael]

On Nov 14, 2008, at 18:24, Mark Lord wrote:

> To clarify, the second number reported should NEVER be less than
> the first number.  If it is, then there's a bug in the low-level
> libata driver for your SATA/IDE controller -- it's not returning
> the high-order-bytes from the SAT command.  Several drivers had
> this bug until 2.6.27 or so, and some may still misbehave.
>
> A newer version of hdparm will verify that, and also show the correct
> value (hopefully) obtained via a completely different mechanism.
>
> Thanks

Here the new output, it still does not look right, or am I wrong?

/dev/sda:
  max sectors   = 66055248/15723600(66055248?), HPA setting seems  
invalid (buggy kernel device driver?)

Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Guntsche Michael wrote:
> 
> On Nov 14, 2008, at 18:24, Mark Lord wrote:
> 
>> To clarify, the second number reported should NEVER be less than
>> the first number.  If it is, then there's a bug in the low-level
>> libata driver for your SATA/IDE controller -- it's not returning
>> the high-order-bytes from the SAT command.  Several drivers had
>> this bug until 2.6.27 or so, and some may still misbehave.
>>
>> A newer version of hdparm will verify that, and also show the correct
>> value (hopefully) obtained via a completely different mechanism.
>>
>> Thanks
> 
> Here the new output, it still does not look right, or am I wrong?
> 
> /dev/sda:
>  max sectors   = 66055248/15723600(66055248?), HPA setting seems invalid 
> (buggy kernel device driver?)
..

Ahh, okay, no HPA in effect.

   max sectors = 66055248/ ... (66055248)

That's the entire drive, ignoring the buggy libata driver
that reports 15723600 (returns zeros for hob_lba[mhl]).

Michael Guntsche wrote:
> On a side note, since we are already talking about old chipsets and mobos.
> Since this is a rather old hardware it was not possible to get the 40GB IBM
> disk running in the first place. 
> What I did back then (2.4 kernel days) was to use "ibmsetmax" to clamp the
> disk to a smaller size and enabled the relevent option in the kernel. This
> way I got past the BIOS (it stopped if the harddisk was saying it was >
> 32GB??) but the linux kernel correctly saw the right size.
> Looking ad the dmesg output right now I think that it is only seeing the
> smaller size.
> 
> sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
..

Well, according to the drive, it *is* a 33GB drive, not a 40GB drive.
So whatever "ibmsetmax" did appears to be outside of the ATA standard
way of doing it.  Unless I've got a bug in hdparm that I don't know about. :)

> /dev/sda:
> 
> ATA device, with non-removable media
> powers-up in standby; SET FEATURES subcmd spins-up.
>         Model Number:       IC35L040AVER07-0
>         Serial Number:      SXPTXGK0136
>         Firmware Revision:  ER4OA45A
> Standards:
>         Used: ATA/ATAPI-5 T13 1321D revision 1
>         Supported: 5 4 3 & some of 6
> Configuration:
>         Logical         max     current
>         cylinders       16383   16383
>         heads           16      16
>         sectors/track   63      63
>         --
>         CHS current addressable sectors:   16514064
>         LBA    user addressable sectors:   66055248
>         device size with M = 1024*1024:       32253 MBytes
>         device size with M = 1000*1000:       33820 MBytes (33 GB)
..
>           *    Host Protected Area feature set
..

That last line bothers me -- the '*' indicates that an HPA is in effect,
but the -N output indicates otherwise.. mmm... 

Let's see:  66055248 = 0x3efec50.
If we mask off all but the lower 24 bits,
we get 0xefec50 = 15723600,
which matches what the buggy device driver
returned when it dropped the top 24 bits.

So probably no bug in hdparm.
Which is odd, because google tells me that
the IC35L040AVER07-0 really is a 40GB drive.

Mmm..

  

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Mark Lord wrote:
..
>>           *    Host Protected Area feature set
> ..
> 
> That last line bothers me -- the '*' indicates that an HPA is in effect,
> but the -N output indicates otherwise.. mmm...
> Let's see:  66055248 = 0x3efec50.
> If we mask off all but the lower 24 bits,
> we get 0xefec50 = 15723600,
> which matches what the buggy device driver
> returned when it dropped the top 24 bits.
> 
> So probably no bug in hdparm.
> Which is odd, because google tells me that
> the IC35L040AVER07-0 really is a 40GB drive.
> 
> Mmm..
..

Oh..
Since you now have hdparm-9.3, what do you get from "hdparm --dco-identify" ??

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Guntsche Michael]

On Nov 15, 2008, at 5:17, Mark Lord wrote:
>
> Oh..
> Since you now have hdparm-9.3, what do you get from "hdparm --dco- 
> identify" ??

DCO Revision: 0x0001
The following features can be selectively disabled via DCO:
	Transfer modes:
		 mdma0 mdma1 mdma2
		 udma0 udma1 udma2 udma3 udma4 udma5
	Real max sectors: 66055248
	ATA command/feature sets:
		 SMART self_test error_log security PUIS TCQ AAM HPA

Hmm, still the same max sectors.

Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Guntsche Michael]

On Nov 15, 2008, at 5:17, Mark Lord wrote:

> Mark Lord wrote:
>> Which is odd, because google tells me that
>> the IC35L040AVER07-0 really is a 40GB drive.
>> Mmm..

I found the problem Mark and I am SOOOOOO sorry for wasting your time  
here.
Your remark about the drive being 40GB made me think about it again.  
As you know this is a quite old router that's running here.
When moving to 2.6 I also replaced the IBM disk with a new one (had  
several of them lying around back then).
The first one was CLAMPED with ibmsetmax. During the install of the  
new harddisk I clamped the new one with the JUMPERS on the DRIVE ond  
not with ibmsetmax.
I know remember that back then I thought that this would easier and  
more hassle free.
The libata stack, hdparm and everything else is working correctly it's  
the drive that's reporting 32GB!!!!!!!

Once again sorry for troubling you (all) with this non-issue.


Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Guntsche Michael wrote:
> 
> On Nov 15, 2008, at 5:17, Mark Lord wrote:
> 
>> Mark Lord wrote:
>>> Which is odd, because google tells me that
>>> the IC35L040AVER07-0 really is a 40GB drive.
>>> Mmm..
> 
> I found the problem Mark and I am SOOOOOO sorry for wasting your time here.
> Your remark about the drive being 40GB made me think about it again. As 
> you know this is a quite old router that's running here.
> When moving to 2.6 I also replaced the IBM disk with a new one (had 
> several of them lying around back then).
> The first one was CLAMPED with ibmsetmax. During the install of the new 
> harddisk I clamped the new one with the JUMPERS on the DRIVE ond not 
> with ibmsetmax.
> I know remember that back then I thought that this would easier and more 
> hassle free.
> The libata stack, hdparm and everything else is working correctly it's 
> the drive that's reporting 32GB!!!!!!!
..

Ahh.. that's better.  Thanks for figuring this one out for us all!

Cheers

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

Mark Lord wrote:
>> The libata stack, hdparm and everything else is working correctly it's
>> the drive that's reporting 32GB!!!!!!!
> ..
> 
> Ahh.. that's better.  Thanks for figuring this one out for us all!

So, the only real bug here is ata_piix not returning HOB registers?
That's weird.  It's ata_piix.  It's not likely to be broken.  :-(

-- 
tejun

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>>> The libata stack, hdparm and everything else is working correctly it's
>>> the drive that's reporting 32GB!!!!!!!
>> ..
>>
>> Ahh.. that's better.  Thanks for figuring this one out for us all!
> 
> So, the only real bug here is ata_piix not returning HOB registers?
> That's weird.  It's ata_piix.  It's not likely to be broken.  :-(
..

ata_piix works here for me here, too.
But I think he's using pata_piix, not ata_piix.

Fyi:  sata_sil24 had the HOB bug a few kernels ago,
but it's also now fine in 2.6.27.

-ml

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Nov 16, 2008, at 6:49, Mark Lord wrote:

> ata_piix works here for me here, too.
> But I think he's using pata_piix, not ata_piix.
>

No, I am using ata_piix here.

Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Nov 16, 2008, at 6:49, Mark Lord wrote:
> ata_piix works here for me here, too.
> But I think he's using pata_piix, not ata_piix.
>
> Fyi:  sata_sil24 had the HOB bug a few kernels ago,
> but it's also now fine in 2.6.27.

Forgot to mention that. I also tried it with the old IDE code and got  
the same results.
The reason why I am clamping the drive in the first place is that my  
BIOS/chipset hangs during boot if a larger disk is connected.
So maybe the results I am seeing here is "normal" since the hardware  
seems to be at it's limit. I know that clamping with ibmsetmax worked  
back then, but could it be that the Jumper on the Harddisk is doing it  
in a different way?

Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Sergei Shtylyov]

Hello.

Mark Lord wrote:

>>>> The libata stack, hdparm and everything else is working correctly it's
>>>> the drive that's reporting 32GB!!!!!!!
>>> ..
>>>
>>> Ahh.. that's better.  Thanks for figuring this one out for us all!
>>
>> So, the only real bug here is ata_piix not returning HOB registers?
>> That's weird.  It's ata_piix.  It's not likely to be broken.  :-(
> ..
>
> ata_piix works here for me here, too.
> But I think he's using pata_piix, not ata_piix.

   Huh? IIRC, there has never been pata_piix, just pata_oldpiix that 
only drives 82371FB.

WBR, Sergei

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Alan Cox]

> ata_piix works here for me here, too.
> But I think he's using pata_piix, not ata_piix.

There is no pata_piix ...