Skip to contentSkip to conversation

ide: make "paranoia" ->handler check in ide_intr() more strict

PATCH New Bartlomiej Zolnierkiewicz 1 participant 6 messages Dec 29, 2008 libata

lore
Share & tools
lore
#0 lore raw

PATCH 1/6 ide: make "paranoia" ->handler check in ide_intr() more strict

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: make "paranoia" ->handler check in ide_intr() more strict

If ->handler is set while it shouldn't be it indicates deep problems so BUG_ON()-ning and preventing further damage is much more appropriate than merely printing an error message.

---
trivia for pata-2.6 tree

 drivers/ide/ide-io.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/ide/ide-io.c +3 -6
Index: b/drivers/ide/ide-io.c
===================================================================
@@ -1159,12 +1159,9 @@ irqreturn_t ide_intr (int irq, void *dev
 	 * won't allow another of the same (on any CPU) until we return.
 	 */
 	if (startstop == ide_stopped) {
-		if (hwif->handler == NULL) {	/* paranoia */
-			ide_unlock_port(hwif);
-			plug_device = 1;
-		} else
-			printk(KERN_ERR "%s: %s: huh? expected NULL handler "
-					"on exit\n", __func__, drive->name);
+		BUG_ON(hwif->handler);
+		ide_unlock_port(hwif);
+		plug_device = 1;
 	}
 out_handled:
 	irq_ret = IRQ_HANDLED;
#1 lore raw

PATCH 2/6 ide: use ide_pci_is_in_compatibility_mode() helper in setup-pci.c

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: use ide_pci_is_in_compatibility_mode() helper in setup-pci.c

There should be no functional changes caused by this patch.

---
 drivers/ide/setup-pci.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/ide/setup-pci.c +1 -2
Index: b/drivers/ide/setup-pci.c
===================================================================
@@ -519,8 +519,7 @@ static int do_ide_setup_pci_device(struc
 	if (ret < 0)
 		goto out;
 
-	/* Is it an "IDE storage" device in non-PCI mode? */
-	if ((dev->class >> 8) == PCI_CLASS_STORAGE_IDE && (dev->class & 5) != 5) {
+	if (ide_pci_is_in_compatibility_mode(dev)) {
 		if (noisy)
 			printk(KERN_INFO "%s %s: not 100%% native mode: will "
 				"probe irqs later\n", d->name, pci_name(dev));
#2 lore raw

PATCH 3/6 ide: remove superfluous hwif variable assignment from ide_timer_expiry()

In reply to Bartlomiej Zolnierkiewicz

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: remove superfluous hwif variable assignment from ide_timer_expiry()

There should be no functional changes caused by this patch.

---
 drivers/ide/ide-io.c |    1 -
 1 file changed, 1 deletion(-)
drivers/ide/ide-io.c +0 -1
Index: b/drivers/ide/ide-io.c
===================================================================
@@ -946,7 +946,6 @@ void ide_timer_expiry (unsigned long dat
 			 * globally mask the specific IRQ:
 			 */
 			spin_unlock(&hwif->lock);
-			hwif = drive->hwif;
 			/* disable_irq_nosync ?? */
 			disable_irq(hwif->irq);
 			/* local CPU only,
#3 lore raw

PATCH 4/6 ide: struct ide_atapi_pc - remove unused fields and update documentation

In reply to Bartlomiej Zolnierkiewicz

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: struct ide_atapi_pc - remove unused fields and update documentation

---
 include/linux/ide.h |    4 ----
 1 file changed, 4 deletions(-)
include/linux/ide.h +0 -4
Index: b/include/linux/ide.h
===================================================================
@@ -426,13 +426,9 @@ struct ide_atapi_pc {
 	struct idetape_bh *bh;
 	char *b_data;
 
-	/* idescsi only for now */
 	struct scatterlist *sg;
 	unsigned int sg_cnt;
 
-	struct scsi_cmnd *scsi_cmd;
-	void (*done) (struct scsi_cmnd *);
-
 	unsigned long timeout;
 };
 
#4 lore raw

PATCH 5/6 ide: remove unused ide_hwif_t.sg_mapped field

In reply to Bartlomiej Zolnierkiewicz

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: remove unused ide_hwif_t.sg_mapped field

---
 include/linux/ide.h |    1 -
 1 file changed, 1 deletion(-)
include/linux/ide.h +0 -1
Index: b/include/linux/ide.h
===================================================================
@@ -819,7 +819,6 @@ typedef struct hwif_s {
 	unsigned	extra_ports;	/* number of extra dma ports */
 
 	unsigned	present    : 1;	/* this interface exists */
-	unsigned	sg_mapped  : 1;	/* sg_table and sg_nents are ready */
 	unsigned	busy	   : 1; /* serializes devices on a port */
 
 	struct device		gendev;
#5 lore raw

PATCH 6/6 ide: remove now redundant ->cur_dev checks

In reply to Bartlomiej Zolnierkiewicz

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: remove now redundant ->cur_dev checks

* ->cur_dev should now be always valid if ->handler is set so remove redundant checks from ide_intr() and ide_timer_expiry().

* Apply CodingStyle fixups in ide_timer_expiry() while at it.

There should be no functional changes caused by this patch.

---
 drivers/ide/ide-io.c |  105 +++++++++++++++++++++------------------------------
 1 file changed, 45 insertions(+), 60 deletions(-)
drivers/ide/ide-io.c +45 -60
Index: b/drivers/ide/ide-io.c
===================================================================
@@ -920,60 +920,55 @@ void ide_timer_expiry (unsigned long dat
 		 * Either way, we don't really want to complain about anything.
 		 */
 	} else {
+		ide_expiry_t *expiry = hwif->expiry;
+		ide_startstop_t startstop = ide_stopped;
+
 		drive = hwif->cur_dev;
-		if (!drive) {
-			printk(KERN_ERR "%s: ->cur_dev was NULL\n", __func__);
-			hwif->handler = NULL;
-		} else {
-			ide_expiry_t *expiry = hwif->expiry;
-			ide_startstop_t startstop = ide_stopped;
 
-			if (expiry) {
-				/* continue */
-				if ((wait = expiry(drive)) > 0) {
-					/* reset timer */
-					hwif->timer.expires  = jiffies + wait;
-					hwif->req_gen_timer = hwif->req_gen;
-					add_timer(&hwif->timer);
-					spin_unlock_irqrestore(&hwif->lock, flags);
-					return;
-				}
-			}
-			hwif->handler = NULL;
-			/*
-			 * We need to simulate a real interrupt when invoking
-			 * the handler() function, which means we need to
-			 * globally mask the specific IRQ:
-			 */
-			spin_unlock(&hwif->lock);
-			/* disable_irq_nosync ?? */
-			disable_irq(hwif->irq);
-			/* local CPU only,
-			 * as if we were handling an interrupt */
-			local_irq_disable();
-			if (hwif->polling) {
-				startstop = handler(drive);
-			} else if (drive_is_ready(drive)) {
-				if (drive->waiting_for_dma)
-					hwif->dma_ops->dma_lost_irq(drive);
-				(void)ide_ack_intr(hwif);
-				printk(KERN_WARNING "%s: lost interrupt\n", drive->name);
-				startstop = handler(drive);
-			} else {
-				if (drive->waiting_for_dma) {
-					startstop = ide_dma_timeout_retry(drive, wait);
-				} else
-					startstop =
-					ide_error(drive, "irq timeout",
-						  hwif->tp_ops->read_status(hwif));
-			}
-			spin_lock_irq(&hwif->lock);
-			enable_irq(hwif->irq);
-			if (startstop == ide_stopped) {
-				ide_unlock_port(hwif);
-				plug_device = 1;
+		if (expiry) {
+			wait = expiry(drive);
+			if (wait > 0) { /* continue */
+				/* reset timer */
+				hwif->timer.expires = jiffies + wait;
+				hwif->req_gen_timer = hwif->req_gen;
+				add_timer(&hwif->timer);
+				spin_unlock_irqrestore(&hwif->lock, flags);
+				return;
 			}
 		}
+		hwif->handler = NULL;
+		/*
+		 * We need to simulate a real interrupt when invoking
+		 * the handler() function, which means we need to
+		 * globally mask the specific IRQ:
+		 */
+		spin_unlock(&hwif->lock);
+		/* disable_irq_nosync ?? */
+		disable_irq(hwif->irq);
+		/* local CPU only, as if we were handling an interrupt */
+		local_irq_disable();
+		if (hwif->polling) {
+			startstop = handler(drive);
+		} else if (drive_is_ready(drive)) {
+			if (drive->waiting_for_dma)
+				hwif->dma_ops->dma_lost_irq(drive);
+			(void)ide_ack_intr(hwif);
+			printk(KERN_WARNING "%s: lost interrupt\n",
+				drive->name);
+			startstop = handler(drive);
+		} else {
+			if (drive->waiting_for_dma)
+				startstop = ide_dma_timeout_retry(drive, wait);
+			else
+				startstop = ide_error(drive, "irq timeout",
+					hwif->tp_ops->read_status(hwif));
+		}
+		spin_lock_irq(&hwif->lock);
+		enable_irq(hwif->irq);
+		if (startstop == ide_stopped) {
+			ide_unlock_port(hwif);
+			plug_device = 1;
+		}
 	}
 	spin_unlock_irqrestore(&hwif->lock, flags);
 
@@ -1115,15 +1110,6 @@ irqreturn_t ide_intr (int irq, void *dev
 	}
 
 	drive = hwif->cur_dev;
-	if (!drive) {
-		/*
-		 * This should NEVER happen, and there isn't much
-		 * we could do about it here.
-		 *
-		 * [Note - this can occur if the drive is hot unplugged]
-		 */
-		goto out_handled;
-	}
 
 	if (!drive_is_ready(drive))
 		/*
@@ -1162,7 +1148,6 @@ irqreturn_t ide_intr (int irq, void *dev
 		ide_unlock_port(hwif);
 		plug_device = 1;
 	}
-out_handled:
 	irq_ret = IRQ_HANDLED;
 out:
 	spin_unlock_irqrestore(&hwif->lock, flags);