Re: Linux v2.6.18-rc5

[Tejun Heo <htejun@gmail.com>]

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

Hello, Keith Owens.

Sorry about late respond.  I'm quite occupied with personal stuff these 
days.

Keith Owens wrote:
>>> (2) I have seen the same intermittent bug on ICH7 SATA but
>>>     PIIX_FLAG_IGNORE_PCS is only set for ich5 and i6300esb_sata.  It
>>>     probably needs to be set for ich7 as well.
>> No, ICH7 up to this point has been believed to have well-behaving PCS. 
>> If you report PCS problem, you'll be the first.  Also, note that ICH7 
>> suffers from ghost device probing problem if PCS is not honored exactly. 
>>  Are you sure it's the same problem?
> 
> It definitely looks like it.  Stock 2.6.18-rc5 plus this patch to
> activate ata_debug from boot until just after probing drives.
[--snip--]
> <3>piix_sata_present_mask: ata1: ENTER, pcs=0x15 base=0
> <3>piix_sata_present_mask: ata1: LEAVE, pcs=0x15 present_mask=0x3
> <3>piix_sata_present_mask: ata1: ENTER, pcs=0x0 base=0
> <3>piix_sata_present_mask: ata1: LEAVE, pcs=0x0 present_mask=0x3
> <3>piix_sata_present_mask: ata1: ENTER, pcs=0x15 base=0
> <3>piix_sata_present_mask: ata1: LEAVE, pcs=0x15 present_mask=0x3
> <3>piix_sata_present_mask: ata1: ENTER, pcs=0x0 base=0
> <3>piix_sata_present_mask: ata1: LEAVE, pcs=0x0 present_mask=0x3
> <3>piix_sata_present_mask: ata1: ENTER, pcs=0x15 base=0

Yeah, it definitely looks like it.  Does the kernel print a message 
which looks like the following before those debug messages?

"updating PCS from 0x0 to 0x5"

> Note the pcs=0x0 values.  Adding PIIX_FLAG_IGNORE_PCS to
> ich6m_sata_ahci gets past the failure to detect pcs, with no sign of
> any ghost devices.  BTW, dropping down to 2.6.17 with the same config
> has no problem detecting the disk, even without PIIX_FLAG_IGNORE_PCS on
> ich6m_sata_ahci.

BTW, you can set PIIX_FLAG_IGNORE_PCS by using force_pcs=1 module 
parameter now.

> lspci extract, this is an ICH7M.
> 
> 00:1f.0 Class 0601: 8086:27b9 (rev 02)
> 	Subsystem: 1033:832c
> 	Flags: bus master, medium devsel, latency 0
> 	Capabilities: [e0] Vendor Specific Information
> 
> 00:1f.2 Class 0101: 8086:27c4 (rev 02) (prog-if 80)
> 	Subsystem: 1033:832c
> 	Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 18
> 	I/O ports at <unassigned>
> 	I/O ports at <unassigned>
> 	I/O ports at <unassigned>
> 	I/O ports at <unassigned>
> 	I/O ports at 18b0 [size=16]
> 	Capabilities: [70] Power Management version 2
> 
> 00:1f.3 Class 0c05: 8086:27da (rev 02)
> 	Subsystem: 1033:832c
> 	Flags: medium devsel, IRQ 11
> 	I/O ports at 18c0 [size=32]
> 

Hmm... Can you try the attached patch and see what happens?  ATM, I'm on 
the road and can't test the patch, so it's only compile-tested.  This 
patch basically reverts some of the effects of the following commit and 
makes PCS update a little bit more aggressive iff necessary.

ea35d29e2fa8b3d766a2ce8fbcce599dce8d2734
[libata] ata_piix: Consolidate PCS register writing

If this works for you ich7m, can you please test this on your formerly 
problematic ich5 with force_pcs=2 specified?  I initially thought that 
the ich5 problem was caused by exact PCS map change and thus added 
IGNORE_PCS as workaround but if the same problem occurs on ich7 and is 
fixed by the attached patch, it's due to conservative PCS update change 
and thus the original IGNORE_PCS fix on ich5 might not be necessary.

Sorry this PCS business causes you and other people so much trouble.  It 
just doesn't work quite as ata_piix developers expect.

Thanks.

-- 
tejun

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3417 bytes --]

diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 2d20caf..46f7c9b 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -553,15 +553,42 @@ static unsigned int piix_sata_present_ma
 {
 	struct pci_dev *pdev = to_pci_dev(ap->host_set->dev);
 	struct piix_host_priv *hpriv = ap->host_set->private_data;
+	const struct piix_map_db *map_db = hpriv->map_db;
 	const unsigned int *map = hpriv->map;
 	int base = 2 * ap->hard_port_no;
 	unsigned int present_mask = 0;
 	int port, i;
-	u16 pcs;
+	u16 pcs, new_pcs;
 
 	pci_read_config_word(pdev, ICH5_PCS, &pcs);
 	DPRINTK("ata%u: ENTER, pcs=0x%x base=%d\n", ap->id, pcs, base);
 
+	new_pcs = pcs | map_db->port_enable;
+
+	if (pcs != new_pcs) {
+		u16 old_pcs = pcs;
+
+		for (i = 0; i < 10; i++) {
+			pci_write_config_word(pdev, ICH5_PCS, new_pcs);
+			msleep(150);
+			pci_read_config_word(pdev, ICH5_PCS, &pcs);
+
+			new_pcs = pcs | map_db->port_enable;
+			if (pcs == new_pcs)
+				break;
+		}
+
+		if (pcs == new_pcs)
+			ata_port_printk(ap, KERN_INFO, "updated PCS from "
+					"0x%x to 0x%x (%d tries)\n",
+					old_pcs, pcs, i);
+		else
+			ata_port_printk(ap, KERN_WARNING,
+					"failed to update PCS after %d tries, "
+					"old=0x%x cur=0x%x new=0x%x\n",
+					i, old_pcs, pcs, new_pcs);
+	}
+
 	for (i = 0; i < 2; i++) {
 		port = map[base + i];
 		if (port < 0)
@@ -816,35 +843,6 @@ static int __devinit piix_check_450nx_er
 	return no_piix_dma;
 }
 
-static void __devinit piix_init_pcs(struct pci_dev *pdev,
-				    struct ata_port_info *pinfo,
-				    const struct piix_map_db *map_db)
-{
-	u16 pcs, new_pcs;
-
-	pci_read_config_word(pdev, ICH5_PCS, &pcs);
-
-	new_pcs = pcs | map_db->port_enable;
-
-	if (new_pcs != pcs) {
-		DPRINTK("updating PCS from 0x%x to 0x%x\n", pcs, new_pcs);
-		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].host_flags |= PIIX_FLAG_IGNORE_PCS;
-		pinfo[1].host_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].host_flags &= ~PIIX_FLAG_IGNORE_PCS;
-		pinfo[1].host_flags &= ~PIIX_FLAG_IGNORE_PCS;
-	}
-}
-
 static void __devinit piix_init_sata_map(struct pci_dev *pdev,
 					 struct ata_port_info *pinfo,
 					 const struct piix_map_db *map_db)
@@ -893,6 +891,17 @@ static void __devinit piix_init_sata_map
 
 	hpriv->map = map;
 	hpriv->map_db = map_db;
+
+	/* handle force_pcs module parameter */
+	if (force_pcs == 1) {
+		dev_printk(KERN_INFO, &pdev->dev, "force ignoring PCS\n");
+		pinfo[0].host_flags |= PIIX_FLAG_IGNORE_PCS;
+		pinfo[1].host_flags |= PIIX_FLAG_IGNORE_PCS;
+	} else if (force_pcs == 2) {
+		dev_printk(KERN_INFO, &pdev->dev, "force honoring PCS\n");
+		pinfo[0].host_flags &= ~PIIX_FLAG_IGNORE_PCS;
+		pinfo[1].host_flags &= ~PIIX_FLAG_IGNORE_PCS;
+	}
 }
 
 /**
@@ -948,12 +957,9 @@ static int piix_init_one (struct pci_dev
 	}
 
 	/* Initialize SATA map */
-	if (host_flags & ATA_FLAG_SATA) {
+	if (host_flags & ATA_FLAG_SATA)
 		piix_init_sata_map(pdev, port_info,
 				   piix_map_db_table[ent->driver_data]);
-		piix_init_pcs(pdev, port_info,
-			      piix_map_db_table[ent->driver_data]);
-	}
 
 	/* On ICH5, some BIOSen disable the interrupt using the
 	 * PCI_COMMAND_INTX_DISABLE bit added in PCI 2.3.