Skip to contentSkip to conversation

libata: fix device iteration bugs

PATCH Applied Tejun Heo 5 participants 17 messages Oct 26, 2008 – Oct 30, 2008 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/4 libata: fix device iteration bugs

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().

---
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(-)
drivers/ata/libata-core.c +8 -10
Index: 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;
 		}
 	}
drivers/ata/libata-eh.c +9 -0
Index: 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;
author
#1 lore raw

PATCH 2/4 libata: beef up iterators

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.

---
 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(-)
drivers/ata/libata-core.c +122 -61
Index: 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);
drivers/ata/libata-eh.c +37 -47

286 lines not on this page · raw message

include/linux/libata.h +30 -20

56 lines not on this page · raw message

drivers/ata/ahci.c +4 -4
Index: 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)
drivers/ata/ata_generic.c +1 -4
Index: 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;
drivers/ata/libata-acpi.c +8 -11
Index: 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);
drivers/ata/libata-pmp.c +11 -11
Index: 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;
 
drivers/ata/libata-scsi.c +11 -11
Index: 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;
drivers/ata/pata_it821x.c +16 -18
Index: 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;
drivers/ata/pata_ixp4xx_cf.c +6 -8
Index: 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;
 }
drivers/ata/pata_legacy.c +7 -10
Index: 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;
drivers/ata/pata_pdc2027x.c +12 -15
Index: 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;
drivers/ata/pata_platform.c +6 -8
Index: 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;
 }
drivers/ata/pata_rz1000.c +7 -9
Index: 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;
 }
drivers/ata/sata_sil.c +1 -1
Index: 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)
author
#2 lore raw

PATCH 3/4 libata: when restoring SControl during detach do the PMP links first

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.

---
 drivers/ata/libata-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ata/libata-core.c +1 -1
Index: 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);
author
#3 lore raw

PATCH 4/4 libata: perform port detach in EH

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.

---
 drivers/ata/libata-core.c |   23 ++++-------------------
 drivers/ata/libata-eh.c   |   32 +++++++++++++++++++++++++++++++-
 include/linux/libata.h    |    3 ++-
 3 files changed, 37 insertions(+), 21 deletions(-)
drivers/ata/libata-eh.c +31 -1
Index: 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);
include/linux/libata.h +2 -1
Index: 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 */
drivers/ata/libata-core.c +4 -19
Index: 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:
#4 lore raw

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

Quoted 8 lines · 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.

Quoted 6 lines
---
 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

#5 lore raw

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

In reply to Tejun Heo
Quoted 33 lines · 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.

---

[...]

Quoted 4 lines

Index: work/drivers/ata/libata-core.c
===================================================================

drivers/ata/libata-core.c +0 -0

[...]

drivers/ata/libata-core.c static void ata_lpm_enable(struct ata_ho · line 1187

Quoted 8 lines
drivers/ata/libata-core.c +2 -2
@@ -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?

[...]

Quoted 4 lines

Index: work/drivers/ata/libata-eh.c
===================================================================

drivers/ata/libata-eh.c +0 -0

[...]

Quoted 13 lines
drivers/ata/libata-eh.c +2 -3
@@ -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;
 }
 

drivers/ata/libata-eh.c static int ata_link_nr_vacant(struct ata · line 2887

Quoted 8 lines
drivers/ata/libata-eh.c +1 -1
@@ -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?

[...]

Quoted 4 lines

Index: work/drivers/ata/libata-scsi.c
===================================================================

drivers/ata/libata-scsi.c +0 -0

[...]

drivers/ata/libata-scsi.c void ata_scsi_scan_host(struct ata_port · line 3262

Quoted 13 lines
drivers/ata/libata-scsi.c +3 -3
@@ -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

author
#6 lore raw

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

Hello,

Quoted 7 lines · Elias Oltmanns

drivers/ata/libata-core.c static void ata_lpm_enable(struct ata_ho · line 1187

Quoted 4 lines
drivers/ata/libata-core.c +2 -2
-		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))

Quoted 4 lines
Quoted 4 lines
drivers/ata/libata-eh.c +0 -0
@@ -2891,7 +2881,7 @@ static int ata_link_nr_vacant(struct ata
 	struct ata_device *dev;
 	int cnt = 0;
 
Quoted 9 lines

drivers/ata/libata-eh.c static int ata_link_nr_vacant(struct ata · line 2887

Quoted 5 lines
drivers/ata/libata-eh.c +1 -1
-	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.

Quoted 15 lines

[...]

Quoted 4 lines

Index: work/drivers/ata/libata-scsi.c
===================================================================

drivers/ata/libata-scsi.c +0 -0

[...]

Quoted 9 lines
drivers/ata/libata-scsi.c +2 -3
@@ -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) {
Quoted 8 lines

drivers/ata/libata-scsi.c void ata_scsi_scan_host(struct ata_port · line 3262

Quoted 4 lines
drivers/ata/libata-scsi.c +1 -0
+			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.

Signature · 1 line
tejun
#7 lore raw

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

Quoted 11 lines · Tejun Heo

Hello,

Quoted 7 lines · Elias Oltmanns

drivers/ata/libata-core.c static void ata_lpm_enable(struct ata_ho · line 1187

Quoted 4 lines
drivers/ata/libata-core.c +2 -2
-		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?

include/linux/libata.h static inline int ata_link_active(struct · line 1313

Quoted 7 lines
include/linux/libata.h +7 -0
+#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.

Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
drivers/ata/libata-eh.c +0 -0
@@ -2891,7 +2881,7 @@ static int ata_link_nr_vacant(struct ata
 	struct ata_device *dev;
 	int cnt = 0;
 
Quoted 9 lines

drivers/ata/libata-eh.c static int ata_link_nr_vacant(struct ata · line 2887

Quoted 5 lines
drivers/ata/libata-eh.c +1 -1
-	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?

Quoted 3 lines

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.

Quoted 25 lines
Quoted 16 lines

[...]

Quoted 4 lines

Index: work/drivers/ata/libata-scsi.c
===================================================================

drivers/ata/libata-scsi.c +0 -0

[...]

Quoted 9 lines
drivers/ata/libata-scsi.c +2 -3
@@ -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) {
Quoted 8 lines

drivers/ata/libata-scsi.c void ata_scsi_scan_host(struct ata_port · line 3262

Quoted 4 lines
drivers/ata/libata-scsi.c +1 -0
+			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 ;-) ).

Quoted 3 lines

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

author
#8 lore raw

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

Quoted 10 lines · Elias Oltmanns
Quoted 9 lines · Tejun Heo

Hello,

Quoted 6 lines · Elias Oltmanns

drivers/ata/libata-core.c static void ata_lpm_enable(struct ata_ho · line 1187

Quoted 4 lines
drivers/ata/libata-core.c +2 -2
-		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?

Quoted 11 lines

include/linux/libata.h static inline int ata_link_active(struct · line 1313

Quoted 7 lines
include/linux/libata.h +7 -0
+#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. :-)

Signature · 1 line
tejun
#9 lore raw

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

Quoted 24 lines · Tejun Heo
Quoted 11 lines · Elias Oltmanns
Quoted 9 lines · Tejun Heo

Hello,

Quoted 6 lines · Elias Oltmanns

drivers/ata/libata-core.c static void ata_lpm_enable(struct ata_ho · line 1187

Quoted 4 lines
drivers/ata/libata-core.c +2 -2
-		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?

Quoted 11 lines

include/linux/libata.h static inline int ata_link_active(struct · line 1313

Quoted 7 lines
include/linux/libata.h +7 -0
+#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.

Quoted 2 lines

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

#10 lore raw

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

In reply to Tejun Heo
Quoted 41 lines · 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.

---
 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 +++----
Quoted 8 lines
 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

#11 lore raw

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

Quoted 52 lines · Jeff Garzik
Quoted 42 lines · 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.

---
 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 +++----
Quoted 8 lines
 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(-)
Quoted 3 lines

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

#12 lore raw

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

Quoted 53 lines · Mark Lord
Quoted 52 lines · Jeff Garzik
Quoted 50 lines · 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.

---
 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(-)
Quoted 6 lines
Quoted 3 lines

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

#13 lore raw
In reply to Tejun Heo

Hello.

Quoted 11 lines · 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().

More nitpicking... :-)

Quoted 15 lines

Index: work/drivers/ata/libata-core.c
===================================================================

drivers/ata/libata-core.c +0 -7
@@ -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

drivers/ata/libata-core.c static int cable_is_40wire(struct ata_po · line 4179

Quoted 10 lines
drivers/ata/libata-core.c +7 -2
-               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

author
#14 lore raw
Quoted 38 lines · Sergei Shtylyov

Hello.

Quoted 11 lines · 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().

More nitpicking... :-)

Quoted 20 lines

Index: work/drivers/ata/libata-core.c
===================================================================

drivers/ata/libata-core.c +3 -9
@@ -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
Quoted 8 lines

drivers/ata/libata-core.c static int cable_is_40wire(struct ata_po

Quoted 5 lines
drivers/ata/libata-core.c +4 -0
+     *   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.

Signature · 1 line
tejun
author
#15 lore raw

PATCH 1/4 libata: fix device iteration bugs

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().

---
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(-)
drivers/ata/libata-core.c +18 -14
Index: 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;
 		}
 	}
drivers/ata/libata-eh.c +9 -0
Index: 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;
Applied
#16 lore raw
Quoted 9 lines · 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().

Quoted 7 lines

---
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