ata_piix: parallel scanning on PATA needs an extra locking

Hi,

This one is quite late but it fixes rather nasty 2.6.31 regression..


From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ata_piix: parallel scanning on PATA needs an extra locking

Commit log for commit 517d3cc15b36392e518abab6bacbb72089658313
("[libata] ata_piix: Enable parallel scan") says:

    This patch turns on parallel scanning for the ata_piix driver.
    This driver is used on most netbooks (no AHCI for cheap storage it seems).
    The scan is the dominating time factor in the kernel boot for these
    devices; with this flag it gets cut in half for the device I used
    for testing (eeepc).
    Alan took a look at the driver source and concluded that it ought to be safe
    to do for this driver.  Alan has also checked with the hardware team.

and it is all true but once we put all things together additional
constraints for PATA controllers show up (some hardware registers
have per-host not per-port atomicity) and we risk misprogramming
the controller.

I used the following test to check whether the issue is real:

@@ -736,8 +736,20 @@ static void piix_set_piomode(struct ata_
 			(timings[pio][1] << 8);
 	}
 	pci_write_config_word(dev, master_port, master_data);
-	if (is_slave)
+	if (is_slave) {
+		if (ap->port_no == 0) {
+			u8 tmp = slave_data;
+
+			while (slave_data == tmp) {
+				pci_read_config_byte(dev, slave_port, &tmp);
+				msleep(50);
+			}
+
+			dev_printk(KERN_ERR, &dev->dev, "PATA parallel scan "
+				   "race detected\n");
+		}
 		pci_write_config_byte(dev, slave_port, slave_data);
+	}
 
 	/* Ensure the UDMA bit is off - it will be turned back on if
 	   UDMA is selected */

and it indeed triggered the error message.

Lets fix all such races by adding an extra locking to ->set_piomode
and ->set_dmamode methods for PATA controllers.

Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Jeff Garzik <jgarzik@redhat.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ata/ata_piix.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

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

> Lets fix all such races by adding an extra locking to ->set_piomode
> and ->set_dmamode methods for PATA controllers.

Would it not be better to take the host lock in libata-core for these
cases so that we fix all the adapters in one swoop. Even if we are doing
this lock taking in the controller specific code it seems the right lock
to use ?

Looks fine as a temproary quickfix tho
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sunday 30 August 2009 16:13:21 Alan Cox wrote:
> > Lets fix all such races by adding an extra locking to ->set_piomode
> > and ->set_dmamode methods for PATA controllers.
> 
> Would it not be better to take the host lock in libata-core for these
> cases so that we fix all the adapters in one swoop. Even if we are doing
> this lock taking in the controller specific code it seems the right lock
> to use ?

Yes, taking the host lock would be a preferred solution in the long-term
and would allow us to enable parallel scanning for many other controllers
later.

The downside is that it requires somebody to audit all host drivers before
making such change and much more testing time..

> Looks fine as a temproary quickfix tho

That was the goal of the patch (I verified the issue only yesterday).
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html