Skip to contentSkip to conversation

libata: implement presence detection using polling IDENTIFY for ata_piix

PATCH In review Tejun Heo 3 participants 14 messages Sep 30, 2006 – Nov 1, 2006 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: implement presence detection using polling IDENTIFY for ata_piix

Hello, all.

I've been thinking about device misdetection and phantom device problems on piix for some time now. Various controllers show different behaviors and even chips sharing the same PCI ID act differently. Some of the symptoms are intermittent making it difficult to debug.

In addition, a few days ago, there was a new bug report from [email protected] saying ICH4-M showed phantom device problem. That's an old old IDE controller which must have worked well for a very long time.

I think that it's just best to do what IDE and old libata used to do - ignore PCS (other than turning on enable bits) and honor reset signature and TF r/w test while using polling PIO for IDENTIFY and detect phantom devices. This formula had been working for years and there is no good reason to cause more pain to ICH users, which are a LOT, to find reliable non-polling device detection mechanism, which might not even exist.

So, this patchset. It implements device detection via polling IDENTIFY. When no device present, !BSY && !DRQ status is seen while trying to read the IDENTIFY block. If a LLD specifies ATA_FLAG_DETECT_POLLING, IDENTIFY command is issued using polling PIO and the HSM marks AC_ERR_NODEV_HINT if !BSY && !DRQ is met. Then the probing logic marks the device non-existent silently and continues probing.

This patchset contains 6 patches.

#01-03 : prep for ATA_FLAG_DETECT_POLLING implementation
#04 : implement ATA_DETECT_POLLING
#05 : apply ATA_DETECT_POLLING to ata_piix
#06 : kill unnecessary parts from ata_piix

This patchset is against

upstream (2d2744fc8be620a2dc469cf48349e3e704119f1b)

This patchset collides with the following patch.

[1] implement-ATA_FLAG_SETXFER_POLLING-and-use-it-in-pata_via

I'll reimplement the above patch to not impact qc_issue fast path and thus can be put off.

Please test this patchset. If you don't feel comfortable with handling patch series, this patchset is also available as...

* Git tree

http://htj.dyndns.org/git/?p=libata-tj.git;a=shortlog;h=detect-polling
git://htj.dyndns.org/libata-tj detect-polling

* Kernel source tarball

http://htj.dyndns.org/linux-libata-dev-upstream-DETECT_POLLING.tar.bz2

Thanks.

Signature · 3 lines
tejun

[1] http://article.gmane.org/gmane.linux.ide/13248
author
#1 lore raw

PATCH 1/6 ata_piix: clean up port flags

* move common flags into PIIX_PATA_FLAGS and PIIX_SATA_FLAGS
* kill unnecessary ATA_FLAG_SRST

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/ata_piix.c |   34 +++++++++++++++-------------------
 1 files changed, 15 insertions(+), 19 deletions(-)
