[PATCH] fix check_region usage in eata_pio

[PATCH] fix check_region usage in eata_pio

[Christoph Hellwig]

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;
 
 	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) {

Re: [PATCH] fix check_region usage in eata_pio

[Randy.Dunlap]

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(-)


diff -Naurp ./drivers/scsi/eata_pio.c~eata_region ./drivers/scsi/eata_pio.c
--- ./drivers/scsi/eata_pio.c~eata_region	2004-06-01 15:13:25.000000000 -0700
+++ ./drivers/scsi/eata_pio.c	2004-06-01 15:29:30.000000000 -0700
@@ -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;
 				}
 

RE: [PATCH] fix check_region usage in eata_pio

[Salyzyn, Mark]

As one of the engineers that developed this card, the EATA ISA cards
take 9 bytes of accessible space. I strongly recommend this patch since
taking 8 carries with it risks.

The EISA and PCI cards took 32 and 16 bytes respectively though, which
should not be a problem since these are (BIOS) managed resources. The
PCI card when forced to occupy ISA addresses (170 and 1F0) typically did
not collide with other ISA resources since most cards occupy several
registers on even boundaries and could not be placed into the touchy +9
to +15 region. 

Sincerely -- Mark Salyzyn

-----Original Message-----
From: linux-scsi-owner@vger.kernel.org
[mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Randy.Dunlap
Sent: Tuesday, June 01, 2004 6:56 PM
To: Christoph Hellwig
Cc: jejb@steeleye.com; linux-scsi@vger.kernel.org
Subject: Re: [PATCH] fix check_region usage in eata_pio

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(-)


diff -Naurp ./drivers/scsi/eata_pio.c~eata_region
./drivers/scsi/eata_pio.c
--- ./drivers/scsi/eata_pio.c~eata_region	2004-06-01
15:13:25.000000000 -0700
+++ ./drivers/scsi/eata_pio.c	2004-06-01 15:29:30.000000000 -0700
@@ -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;
 				}
 
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix check_region usage in eata_pio

[Christoph Hellwig]

On Wed, Jun 02, 2004 at 09:24:40AM -0400, Salyzyn, Mark wrote:
> As one of the engineers that developed this card, the EATA ISA cards
> take 9 bytes of accessible space. I strongly recommend this patch since
> taking 8 carries with it risks.
> 
> The EISA and PCI cards took 32 and 16 bytes respectively though, which
> should not be a problem since these are (BIOS) managed resources. The
> PCI card when forced to occupy ISA addresses (170 and 1F0) typically did
> not collide with other ISA resources since most cards occupy several
> registers on even boundaries and could not be placed into the touchy +9
> to +15 region. 

Thanks a lot for the information.  I'll rework the patch to take that
into account.

You don't happen to have some of these boards still around at adaptec,
do you? :)