[PATCHSET] libata: improve ata_bus_probe failure handling, take 2

[PATCHSET] libata: improve ata_bus_probe failure handling, take 2

[Tejun Heo]

Hello, Jeff, Alan and Albert.

This is the second take of improve-ata_bus_probe-failure-handling
patchset.  9 patches were accepted (%1-%8, %11) from the last take[1].
Changes from the last take are...

* ata_dev_absent() implemented (new patch)
* move sata_spd_limit from dev to ap
* move spd configuration into hardreset
* lengthen inter-reset delay to 5 sencs and add info message
* implement-ata_down_xfermask_limit patch is pushed back in series to
  just before its usage.

This patchset is against the current #upstream[2].

Jeff, ICH7 doesn't apply SPD configuration set while PHY is turned
off.  Sil3124 applies the change whether it's done before turning off
PHY or while PHY is off.  It seems that, SPD configuration should be
done before turning off PHY until more information is collected.

Note that even if the SPD configuration doesn't work, it doesn't cause
any harm other than retrying limited number of resets, and it is easy
to tell whether SATA SPD configuration works from the info messages.

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/9170
[2] 852ee16a914fb3ada2f81e222677c04defc2f15f

[PATCH 4/7] libata: add 5s sleep between resets

[Tejun Heo]

Some devices react badly if resets are performed back-to-back.  Give
devices some time to breath and tell user that we're taking a nap.

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

---

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

648928e9d46f4d2d8426d83614c101c4a41d789e
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index f8ee314..dfa2b36 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2577,6 +2577,9 @@ int ata_drive_probe_reset(struct ata_por
 		rc = ata_do_reset(ap, softreset, postreset, 0, classes);
 		if (rc == 0 && classes[0] != ATA_DEV_UNKNOWN)
 			goto done;
+		printk(KERN_INFO "ata%u: softreset failed, will try "
+		       "hardreset in 5 secs\n", ap->id);
+		ssleep(5);
 	}
 
 	if (!hardreset)
@@ -2592,10 +2595,20 @@ int ata_drive_probe_reset(struct ata_por
 
 		if (ata_down_sata_spd_limit(ap))
 			goto done;
+
+		printk(KERN_INFO "ata%u: hardreset failed, will retry "
+		       "in 5 secs\n", ap->id);
+		ssleep(5);
 	}
 
-	if (softreset)
+	if (softreset) {
+		printk(KERN_INFO "ata%u: hardreset succeeded without "
+		       "classification, will retry softreset in 5 secs",
+		       ap->id);
+		ssleep(5);
+
 		rc = ata_do_reset(ap, softreset, postreset, 0, classes);
+	}
 
  done:
 	if (rc == 0 && classes[0] == ATA_DEV_UNKNOWN)
-- 
1.2.4

[PATCH 4/7] libata: add 5s sleep between resets

[Tejun Heo]

Some devices react badly if resets are performed back-to-back.  Give
devices some time to breath and tell user that we're taking a nap.

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

---

The last message was missing '\n'.  Other than that, identical patch.

 libata-core.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2006-04-02 20:38:21.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2006-04-02 20:38:21.000000000 +0900
@@ -2585,6 +2585,9 @@ int ata_drive_probe_reset(struct ata_por
 		rc = ata_do_reset(ap, softreset, postreset, 0, classes);
 		if (rc == 0 && classes[0] != ATA_DEV_UNKNOWN)
 			goto done;
+		printk(KERN_INFO "ata%u: softreset failed, will try "
+		       "hardreset in 5 secs\n", ap->id);
+		ssleep(5);
 	}
 
 	if (!hardreset)
@@ -2600,10 +2603,20 @@ int ata_drive_probe_reset(struct ata_por
 
 		if (ata_down_sata_spd_limit(ap))
 			goto done;
+
+		printk(KERN_INFO "ata%u: hardreset failed, will retry "
+		       "in 5 secs\n", ap->id);
+		ssleep(5);
 	}
 
-	if (softreset)
+	if (softreset) {
+		printk(KERN_INFO "ata%u: hardreset succeeded without "
+		       "classification, will retry softreset in 5 secs\n",
+		       ap->id);
+		ssleep(5);
+
 		rc = ata_do_reset(ap, softreset, postreset, 0, classes);
+	}
 
  done:
 	if (rc == 0 && classes[0] == ATA_DEV_UNKNOWN)

[PATCH 6/7] libata: improve ata_bus_probe()

[Tejun Heo]

Improve ata_bus_probe() such that configuration failures are handled
better.  Each device is given ATA_PROBE_MAX_TRIES chances, but any
non-transient error (revalidation failure with -ENODEV, configuration
failure with -EINVAL...) disables the device directly.  Any IO error
results in SATA PHY speed down and ata_set_mode() failure lowers
transfer mode.  The last try always puts a device into PIO-0.

After each failure, the whole port is reset to make sure that the
controller and all the devices are in a known and stable state.  The
reset also applies SATA SPD configuration if necessary.

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

---

 drivers/scsi/libata-core.c |   65 +++++++++++++++++++++++++++++++++-----------
 include/linux/libata.h     |    3 ++
 2 files changed, 52 insertions(+), 16 deletions(-)

