libata: cleanup ata_sff_interrupt()

Traditional IDE interface sucks in that it doesn't have a reliable IRQ pending bit, so if the controller raises IRQ while the driver is expecting it not to, the IRQ won't be cleared and eventually the IRQ line will be killed by interrupt subsystem. Some controllers have non-standard mechanism to indicate IRQ pending so that this condition can be detected and worked around.

This patch adds an optional operation ->sff_irq_check() which will be called for each port from the ata_sff_interrupt() if an unexpected interrupt is received. If the operation returns %true, ->sff_check_status() and ->sff_irq_clear() will be cleared for the port. Note that this doesn't mark the interrupt as handled so it won't prevent IRQ subsystem from killing the IRQ if this mechanism fails to clear the spurious IRQ.

This patch also implements ->sff_irq_check() for ata_piix. Note that this adds slight overhead to shared IRQ operation as IRQs which are destined for other controllers will trigger extra register accesses to check whether IDE interrupt is pending but this solves rare screaming IRQ cases and for some curious reason also helps weird BIOS related glitch on Samsung n130 as reported in bko#14314.

http://bugzilla.kernel.org/show_bug.cgi?id=14314

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

Reported-by Johannes Stezenbach <[email protected]>

Reported-by Hans Werner <[email protected]>

Cc Alan Cox <[email protected]>

Cc Sergei Shtylyov <[email protected]>

---
This is generalized safe version of the previous patch which only worked for ata_piix and marked the interrupt as handled.

Thanks.

 drivers/ata/ata_piix.c   |   18 ++++++++++++++++--
 drivers/ata/libata-sff.c |   22 ++++++++++++++++++++++
 include/linux/libata.h   |    1 +
 3 files changed, 39 insertions(+), 2 deletions(-)

Index: ata/include/linux/libata.h
===================================================================

include/linux/libata.h +1 -0

@@ -857,6 +857,7 @@ struct ata_port_operations {
 	unsigned int (*sff_data_xfer)(struct ata_device *dev,
 			unsigned char *buf, unsigned int buflen, int rw);
 	u8   (*sff_irq_on)(struct ata_port *);
+	bool (*sff_irq_check)(struct ata_port *);
 	void (*sff_irq_clear)(struct ata_port *);
 
 	void (*bmdma_setup)(struct ata_queued_cmd *qc);

Index: ata/drivers/ata/ata_piix.c
===================================================================

drivers/ata/ata_piix.c +7 -1

@@ -173,6 +173,7 @@ static int piix_sidpr_scr_read(struct at
 			       unsigned int reg, u32 *val);
 static int piix_sidpr_scr_write(struct ata_link *link,
 				unsigned int reg, u32 val);
+static bool piix_irq_check(struct ata_port *ap);
 #ifdef CONFIG_PM
 static int piix_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
 static int piix_pci_device_resume(struct pci_dev *pdev);

@@ -309,8 +310,13 @@ static struct scsi_host_template piix_sh
 	ATA_BMDMA_SHT(DRV_NAME),
 };
 
-static struct ata_port_operations piix_pata_ops = {
+static struct ata_port_operations piix_base_ops = {
 	.inherits		= &ata_bmdma32_port_ops,
+	.sff_irq_check		= piix_irq_check,
+};
+
+static struct ata_port_operations piix_pata_ops = {
+	.inherits		= &piix_base_ops,
 	.cable_detect		= ata_cable_40wire,
 	.set_piomode		= piix_set_piomode,
 	.set_dmamode		= piix_set_dmamode,

drivers/ata/ata_piix.c +0 -0

 static struct ata_port_operations piix_sidpr_sata_ops = {

@@ -962,6 +968,14 @@ static int piix_sidpr_scr_write(struct a
 	return 0;
 }

Quoted 6 lines

drivers/ata/ata_piix.c +6 -0

+static bool piix_irq_check(struct ata_port *ap)
+{
+    if (unlikely(!ap->ioaddr.bmdma_addr))
+        return false;
+
+    return ap->ops->bmdma_status(ap) & ATA_DMA_INTR;

Review

Quoted 3 lines

(quoted diff) +2 -0

+}
+

I'm not at all sure that old, pre-ICH controllers set this bit also
for the PIO mode commands, not only for DMA. And if you didn't make such assumption, I don't see why this can't be generic and placed into libata-sff.c instead...

Quoted 6 lines

Quoted 6 lines

drivers/ata/ata_piix.c +6 -0

+static bool piix_irq_check(struct ata_port *ap)
+{
+    if (unlikely(!ap->ioaddr.bmdma_addr))
+        return false;
+
+    return ap->ops->bmdma_status(ap) & ATA_DMA_INTR;

Quoted 9 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+}
+

I'm not at all sure that old, pre-ICH controllers set this bit also
for the PIO mode commands, not only for DMA. And if you didn't make such assumption, I don't see why this can't be generic and placed into libata-sff.c instead...

Because different controllers have different mechanisms for detecting pending IRQ?

Quoted 2 lines

Because different controllers have different mechanisms for detecting pending IRQ?

All SFF-8038i (BMIDE) controllers have the same mechanism. They may
have some additional interrupt bits though, reflecting the interrupt status in both PIO and DMA mode though.

host->ports[i] is never NULL if i < host->n_ports and non-NULL return from ata_qc_from_tag() guarantees that the returned qc is active. Drop unnecessary tests.

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

---
 drivers/ata/libata-sff.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Index: ata/drivers/ata/libata-sff.c
===================================================================

drivers/ata/libata-sff.c +3 -5

@@ -1767,18 +1767,15 @@ irqreturn_t ata_sff_interrupt(int irq, v
 	spin_lock_irqsave(&host->lock, flags);
 
 	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap;
+		struct ata_port *ap = host->ports[i];
+		struct ata_queued_cmd *qc;
 
-		ap = host->ports[i];
-		if (ap &&
-		    !(ap->flags & ATA_FLAG_DISABLED)) {
-			struct ata_queued_cmd *qc;
+		if (unlikely(ap->flags & ATA_FLAG_DISABLED))

drivers/ata/libata-sff.c irqreturn_t ata_sff_interrupt(int irq, v · line 1777

Quoted 3 lines

drivers/ata/libata-sff.c +2 -0

+        qc = ata_qc_from_tag(ap, ap->link.active_tag);
+        if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))

() not needed around !x.

host->ports[i] is never NULL if i< host->n_ports and non-NULL return from ata_qc_from_tag() guarantees that the returned qc is active. Drop unnecessary tests.

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

---
  drivers/ata/libata-sff.c |   17 +++++++----------
  1 file changed, 7 insertions(+), 10 deletions(-)

Quoted 4 lines · Tejun Heo

host->ports[i] is never NULL if i< host->n_ports and non-NULL return from ata_qc_from_tag() guarantees that the returned qc is active. Drop unnecessary tests.

Quoted 4 lines

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

---
  drivers/ata/libata-sff.c |   17 +++++++----------
  1 file changed, 7 insertions(+), 10 deletions(-)

This is more #upstream material, don't you think? We are pretty deep into -rc at this point.