Skip to contentSkip to conversation

fix check_region usage in eata_pio

PATCH In review Christoph Hellwig 2 participants 2 messages May 31, 2004 – Jun 1, 2004 SCSI

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 fix check_region usage in eata_pio

I'd love to rework the init sequence a bit more, but without beeing able to actually test the driver I'd rather stick to the bulletproof fix.

drivers/scsi/eata_pio.c +58 -44
@@ -593,14 +593,14 @@
 	int z;
 	unsigned short *p;
 
-	if (check_region(base, 9))
-		return (FALSE);
+	if (!request_region(base, 8, "eata_pio"))
+		return 0;
 
 	memset(buf, 0, sizeof(struct get_conf));
 
 	while (inb(base + HA_RSTATUS) & HA_SBUSY)
 		if (--loop == 0)
-			return (FALSE);
+			goto fail;
 
 	DBG(DBG_PIO && DBG_PROBE, printk(KERN_DEBUG "Issuing PIO READ CONFIG to HBA at %#x\n", base));
 	eata_pio_send_command(base, EATA_CMD_PIO_READ_CONFIG);
@@ -609,30 +609,40 @@
 	for (p = (unsigned short *) buf; (long) p <= ((long) buf + (sizeof(struct get_conf) / 2)); p++) {
 		while (!(inb(base + HA_RSTATUS) & HA_SDRQ))
 			if (--loop == 0)
-				return (FALSE);
+				goto fail;
 
 		loop = HZ / 2;
 		*p = inw(base + HA_RDATA);
 	}
-	if (!(inb(base + HA_RSTATUS) & HA_SERROR)) {	/* Error ? */
-		if (htonl(EATA_SIGNATURE) == buf->signature) {
-			DBG(DBG_PIO && DBG_PROBE, printk(KERN_NOTICE "EATA Controller found " "at %#4x EATA Level: %x\n", base, (uint) (buf->version)));
-
-			while (inb(base + HA_RSTATUS) & HA_SDRQ)
-				inw(base + HA_RDATA);
-			if (ALLOW_DMA_BOARDS == FALSE) {
-				for (z = 0; z < MAXISA; z++)
-					if (base == ISAbases[z]) {
-						buf->IRQ = ISAirqs[z];
-						break;
-					}
+	if (inb(base + HA_RSTATUS) & HA_SERROR) {
+		DBG(DBG_PROBE, printk("eata_dma: get_conf_PIO, error during "
+					"transfer for HBA at %x\n", base));
+		goto fail;
+	}
+
+	if (htonl(EATA_SIGNATURE) != buf->signature)
+		goto fail;
+
+	DBG(DBG_PIO && DBG_PROBE, printk(KERN_NOTICE "EATA Controller found "
+				"at %#4x EATA Level: %x\n",
+				base, (uint) (buf->version)));
+
+	while (inb(base + HA_RSTATUS) & HA_SDRQ)
+		inw(base + HA_RDATA);
+
+	if (ALLOW_DMA_BOARDS == FALSE) {
+		for (z = 0; z < MAXISA; z++)
+			if (base == ISAbases[z]) {
+				buf->IRQ = ISAirqs[z];
+				break;
 			}
-			return (TRUE);
-		}
-	} else {
-		DBG(DBG_PROBE, printk("eata_dma: get_conf_PIO, error during transfer " "for HBA at %x\n", base));
 	}
-	return (FALSE);
+
+	return 1;
+
+ fail:
+	release_region(base, 8);
+	return 0;
 }
 
 static void print_pio_config(struct get_conf *gc)
@@ -689,23 +699,19 @@
 
 	if ((buff = get_pio_board_data((uint) base, gc->IRQ, gc->scsi_id[3], cplen = (htonl(gc->cplen) + 1) / 2, cppadlen = (htons(gc->cppadlen) + 1) / 2)) == NULL) {
 		printk("HBA at %#lx didn't react on INQUIRY. Sorry.\n", (unsigned long) base);
-		return (FALSE);
+		return 0;
 	}
 
 	if (print_selftest(base) == FALSE && ALLOW_DMA_BOARDS == FALSE) {
 		printk("HBA at %#lx failed while performing self test & setup.\n", (unsigned long) base);
-		return (FALSE);
+		return 0;
 	}
 
-	request_region(base, 8, "eata_pio");
-
 	size = sizeof(hostdata) + (sizeof(struct eata_ccb) * ntohs(gc->queuesiz));
 
 	sh = scsi_register(tpnt, size);
-	if (sh == NULL) {
-		release_region(base, 8);
-		return FALSE;
-	}
+	if (sh == NULL)
+		return 0;
 
 	if (!reg_IRQ[gc->IRQ]) {	/* Interrupt already registered ? */
 		if (!request_irq(gc->IRQ, do_eata_pio_int_handler, SA_INTERRUPT, "EATA-PIO", sh)) {
@@ -714,14 +720,12 @@
 				reg_IRQL[gc->IRQ] = TRUE;	/* IRQ is edge triggered */
 		} else {
 			printk("Couldn't allocate IRQ %d, Sorry.\n", gc->IRQ);
-			release_region(base, 8);
-			return (FALSE);
+			return 0;
 		}
 	} else {		/* More than one HBA on this IRQ */
 		if (reg_IRQL[gc->IRQ] == TRUE) {
 			printk("Can't support more than one HBA on this IRQ,\n" "  if the IRQ is edge triggered. Sorry.\n");
-			release_region(base, 8);
-			return (FALSE);
+			return 0;
 		} else
 			reg_IRQ[gc->IRQ]++;
 	}
@@ -816,12 +820,14 @@
 	int i;
 
 	for (i = 0; i < MAXISA; i++) {
-		if (ISAbases[i]) {
-			if (get_pio_conf_PIO(ISAbases[i], buf) == TRUE) {
-				register_pio_HBA(ISAbases[i], buf, tpnt);
-			}
+		if (!ISAbases[i])
+			continue;
+		if (!get_pio_conf_PIO(ISAbases[i], buf))
+			continue;
+		if (!register_pio_HBA(ISAbases[i], buf, tpnt))
+			release_region(ISAbases[i], 8);
+		else
 			ISAbases[i] = 0;
-		}
 	}
 	return;
 }