5dfcc0a5314aa0524b389503fe12a7ee4446ea99
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 42a3147..1460cb3 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1370,11 +1370,18 @@ err_out_nosup:
 static int ata_bus_probe(struct ata_port *ap)
 {
 	unsigned int classes[ATA_MAX_DEVICES];
-	int i, rc, found = 0;
+	int tries[ATA_MAX_DEVICES];
+	int i, rc, down_xfermask;
 	struct ata_device *dev;
 
 	ata_port_probe(ap);
 
+	for (i = 0; i < ATA_MAX_DEVICES; i++)
+		tries[i] = ATA_PROBE_MAX_TRIES;
+
+ retry:
+	down_xfermask = 0;
+
 	/* reset and determine device classes */
 	for (i = 0; i < ATA_MAX_DEVICES; i++)
 		classes[i] = ATA_DEV_UNKNOWN;
@@ -1404,21 +1411,23 @@ static int ata_bus_probe(struct ata_port
 		dev = &ap->device[i];
 		dev->class = classes[i];
 
-		if (!ata_dev_enabled(dev))
-			continue;
-
-		WARN_ON(dev->id != NULL);
-		if (ata_dev_read_id(ap, dev, &dev->class, 1, &dev->id)) {
-			dev->class = ATA_DEV_NONE;
-			continue;
+		if (!tries[i]) {
+			ata_down_xfermask_limit(ap, dev, 1);
+			ata_dev_disable(ap, dev);
 		}
 
-		if (ata_dev_configure(ap, dev, 1)) {
-			ata_dev_disable(ap, dev);
+		if (!ata_dev_enabled(dev))
 			continue;
-		}
 
-		found = 1;
+		kfree(dev->id);
+		dev->id = NULL;
+		rc = ata_dev_read_id(ap, dev, &dev->class, 1, &dev->id);
+		if (rc)
+			goto fail;
+
+		rc = ata_dev_configure(ap, dev, 1);
+		if (rc)
+			goto fail;
 	}
 
 	/* configure transfer mode */
@@ -1427,12 +1436,18 @@ static int ata_bus_probe(struct ata_port
 		 * return error code and failing device on failure as
 		 * ata_set_mode() does.
 		 */
-		if (found)
-			ap->ops->set_mode(ap);
+		for (i = 0; i < ATA_MAX_DEVICES; i++)
+			if (ata_dev_enabled(&ap->device[i])) {
+				ap->ops->set_mode(ap);
+				break;
+			}
 		rc = 0;
 	} else {
-		while (ata_set_mode(ap, &dev))
-			ata_dev_disable(ap, dev);
+		rc = ata_set_mode(ap, &dev);
+		if (rc) {
+			down_xfermask = 1;
+			goto fail;
+		}
 	}
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++)
@@ -1443,6 +1458,24 @@ static int ata_bus_probe(struct ata_port
 	ata_port_disable(ap);
 	ap->ops->port_disable(ap);
 	return -ENODEV;
+
+ fail:
+	switch (rc) {
+	case -EINVAL:
+	case -ENODEV:
+		tries[dev->devno] = 0;
+		break;
+	case -EIO:
+		ata_down_sata_spd_limit(ap);
+		/* fall through */
+	default:
+		tries[dev->devno]--;
+		if (down_xfermask &&
+		    ata_down_xfermask_limit(ap, dev, tries[dev->devno] == 1))
+			tries[dev->devno] = 0;
+	}
+
+	goto retry;
 }
 
 /**
diff --git a/include/linux/libata.h b/include/linux/libata.h
index a5207e6..a4a1e63 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -211,6 +211,9 @@ enum {
 	/* Masks for port functions */
 	ATA_PORT_PRIMARY	= (1 << 0),
 	ATA_PORT_SECONDARY	= (1 << 1),
+
+	/* how hard are we gonna try to probe/recover devices */
+	ATA_PROBE_MAX_TRIES	= 3,
 };
 
 enum hsm_task_states {
-- 
1.2.4

Re: [PATCH 6/7] libata: improve ata_bus_probe()

[Jeff Garzik]

Tejun Heo wrote:
> +
> + fail:
> +	switch (rc) {
> +	case -EINVAL:
> +	case -ENODEV:
> +		tries[dev->devno] = 0;
> +		break;
> +	case -EIO:
> +		ata_down_sata_spd_limit(ap);


I starting to think that separating ata_down_sata_spd_limit() from the 
actual phy programming is a bad idea.

However, I'm not going to NAK it just yet, we'll see where this goes.

	Jeff

[PATCH 3/7] libata: use SATA speed down in ata_drive_probe_reset()

[Tejun Heo]

Make ata_drive_probe_reset() use SATA SPD configuration.  Hardreset
will be force if speed renegotiation is necessary.  Also, if a
hardreset fails, PHY speed is stepped down and hardreset is retried
until the lowest speed is reached.

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

---

 drivers/scsi/libata-core.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

9281b880f7c1b2c88373eee9b5e33f5909714124
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 39afae1..f8ee314 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2573,7 +2573,7 @@ int ata_drive_probe_reset(struct ata_por
 	if (probeinit)
 		probeinit(ap);
 
-	if (softreset) {
+	if (softreset && !ata_set_sata_spd_needed(ap)) {
 		rc = ata_do_reset(ap, softreset, postreset, 0, classes);
 		if (rc == 0 && classes[0] != ATA_DEV_UNKNOWN)
 			goto done;
@@ -2582,9 +2582,17 @@ int ata_drive_probe_reset(struct ata_por
 	if (!hardreset)
 		goto done;
 
-	rc = ata_do_reset(ap, hardreset, postreset, 0, classes);
-	if (rc || classes[0] != ATA_DEV_UNKNOWN)
-		goto done;
+	while (1) {
+		rc = ata_do_reset(ap, hardreset, postreset, 0, classes);
+		if (rc == 0) {
+			if (classes[0] != ATA_DEV_UNKNOWN)
+				goto done;
+			break;
+		}
+
+		if (ata_down_sata_spd_limit(ap))
+			goto done;
+	}
 
 	if (softreset)
 		rc = ata_do_reset(ap, softreset, postreset, 0, classes);
-- 
1.2.4

[PATCH 5/7] libata: implement ata_down_xfermask_limit()

[Tejun Heo]

Implement ata_down_xfermask_limit().  This function manipulates
@dev->pio/mwdma/udma_mask such that the next lower transfer mode is
selected.  This will be used to improve ata_bus_probe() failure
handling and later by EH.

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

---

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

be65a1d4949d11d333866d10f79e6ef2165dd3cb
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index dfa2b36..42a3147 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -65,6 +65,8 @@ static unsigned int ata_dev_init_params(
 					struct ata_device *dev,
 					u16 heads,
 					u16 sectors);
+static int ata_down_xfermask_limit(struct ata_port *ap, struct ata_device *dev,
+				   int force_pio0);
 static int ata_down_sata_spd_limit(struct ata_port *ap);
 static int ata_set_mode(struct ata_port *ap, struct ata_device **r_failed_dev);
 static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
@@ -1859,6 +1861,56 @@ int ata_timing_compute(struct ata_device
 	return 0;
 }
 
+/**
+ *	ata_down_xfermask_limit - adjust dev xfer masks downward
+ *	@ap: Port associated with device @dev
+ *	@dev: Device to adjust xfer masks
+ *	@force_pio0: Force PIO0
+ *
+ *	Adjust xfer masks of @dev downward.  Note that this function
+ *	does not apply the change.  Invoking ata_set_mode() afterwards
+ *	will apply the limit.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno on failure
+ */
+static int ata_down_xfermask_limit(struct ata_port *ap, struct ata_device *dev,
+				   int force_pio0)
+{
+	unsigned long xfer_mask;
+	int highbit;
+
+	xfer_mask = ata_pack_xfermask(dev->pio_mask, dev->mwdma_mask,
+				      dev->udma_mask);
+
+	if (!xfer_mask)
+		goto fail;
+	/* don't gear down to MWDMA from UDMA, go directly to PIO */
+	if (xfer_mask & ATA_MASK_UDMA)
+		xfer_mask &= ~ATA_MASK_MWDMA;
+
+	highbit = fls(xfer_mask) - 1;
+	xfer_mask &= ~(1 << highbit);
+	if (force_pio0)
+		xfer_mask &= 1 << ATA_SHIFT_PIO;
+	if (!xfer_mask)
+		goto fail;
+
+	ata_unpack_xfermask(xfer_mask, &dev->pio_mask, &dev->mwdma_mask,
+			    &dev->udma_mask);
+
+	printk(KERN_WARNING "ata%u: dev %u limiting speed to %s\n",
+	       ap->id, dev->devno, ata_mode_string(xfer_mask));
+
+	return 0;
+
+ fail:
+	return -EINVAL;
+}
+
 static int ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
 {
 	unsigned int err_mask;
-- 
1.2.4

[PATCH 2/7] libata: implement ap->sata_spd_limit and helpers

[Tejun Heo]

ap->sata_spd_limit contrains SATA PHY speed of the port.  It is
initialized to the configured value prior to probing thus preserving
BIOS configured value.  hardreset is responsible for applying SPD
limit and sata_std_hardreset() is updated to do that.  SATA SPD limit
will be used to enhance failure handling during probing and later by
EH.

This patch also normalizes some comments around affected code.

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

---

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

1e78d28ef3d3c87b059e74e4414071952cab0493
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 3acf562..39afae1 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -65,6 +65,7 @@ static unsigned int ata_dev_init_params(
 					struct ata_device *dev,
 					u16 heads,
 					u16 sectors);
+static int ata_down_sata_spd_limit(struct ata_port *ap);
 static int ata_set_mode(struct ata_port *ap, struct ata_device **r_failed_dev);
 static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
 					 struct ata_device *dev);
@@ -1596,6 +1597,120 @@ void ata_port_disable(struct ata_port *a
 	ap->flags |= ATA_FLAG_PORT_DISABLED;
 }
 
+/**
+ *	ata_down_sata_spd_limit - adjust SATA spd limit downward
+ *	@ap: Port to adjust SATA spd limit for
+ *
+ *	Adjust SATA spd limit of @ap downward.  Note that this
+ *	function only adjusts the limit.  The change must be applied
+ *	using ata_set_sata_spd().
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno on failure
+ */
+static int ata_down_sata_spd_limit(struct ata_port *ap)
+{
+	u32 spd, mask;
+	int highbit;
+
+	if (ap->cbl != ATA_CBL_SATA || !ap->ops->scr_read)
+		return -EOPNOTSUPP;
+
+	mask = ap->sata_spd_limit;
+	if (mask <= 1)
+		return -EINVAL;
+	highbit = fls(mask) - 1;
+	mask &= ~(1 << highbit);
+
+	spd = (scr_read(ap, SCR_STATUS) >> 4) & 0xf;
+	if (spd <= 1)
+		return -EINVAL;
+	spd--;
+	mask &= (1 << spd) - 1;
+	if (!mask)
+		return -EINVAL;
+
+	ap->sata_spd_limit = mask;
+
+	printk(KERN_WARNING "ata%u: limiting SATA link speed to %s\n",
+	       ap->id, sata_spd_string(fls(mask)));
+
+	return 0;
+}
+
+static int __ata_set_sata_spd_needed(struct ata_port *ap, u32 *scontrol)
+{
+	u32 spd, limit;
+
+	if (ap->sata_spd_limit == UINT_MAX)
+		limit = 0;
+	else
+		limit = fls(ap->sata_spd_limit);
+
+	spd = (*scontrol >> 4) & 0xf;
+	*scontrol = (*scontrol & ~0xf0) | ((limit & 0xf) << 4);
+
+	return spd != limit;
+}
+
+/**
+ *	ata_set_sata_spd_needed - is SATA spd configuration needed
+ *	@ap: Port in question
+ *
+ *	Test whether the spd limit in SControl matches
+ *	@ap->sata_spd_limit.  This function is used to determine
+ *	whether hardreset is necessary to apply SATA spd
+ *	configuration.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	1 if SATA spd configuration is needed, 0 otherwise.
+ */
+static int ata_set_sata_spd_needed(struct ata_port *ap)
+{
+	u32 scontrol;
+
+	if (ap->cbl != ATA_CBL_SATA || !ap->ops->scr_read)
+		return 0;
+
+	scontrol = scr_read(ap, SCR_CONTROL);
+
+	return __ata_set_sata_spd_needed(ap, &scontrol);
+}
+
+/**
+ *	ata_set_sata_spd - set SATA spd according to spd limit
+ *	@ap: Port to set SATA spd for
+ *
+ *	Set SATA spd of @ap according to sata_spd_limit.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	0 if spd doesn't need to be changed, 1 if spd has been
+ *	changed.  -EOPNOTSUPP if SCR registers are inaccessible.
+ */
+static int ata_set_sata_spd(struct ata_port *ap)
+{
+	u32 scontrol;
+
+	if (ap->cbl != ATA_CBL_SATA || !ap->ops->scr_read)
+		return -EOPNOTSUPP;
+
+	scontrol = scr_read(ap, SCR_CONTROL);
+	if (!__ata_set_sata_spd_needed(ap, &scontrol))
+		return 0;
+
+	scr_write(ap, SCR_CONTROL, scontrol);
+	return 1;
+}
+
 /*
  * This mode timing computation functionality is ported over from
  * drivers/ide/ide-timing.h and was originally written by Vojtech Pavlik
@@ -2165,7 +2280,14 @@ static int sata_phy_resume(struct ata_po
 void ata_std_probeinit(struct ata_port *ap)
 {
 	if ((ap->flags & ATA_FLAG_SATA) && ap->ops->scr_read) {
+		u32 spd;
+
 		sata_phy_resume(ap);
+
+		spd = (scr_read(ap, SCR_CONTROL) & 0xf0) >> 4;
+		if (spd)
+			ap->sata_spd_limit &= (1 << spd) - 1;
+
 		if (sata_dev_present(ap))
 			ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT);
 	}
@@ -2253,18 +2375,25 @@ int sata_std_hardreset(struct ata_port *
 
 	DPRINTK("ENTER\n");
 
-	/* Issue phy wake/reset */
+	/* SATA spec says nothing about how to reconfigure spd.
+	 * Configuring before starting hardreset works for both of my
+	 * test cases - ICH7 AHCI and sil3124.  Configuring while phy
+	 * is off sounds nice but ICH7 chokes on that.  It preserves
+	 * configured value but does not apply it.
+	 */
+	ata_set_sata_spd(ap);
+
+	/* issue phy wake/reset */
 	scontrol = scr_read(ap, SCR_CONTROL);
 	scontrol = (scontrol & 0x0f0) | 0x301;
 	scr_write_flush(ap, SCR_CONTROL, scontrol);
 
-	/*
-	 * Couldn't find anything in SATA I/II specs, but AHCI-1.1
+	/* Couldn't find anything in SATA I/II specs, but AHCI-1.1
 	 * 10.4.2 says at least 1 ms.
 	 */
 	msleep(1);
 
-	/* Bring phy back */
+	/* bring phy back */
 	sata_phy_resume(ap);
 
 	/* TODO: phy layer with polling, timeouts, etc. */
@@ -4454,6 +4583,7 @@ static void ata_host_init(struct ata_por
 	ap->flags |= ent->host_flags;
 	ap->ops = ent->port_ops;
 	ap->cbl = ATA_CBL_NONE;
+	ap->sata_spd_limit = UINT_MAX;
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 0f8e372..a5207e6 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -397,6 +397,7 @@ struct ata_port {
 	unsigned int		mwdma_mask;
 	unsigned int		udma_mask;
 	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
+	unsigned int		sata_spd_limit;	/* SATA PHY speed limit */
 
 	struct ata_device	device[ATA_MAX_DEVICES];
 
-- 
1.2.4

Re: [PATCH 2/7] libata: implement ap->sata_spd_limit and helpers

[Jeff Garzik]

Tejun Heo wrote:
> -	/* Issue phy wake/reset */
> +	/* SATA spec says nothing about how to reconfigure spd.
> +	 * Configuring before starting hardreset works for both of my
> +	 * test cases - ICH7 AHCI and sil3124.  Configuring while phy
> +	 * is off sounds nice but ICH7 chokes on that.  It preserves
> +	 * configured value but does not apply it.
> +	 */
> +	ata_set_sata_spd(ap);
> +
> +	/* issue phy wake/reset */
>  	scontrol = scr_read(ap, SCR_CONTROL);
>  	scontrol = (scontrol & 0x0f0) | 0x301;
>  	scr_write_flush(ap, SCR_CONTROL, scontrol);

This code is an example of configuring the phy while its off...  You put 
the speed setting before the phy wake.

Everything else looks OK.

	Jeff

Re: [PATCH 2/7] libata: implement ap->sata_spd_limit and helpers

[Tejun Heo]

On Sun, Apr 02, 2006 at 06:43:01AM -0400, Jeff Garzik wrote:
> Tejun Heo wrote:
> >-	/* Issue phy wake/reset */
> >+	/* SATA spec says nothing about how to reconfigure spd.
> >+	 * Configuring before starting hardreset works for both of my
> >+	 * test cases - ICH7 AHCI and sil3124.  Configuring while phy
> >+	 * is off sounds nice but ICH7 chokes on that.  It preserves
> >+	 * configured value but does not apply it.
> >+	 */
> >+	ata_set_sata_spd(ap);
> >+
> >+	/* issue phy wake/reset */
> > 	scontrol = scr_read(ap, SCR_CONTROL);
> > 	scontrol = (scontrol & 0x0f0) | 0x301;
> > 	scr_write_flush(ap, SCR_CONTROL, scontrol);
> 
> This code is an example of configuring the phy while its off...  You put 
> the speed setting before the phy wake.
> 
> Everything else looks OK.
> 

This is embarrasing. :( I was being delusional.  I somehow thought
DET=1 was PHY off and DET=0 was wake.  I'll test with actual PHY off
(DET=2) and redo this patch.  Sorry about the confusion.

-- 
tejun

Re: [PATCH 2/7] libata: implement ap->sata_spd_limit and helpers

[Jeff Garzik]

Tejun Heo wrote:
> On Sun, Apr 02, 2006 at 06:43:01AM -0400, Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> -	/* Issue phy wake/reset */
>>> +	/* SATA spec says nothing about how to reconfigure spd.
>>> +	 * Configuring before starting hardreset works for both of my
>>> +	 * test cases - ICH7 AHCI and sil3124.  Configuring while phy
>>> +	 * is off sounds nice but ICH7 chokes on that.  It preserves
>>> +	 * configured value but does not apply it.
>>> +	 */
>>> +	ata_set_sata_spd(ap);
>>> +
>>> +	/* issue phy wake/reset */
>>> 	scontrol = scr_read(ap, SCR_CONTROL);
>>> 	scontrol = (scontrol & 0x0f0) | 0x301;
>>> 	scr_write_flush(ap, SCR_CONTROL, scontrol);
>> This code is an example of configuring the phy while its off...  You put 
>> the speed setting before the phy wake.
>>
>> Everything else looks OK.
>>
> 
> This is embarrasing. :( I was being delusional.  I somehow thought

Well on the bright side the other 21 patches are OK :)


> DET=1 was PHY off and DET=0 was wake.  I'll test with actual PHY off
> (DET=2) and redo this patch.  Sorry about the confusion.

Well, whatever the results of testing, I don't want to deviate from 
scr_write([0x301 or 0x300]) being the first thing we do with the phy. 
That's what's tested and working in both Linux and Windows.  Don't be 
surprised at nutty hackery on Gen-1 controllers (PATA chips w/ SATA 
bridge), where deviating from the few tested SControl values and 
"programming situations" (chip states) can lead to trouble.

	Jeff

[PATCH 2/7] libata: implement ap->sata_spd_limit and helpers

[Tejun Heo]

ap->sata_spd_limit contrains SATA PHY speed of the port.  It is
initialized to the configured value prior to probing thus preserving
BIOS configured value.  hardreset is responsible for applying SPD
limit and sata_std_hardreset() is updated to do that.  SATA SPD limit
will be used to enhance failure handling during probing and later by
EH.

This patch also normalizes some comments around affected code.

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

---

Reconfiguring while PHY off works on both ICH7 AHCI and sil3124.
Althought it might read SCR_CONTROL one more time, this does not
change initial probing sequence as initial value is always identical
to the current value.  Also, this code never gets trigerred for Gen1
controllers as ata_down_sata_spd_limit() does not adjust the limit if
the current speed is Gen1.

There in one more patch to update in this set.  I forgot a new line in
patch#4.  I'll repost that.  All following patches in this set and in
prepare-for-new-EH patchset apply are oky with these changes.  They
apply nicely with offset messages.

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

Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h	2006-04-02 20:38:20.000000000 +0900
+++ work/include/linux/libata.h	2006-04-02 20:38:20.000000000 +0900
@@ -397,6 +397,7 @@ struct ata_port {
 	unsigned int		mwdma_mask;
 	unsigned int		udma_mask;
 	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
+	unsigned int		sata_spd_limit;	/* SATA PHY speed limit */
 
 	struct ata_device	device[ATA_MAX_DEVICES];
 
Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2006-04-02 20:38:18.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2006-04-02 20:38:21.000000000 +0900
@@ -65,6 +65,7 @@ static unsigned int ata_dev_init_params(
 					struct ata_device *dev,
 					u16 heads,
 					u16 sectors);
+static int ata_down_sata_spd_limit(struct ata_port *ap);
 static int ata_set_mode(struct ata_port *ap, struct ata_device **r_failed_dev);
 static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
 					 struct ata_device *dev);
@@ -1596,6 +1597,120 @@ void ata_port_disable(struct ata_port *a
 	ap->flags |= ATA_FLAG_PORT_DISABLED;
 }
 
+/**
+ *	ata_down_sata_spd_limit - adjust SATA spd limit downward
+ *	@ap: Port to adjust SATA spd limit for
+ *
+ *	Adjust SATA spd limit of @ap downward.  Note that this
+ *	function only adjusts the limit.  The change must be applied
+ *	using ata_set_sata_spd().
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno on failure
+ */
+static int ata_down_sata_spd_limit(struct ata_port *ap)
+{
+	u32 spd, mask;
+	int highbit;
+
+	if (ap->cbl != ATA_CBL_SATA || !ap->ops->scr_read)
+		return -EOPNOTSUPP;
+
+	mask = ap->sata_spd_limit;
+	if (mask <= 1)
+		return -EINVAL;
+	highbit = fls(mask) - 1;
+	mask &= ~(1 << highbit);
+
+	spd = (scr_read(ap, SCR_STATUS) >> 4) & 0xf;
+	if (spd <= 1)
+		return -EINVAL;
+	spd--;
+	mask &= (1 << spd) - 1;
+	if (!mask)
+		return -EINVAL;
+
+	ap->sata_spd_limit = mask;
+
+	printk(KERN_WARNING "ata%u: limiting SATA link speed to %s\n",
+	       ap->id, sata_spd_string(fls(mask)));
+
+	return 0;
+}
+
+static int __ata_set_sata_spd_needed(struct ata_port *ap, u32 *scontrol)
+{
+	u32 spd, limit;
+
+	if (ap->sata_spd_limit == UINT_MAX)
+		limit = 0;
+	else
+		limit = fls(ap->sata_spd_limit);
+
+	spd = (*scontrol >> 4) & 0xf;
+	*scontrol = (*scontrol & ~0xf0) | ((limit & 0xf) << 4);
+
+	return spd != limit;
+}
+
+/**
+ *	ata_set_sata_spd_needed - is SATA spd configuration needed
+ *	@ap: Port in question
+ *
+ *	Test whether the spd limit in SControl matches
+ *	@ap->sata_spd_limit.  This function is used to determine
+ *	whether hardreset is necessary to apply SATA spd
+ *	configuration.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	1 if SATA spd configuration is needed, 0 otherwise.
+ */
+static int ata_set_sata_spd_needed(struct ata_port *ap)
+{
+	u32 scontrol;
+
+	if (ap->cbl != ATA_CBL_SATA || !ap->ops->scr_read)
+		return 0;
+
+	scontrol = scr_read(ap, SCR_CONTROL);
+
+	return __ata_set_sata_spd_needed(ap, &scontrol);
+}
+
+/**
+ *	ata_set_sata_spd - set SATA spd according to spd limit
+ *	@ap: Port to set SATA spd for
+ *
+ *	Set SATA spd of @ap according to sata_spd_limit.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	0 if spd doesn't need to be changed, 1 if spd has been
+ *	changed.  -EOPNOTSUPP if SCR registers are inaccessible.
+ */
+static int ata_set_sata_spd(struct ata_port *ap)
+{
+	u32 scontrol;
+
+	if (ap->cbl != ATA_CBL_SATA || !ap->ops->scr_read)
+		return -EOPNOTSUPP;
+
+	scontrol = scr_read(ap, SCR_CONTROL);
+	if (!__ata_set_sata_spd_needed(ap, &scontrol))
+		return 0;
+
+	scr_write(ap, SCR_CONTROL, scontrol);
+	return 1;
+}
+
 /*
  * This mode timing computation functionality is ported over from
  * drivers/ide/ide-timing.h and was originally written by Vojtech Pavlik
@@ -2165,7 +2280,14 @@ static int sata_phy_resume(struct ata_po
 void ata_std_probeinit(struct ata_port *ap)
 {
 	if ((ap->flags & ATA_FLAG_SATA) && ap->ops->scr_read) {
+		u32 spd;
+
 		sata_phy_resume(ap);
+
+		spd = (scr_read(ap, SCR_CONTROL) & 0xf0) >> 4;
+		if (spd)
+			ap->sata_spd_limit &= (1 << spd) - 1;
+
 		if (sata_dev_present(ap))
 			ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT);
 	}
@@ -2253,18 +2375,30 @@ int sata_std_hardreset(struct ata_port *
 
 	DPRINTK("ENTER\n");
 
-	/* Issue phy wake/reset */
+	if (ata_set_sata_spd_needed(ap)) {
+		/* SATA spec says nothing about how to reconfigure
+		 * spd.  To be on the safe side, turn off phy during
+		 * reconfiguration.  This works for at least ICH7 AHCI
+		 * and Sil3124.
+		 */
+		scontrol = scr_read(ap, SCR_CONTROL);
+		scontrol = (scontrol & 0x0f0) | 0x302;
+		scr_write_flush(ap, SCR_CONTROL, scontrol);
+
+		ata_set_sata_spd(ap);
+	}
+
+	/* issue phy wake/reset */
 	scontrol = scr_read(ap, SCR_CONTROL);
 	scontrol = (scontrol & 0x0f0) | 0x301;
 	scr_write_flush(ap, SCR_CONTROL, scontrol);
 
-	/*
-	 * Couldn't find anything in SATA I/II specs, but AHCI-1.1
+	/* Couldn't find anything in SATA I/II specs, but AHCI-1.1
 	 * 10.4.2 says at least 1 ms.
 	 */
 	msleep(1);
 
-	/* Bring phy back */
+	/* bring phy back */
 	sata_phy_resume(ap);
 
 	/* TODO: phy layer with polling, timeouts, etc. */
@@ -4454,6 +4588,7 @@ static void ata_host_init(struct ata_por
 	ap->flags |= ent->host_flags;
 	ap->ops = ent->port_ops;
 	ap->cbl = ATA_CBL_NONE;
+	ap->sata_spd_limit = UINT_MAX;
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
 

[PATCH 1/7] libata: implement ata_dev_absent()

[Tejun Heo]

For the time being we cannot use ata_dev_present() as it was renamed
to ata_dev_enabled() but we still need presence test.  Implement
negation of the test.  Conveniently, the negated result is needed in
more places.  This is suggested by Jeff Garzik.

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

---

 include/linux/libata.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

21da2f0caf63d617a633a97635ab5d1ca5a407b1
diff --git a/include/linux/libata.h b/include/linux/libata.h
index c6883ba..0f8e372 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -682,6 +682,11 @@ static inline unsigned int ata_class_dis
 	return class == ATA_DEV_ATA_UNSUP || class == ATA_DEV_ATAPI_UNSUP;
 }
 
+static inline unsigned int ata_class_absent(unsigned int class)
+{
+	return !ata_class_enabled(class) && !ata_class_disabled(class);
+}
+
 static inline unsigned int ata_dev_enabled(const struct ata_device *dev)
 {
 	return ata_class_enabled(dev->class);
@@ -692,6 +697,11 @@ static inline unsigned int ata_dev_disab
 	return ata_class_disabled(dev->class);
 }
 
+static inline unsigned int ata_dev_absent(const struct ata_device *dev)
+{
+	return ata_class_absent(dev->class);
+}
+
 static inline u8 ata_chk_status(struct ata_port *ap)
 {
 	return ap->ops->check_status(ap);
-- 
1.2.4

[PATCH 7/7] libata: consider disabled devices in ata_dev_xfermask()

[Tejun Heo]

ata_bus_probe() now marks failed devices properly and leaves
meaningful transfer mode masks.  This patch makes ata_dev_xfermask()
consider disable devices when determining PIO mode to avoid violating
device selection timing.

While at it, move port-wide resttriction out of device iteration loop
and try to make the function look a bit prettier.

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

---

 drivers/scsi/libata-core.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)

5d1704cd08f45e1d66415eb0ad45e61793b385ef
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 1460cb3..b058602 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2908,23 +2908,34 @@ static void ata_dev_xfermask(struct ata_
 	unsigned long xfer_mask;
 	int i;
 
-	xfer_mask = ata_pack_xfermask(ap->pio_mask, ap->mwdma_mask,
-				      ap->udma_mask);
+	xfer_mask = ata_pack_xfermask(ap->pio_mask,
+				      ap->mwdma_mask, ap->udma_mask);
+
+	/* Apply cable rule here.  Don't apply it early because when
+	 * we handle hot plug the cable type can itself change.
+	 */
+	if (ap->cbl == ATA_CBL_PATA40)
+		xfer_mask &= ~(0xF8 << ATA_SHIFT_UDMA);
 
 	/* FIXME: Use port-wide xfermask for now */
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *d = &ap->device[i];
-		if (!ata_dev_enabled(d))
+
+		if (ata_dev_absent(d))
+			continue;
+
+		if (ata_dev_disabled(d)) {
+			/* to avoid violating device selection timing */
+			xfer_mask &= ata_pack_xfermask(d->pio_mask,
+						       UINT_MAX, UINT_MAX);
 			continue;
-		xfer_mask &= ata_pack_xfermask(d->pio_mask, d->mwdma_mask,
-					       d->udma_mask);
+		}
+
+		xfer_mask &= ata_pack_xfermask(d->pio_mask,
+					       d->mwdma_mask, d->udma_mask);
 		xfer_mask &= ata_id_xfermask(d->id);
 		if (ata_dma_blacklisted(d))
 			xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
-		/* Apply cable rule here. Don't apply it early because when
-		   we handle hot plug the cable type can itself change */
-		if (ap->cbl == ATA_CBL_PATA40)
-			xfer_mask &= ~(0xF8 << ATA_SHIFT_UDMA);
 	}
 
 	if (ata_dma_blacklisted(dev))
@@ -2935,11 +2946,12 @@ static void ata_dev_xfermask(struct ata_
 		if (hs->simplex_claimed)
 			xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
 	}
+
 	if (ap->ops->mode_filter)
 		xfer_mask = ap->ops->mode_filter(ap, dev, xfer_mask);
 
-	ata_unpack_xfermask(xfer_mask, &dev->pio_mask, &dev->mwdma_mask,
-			    &dev->udma_mask);
+	ata_unpack_xfermask(xfer_mask, &dev->pio_mask,
+			    &dev->mwdma_mask, &dev->udma_mask);
 }
 
 /**
-- 
1.2.4

Re: [PATCHSET] libata: improve ata_bus_probe failure handling, take 2

[Jeff Garzik]

Applied both series to #upstream.

Albert, please check the irq-pio branch and make sure my merge is correct.

	Jeff

Re: [PATCHSET] libata: improve ata_bus_probe failure handling, take 2

[Albert Lee]

Jeff Garzik wrote:
> Applied both series to #upstream.
> 
> Albert, please check the irq-pio branch and make sure my merge is correct.
> 

The irq-pio branch looks good.

I've a minor fix for cleanup, etc.
Will submit it later.

Albert

Re: [PATCHSET] libata: improve ata_bus_probe failure handling, take 2

[Jeff Garzik]

Albert Lee wrote:
> Jeff Garzik wrote:
>> Applied both series to #upstream.
>>
>> Albert, please check the irq-pio branch and make sure my merge is correct.
>>
> 
> The irq-pio branch looks good.
> 
> I've a minor fix for cleanup, etc.
> Will submit it later.


Thanks.

For the record, I do consider irq-pio upstream ready.  It still needs to 
wait for Tejun's EH work to make it upstream, to give upstream people 
the test sets of

	- prep EH (2.6.17-rc1)
	- EH
	- NCQ
	- irq-pio

Since each of these sets is staged, upstream testers will be able to say 
"2.6.17-rc1 works, but 2.6.18-git1 doesn't", which gives us a lot of 
good info.

	Jeff

Re: [PATCHSET] libata: improve ata_bus_probe failure handling, take 2

[Greg Freemyer]

On 4/3/06, Jeff Garzik <jgarzik@pobox.com> wrote:

> For the record, I do consider irq-pio upstream ready.  It still needs to
> wait for Tejun's EH work to make it upstream, to give upstream people
> the test sets of
>
>         - prep EH (2.6.17-rc1)
>         - EH
>         - NCQ
>         - irq-pio
>
> Since each of these sets is staged, upstream testers will be able to say
> "2.6.17-rc1 works, but 2.6.18-git1 doesn't", which gives us a lot of
> good info.
>
>         Jeff

So for those of us anxiously waiting for the above great enhancements
it looks like it will be 5 months or so before it is in a released
vanilla kernel?  ie. assuming 2 1/2 months per kernel release.

Not as good as I hoped, but not horrible either.

Greg
--
Greg Freemyer
The Norcross Group
Forensics for the 21st Century

Re: [PATCHSET] libata: improve ata_bus_probe failure handling, take 2

[Jeff Garzik]

Greg Freemyer wrote:
> On 4/3/06, Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>> For the record, I do consider irq-pio upstream ready.  It still needs to
>> wait for Tejun's EH work to make it upstream, to give upstream people
>> the test sets of
>>
>>         - prep EH (2.6.17-rc1)
>>         - EH
>>         - NCQ
>>         - irq-pio
>>
>> Since each of these sets is staged, upstream testers will be able to say
>> "2.6.17-rc1 works, but 2.6.18-git1 doesn't", which gives us a lot of
>> good info.
>>
>>         Jeff
> 
> So for those of us anxiously waiting for the above great enhancements
> it looks like it will be 5 months or so before it is in a released
> vanilla kernel?  ie. assuming 2 1/2 months per kernel release.

Heavens no :)

The stages above just need to be separated out by a few 2.6.X-gitX or 
2.6.X-rcX releases.

	Jeff

Re: [PATCHSET] libata: improve ata_bus_probe failure handling, take 2

[Greg Freemyer]

On 4/3/06, Jeff Garzik <jgarzik@pobox.com> wrote:
> Greg Freemyer wrote:
> > On 4/3/06, Jeff Garzik <jgarzik@pobox.com> wrote:
> >
> >> For the record, I do consider irq-pio upstream ready.  It still needs to
> >> wait for Tejun's EH work to make it upstream, to give upstream people
> >> the test sets of
> >>
> >>         - prep EH (2.6.17-rc1)
> >>         - EH
> >>         - NCQ
> >>         - irq-pio
> >>
> >> Since each of these sets is staged, upstream testers will be able to say
> >> "2.6.17-rc1 works, but 2.6.18-git1 doesn't", which gives us a lot of
> >> good info.
> >>
> >>         Jeff
> >
> > So for those of us anxiously waiting for the above great enhancements
> > it looks like it will be 5 months or so before it is in a released
> > vanilla kernel?  ie. assuming 2 1/2 months per kernel release.
>
> Heavens no :)
>
> The stages above just need to be separated out by a few 2.6.X-gitX or
> 2.6.X-rcX releases.
>
>         Jeff

I'm confused.  You just poseted:

> Due to the merge window closing, the three pending patchsets will be in
> 2.6.18.

That sure sounds like 5 months before it is in a released vanilla
kernel.  ie. it has been taking about 10 weeks per kernel release. 
2.6.17 is likely to take another 8 weeks.  Then 10 weeks for 2.6.18 to
come out.

It seems that the only way around it is to petition Linus to get as
much of the above into 2.6.17 as possible.  Somehow I doubt that will
be successful, but I don't monitor LKML, so I don't know if he ever
responds to requests like that or not.

Greg
--
Greg Freemyer
The Norcross Group
Forensics for the 21st Century

Re: [PATCHSET] libata: improve ata_bus_probe failure handling, take 2

[Jeff Garzik]

Greg Freemyer wrote:
> On 4/3/06, Jeff Garzik <jgarzik@pobox.com> wrote:
>> Greg Freemyer wrote:
>>> On 4/3/06, Jeff Garzik <jgarzik@pobox.com> wrote:
>>>
>>>> For the record, I do consider irq-pio upstream ready.  It still needs to
>>>> wait for Tejun's EH work to make it upstream, to give upstream people
>>>> the test sets of
>>>>
>>>>         - prep EH (2.6.17-rc1)
>>>>         - EH
>>>>         - NCQ
>>>>         - irq-pio
>>>>
>>>> Since each of these sets is staged, upstream testers will be able to say
>>>> "2.6.17-rc1 works, but 2.6.18-git1 doesn't", which gives us a lot of
>>>> good info.
>>>>
>>>>         Jeff
>>> So for those of us anxiously waiting for the above great enhancements
>>> it looks like it will be 5 months or so before it is in a released
>>> vanilla kernel?  ie. assuming 2 1/2 months per kernel release.
>> Heavens no :)
>>
>> The stages above just need to be separated out by a few 2.6.X-gitX or
>> 2.6.X-rcX releases.
>>
>>         Jeff
> 
> I'm confused.  You just poseted:
> 
>> Due to the merge window closing, the three pending patchsets will be in
>> 2.6.18.
> 
> That sure sounds like 5 months before it is in a released vanilla
> kernel.  ie. it has been taking about 10 weeks per kernel release. 
> 2.6.17 is likely to take another 8 weeks.  Then 10 weeks for 2.6.18 to
> come out.

Hopefully the releases won't take that long :)

	Jeff

Re: [PATCHSET] libata: improve ata_bus_probe failure handling, take 2

[Albert Lee]

Jeff Garzik wrote:
> 
> For the record, I do consider irq-pio upstream ready.  It still needs to
> wait for Tejun's EH work to make it upstream, to give upstream people
> the test sets of
> 
>     - prep EH (2.6.17-rc1)
>     - EH
>     - NCQ
>     - irq-pio
> 
> Since each of these sets is staged, upstream testers will be able to say
> "2.6.17-rc1 works, but 2.6.18-git1 doesn't", which gives us a lot of
> good info.
> 

Nice to see those features upstream. Thanks. :)

--
albert

[PATCH] libata-dev: irq-pio minor fix

[Albert Lee]

irq-pio minor fix:
- remove the redundant hsm_task_state = HSM_ST_IDLE
- add devno to printk() as done in upstream 

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---

--- irq-pio0/drivers/scsi/libata-eh.c	2006-04-03 13:50:15.000000000 +0800
+++ irq-pio1/drivers/scsi/libata-eh.c	2006-04-03 16:28:52.000000000 +0800
@@ -137,7 +137,6 @@ int ata_scsi_error(struct Scsi_Host *hos
  *	LOCKING:
  *	Inherited from SCSI layer (none, can sleep)
  */
-
 static void ata_qc_timeout(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
@@ -172,8 +171,6 @@ static void ata_qc_timeout(struct ata_qu
 		printk(KERN_ERR "ata%u: command 0x%x timeout, stat 0x%x host_stat 0x%x\n",
 		       ap->id, qc->tf.command, drv_stat, host_stat);
 
-		ap->hsm_task_state = HSM_ST_IDLE;
-
 		/* complete taskfile transaction */
 		qc->err_mask |= AC_ERR_TIMEOUT;
 		break;
--- irq-pio0/drivers/scsi/libata-core.c	2006-04-03 13:50:15.000000000 +0800
+++ irq-pio1/drivers/scsi/libata-core.c	2006-04-03 15:36:10.000000000 +0800
@@ -4043,8 +4043,8 @@ fsm_start:
 		}
 
 		/* no more data to transfer */
-		DPRINTK("ata%u: command complete, drv_stat 0x%x\n",
-			ap->id, status);
+		DPRINTK("ata%u: dev %u command complete, drv_stat 0x%x\n",
+			ap->id, qc->dev->devno, status);
 
 		WARN_ON(qc->err_mask);
 
@@ -4061,8 +4061,8 @@ fsm_start:
 
 	case HSM_ST_ERR:
 		if (qc->tf.command != ATA_CMD_PACKET)
-			printk(KERN_ERR "ata%u: command error, drv_stat 0x%x\n",
-			       ap->id, status);
+			printk(KERN_ERR "ata%u: dev %u command error, drv_stat 0x%x\n",
+			       ap->id, qc->dev->devno, status);
 
 		/* make sure qc->err_mask is available to
 		 * know what's wrong and recover

Re: [PATCH] libata-dev: irq-pio minor fix

[Jeff Garzik]

Albert Lee wrote:
> irq-pio minor fix:
> - remove the redundant hsm_task_state = HSM_ST_IDLE
> - add devno to printk() as done in upstream 
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

applied both minor-fix patches

[PATCH] libata-dev: irq-pio minor fix 2

[Albert Lee]

irq-pio minor fix 2:
- Use qc as data for ata_pio_task().

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
I forgot this. One more minor fix for irq-pio.

Patch against irq-pio branch
(7819d64e31a685e35e4819515c84168d0879816e).

--- irq-pio1/drivers/scsi/libata-core.c	2006-04-03 15:36:10.000000000 +0800
+++ irq-pio2/drivers/scsi/libata-core.c	2006-04-03 18:05:45.000000000 +0800
@@ -4097,9 +4097,6 @@ static void ata_pio_task(void *_data)
 fsm_start:
 	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
 
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-
 	/*
 	 * This is purely heuristic.  This is a fast path.
 	 * Sometimes when we enter, BSY will be cleared in
@@ -4112,7 +4109,7 @@ fsm_start:
 		msleep(2);
 		status = ata_busy_wait(ap, ATA_BUSY, 10);
 		if (status & ATA_BUSY) {
-			ata_port_queue_task(ap, ata_pio_task, ap, ATA_SHORT_PAUSE);
+			ata_port_queue_task(ap, ata_pio_task, qc, ATA_SHORT_PAUSE);
 			return;
 		}
 	}
@@ -4347,7 +4344,7 @@ unsigned int ata_qc_issue_prot(struct at
 		ap->hsm_task_state = HSM_ST_LAST;
 
 		if (qc->tf.flags & ATA_TFLAG_POLLING)
-			ata_port_queue_task(ap, ata_pio_task, ap, 0);
+			ata_port_queue_task(ap, ata_pio_task, qc, 0);
 
 		break;
 
@@ -4369,7 +4366,7 @@ unsigned int ata_qc_issue_prot(struct at
 		if (qc->tf.flags & ATA_TFLAG_WRITE) {
 			/* PIO data out protocol */
 			ap->hsm_task_state = HSM_ST_FIRST;
-			ata_port_queue_task(ap, ata_pio_task, ap, 0);
+			ata_port_queue_task(ap, ata_pio_task, qc, 0);
 
 			/* always send first data block using
 			 * the ata_pio_task() codepath.
@@ -4379,7 +4376,7 @@ unsigned int ata_qc_issue_prot(struct at
 			ap->hsm_task_state = HSM_ST;
 
 			if (qc->tf.flags & ATA_TFLAG_POLLING)
-				ata_port_queue_task(ap, ata_pio_task, ap, 0);
+				ata_port_queue_task(ap, ata_pio_task, qc, 0);
 
 			/* if polling, ata_pio_task() handles the rest.
 			 * otherwise, interrupt handler takes over from here.
@@ -4400,7 +4397,7 @@ unsigned int ata_qc_issue_prot(struct at
 		/* send cdb by polling if no cdb interrupt */
 		if ((!(qc->dev->flags & ATA_DFLAG_CDB_INTR)) ||
 		    (qc->tf.flags & ATA_TFLAG_POLLING))
-			ata_port_queue_task(ap, ata_pio_task, ap, 0);
+			ata_port_queue_task(ap, ata_pio_task, qc, 0);
 		break;
 
 	case ATA_PROT_ATAPI_DMA:
@@ -4412,7 +4409,7 @@ unsigned int ata_qc_issue_prot(struct at
 
 		/* send cdb by polling if no cdb interrupt */
 		if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
-			ata_port_queue_task(ap, ata_pio_task, ap, 0);
+			ata_port_queue_task(ap, ata_pio_task, qc, 0);
 		break;
 
 	default: