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.

[ Alan: would be better to take the host lock in libata-core for these
  cases so that we fix all the adapters in one swoop.  "Looks fine as a
  temproary quickfix tho" ]

Cc: Arjan van de Ven <arjan@linux.intel.com>
Acked-by: Alan Cox <alan@linux.intel.com>
Cc: Jeff Garzik <jgarzik@redhat.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 56b8a3ff12865041af813ca0bcfa49db143e5169..9ac4e378992ef60d71634121379b91479c8660f2 100644 (file)
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -664,6 +664,8 @@ static int piix_pata_prereset(struct ata_link *link, unsigned long deadline)
        return ata_sff_prereset(link, deadline);
 }
 
+static DEFINE_SPINLOCK(piix_lock);
+
 /**
  *     piix_set_piomode - Initialize host controller PATA PIO timings
  *     @ap: Port whose timings we are configuring
@@ -677,8 +679,9 @@ static int piix_pata_prereset(struct ata_link *link, unsigned long deadline)
 
 static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
 {
-       unsigned int pio        = adev->pio_mode - XFER_PIO_0;
        struct pci_dev *dev     = to_pci_dev(ap->host->dev);
+       unsigned long flags;
+       unsigned int pio        = adev->pio_mode - XFER_PIO_0;
        unsigned int is_slave   = (adev->devno != 0);
        unsigned int master_port= ap->port_no ? 0x42 : 0x40;
        unsigned int slave_port = 0x44;
@@ -708,6 +711,8 @@ static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
        if (adev->class == ATA_DEV_ATA)
                control |= 4;   /* PPE enable */
 
+       spin_lock_irqsave(&piix_lock, flags);
+
        /* PIO configuration clears DTE unconditionally.  It will be
         * programmed in set_dmamode which is guaranteed to be called
         * after set_piomode if any DMA mode is available.
@@ -747,6 +752,8 @@ static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
                udma_enable &= ~(1 << (2 * ap->port_no + adev->devno));
                pci_write_config_byte(dev, 0x48, udma_enable);
        }
+
+       spin_unlock_irqrestore(&piix_lock, flags);
 }
 
 /**
@@ -764,6 +771,7 @@ static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev)
 static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, int isich)
 {
        struct pci_dev *dev     = to_pci_dev(ap->host->dev);
+       unsigned long flags;
        u8 master_port          = ap->port_no ? 0x42 : 0x40;
        u16 master_data;
        u8 speed                = adev->dma_mode;
@@ -777,6 +785,8 @@ static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in
                            { 2, 1 },
                            { 2, 3 }, };
 
+       spin_lock_irqsave(&piix_lock, flags);
+
        pci_read_config_word(dev, master_port, &master_data);
        if (ap->udma_mask)
                pci_read_config_byte(dev, 0x48, &udma_enable);
@@ -867,6 +877,8 @@ static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in
        /* Don't scribble on 0x48 if the controller does not support UDMA */
        if (ap->udma_mask)
                pci_write_config_byte(dev, 0x48, udma_enable);
+
+       spin_unlock_irqrestore(&piix_lock, flags);
 }
 
 /**