drivers/ata/ata_piix.c +15 -19
index 5719704..cdb99dc 100644
@@ -106,6 +106,9 @@ enum {
 	PIIX_FLAG_AHCI		= (1 << 27), /* AHCI possible */
 	PIIX_FLAG_CHECKINTR	= (1 << 28), /* make sure PCI INTx enabled */
 
+	PIIX_PATA_FLAGS		= ATA_FLAG_SLAVE_POSS,
+	PIIX_SATA_FLAGS		= ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,
+
 	/* combined mode.  if set, PATA is channel 0.
 	 * if clear, PATA is channel 1.
 	 */

@@ -453,7 +456,7 @@ static struct ata_port_info piix_port_in
 	/* piix_pata_33: 0:  PIIX3 or 4 at 33MHz */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SLAVE_POSS | ATA_FLAG_SRST,
+		.flags		= PIIX_PATA_FLAGS,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x06, /* mwdma1-2 ?? CHECK 0 should be ok but slow */
 		.udma_mask	= ATA_UDMA_MASK_40C,

@@ -463,7 +466,7 @@ static struct ata_port_info piix_port_in
 	/* ich_pata_33: 1 	ICH0 - ICH at 33Mhz*/
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SRST | ATA_FLAG_SLAVE_POSS,
+		.flags		= PIIX_PATA_FLAGS,
 		.pio_mask 	= 0x1f,	/* pio 0-4 */
 		.mwdma_mask	= 0x06, /* Check: maybe 0x07  */
 		.udma_mask	= ATA_UDMA2, /* UDMA33 */

@@ -472,7 +475,7 @@ static struct ata_port_info piix_port_in
 	/* ich_pata_66: 2 	ICH controllers up to 66MHz */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SRST | ATA_FLAG_SLAVE_POSS,
+		.flags		= PIIX_PATA_FLAGS,
 		.pio_mask 	= 0x1f,	/* pio 0-4 */
 		.mwdma_mask	= 0x06, /* MWDMA0 is broken on chip */
 		.udma_mask	= ATA_UDMA4,

@@ -482,7 +485,7 @@ static struct ata_port_info piix_port_in
 	/* ich_pata_100: 3 */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SRST | ATA_FLAG_SLAVE_POSS | PIIX_FLAG_CHECKINTR,
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x06, /* mwdma1-2 */
 		.udma_mask	= ATA_UDMA5, /* udma0-5 */

@@ -492,7 +495,7 @@ static struct ata_port_info piix_port_in
 	/* ich_pata_133: 4 	ICH with full UDMA6 */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SRST | ATA_FLAG_SLAVE_POSS | PIIX_FLAG_CHECKINTR,
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
 		.pio_mask 	= 0x1f,	/* pio 0-4 */
 		.mwdma_mask	= 0x06, /* Check: maybe 0x07  */
 		.udma_mask	= ATA_UDMA6, /* UDMA133 */

@@ -502,8 +505,7 @@ static struct ata_port_info piix_port_in
 	/* ich5_sata: 5 */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR |
-				  PIIX_FLAG_IGNORE_PCS,
+		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_IGNORE_PCS,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */

@@ -513,8 +515,7 @@ static struct ata_port_info piix_port_in
 	/* i6300esb_sata: 6 */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SATA |
-				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_IGNORE_PCS,
+		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_IGNORE_PCS,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */

@@ -524,8 +525,7 @@ static struct ata_port_info piix_port_in
 	/* ich6_sata: 7 */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SATA |
-				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_SCR,
+		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SCR,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */

@@ -535,8 +535,7 @@ static struct ata_port_info piix_port_in
 	/* ich6_sata_ahci: 8 */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SATA |
-				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_SCR |
+		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SCR |
 				  PIIX_FLAG_AHCI,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */

@@ -547,8 +546,7 @@ static struct ata_port_info piix_port_in
 	/* ich6m_sata_ahci: 9 */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SATA |
-				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_SCR |
+		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SCR |
 				  PIIX_FLAG_AHCI,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */

@@ -559,8 +557,7 @@ static struct ata_port_info piix_port_in
 	/* ich7m_sata_ahci: 10 */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SATA |
-				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_SCR |
+		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SCR |
 				  PIIX_FLAG_AHCI,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */

@@ -571,8 +568,7 @@ static struct ata_port_info piix_port_in
 	/* ich8_sata_ahci: 11 */
 	{
 		.sht		= &piix_sht,
-		.flags		= ATA_FLAG_SATA |
-				  PIIX_FLAG_CHECKINTR | PIIX_FLAG_SCR |
+		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SCR |
 				  PIIX_FLAG_AHCI,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
Signature · 1 line
1.4.2.1
#2 lore raw

Re: [PATCH 1/6] ata_piix: clean up port flags

Quoted 4 lines · Tejun Heo

* move common flags into PIIX_PATA_FLAGS and PIIX_SATA_FLAGS
* kill unnecessary ATA_FLAG_SRST

Signed-off-by Tejun Heo <[email protected]>

AFAICS, SRST is quite necessary on PATA. What's going on? There isn't much of any explanation for the SRST removal here :/

Jeff

author
#3 lore raw

Re: [PATCH 1/6] ata_piix: clean up port flags

Quoted 7 lines · Jeff Garzik
Quoted 4 lines · Tejun Heo

* move common flags into PIIX_PATA_FLAGS and PIIX_SATA_FLAGS
* kill unnecessary ATA_FLAG_SRST

Signed-off-by Tejun Heo <[email protected]>

AFAICS, SRST is quite necessary on PATA. What's going on? There isn't much of any explanation for the SRST removal here :/

Both reset flags ATA_FLAG_SATA_RESET and ATA_FLAG_SRST are used to tell old EH how to reset and probe the port. In new EH, those flags don't play any role. What method to use and in what sequence is upto ->error_handler() now. For PATA piix, it only implements SRST, so that's gonna be used. For SATA piix, ata_bmdma_drive_eh() always prefers SRST on boot probe.

Once sata_promise/mv/.. are converted to new EH, the above two reset flags can be removed.

Thanks.

Signature · 1 line
tejun
#4 lore raw

Re: [PATCH 1/6] ata_piix: clean up port flags

Quoted 7 lines · Tejun Heo
Quoted 6 lines · Jeff Garzik
Quoted 4 lines · Tejun Heo

* move common flags into PIIX_PATA_FLAGS and PIIX_SATA_FLAGS
* kill unnecessary ATA_FLAG_SRST

Signed-off-by Tejun Heo <[email protected]>
Quoted 7 lines
Quoted 1 line

AFAICS, SRST is quite necessary on PATA. What's going on? There isn't much of any explanation for the SRST removal here :/

Both reset flags ATA_FLAG_SATA_RESET and ATA_FLAG_SRST are used to tell old EH how to reset and probe the port. In new EH, those flags don't play any role. What method to use and in what sequence is upto ->error_handler() now. For PATA piix, it only implements SRST, so
that's gonna be used. For SATA piix, ata_bmdma_drive_eh() always prefers SRST on boot probe.

Once sata_promise/mv/.. are converted to new EH, the above two reset flags can be removed.

Ah, duh. I knew that, thanks for the clue-hammer :)

Jeff

author
#6 lore raw

PATCH 2/6 libata: unexport ata_dev_revalidate()

In reply to Tejun Heo

ata_dev_revalidate() isn't used outside of libata core. Unexport it.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |    1 -
 drivers/ata/libata.h      |    1 +
 include/linux/libata.h    |    1 -
 3 files changed, 1 insertions(+), 2 deletions(-)
drivers/ata/libata-core.c +0 -1
index 7ab45f4..6cbb5e7 100644
@@ -6119,7 +6119,6 @@ EXPORT_SYMBOL_GPL(ata_std_prereset);
 EXPORT_SYMBOL_GPL(ata_std_softreset);
 EXPORT_SYMBOL_GPL(sata_std_hardreset);
 EXPORT_SYMBOL_GPL(ata_std_postreset);
-EXPORT_SYMBOL_GPL(ata_dev_revalidate);
 EXPORT_SYMBOL_GPL(ata_dev_classify);
 EXPORT_SYMBOL_GPL(ata_dev_pair);
 EXPORT_SYMBOL_GPL(ata_port_disable);
drivers/ata/libata.h +1 -0
index a5ecb71..0ed263b 100644
@@ -53,6 +53,7 @@ extern unsigned ata_exec_internal(struct
 extern unsigned int ata_do_simple_cmd(struct ata_device *dev, u8 cmd);
 extern int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 			   int post_reset, u16 *id);
+extern int ata_dev_revalidate(struct ata_device *dev, int post_reset);
 extern int ata_dev_configure(struct ata_device *dev, int print_info);
 extern int sata_down_spd_limit(struct ata_port *ap);
 extern int sata_set_spd_needed(struct ata_port *ap);
include/linux/libata.h +0 -1
index df44b09..0cd6699 100644
@@ -701,7 +701,6 @@ extern int ata_std_prereset(struct ata_p
 extern int ata_std_softreset(struct ata_port *ap, unsigned int *classes);
 extern int sata_std_hardreset(struct ata_port *ap, unsigned int *class);
 extern void ata_std_postreset(struct ata_port *ap, unsigned int *classes);
-extern int ata_dev_revalidate(struct ata_device *dev, int post_reset);
 extern void ata_port_disable(struct ata_port *);
 extern void ata_std_ports(struct ata_ioports *ioaddr);
 #ifdef CONFIG_PCI
Signature · 1 line
1.4.2.1
author
#7 lore raw

PATCH 3/6 libata: convert @post_reset to @flags in ata_dev_read_id()

In reply to Tejun Heo

Make ata_dev_read_id() take @flags instead of @post_reset. Currently there is only one flag defined - ATA_READID_POSTRESET, which is equivalent to @post_reset. This is preparation for polling presence detection.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |   17 +++++++++--------
 drivers/ata/libata-eh.c   |   11 +++++++----
 drivers/ata/libata.h      |    9 +++++++--
 3 files changed, 23 insertions(+), 14 deletions(-)
drivers/ata/libata-core.c +9 -8
index 6cbb5e7..8e8f359 100644
@@ -1220,7 +1220,7 @@ unsigned int ata_pio_need_iordy(const st
  *	ata_dev_read_id - Read ID data from the specified device
  *	@dev: target device
  *	@p_class: pointer to class of the target device (may be changed)
- *	@post_reset: is this read ID post-reset?
+ *	@flags: ATA_READID_* flags
  *	@id: buffer to read IDENTIFY data into
  *
  *	Read ID data from the specified device.  ATA_CMD_ID_ATA is

@@ -1235,7 +1235,7 @@ unsigned int ata_pio_need_iordy(const st
  *	0 on success, -errno otherwise.
  */
 int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
-		    int post_reset, u16 *id)
+		    unsigned int flags, u16 *id)
 {
 	struct ata_port *ap = dev->ap;
 	unsigned int class = *p_class;

@@ -1290,7 +1290,7 @@ int ata_dev_read_id(struct ata_device *d
 			goto err_out;
 	}
 
-	if (post_reset && class == ATA_DEV_ATA) {
+	if ((flags & ATA_READID_POSTRESET) && class == ATA_DEV_ATA) {
 		/*
 		 * The exact sequence expected by certain pre-ATA4 drives is:
 		 * SRST RESET

@@ -1310,7 +1310,7 @@ int ata_dev_read_id(struct ata_device *d
 			/* current CHS translation info (id[53-58]) might be
 			 * changed. reread the identify device info.
 			 */
-			post_reset = 0;
+			flags &= ~ATA_READID_POSTRESET;
 			goto retry;
 		}
 	}

@@ -1627,7 +1627,8 @@ int ata_bus_probe(struct ata_port *ap)
 		if (!ata_dev_enabled(dev))
 			continue;
 
-		rc = ata_dev_read_id(dev, &dev->class, 1, dev->id);
+		rc = ata_dev_read_id(dev, &dev->class, ATA_READID_POSTRESET,
+				     dev->id);
 		if (rc)
 			goto fail;
 

@@ -2965,7 +2966,7 @@ static int ata_dev_same_device(struct at
 /**
  *	ata_dev_revalidate - Revalidate ATA device
  *	@dev: device to revalidate
- *	@post_reset: is this revalidation after reset?
+ *	@readid_flags: read ID flags
  *
  *	Re-read IDENTIFY page and make sure @dev is still attached to
  *	the port.

@@ -2976,7 +2977,7 @@ static int ata_dev_same_device(struct at
  *	RETURNS:
  *	0 on success, negative errno otherwise
  */
-int ata_dev_revalidate(struct ata_device *dev, int post_reset)
+int ata_dev_revalidate(struct ata_device *dev, unsigned int readid_flags)
 {
 	unsigned int class = dev->class;
 	u16 *id = (void *)dev->ap->sector_buf;

@@ -2988,7 +2989,7 @@ int ata_dev_revalidate(struct ata_device
 	}
 
 	/* read ID data */
-	rc = ata_dev_read_id(dev, &class, post_reset, id);
+	rc = ata_dev_read_id(dev, &class, readid_flags, id);
 	if (rc)
 		goto fail;
drivers/ata/libata-eh.c +7 -4
index 02b2b27..94faeee 100644
@@ -1634,11 +1634,14 @@ static int ata_eh_revalidate_and_attach(
 	DPRINTK("ENTER\n");
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
-		unsigned int action;
+		unsigned int action, readid_flags = 0;
 
 		dev = &ap->device[i];
 		action = ata_eh_dev_action(dev);
 
+		if (ehc->i.flags & ATA_EHI_DID_RESET)
+			readid_flags |= ATA_READID_POSTRESET;
+
 		if (action & ATA_EH_REVALIDATE && ata_dev_ready(dev)) {
 			if (ata_port_offline(ap)) {
 				rc = -EIO;

@@ -1646,8 +1649,7 @@ static int ata_eh_revalidate_and_attach(
 			}
 
 			ata_eh_about_to_do(ap, dev, ATA_EH_REVALIDATE);
-			rc = ata_dev_revalidate(dev,
-					ehc->i.flags & ATA_EHI_DID_RESET);
+			rc = ata_dev_revalidate(dev, readid_flags);
 			if (rc)
 				break;
 

@@ -1660,7 +1662,8 @@ static int ata_eh_revalidate_and_attach(
 			   ata_class_enabled(ehc->classes[dev->devno])) {
 			dev->class = ehc->classes[dev->devno];
 
-			rc = ata_dev_read_id(dev, &dev->class, 1, dev->id);
+			rc = ata_dev_read_id(dev, &dev->class, readid_flags,
+					     dev->id);
 			if (rc == 0)
 				rc = ata_dev_configure(dev, 1);
drivers/ata/libata.h +7 -2
index 0ed263b..34c4054 100644
@@ -39,6 +39,11 @@ struct ata_scsi_args {
 };
 
 /* libata-core.c */
+enum {
+	/* flags for ata_dev_read_id() */
+	ATA_READID_POSTRESET	= (1 << 0), /* reading ID after reset */
+};
+
 extern struct workqueue_struct *ata_aux_wq;
 extern int atapi_enabled;
 extern int atapi_dmadir;

@@ -52,8 +57,8 @@ extern unsigned ata_exec_internal(struct
 				  int dma_dir, void *buf, unsigned int buflen);
 extern unsigned int ata_do_simple_cmd(struct ata_device *dev, u8 cmd);
 extern int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
-			   int post_reset, u16 *id);
-extern int ata_dev_revalidate(struct ata_device *dev, int post_reset);
+			   unsigned int flags, u16 *id);
+extern int ata_dev_revalidate(struct ata_device *dev, unsigned int flags);
 extern int ata_dev_configure(struct ata_device *dev, int print_info);
 extern int sata_down_spd_limit(struct ata_port *ap);
 extern int sata_set_spd_needed(struct ata_port *ap);
Signature · 1 line
1.4.2.1
#8 lore raw

Re: [PATCH 3/6] libata: convert @post_reset to @flags in ata_dev_read_id()

Quoted 6 lines · Tejun Heo

Make ata_dev_read_id() take @flags instead of @post_reset. Currently there is only one flag defined - ATA_READID_POSTRESET, which is equivalent to @post_reset. This is preparation for polling presence detection.

Signed-off-by Tejun Heo <[email protected]>

ACK

author
#9 lore raw

PATCH 4/6 libata: implement presence detection via polling IDENTIFY

In reply to Tejun Heo

On some controllers (ICHs in piix mode), there is *NO* reliable way to determine device presence other than issuing IDENTIFY and see how the transaction proceeds by watching the TF status register.

libata acted this way before irq-pio and phantom devices caused very little problem but now that IDENTIFY is performed using IRQ drive PIO, such phantom devices now result in multiple 30sec timeouts during boot.

This patch implements ATA_FLAG_DETECT_POLLING. If a LLD sets this flag, libata core issues the initial IDENTIFY in polling mode and if the initial data transfer fails w/ HSM violation, the port is considered to be empty thus replicating the old libata and IDE behavior.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |   19 +++++++++++++++++--
 drivers/ata/libata-eh.c   |   20 +++++++++++++++++---
 drivers/ata/libata.h      |    2 ++
 include/linux/libata.h    |    3 +++
 4 files changed, 39 insertions(+), 5 deletions(-)
drivers/ata/libata-core.c +17 -2
index 8e8f359..9c3963e 100644
@@ -1268,9 +1268,20 @@ int ata_dev_read_id(struct ata_device *d
 
 	tf.protocol = ATA_PROT_PIO;
 
+	/* presence detection using polling IDENTIFY? */
+	if (flags & ATA_READID_DETECT)
+		tf.flags |= ATA_TFLAG_POLLING;
+
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
 				     id, sizeof(id[0]) * ATA_ID_WORDS);
 	if (err_mask) {
+		if ((flags & ATA_READID_DETECT) &&
+		    (err_mask & AC_ERR_NODEV_HINT)) {
+			DPRINTK("ata%u.%d: NODEV after polling detection\n",
+				ap->id, dev->devno);
+			return -ENOENT;
+		}
+
 		rc = -EIO;
 		reason = "I/O error";
 		goto err_out;

@@ -4197,8 +4208,12 @@ fsm_start:
 					/* device stops HSM for abort/error */
 					qc->err_mask |= AC_ERR_DEV;
 				else
-					/* HSM violation. Let EH handle this */
-					qc->err_mask |= AC_ERR_HSM;
+					/* HSM violation. Let EH handle this.
+					 * Phantom devices also trigger this
+					 * condition.  Mark hint.
+					 */
+					qc->err_mask |= AC_ERR_HSM |
+							AC_ERR_NODEV_HINT;
 
 				ap->hsm_task_state = HSM_ST_ERR;
 				goto fsm_start;
drivers/ata/libata-eh.c +17 -3
index 94faeee..76405fe 100644
@@ -1662,19 +1662,33 @@ static int ata_eh_revalidate_and_attach(
 			   ata_class_enabled(ehc->classes[dev->devno])) {
 			dev->class = ehc->classes[dev->devno];
 
+			if (ap->flags & ATA_FLAG_DETECT_POLLING)
+				readid_flags |= ATA_READID_DETECT;
+
 			rc = ata_dev_read_id(dev, &dev->class, readid_flags,
 					     dev->id);
 			if (rc == 0)
 				rc = ata_dev_configure(dev, 1);
+			else if (rc == -ENOENT) {
+				/* IDENTIFY was issued to non-existent
+				 * device.  No need to reset.  Just
+				 * thaw and kill the device.
+				 */
+				ata_eh_thaw_port(ap);
+				dev->class = ATA_DEV_UNKNOWN;
+				rc = 0;
+			}
 
 			if (rc) {
 				dev->class = ATA_DEV_UNKNOWN;
 				break;
 			}
 
-			spin_lock_irqsave(ap->lock, flags);
-			ap->pflags |= ATA_PFLAG_SCSI_HOTPLUG;
-			spin_unlock_irqrestore(ap->lock, flags);
+			if (ata_dev_enabled(dev)) {
+				spin_lock_irqsave(ap->lock, flags);
+				ap->pflags |= ATA_PFLAG_SCSI_HOTPLUG;
+				spin_unlock_irqrestore(ap->lock, flags);
+			}
 		}
 	}
drivers/ata/libata.h +2 -0
index 34c4054..9d830ef 100644
@@ -42,6 +42,8 @@ struct ata_scsi_args {
 enum {
 	/* flags for ata_dev_read_id() */
 	ATA_READID_POSTRESET	= (1 << 0), /* reading ID after reset */
+	ATA_READID_DETECT	= (1 << 1), /* perform presence detection
+					     * using polling IDENTIFY */
 };
 
 extern struct workqueue_struct *ata_aux_wq;
include/linux/libata.h +3 -0
index 0cd6699..41180cc 100644
@@ -174,6 +174,8 @@ enum {
 	ATA_FLAG_SKIP_D2H_BSY	= (1 << 12), /* can't wait for the first D2H
 					      * Register FIS clearing BSY */
 	ATA_FLAG_DEBUGMSG	= (1 << 13),
+	ATA_FLAG_DETECT_POLLING	= (1 << 14), /* detect device presence by
+					      * polling IDENTIFY */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be

@@ -328,6 +330,7 @@ enum ata_completion_errors {
 	AC_ERR_SYSTEM		= (1 << 6), /* system error */
 	AC_ERR_INVALID		= (1 << 7), /* invalid argument */
 	AC_ERR_OTHER		= (1 << 8), /* unknown */
+	AC_ERR_NODEV_HINT	= (1 << 9), /* polling device detection hint */
 };
 
 /* forward declarations */
Signature · 1 line
1.4.2.1
author
#10 lore raw

PATCH 5/6 ata_piix: apply device detection via polling IDENTIFY

In reply to Tejun Heo

PATA PIIX uses reset signature + TF r/w test for device presence detection, which doesn't always work. It sometimes reports phantom device which results in IDENTIFY timeouts.

SATA PIIX uses some combination of PCS + reset signature + TF r/w test for device presence detection. No combination satifies all and for some controllers, there doesn't seem to be any combination which works reliably.

This patch makes both PATA and SATA piix's use reset signature + TF r/w + polling IDENTIFY for device detection. This is what the old libata (before irq-pio and new EH) did and what IDE does.

This patch also removes now obsolete PIIX_FLAG_IGNORE_PCS, force_pcs and related code.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/ata_piix.c |  104 +++---------------------------------------------
 1 files changed, 6 insertions(+), 98 deletions(-)
drivers/ata/ata_piix.c +6 -98
index cdb99dc..a92c798 100644
@@ -101,13 +101,13 @@ enum {
 	ICH5_PCS		= 0x92,	/* port control and status */
 	PIIX_SCC		= 0x0A, /* sub-class code register */
 
-	PIIX_FLAG_IGNORE_PCS	= (1 << 25), /* ignore PCS present bits */
 	PIIX_FLAG_SCR		= (1 << 26), /* SCR available */
 	PIIX_FLAG_AHCI		= (1 << 27), /* AHCI possible */
 	PIIX_FLAG_CHECKINTR	= (1 << 28), /* make sure PCI INTx enabled */
 
-	PIIX_PATA_FLAGS		= ATA_FLAG_SLAVE_POSS,
-	PIIX_SATA_FLAGS		= ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,
+	PIIX_PATA_FLAGS		= ATA_FLAG_SLAVE_POSS | ATA_FLAG_DETECT_POLLING,
+	PIIX_SATA_FLAGS		= ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR |
+				  ATA_FLAG_DETECT_POLLING,
 
 	/* combined mode.  if set, PATA is channel 0.
 	 * if clear, PATA is channel 1.

@@ -505,7 +505,7 @@ static struct ata_port_info piix_port_in
 	/* ich5_sata: 5 */
 	{
 		.sht		= &piix_sht,
-		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_IGNORE_PCS,
+		.flags		= PIIX_SATA_FLAGS,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */

@@ -515,7 +515,7 @@ static struct ata_port_info piix_port_in
 	/* i6300esb_sata: 6 */
 	{
 		.sht		= &piix_sht,
-		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_IGNORE_PCS,
+		.flags		= PIIX_SATA_FLAGS,
 		.pio_mask	= 0x1f,	/* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */

@@ -589,11 +589,6 @@ MODULE_LICENSE("GPL");
 MODULE_DEVICE_TABLE(pci, piix_pci_tbl);
 MODULE_VERSION(DRV_VERSION);
 
-static int force_pcs = 0;
-module_param(force_pcs, int, 0444);
-MODULE_PARM_DESC(force_pcs, "force honoring or ignoring PCS to work around "
-		 "device mis-detection (0=default, 1=ignore PCS, 2=honor PCS)");
-
 /**
  *	piix_pata_cbl_detect - Probe host controller cable detect info
  *	@ap: Port for which cable detect info is desired

@@ -682,84 +677,9 @@ static void ich_pata_error_handler(struc
 			   ata_std_postreset);
 }
 
-/**
- *	piix_sata_present_mask - determine present mask for SATA host controller
- *	@ap: Target port
- *
- *	Reads SATA PCI device's PCI config register Port Configuration
- *	and Status (PCS) to determine port and device availability.
- *
- *	LOCKING:
- *	None (inherited from caller).
- *
- *	RETURNS:
- *	determined present_mask
- */
-static unsigned int piix_sata_present_mask(struct ata_port *ap)
-{
-	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
-	struct piix_host_priv *hpriv = ap->host->private_data;
-	const unsigned int *map = hpriv->map;
-	int base = 2 * ap->port_no;
-	unsigned int present_mask = 0;
-	int port, i;
-	u16 pcs;
-
-	pci_read_config_word(pdev, ICH5_PCS, &pcs);
-	DPRINTK("ata%u: ENTER, pcs=0x%x base=%d\n", ap->id, pcs, base);
-
-	for (i = 0; i < 2; i++) {
-		port = map[base + i];
-		if (port < 0)
-			continue;
-		if ((ap->flags & PIIX_FLAG_IGNORE_PCS) ||
-		    (pcs & 1 << (hpriv->map_db->present_shift + port)))
-			present_mask |= 1 << i;
-	}
-
-	DPRINTK("ata%u: LEAVE, pcs=0x%x present_mask=0x%x\n",
-		ap->id, pcs, present_mask);
-
-	return present_mask;
-}
-
-/**
- *	piix_sata_softreset - reset SATA host port via ATA SRST
- *	@ap: port to reset
- *	@classes: resulting classes of attached devices
- *
- *	Reset SATA host port via ATA SRST.  On controllers with
- *	reliable PCS present bits, the bits are used to determine
- *	device presence.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep)
- *
- *	RETURNS:
- *	0 on success, -errno otherwise.
- */
-static int piix_sata_softreset(struct ata_port *ap, unsigned int *classes)
-{
-	unsigned int present_mask;
-	int i, rc;
-
-	present_mask = piix_sata_present_mask(ap);
-
-	rc = ata_std_softreset(ap, classes);
-	if (rc)
-		return rc;
-
-	for (i = 0; i < ATA_MAX_DEVICES; i++) {
-		if (!(present_mask & (1 << i)))
-			classes[i] = ATA_DEV_NONE;
-	}
-
-	return 0;
-}
-
 static void piix_sata_error_handler(struct ata_port *ap)
 {
-	ata_bmdma_drive_eh(ap, ata_std_prereset, piix_sata_softreset, NULL,
+	ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, NULL,
 			   ata_std_postreset);
 }
 

@@ -1074,18 +994,6 @@ static void __devinit piix_init_pcs(stru
 		pci_write_config_word(pdev, ICH5_PCS, new_pcs);
 		msleep(150);
 	}
-
-	if (force_pcs == 1) {
-		dev_printk(KERN_INFO, &pdev->dev,
-			   "force ignoring PCS (0x%x)\n", new_pcs);
-		pinfo[0].flags |= PIIX_FLAG_IGNORE_PCS;
-		pinfo[1].flags |= PIIX_FLAG_IGNORE_PCS;
-	} else if (force_pcs == 2) {
-		dev_printk(KERN_INFO, &pdev->dev,
-			   "force honoring PCS (0x%x)\n", new_pcs);
-		pinfo[0].flags &= ~PIIX_FLAG_IGNORE_PCS;
-		pinfo[1].flags &= ~PIIX_FLAG_IGNORE_PCS;
-	}
 }
 
 static void __devinit piix_init_sata_map(struct pci_dev *pdev,
Signature · 1 line
1.4.2.1
author
#11 lore raw

PATCH 6/6 ata_piix: strip now unneded MAP related stuff

In reply to Tejun Heo

Now that PCS isn't used for device detection anymore...

* esb_sata is identical to ich5_sata
* no reason to know present_shift
* no reason to store map_db in host private area

The MAP table itself is left because it can be used for SCR access.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/ata_piix.c |   44 ++++++++++++--------------------------------
 1 files changed, 12 insertions(+), 32 deletions(-)
drivers/ata/ata_piix.c +12 -32
index a92c798..944d534 100644
@@ -125,12 +125,11 @@ enum {
 	ich_pata_100		= 3,	/* ICH up to UDMA 100 */
 	ich_pata_133		= 4,	/* ICH up to UDMA 133 */
 	ich5_sata		= 5,
-	esb_sata		= 6,
-	ich6_sata		= 7,
-	ich6_sata_ahci		= 8,
-	ich6m_sata_ahci		= 9,
-	ich7m_sata_ahci		= 10,
-	ich8_sata_ahci		= 11,
+	ich6_sata		= 6,
+	ich6_sata_ahci		= 7,
+	ich6m_sata_ahci		= 8,
+	ich7m_sata_ahci		= 9,
+	ich8_sata_ahci		= 10,
 
 	/* constants for mapping table */
 	P0			= 0,  /* port 0 */

@@ -147,13 +146,11 @@ enum {
 struct piix_map_db {
 	const u32 mask;
 	const u16 port_enable;
-	const int present_shift;
 	const int map[][4];
 };
 
 struct piix_host_priv {
 	const int *map;
-	const struct piix_map_db *map_db;
 };
 
 static int piix_init_one (struct pci_dev *pdev,

@@ -218,9 +215,9 @@ #endif
 	/* 82801EB (ICH5) */
 	{ 0x8086, 0x24df, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich5_sata },
 	/* 6300ESB (ICH5 variant with broken PCS present bits) */
-	{ 0x8086, 0x25a3, PCI_ANY_ID, PCI_ANY_ID, 0, 0, esb_sata },
+	{ 0x8086, 0x25a3, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich5_sata },
 	/* 6300ESB pretending RAID */
-	{ 0x8086, 0x25b0, PCI_ANY_ID, PCI_ANY_ID, 0, 0, esb_sata },
+	{ 0x8086, 0x25b0, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich5_sata },
 	/* 82801FB/FW (ICH6/ICH6W) */
 	{ 0x8086, 0x2651, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata },
 	/* 82801FR/FRW (ICH6R/ICH6RW) */

@@ -371,7 +368,6 @@ static const struct ata_port_operations 
 static const struct piix_map_db ich5_map_db = {
 	.mask = 0x7,
 	.port_enable = 0x3,
-	.present_shift = 4,
 	.map = {
 		/* PM   PS   SM   SS       MAP  */
 		{  P0,  NA,  P1,  NA }, /* 000b */

@@ -388,7 +384,6 @@ static const struct piix_map_db ich5_map
 static const struct piix_map_db ich6_map_db = {
 	.mask = 0x3,
 	.port_enable = 0xf,
-	.present_shift = 4,
 	.map = {
 		/* PM   PS   SM   SS       MAP */
 		{  P0,  P2,  P1,  P3 }, /* 00b */

@@ -401,7 +396,6 @@ static const struct piix_map_db ich6_map
 static const struct piix_map_db ich6m_map_db = {
 	.mask = 0x3,
 	.port_enable = 0x5,
-	.present_shift = 4,
 	.map = {
 		/* PM   PS   SM   SS       MAP */
 		{  P0,  P2,  RV,  RV }, /* 00b */

@@ -414,7 +408,6 @@ static const struct piix_map_db ich6m_ma
 static const struct piix_map_db ich7m_map_db = {
 	.mask = 0x3,
 	.port_enable = 0x5,
-	.present_shift = 4,
 
 	/* Map 01b isn't specified in the doc but some notebooks use
 	 * it anyway.  ATM, the only case spotted carries subsystem ID

@@ -432,7 +425,6 @@ static const struct piix_map_db ich7m_ma
 static const struct piix_map_db ich8_map_db = {
 	.mask = 0x3,
 	.port_enable = 0x3,
-	.present_shift = 8,
 	.map = {
 		/* PM   PS   SM   SS       MAP */
 		{  P0,  NA,  P1,  NA }, /* 00b (hardwired) */

@@ -444,7 +436,6 @@ static const struct piix_map_db ich8_map
 
 static const struct piix_map_db *piix_map_db_table[] = {
 	[ich5_sata]		= &ich5_map_db,
-	[esb_sata]		= &ich5_map_db,
 	[ich6_sata]		= &ich6_map_db,
 	[ich6_sata_ahci]	= &ich6_map_db,
 	[ich6m_sata_ahci]	= &ich6m_map_db,

@@ -512,17 +503,7 @@ static struct ata_port_info piix_port_in
 		.port_ops	= &piix_sata_ops,
 	},
 
-	/* i6300esb_sata: 6 */
-	{
-		.sht		= &piix_sht,
-		.flags		= PIIX_SATA_FLAGS,
-		.pio_mask	= 0x1f,	/* pio0-4 */
-		.mwdma_mask	= 0x07, /* mwdma0-2 */
-		.udma_mask	= 0x7f,	/* udma0-6 */
-		.port_ops	= &piix_sata_ops,
-	},
-
-	/* ich6_sata: 7 */
+	/* ich6_sata: 6 */
 	{
 		.sht		= &piix_sht,
 		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SCR,

@@ -532,7 +513,7 @@ static struct ata_port_info piix_port_in
 		.port_ops	= &piix_sata_ops,
 	},
 
-	/* ich6_sata_ahci: 8 */
+	/* ich6_sata_ahci: 7 */
 	{
 		.sht		= &piix_sht,
 		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SCR |

@@ -543,7 +524,7 @@ static struct ata_port_info piix_port_in
 		.port_ops	= &piix_sata_ops,
 	},
 
-	/* ich6m_sata_ahci: 9 */
+	/* ich6m_sata_ahci: 8 */
 	{
 		.sht		= &piix_sht,
 		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SCR |

@@ -554,7 +535,7 @@ static struct ata_port_info piix_port_in
 		.port_ops	= &piix_sata_ops,
 	},
 
-	/* ich7m_sata_ahci: 10 */
+	/* ich7m_sata_ahci: 9 */
 	{
 		.sht		= &piix_sht,
 		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SCR |

@@ -565,7 +546,7 @@ static struct ata_port_info piix_port_in
 		.port_ops	= &piix_sata_ops,
 	},
 
-	/* ich8_sata_ahci: 11 */
+	/* ich8_sata_ahci: 10 */
 	{
 		.sht		= &piix_sht,
 		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SCR |

@@ -1043,7 +1024,6 @@ static void __devinit piix_init_sata_map
 			   "invalid MAP value %u\n", map_value);
 
 	hpriv->map = map;
-	hpriv->map_db = map_db;
 }
 
 /**
Signature · 1 line
1.4.2.1
#12 lore raw
In reply to Tejun Heo
Quoted 2 lines · Tejun Heo

Please test this patchset. If you don't feel comfortable with handling patch series, this patchset is also available as...

Hi Tejun

I can confirm that 2.6.18-git3 + this patchset boot perfectly without any delay

libata version 2.00 loaded.
ata_piix 0000:00:1f.1: version 2.00ac6
PCI: Enabling device 0000:00:1f.1 (0005 -> 0007)
ACPI: PCI Interrupt Link [LNKC] enabled at IRQ 7
PCI: setting IRQ 7 as level-triggered
ACPI: PCI Interrupt 0000:00:1f.1[A] -> Link [LNKC] -> GSI 7 (level, low) -> IRQ 7
PCI: Setting latency timer of device 0000:00:1f.1 to 64
ata1: PATA max UDMA/100 cmd 0x1F0 ctl 0x3F6 bmdma 0xFFA0 irq 14
ata2: PATA max UDMA/100 cmd 0x170 ctl 0x376 bmdma 0xFFA8 irq 15 scsi0 : ata_piix
ata1.00: ATA-6, max UDMA/100, 195371568 sectors: LBA
ata1.00: ata1: dev 0 multi count 16
ata1.00: configured for UDMA/100
scsi1 : ata_piix
input: AT Translated Set 2 keyboard as /class/input/input0
input: AT Translated Set 2 keyboard as /class/input/input1 ata2.00: ATAPI, max UDMA/33
ata2.00: configured for UDMA/33
scsi 0:0:0:0: Direct-Access ATA FUJITSU MHV2100A 0000 PQ: 0 ANSI: 5
scsi 1:0:0:0: CD-ROM MATSHITA UJ-820D 1.01 PQ: 0 ANSI: 5
SCSI device sda: 195371568 512-byte hdwr sectors (100030 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: drive cache: write back
SCSI device sda: 195371568 512-byte hdwr sectors (100030 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: drive cache: write back
sda: sda1 sda2
sd 0:0:0:0: Attached scsi disk sda
.....
sr0: scsi3-mmc drive: 24x/24x writer cd/rw xa/form2 cdda tray Uniform CD-ROM driver Revision: 3.20
sr 1:0:0:0: Attached scsi CD-ROM sr0
.....
sd 0:0:0:0: Attached scsi generic sg0 type 0
sr 1:0:0:0: Attached scsi generic sg1 type 5

#13 lore raw
Quoted 8 lines · Alessandro Bono
Quoted 2 lines · Tejun Heo

Please test this patchset. If you don't feel comfortable with handling patch series, this patchset is also available as...

Hi Tejun

I can confirm that 2.6.18-git3 + this patchset boot perfectly without any

                        ^^^^^^^^
                    2.6.18-git13
Quoted 50 lines

delay

libata version 2.00 loaded.
ata_piix 0000:00:1f.1: version 2.00ac6
PCI: Enabling device 0000:00:1f.1 (0005 -> 0007)
ACPI: PCI Interrupt Link [LNKC] enabled at IRQ 7
PCI: setting IRQ 7 as level-triggered
ACPI: PCI Interrupt 0000:00:1f.1[A] -> Link [LNKC] -> GSI 7 (level, low) -> IRQ 7
PCI: Setting latency timer of device 0000:00:1f.1 to 64
ata1: PATA max UDMA/100 cmd 0x1F0 ctl 0x3F6 bmdma 0xFFA0 irq 14
ata2: PATA max UDMA/100 cmd 0x170 ctl 0x376 bmdma 0xFFA8 irq 15 scsi0 : ata_piix
ata1.00: ATA-6, max UDMA/100, 195371568 sectors: LBA ata1.00: ata1: dev 0 multi count 16
ata1.00: configured for UDMA/100
scsi1 : ata_piix
input: AT Translated Set 2 keyboard as /class/input/input0
input: AT Translated Set 2 keyboard as /class/input/input1 ata2.00: ATAPI, max UDMA/33
ata2.00: configured for UDMA/33
scsi 0:0:0:0: Direct-Access ATA FUJITSU MHV2100A 0000 PQ: 0 ANSI:
5
scsi 1:0:0:0: CD-ROM MATSHITA UJ-820D 1.01 PQ: 0 ANSI:
5 SCSI device sda: 195371568 512-byte hdwr sectors (100030 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: drive cache: write back
SCSI device sda: 195371568 512-byte hdwr sectors (100030 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: drive cache: write back
sda: sda1 sda2
sd 0:0:0:0: Attached scsi disk sda
.....
sr0: scsi3-mmc drive: 24x/24x writer cd/rw xa/form2 cdda tray Uniform CD-ROM driver Revision: 3.20
sr 1:0:0:0: Attached scsi CD-ROM sr0
.....
sd 0:0:0:0: Attached scsi generic sg0 type 0
sr 1:0:0:0: Attached scsi generic sg1 type 5

-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html