@@ -847,12 +853,15 @@
 			if (((pal1 == 0x12) && (pal2 == 0x14)) || ((pal1 == 0x38) && (pal2 == 0xa3) && (pal3 == 0x82)) || ((pal1 == 0x06) && (pal2 == 0x94) && (pal3 == 0x24))) {
 				DBG(DBG_PROBE, printk(KERN_NOTICE "EISA EATA id tags found: " "%x %x %x \n", (int) pal1, (int) pal2, (int) pal3));
 #endif
-				if (get_pio_conf_PIO(base, buf) == TRUE) {
+				if (get_pio_conf_PIO(base, buf)) {
 					DBG(DBG_PROBE && DBG_EISA, print_pio_config(buf));
 					if (buf->IRQ) {
-						register_pio_HBA(base, buf, tpnt);
-					} else
+						if (!register_pio_HBA(base, buf, tpnt))
+							release_region(base, 8);
+					} else {
 						printk(KERN_NOTICE "eata_dma: No valid IRQ. HBA " "removed from list\n");
+						release_region(base, 8);
+					}
 				}
 				/* Nothing found here so we take it from the list */
 				EISAbases[i] = 0;
@@ -889,16 +898,21 @@
 		base += 0x10;	/* Now, THIS is the real address */
 		if (base != 0x1f8) {
 			/* We didn't find it in the primary search */
-			if (get_pio_conf_PIO(base, buf) == TRUE) {
-				if (buf->FORCADR)	/* If the address is forced */
+			if (get_pio_conf_PIO(base, buf)) {
+				if (buf->FORCADR) {	/* If the address is forced */
+					release_region(base, 8);
 					continue;	/* we'll find it later      */
+				}
 
 				/* OK. We made it till here, so we can go now  
 				 * and register it. We  only have to check and 
 				 * eventually remove it from the EISA and ISA list 
 				 */
 
-				register_pio_HBA(base, buf, tpnt);
+				if (!register_pio_HBA(base, buf, tpnt)) {
+					release_region(base, 8);
+					continue;
+				}
 
 				if (base < 0x1000) {
 					for (x = 0; x < MAXISA; ++x) {
#1 lore raw

On Mon, 31 May 2004 14:00:10 +0200 Christoph Hellwig wrote:

| I'd love to rework the init sequence a bit more, but without beeing able
| to actually test the driver I'd rather stick to the bulletproof fix.
| 
| 
| --- 1.21/drivers/scsi/eata_pio.c	2003-08-16 16:01:30 +02:00
| +++ edited/drivers/scsi/eata_pio.c	2004-05-31 13:30:48 +02:00
| @@ -593,14 +593,14 @@
|  	int z;
|  	unsigned short *p;
|  
| -	if (check_region(base, 9))
| -		return (FALSE);
| +	if (!request_region(base, 8, "eata_pio"))
| +		return 0;

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Using region size of 8 instead of 9 caught my eye. The driver isn't very consistent about it. However, "eata_generic.h" says that there is a register at offset 8 and the code does inb() on that register, so region size (apparently) should be 9.

Here's an update that applies on top of Christoph's patch.

--
~Randy

Based on "eata_generic.h", this SCSI controller has 9 bytes of IO space, not 8, so update request_region(), release_region(), and n_io_port to use 9 instead of 8.

diffstat:=

 drivers/scsi/eata_pio.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
drivers/scsi/eata_pio.c renamed from drivers/scsi/eata_pio.c~eata_region +8 -8
diff -Naurp ./drivers/scsi/eata_pio.c~eata_region ./drivers/scsi/eata_pio.c
@@ -593,7 +593,7 @@ static int get_pio_conf_PIO(u32 base, st
 	int z;
 	unsigned short *p;
 
-	if (!request_region(base, 8, "eata_pio"))
+	if (!request_region(base, 9, "eata_pio"))
 		return 0;
 
 	memset(buf, 0, sizeof(struct get_conf));
@@ -641,7 +641,7 @@ static int get_pio_conf_PIO(u32 base, st
 	return 1;
 
  fail:
-	release_region(base, 8);
+	release_region(base, 9);
 	return 0;
 }
 
@@ -784,7 +784,7 @@ static int register_pio_HBA(long base, s
 	sh->unique_id = base;
 	sh->base = base;
 	sh->io_port = base;
-	sh->n_io_port = 8;
+	sh->n_io_port = 9;
 	sh->irq = gc->IRQ;
 	sh->dma_channel = PIO;
 	sh->this_id = gc->scsi_id[3];
@@ -825,7 +825,7 @@ static void find_pio_ISA(struct get_conf
 		if (!get_pio_conf_PIO(ISAbases[i], buf))
 			continue;
 		if (!register_pio_HBA(ISAbases[i], buf, tpnt))
-			release_region(ISAbases[i], 8);
+			release_region(ISAbases[i], 9);
 		else
 			ISAbases[i] = 0;
 	}
@@ -857,10 +857,10 @@ static void find_pio_EISA(struct get_con
 					DBG(DBG_PROBE && DBG_EISA, print_pio_config(buf));
 					if (buf->IRQ) {
 						if (!register_pio_HBA(base, buf, tpnt))
-							release_region(base, 8);
+							release_region(base, 9);
 					} else {
 						printk(KERN_NOTICE "eata_dma: No valid IRQ. HBA " "removed from list\n");
-						release_region(base, 8);
+						release_region(base, 9);
 					}
 				}
 				/* Nothing found here so we take it from the list */
@@ -900,7 +900,7 @@ static void find_pio_PCI(struct get_conf
 			/* We didn't find it in the primary search */
 			if (get_pio_conf_PIO(base, buf)) {
 				if (buf->FORCADR) {	/* If the address is forced */
-					release_region(base, 8);
+					release_region(base, 9);
 					continue;	/* we'll find it later      */
 				}
 
@@ -910,7 +910,7 @@ static void find_pio_PCI(struct get_conf
 				 */
 
 				if (!register_pio_HBA(base, buf, tpnt)) {
-					release_region(base, 8);
+					release_region(base, 9);
 					continue;
 				}