public inbox for linux-scsi@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
       [not found] <09821349085390234lkjasdflkjasflkdj24746@havoc.gtf.org>
@ 2007-10-24 23:48 ` Jeff Garzik
  2007-10-25  4:27   ` Andrew Morton
  2007-10-25 14:32   ` Salyzyn, Mark
  2007-10-24 23:48 ` [PATCH 2/4] [SCSI] ips: trim trailing whitespace Jeff Garzik
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 16+ messages in thread
From: Jeff Garzik @ 2007-10-24 23:48 UTC (permalink / raw)
  To: LKML, linux-scsi; +Cc: akpm

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/scsi/ips.c |  178 ++++++++++++++++++++++++----------------------------
 drivers/scsi/ips.h |   20 +++----
 2 files changed, 91 insertions(+), 107 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index 5c5a9b2..595a91a 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -707,7 +707,7 @@ ips_release(struct Scsi_Host *sh)
 		release_region(ha->io_addr, ha->io_len);
 
 	/* free IRQ */
-	free_irq(ha->irq, ha);
+	free_irq(ha->pcidev->irq, ha);
 
 	scsi_host_put(sh);
 
@@ -1637,7 +1637,7 @@ ips_make_passthru(ips_ha_t *ha, struct scsi_cmnd *SC, ips_scb_t *scb, int intr)
 				return (IPS_FAILURE);
 			}
 
-			if (ha->device_id == IPS_DEVICEID_COPPERHEAD &&
+			if (ha->pcidev->device == IPS_DEVICEID_COPPERHEAD &&
 			    pt->CoppCP.cmd.flashfw.op_code ==
 			    IPS_CMD_RW_BIOSFW) {
 				ret = ips_flash_copperhead(ha, pt, scb);
@@ -2021,7 +2021,7 @@ ips_cleanup_passthru(ips_ha_t * ha, ips_scb_t * scb)
 	pt->ExtendedStatus = scb->extended_status;
 	pt->AdapterType = ha->ad_type;
 
-	if (ha->device_id == IPS_DEVICEID_COPPERHEAD &&
+	if (ha->pcidev->device == IPS_DEVICEID_COPPERHEAD &&
 	    (scb->cmd.flashfw.op_code == IPS_CMD_DOWNLOAD ||
 	     scb->cmd.flashfw.op_code == IPS_CMD_RW_BIOSFW))
 		ips_free_flash_copperhead(ha);
@@ -2075,7 +2075,7 @@ ips_host_info(ips_ha_t * ha, char *ptr, off_t offset, int len)
 			  ha->mem_ptr);
 	}
 
-	copy_info(&info, "\tIRQ number                        : %d\n", ha->irq);
+	copy_info(&info, "\tIRQ number                        : %d\n", ha->pcidev->irq);
 
     /* For the Next 3 lines Check for Binary 0 at the end and don't include it if it's there. */
     /* That keeps everything happy for "text" operations on the proc file.                    */
@@ -2232,31 +2232,31 @@ ips_identify_controller(ips_ha_t * ha)
 {
 	METHOD_TRACE("ips_identify_controller", 1);
 
-	switch (ha->device_id) {
+	switch (ha->pcidev->device) {
 	case IPS_DEVICEID_COPPERHEAD:
-		if (ha->revision_id <= IPS_REVID_SERVERAID) {
+		if (ha->pcidev->revision <= IPS_REVID_SERVERAID) {
 			ha->ad_type = IPS_ADTYPE_SERVERAID;
-		} else if (ha->revision_id == IPS_REVID_SERVERAID2) {
+		} else if (ha->pcidev->revision == IPS_REVID_SERVERAID2) {
 			ha->ad_type = IPS_ADTYPE_SERVERAID2;
-		} else if (ha->revision_id == IPS_REVID_NAVAJO) {
+		} else if (ha->pcidev->revision == IPS_REVID_NAVAJO) {
 			ha->ad_type = IPS_ADTYPE_NAVAJO;
-		} else if ((ha->revision_id == IPS_REVID_SERVERAID2)
+		} else if ((ha->pcidev->revision == IPS_REVID_SERVERAID2)
 			   && (ha->slot_num == 0)) {
 			ha->ad_type = IPS_ADTYPE_KIOWA;
-		} else if ((ha->revision_id >= IPS_REVID_CLARINETP1) &&
-			   (ha->revision_id <= IPS_REVID_CLARINETP3)) {
+		} else if ((ha->pcidev->revision >= IPS_REVID_CLARINETP1) &&
+			   (ha->pcidev->revision <= IPS_REVID_CLARINETP3)) {
 			if (ha->enq->ucMaxPhysicalDevices == 15)
 				ha->ad_type = IPS_ADTYPE_SERVERAID3L;
 			else
 				ha->ad_type = IPS_ADTYPE_SERVERAID3;
-		} else if ((ha->revision_id >= IPS_REVID_TROMBONE32) &&
-			   (ha->revision_id <= IPS_REVID_TROMBONE64)) {
+		} else if ((ha->pcidev->revision >= IPS_REVID_TROMBONE32) &&
+			   (ha->pcidev->revision <= IPS_REVID_TROMBONE64)) {
 			ha->ad_type = IPS_ADTYPE_SERVERAID4H;
 		}
 		break;
 
 	case IPS_DEVICEID_MORPHEUS:
-		switch (ha->subdevice_id) {
+		switch (ha->pcidev->subsystem_device) {
 		case IPS_SUBDEVICEID_4L:
 			ha->ad_type = IPS_ADTYPE_SERVERAID4L;
 			break;
@@ -2285,7 +2285,7 @@ ips_identify_controller(ips_ha_t * ha)
 		break;
 
 	case IPS_DEVICEID_MARCO:
-		switch (ha->subdevice_id) {
+		switch (ha->pcidev->subsystem_device) {
 		case IPS_SUBDEVICEID_6M:
 			ha->ad_type = IPS_ADTYPE_SERVERAID6M;
 			break;
@@ -2332,20 +2332,20 @@ ips_get_bios_version(ips_ha_t * ha, int intr)
 
 	strncpy(ha->bios_version, "       ?", 8);
 
-	if (ha->device_id == IPS_DEVICEID_COPPERHEAD) {
+	if (ha->pcidev->device == IPS_DEVICEID_COPPERHEAD) {
 		if (IPS_USE_MEMIO(ha)) {
 			/* Memory Mapped I/O */
 
 			/* test 1st byte */
 			writel(0, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			if (readb(ha->mem_ptr + IPS_REG_FLDP) != 0x55)
 				return;
 
 			writel(1, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			if (readb(ha->mem_ptr + IPS_REG_FLDP) != 0xAA)
@@ -2353,20 +2353,20 @@ ips_get_bios_version(ips_ha_t * ha, int intr)
 
 			/* Get Major version */
 			writel(0x1FF, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			major = readb(ha->mem_ptr + IPS_REG_FLDP);
 
 			/* Get Minor version */
 			writel(0x1FE, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 			minor = readb(ha->mem_ptr + IPS_REG_FLDP);
 
 			/* Get SubMinor version */
 			writel(0x1FD, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 			subminor = readb(ha->mem_ptr + IPS_REG_FLDP);
 
@@ -2375,14 +2375,14 @@ ips_get_bios_version(ips_ha_t * ha, int intr)
 
 			/* test 1st byte */
 			outl(0, ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			if (inb(ha->io_addr + IPS_REG_FLDP) != 0x55)
 				return;
 
 			outl(cpu_to_le32(1), ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			if (inb(ha->io_addr + IPS_REG_FLDP) != 0xAA)
@@ -2390,21 +2390,21 @@ ips_get_bios_version(ips_ha_t * ha, int intr)
 
 			/* Get Major version */
 			outl(cpu_to_le32(0x1FF), ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			major = inb(ha->io_addr + IPS_REG_FLDP);
 
 			/* Get Minor version */
 			outl(cpu_to_le32(0x1FE), ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			minor = inb(ha->io_addr + IPS_REG_FLDP);
 
 			/* Get SubMinor version */
 			outl(cpu_to_le32(0x1FD), ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			subminor = inb(ha->io_addr + IPS_REG_FLDP);
@@ -4903,7 +4903,7 @@ ips_init_copperhead(ips_ha_t * ha)
 	/* Enable busmastering */
 	outb(IPS_BIT_EBM, ha->io_addr + IPS_REG_SCPR);
 
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		/* fix for anaconda64 */
 		outl(0, ha->io_addr + IPS_REG_NDAE);
 
@@ -4997,7 +4997,7 @@ ips_init_copperhead_memio(ips_ha_t * ha)
 	/* Enable busmastering */
 	writeb(IPS_BIT_EBM, ha->mem_ptr + IPS_REG_SCPR);
 
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		/* fix for anaconda64 */
 		writel(0, ha->mem_ptr + IPS_REG_NDAE);
 
@@ -5142,7 +5142,7 @@ ips_reset_copperhead(ips_ha_t * ha)
 	METHOD_TRACE("ips_reset_copperhead", 1);
 
 	DEBUG_VAR(1, "(%s%d) ips_reset_copperhead: io addr: %x, irq: %d",
-		  ips_name, ha->host_num, ha->io_addr, ha->irq);
+		  ips_name, ha->host_num, ha->io_addr, ha->pcidev->irq);
 
 	reset_counter = 0;
 
@@ -5187,7 +5187,7 @@ ips_reset_copperhead_memio(ips_ha_t * ha)
 	METHOD_TRACE("ips_reset_copperhead_memio", 1);
 
 	DEBUG_VAR(1, "(%s%d) ips_reset_copperhead_memio: mem addr: %x, irq: %d",
-		  ips_name, ha->host_num, ha->mem_addr, ha->irq);
+		  ips_name, ha->host_num, ha->mem_addr, ha->pcidev->irq);
 
 	reset_counter = 0;
 
@@ -5233,7 +5233,7 @@ ips_reset_morpheus(ips_ha_t * ha)
 	METHOD_TRACE("ips_reset_morpheus", 1);
 
 	DEBUG_VAR(1, "(%s%d) ips_reset_morpheus: mem addr: %x, irq: %d",
-		  ips_name, ha->host_num, ha->mem_addr, ha->irq);
+		  ips_name, ha->host_num, ha->mem_addr, ha->pcidev->irq);
 
 	reset_counter = 0;
 
@@ -6196,32 +6196,32 @@ ips_erase_bios(ips_ha_t * ha)
 
 	/* Clear the status register */
 	outl(0, ha->io_addr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	outb(0x50, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Setup */
 	outb(0x20, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Confirm */
 	outb(0xD0, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Status */
 	outb(0x70, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	timeout = 80000;	/* 80 seconds */
 
 	while (timeout > 0) {
-		if (ha->revision_id == IPS_REVID_TROMBONE64) {
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 			outl(0, ha->io_addr + IPS_REG_FLAP);
 			udelay(25);	/* 25 us */
 		}
@@ -6241,13 +6241,13 @@ ips_erase_bios(ips_ha_t * ha)
 
 		/* try to suspend the erase */
 		outb(0xB0, ha->io_addr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		/* wait for 10 seconds */
 		timeout = 10000;
 		while (timeout > 0) {
-			if (ha->revision_id == IPS_REVID_TROMBONE64) {
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 				outl(0, ha->io_addr + IPS_REG_FLAP);
 				udelay(25);	/* 25 us */
 			}
@@ -6277,12 +6277,12 @@ ips_erase_bios(ips_ha_t * ha)
 	/* Otherwise, we were successful */
 	/* clear status */
 	outb(0x50, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* enable reads */
 	outb(0xFF, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	return (0);
@@ -6308,32 +6308,32 @@ ips_erase_bios_memio(ips_ha_t * ha)
 
 	/* Clear the status register */
 	writel(0, ha->mem_ptr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	writeb(0x50, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Setup */
 	writeb(0x20, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Confirm */
 	writeb(0xD0, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* Erase Status */
 	writeb(0x70, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	timeout = 80000;	/* 80 seconds */
 
 	while (timeout > 0) {
-		if (ha->revision_id == IPS_REVID_TROMBONE64) {
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 			writel(0, ha->mem_ptr + IPS_REG_FLAP);
 			udelay(25);	/* 25 us */
 		}
@@ -6353,13 +6353,13 @@ ips_erase_bios_memio(ips_ha_t * ha)
 
 		/* try to suspend the erase */
 		writeb(0xB0, ha->mem_ptr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		/* wait for 10 seconds */
 		timeout = 10000;
 		while (timeout > 0) {
-			if (ha->revision_id == IPS_REVID_TROMBONE64) {
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 				writel(0, ha->mem_ptr + IPS_REG_FLAP);
 				udelay(25);	/* 25 us */
 			}
@@ -6389,12 +6389,12 @@ ips_erase_bios_memio(ips_ha_t * ha)
 	/* Otherwise, we were successful */
 	/* clear status */
 	writeb(0x50, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	/* enable reads */
 	writeb(0xFF, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	return (0);
@@ -6423,21 +6423,21 @@ ips_program_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 	for (i = 0; i < buffersize; i++) {
 		/* write a byte */
 		outl(cpu_to_le32(i + offset), ha->io_addr + IPS_REG_FLAP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		outb(0x40, ha->io_addr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		outb(buffer[i], ha->io_addr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		/* wait up to one second */
 		timeout = 1000;
 		while (timeout > 0) {
-			if (ha->revision_id == IPS_REVID_TROMBONE64) {
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 				outl(0, ha->io_addr + IPS_REG_FLAP);
 				udelay(25);	/* 25 us */
 			}
@@ -6454,11 +6454,11 @@ ips_program_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 		if (timeout == 0) {
 			/* timeout error */
 			outl(0, ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			outb(0xFF, ha->io_addr + IPS_REG_FLDP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			return (1);
@@ -6468,11 +6468,11 @@ ips_program_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 		if (status & 0x18) {
 			/* programming error */
 			outl(0, ha->io_addr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			outb(0xFF, ha->io_addr + IPS_REG_FLDP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			return (1);
@@ -6481,11 +6481,11 @@ ips_program_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 
 	/* Enable reading */
 	outl(0, ha->io_addr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	outb(0xFF, ha->io_addr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	return (0);
@@ -6514,21 +6514,21 @@ ips_program_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 	for (i = 0; i < buffersize; i++) {
 		/* write a byte */
 		writel(i + offset, ha->mem_ptr + IPS_REG_FLAP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		writeb(0x40, ha->mem_ptr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		writeb(buffer[i], ha->mem_ptr + IPS_REG_FLDP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		/* wait up to one second */
 		timeout = 1000;
 		while (timeout > 0) {
-			if (ha->revision_id == IPS_REVID_TROMBONE64) {
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64) {
 				writel(0, ha->mem_ptr + IPS_REG_FLAP);
 				udelay(25);	/* 25 us */
 			}
@@ -6545,11 +6545,11 @@ ips_program_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 		if (timeout == 0) {
 			/* timeout error */
 			writel(0, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			writeb(0xFF, ha->mem_ptr + IPS_REG_FLDP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			return (1);
@@ -6559,11 +6559,11 @@ ips_program_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 		if (status & 0x18) {
 			/* programming error */
 			writel(0, ha->mem_ptr + IPS_REG_FLAP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			writeb(0xFF, ha->mem_ptr + IPS_REG_FLDP);
-			if (ha->revision_id == IPS_REVID_TROMBONE64)
+			if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 				udelay(25);	/* 25 us */
 
 			return (1);
@@ -6572,11 +6572,11 @@ ips_program_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 
 	/* Enable reading */
 	writel(0, ha->mem_ptr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	writeb(0xFF, ha->mem_ptr + IPS_REG_FLDP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	return (0);
@@ -6601,14 +6601,14 @@ ips_verify_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 
 	/* test 1st byte */
 	outl(0, ha->io_addr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	if (inb(ha->io_addr + IPS_REG_FLDP) != 0x55)
 		return (1);
 
 	outl(cpu_to_le32(1), ha->io_addr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 	if (inb(ha->io_addr + IPS_REG_FLDP) != 0xAA)
 		return (1);
@@ -6617,7 +6617,7 @@ ips_verify_bios(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 	for (i = 2; i < buffersize; i++) {
 
 		outl(cpu_to_le32(i + offset), ha->io_addr + IPS_REG_FLAP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		checksum = (uint8_t) checksum + inb(ha->io_addr + IPS_REG_FLDP);
@@ -6650,14 +6650,14 @@ ips_verify_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 
 	/* test 1st byte */
 	writel(0, ha->mem_ptr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 
 	if (readb(ha->mem_ptr + IPS_REG_FLDP) != 0x55)
 		return (1);
 
 	writel(1, ha->mem_ptr + IPS_REG_FLAP);
-	if (ha->revision_id == IPS_REVID_TROMBONE64)
+	if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 		udelay(25);	/* 25 us */
 	if (readb(ha->mem_ptr + IPS_REG_FLDP) != 0xAA)
 		return (1);
@@ -6666,7 +6666,7 @@ ips_verify_bios_memio(ips_ha_t * ha, char *buffer, uint32_t buffersize,
 	for (i = 2; i < buffersize; i++) {
 
 		writel(i + offset, ha->mem_ptr + IPS_REG_FLAP);
-		if (ha->revision_id == IPS_REVID_TROMBONE64)
+		if (ha->pcidev->revision == IPS_REVID_TROMBONE64)
 			udelay(25);	/* 25 us */
 
 		checksum =
@@ -6837,9 +6837,9 @@ ips_register_scsi(int index)
 	}
 	ha = IPS_HA(sh);
 	memcpy(ha, oldha, sizeof (ips_ha_t));
-	free_irq(oldha->irq, oldha);
+	free_irq(oldha->pcidev->irq, oldha);
 	/* Install the interrupt handler with the new ha */
-	if (request_irq(ha->irq, do_ipsintr, IRQF_SHARED, ips_name, ha)) {
+	if (request_irq(ha->pcidev->irq, do_ipsintr, IRQF_SHARED, ips_name, ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to install interrupt handler\n");
 		scsi_host_put(sh);
@@ -6851,10 +6851,7 @@ ips_register_scsi(int index)
 	ips_ha[index] = ha;
 
 	/* Store away needed values for later use */
-	sh->io_port = ha->io_addr;
-	sh->n_io_port = ha->io_addr ? 255 : 0;
 	sh->unique_id = (ha->io_addr) ? ha->io_addr : ha->mem_addr;
-	sh->irq = ha->irq;
 	sh->sg_tablesize = sh->hostt->sg_tablesize;
 	sh->can_queue = sh->hostt->can_queue;
 	sh->cmd_per_lun = sh->hostt->cmd_per_lun;
@@ -6992,8 +6989,6 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 	uint32_t mem_len;
 	uint8_t bus;
 	uint8_t func;
-	uint8_t irq;
-	uint16_t subdevice_id;
 	int j;
 	int index;
 	dma_addr_t dma_address;
@@ -7014,7 +7009,6 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 		return -1;
 
 	/* stuff that we get in dev */
-	irq = pci_dev->irq;
 	bus = pci_dev->bus->number;
 	func = pci_dev->devfn;
 
@@ -7068,8 +7062,6 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 		}
 	}
 
-	subdevice_id = pci_dev->subsystem_device;
-
 	/* found a controller */
 	ha = kzalloc(sizeof (ips_ha_t), GFP_KERNEL);
 	if (ha == NULL) {
@@ -7084,7 +7076,6 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 	ha->active = 1;
 
 	/* Store info in HA structure */
-	ha->irq = irq;
 	ha->io_addr = io_addr;
 	ha->io_len = io_len;
 	ha->mem_addr = mem_addr;
@@ -7092,10 +7083,7 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 	ha->mem_ptr = mem_ptr;
 	ha->ioremap_ptr = ioremap_ptr;
 	ha->host_num = (uint32_t) index;
-	ha->revision_id = pci_dev->revision;
 	ha->slot_num = PCI_SLOT(pci_dev->devfn);
-	ha->device_id = pci_dev->device;
-	ha->subdevice_id = subdevice_id;
 	ha->pcidev = pci_dev;
 
 	/*
@@ -7240,7 +7228,7 @@ ips_init_phase2(int index)
 	}
 
 	/* Install the interrupt handler */
-	if (request_irq(ha->irq, do_ipsintr, IRQF_SHARED, ips_name, ha)) {
+	if (request_irq(ha->pcidev->irq, do_ipsintr, IRQF_SHARED, ips_name, ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to install interrupt handler\n");
 		return ips_abort_init(ha, index);
@@ -7253,14 +7241,14 @@ ips_init_phase2(int index)
 	if (!ips_allocatescbs(ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to allocate a CCB\n");
-		free_irq(ha->irq, ha);
+		free_irq(ha->pcidev->irq, ha);
 		return ips_abort_init(ha, index);
 	}
 
 	if (!ips_hainit(ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to initialize controller\n");
-		free_irq(ha->irq, ha);
+		free_irq(ha->pcidev->irq, ha);
 		return ips_abort_init(ha, index);
 	}
 	/* Free the temporary SCB */
@@ -7270,7 +7258,7 @@ ips_init_phase2(int index)
 	if (!ips_allocatescbs(ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to allocate CCBs\n");
-		free_irq(ha->irq, ha);
+		free_irq(ha->pcidev->irq, ha);
 		return ips_abort_init(ha, index);
 	}
 
diff --git a/drivers/scsi/ips.h b/drivers/scsi/ips.h
index 3bcbd9f..fb4a036 100644
--- a/drivers/scsi/ips.h
+++ b/drivers/scsi/ips.h
@@ -60,14 +60,14 @@
     */
    #define IPS_HA(x)                   ((ips_ha_t *) x->hostdata)
    #define IPS_COMMAND_ID(ha, scb)     (int) (scb - ha->scbs)
-   #define IPS_IS_TROMBONE(ha)         (((ha->device_id == IPS_DEVICEID_COPPERHEAD) && \
-                                         (ha->revision_id >= IPS_REVID_TROMBONE32) && \
-                                         (ha->revision_id <= IPS_REVID_TROMBONE64)) ? 1 : 0)
-   #define IPS_IS_CLARINET(ha)         (((ha->device_id == IPS_DEVICEID_COPPERHEAD) && \
-                                         (ha->revision_id >= IPS_REVID_CLARINETP1) && \
-                                         (ha->revision_id <= IPS_REVID_CLARINETP3)) ? 1 : 0)
-   #define IPS_IS_MORPHEUS(ha)         (ha->device_id == IPS_DEVICEID_MORPHEUS)
-   #define IPS_IS_MARCO(ha)            (ha->device_id == IPS_DEVICEID_MARCO)
+   #define IPS_IS_TROMBONE(ha)         (((ha->pcidev->device == IPS_DEVICEID_COPPERHEAD) && \
+                                         (ha->pcidev->revision >= IPS_REVID_TROMBONE32) && \
+                                         (ha->pcidev->revision <= IPS_REVID_TROMBONE64)) ? 1 : 0)
+   #define IPS_IS_CLARINET(ha)         (((ha->pcidev->device == IPS_DEVICEID_COPPERHEAD) && \
+                                         (ha->pcidev->revision >= IPS_REVID_CLARINETP1) && \
+                                         (ha->pcidev->revision <= IPS_REVID_CLARINETP3)) ? 1 : 0)
+   #define IPS_IS_MORPHEUS(ha)         (ha->pcidev->device == IPS_DEVICEID_MORPHEUS)
+   #define IPS_IS_MARCO(ha)            (ha->pcidev->device == IPS_DEVICEID_MARCO)
    #define IPS_USE_I2O_DELIVER(ha)     ((IPS_IS_MORPHEUS(ha) || \
                                          (IPS_IS_TROMBONE(ha) && \
                                           (ips_force_i2o))) ? 1 : 0)
@@ -1034,7 +1034,6 @@ typedef struct ips_ha {
    uint8_t            ha_id[IPS_MAX_CHANNELS+1];
    uint32_t           dcdb_active[IPS_MAX_CHANNELS];
    uint32_t           io_addr;            /* Base I/O address           */
-   uint8_t            irq;                /* IRQ for adapter            */
    uint8_t            ntargets;           /* Number of targets          */
    uint8_t            nbus;               /* Number of buses            */
    uint8_t            nlun;               /* Number of Luns             */
@@ -1066,10 +1065,7 @@ typedef struct ips_ha {
    int                ioctl_reset;        /* IOCTL Requested Reset Flag */
    uint16_t           reset_count;        /* number of resets           */
    time_t             last_ffdc;          /* last time we sent ffdc info*/
-   uint8_t            revision_id;        /* Revision level             */
-   uint16_t           device_id;          /* PCI device ID              */
    uint8_t            slot_num;           /* PCI Slot Number            */
-   uint16_t           subdevice_id;       /* Subsystem device ID        */
    int                ioctl_len;          /* size of ioctl buffer       */
    dma_addr_t         ioctl_busaddr;      /* dma address of ioctl buffer*/
    uint8_t            bios_version[8];    /* BIOS Revision              */
-- 
1.5.2.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/4] [SCSI] ips: trim trailing whitespace
       [not found] <09821349085390234lkjasdflkjasflkdj24746@havoc.gtf.org>
  2007-10-24 23:48 ` [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members Jeff Garzik
@ 2007-10-24 23:48 ` Jeff Garzik
  2007-10-25 14:33   ` Salyzyn, Mark
  2007-10-24 23:48 ` [PATCH 3/4] [SCSI] ips: PCI API cleanups Jeff Garzik
  2007-10-24 23:48 ` [PATCH 4/4] [SCSI] ips: handle scsi_add_host() failure, and other err cleanups Jeff Garzik
  3 siblings, 1 reply; 16+ messages in thread
From: Jeff Garzik @ 2007-10-24 23:48 UTC (permalink / raw)
  To: LKML, linux-scsi; +Cc: akpm

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/scsi/ips.c |   44 ++++++++++++++++++++++----------------------
 drivers/scsi/ips.h |   12 ++++++------
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index 595a91a..c9f3e1f 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -389,17 +389,17 @@ static struct  pci_device_id  ips_pci_table[] = {
 MODULE_DEVICE_TABLE( pci, ips_pci_table );
 
 static char ips_hot_plug_name[] = "ips";
-   
+
 static int __devinit  ips_insert_device(struct pci_dev *pci_dev, const struct pci_device_id *ent);
 static void __devexit ips_remove_device(struct pci_dev *pci_dev);
-   
+
 static struct pci_driver ips_pci_driver = {
 	.name		= ips_hot_plug_name,
 	.id_table	= ips_pci_table,
 	.probe		= ips_insert_device,
 	.remove		= __devexit_p(ips_remove_device),
 };
-           
+
 
 /*
  * Necessary forward function protoypes
@@ -587,7 +587,7 @@ static void
 ips_setup_funclist(ips_ha_t * ha)
 {
 
-	/*                                
+	/*
 	 * Setup Functions
 	 */
 	if (IPS_IS_MORPHEUS(ha) || IPS_IS_MARCO(ha)) {
@@ -2081,7 +2081,7 @@ ips_host_info(ips_ha_t * ha, char *ptr, off_t offset, int len)
     /* That keeps everything happy for "text" operations on the proc file.                    */
 
 	if (le32_to_cpu(ha->nvram->signature) == IPS_NVRAM_P5_SIG) {
-        if (ha->nvram->bios_low[3] == 0) { 
+        if (ha->nvram->bios_low[3] == 0) {
             copy_info(&info,
 			          "\tBIOS Version                      : %c%c%c%c%c%c%c\n",
 			          ha->nvram->bios_high[0], ha->nvram->bios_high[1],
@@ -2782,8 +2782,8 @@ ips_next(ips_ha_t * ha, int intr)
 
         /* Allow a WRITE BUFFER Command to Have no Data */
         /* This is Used by Tape Flash Utilites          */
-        if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0)) 
-            scb->dcdb.cmd_attribute = 0;                  
+        if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0))
+            scb->dcdb.cmd_attribute = 0;
 
 		if (!(scb->dcdb.cmd_attribute & 0x3))
 			scb->dcdb.transfer_length = 0;
@@ -3404,7 +3404,7 @@ ips_map_status(ips_ha_t * ha, ips_scb_t * scb, ips_stat_t * sp)
 
 				/* Restrict access to physical DASD */
 				if (scb->scsi_cmd->cmnd[0] == INQUIRY) {
-				    ips_scmd_buf_read(scb->scsi_cmd, 
+				    ips_scmd_buf_read(scb->scsi_cmd,
                                       &inquiryData, sizeof (inquiryData));
  				    if ((inquiryData.DeviceType & 0x1f) == TYPE_DISK) {
 				        errcode = DID_TIME_OUT;
@@ -4090,10 +4090,10 @@ ips_chkstatus(ips_ha_t * ha, IPS_STATUS * pstatus)
 			scb->scsi_cmd->result = errcode << 16;
 		} else {	/* bus == 0 */
 			/* restrict access to physical drives */
-			if (scb->scsi_cmd->cmnd[0] == INQUIRY) { 
-			    ips_scmd_buf_read(scb->scsi_cmd, 
+			if (scb->scsi_cmd->cmnd[0] == INQUIRY) {
+			    ips_scmd_buf_read(scb->scsi_cmd,
                                   &inquiryData, sizeof (inquiryData));
-			    if ((inquiryData.DeviceType & 0x1f) == TYPE_DISK) 
+			    if ((inquiryData.DeviceType & 0x1f) == TYPE_DISK)
 			        scb->scsi_cmd->result = DID_TIME_OUT << 16;
 			}
 		}		/* else */
@@ -4661,8 +4661,8 @@ ips_isinit_morpheus(ips_ha_t * ha)
 	uint32_t bits;
 
 	METHOD_TRACE("ips_is_init_morpheus", 1);
-   
-	if (ips_isintr_morpheus(ha)) 
+
+	if (ips_isintr_morpheus(ha))
 	    ips_flush_and_reset(ha);
 
 	post = readl(ha->mem_ptr + IPS_REG_I960_MSG0);
@@ -4686,7 +4686,7 @@ ips_isinit_morpheus(ips_ha_t * ha)
 /*   state ( was trying to INIT and an interrupt was already pending ) ...  */
 /*                                                                          */
 /****************************************************************************/
-static void 
+static void
 ips_flush_and_reset(ips_ha_t *ha)
 {
 	ips_scb_t *scb;
@@ -4718,9 +4718,9 @@ ips_flush_and_reset(ips_ha_t *ha)
 	    if (ret == IPS_SUCCESS) {
 	        time = 60 * IPS_ONE_SEC;	              /* Max Wait time is 60 seconds */
 	        done = 0;
-	            
+
 	        while ((time > 0) && (!done)) {
-	           done = ips_poll_for_flush_complete(ha); 	   
+	           done = ips_poll_for_flush_complete(ha);
 	           /* This may look evil, but it's only done during extremely rare start-up conditions ! */
 	           udelay(1000);
 	           time--;
@@ -4749,17 +4749,17 @@ static int
 ips_poll_for_flush_complete(ips_ha_t * ha)
 {
 	IPS_STATUS cstatus;
-    
+
 	while (TRUE) {
 	    cstatus.value = (*ha->func.statupd) (ha);
 
 	    if (cstatus.value == 0xffffffff)      /* If No Interrupt to process */
 			break;
-            
+
 	    /* Success is when we see the Flush Command ID */
-	    if (cstatus.fields.command_id == IPS_MAX_CMDS ) 
+	    if (cstatus.fields.command_id == IPS_MAX_CMDS )
 	        return 1;
-	 }	
+	 }
 
 	return 0;
 }
@@ -5920,7 +5920,7 @@ ips_read_config(ips_ha_t * ha, int intr)
 
 		return (0);
 	}
-	
+
 	memcpy(ha->conf, ha->ioctl_data, sizeof(*ha->conf));
 	return (1);
 }
@@ -5959,7 +5959,7 @@ ips_readwrite_page5(ips_ha_t * ha, int write, int intr)
 	scb->cmd.nvram.buffer_addr = ha->ioctl_busaddr;
 	if (write)
 		memcpy(ha->ioctl_data, ha->nvram, sizeof(*ha->nvram));
-	
+
 	/* issue the command */
 	if (((ret =
 	      ips_send_wait(ha, scb, ips_cmd_timeout, intr)) == IPS_FAILURE)
diff --git a/drivers/scsi/ips.h b/drivers/scsi/ips.h
index fb4a036..e0657b6 100644
--- a/drivers/scsi/ips.h
+++ b/drivers/scsi/ips.h
@@ -92,7 +92,7 @@
    #ifndef min
       #define min(x,y) ((x) < (y) ? x : y)
    #endif
-   
+
    #ifndef __iomem       /* For clean compiles in earlier kernels without __iomem annotations */
       #define __iomem
    #endif
@@ -171,7 +171,7 @@
    #define IPS_CMD_DOWNLOAD             0x20
    #define IPS_CMD_RW_BIOSFW            0x22
    #define IPS_CMD_GET_VERSION_INFO     0xC6
-   #define IPS_CMD_RESET_CHANNEL        0x1A  
+   #define IPS_CMD_RESET_CHANNEL        0x1A
 
    /*
     * Adapter Equates
@@ -458,7 +458,7 @@ typedef struct {
    uint32_t reserved3;
    uint32_t buffer_addr;
    uint32_t reserved4;
-} IPS_IOCTL_CMD, *PIPS_IOCTL_CMD; 
+} IPS_IOCTL_CMD, *PIPS_IOCTL_CMD;
 
 typedef struct {
    uint8_t  op_code;
@@ -552,7 +552,7 @@ typedef struct {
    uint32_t cccr;
 } IPS_NVRAM_CMD, *PIPS_NVRAM_CMD;
 
-typedef struct 
+typedef struct
 {
     uint8_t  op_code;
     uint8_t  command_id;
@@ -650,7 +650,7 @@ typedef struct {
    uint8_t   device_address;
    uint8_t   cmd_attribute;
    uint8_t   cdb_length;
-   uint8_t   reserved_for_LUN; 	 
+   uint8_t   reserved_for_LUN;
    uint32_t  transfer_length;
    uint32_t  buffer_pointer;
    uint16_t  sg_count;
@@ -790,7 +790,7 @@ typedef struct {
                                              /* SubSystem Parameter[4]      */
 #define  IPS_GET_VERSION_SUPPORT 0x00018000  /* Mask for Versioning Support */
 
-typedef struct 
+typedef struct
 {
    uint32_t  revision;
    uint8_t   bootBlkVersion[32];
-- 
1.5.2.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/4] [SCSI] ips: PCI API cleanups
       [not found] <09821349085390234lkjasdflkjasflkdj24746@havoc.gtf.org>
  2007-10-24 23:48 ` [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members Jeff Garzik
  2007-10-24 23:48 ` [PATCH 2/4] [SCSI] ips: trim trailing whitespace Jeff Garzik
@ 2007-10-24 23:48 ` Jeff Garzik
  2007-10-25  6:54   ` Rolf Eike Beer
  2007-10-25 14:37   ` Salyzyn, Mark
  2007-10-24 23:48 ` [PATCH 4/4] [SCSI] ips: handle scsi_add_host() failure, and other err cleanups Jeff Garzik
  3 siblings, 2 replies; 16+ messages in thread
From: Jeff Garzik @ 2007-10-24 23:48 UTC (permalink / raw)
  To: LKML, linux-scsi; +Cc: akpm

* pass Scsi_Host to ips_remove_device() via pci_set_drvdata(),
  allowing us to eliminate the ips_ha[] search loop and call
  ips_release() directly.

* call pci_{request,release}_regions() and eliminate individual
  request/release_[mem_]region() calls

* call pci_disable_device(), paired with pci_enable_device()

* s/0/NULL/ in a few places

* check ioremap() return value

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/scsi/ips.c |   72 ++++++++++++++++++++++-----------------------------
 1 files changed, 31 insertions(+), 41 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index c9f3e1f..fb90b6c 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -702,10 +702,6 @@ ips_release(struct Scsi_Host *sh)
 	/* free extra memory */
 	ips_free(ha);
 
-	/* Free I/O Region */
-	if (ha->io_addr)
-		release_region(ha->io_addr, ha->io_len);
-
 	/* free IRQ */
 	free_irq(ha->pcidev->irq, ha);
 
@@ -4393,8 +4389,6 @@ ips_free(ips_ha_t * ha)
 			ha->mem_ptr = NULL;
 		}
 
-		if (ha->mem_addr)
-			release_mem_region(ha->mem_addr, ha->mem_len);
 		ha->mem_addr = 0;
 
 	}
@@ -6879,20 +6873,14 @@ ips_register_scsi(int index)
 static void __devexit
 ips_remove_device(struct pci_dev *pci_dev)
 {
-	int i;
-	struct Scsi_Host *sh;
-	ips_ha_t *ha;
+	struct Scsi_Host *sh = pci_get_drvdata(pci_dev);
 
-	for (i = 0; i < IPS_MAX_ADAPTERS; i++) {
-		ha = ips_ha[i];
-		if (ha) {
-			if ((pci_dev->bus->number == ha->pcidev->bus->number) &&
-			    (pci_dev->devfn == ha->pcidev->devfn)) {
-				sh = ips_sh[i];
-				ips_release(sh);
-			}
-		}
-	}
+	pci_set_drvdata(pci_dev, NULL);
+
+	ips_release(sh);
+
+	pci_release_regions(pci_dev);
+	pci_disable_device(pci_dev);
 }
 
 /****************************************************************************/
@@ -6946,12 +6934,17 @@ module_exit(ips_module_exit);
 static int __devinit
 ips_insert_device(struct pci_dev *pci_dev, const struct pci_device_id *ent)
 {
-	int uninitialized_var(index);
+	int index = -1;
 	int rc;
 
 	METHOD_TRACE("ips_insert_device", 1);
-	if (pci_enable_device(pci_dev))
-		return -1;
+	rc = pci_enable_device(pci_dev);
+	if (rc)
+		return rc;
+
+	rc = pci_request_regions(pci_dev, "ips");
+	if (rc)
+		goto err_out;
 
 	rc = ips_init_phase1(pci_dev, &index);
 	if (rc == SUCCESS)
@@ -6967,6 +6960,19 @@ ips_insert_device(struct pci_dev *pci_dev, const struct pci_device_id *ent)
 		ips_num_controllers++;
 
 	ips_next_controller = ips_num_controllers;
+
+	if (rc < 0) {
+		rc = -ENODEV;
+		goto err_out_regions;
+	}
+
+	pci_set_drvdata(pci_dev, ips_sh[index]);
+	return 0;
+
+err_out_regions:
+	pci_release_regions(pci_dev);
+err_out:
+	pci_disable_device(pci_dev);
 	return rc;
 }
 
@@ -6999,7 +7005,7 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 	METHOD_TRACE("ips_init_phase1", 1);
 	index = IPS_MAX_ADAPTERS;
 	for (j = 0; j < IPS_MAX_ADAPTERS; j++) {
-		if (ips_ha[j] == 0) {
+		if (ips_ha[j] == NULL) {
 			index = j;
 			break;
 		}
@@ -7036,32 +7042,17 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 		uint32_t base;
 		uint32_t offs;
 
-		if (!request_mem_region(mem_addr, mem_len, "ips")) {
-			IPS_PRINTK(KERN_WARNING, pci_dev,
-				   "Couldn't allocate IO Memory space %x len %d.\n",
-				   mem_addr, mem_len);
-			return -1;
-		}
-
 		base = mem_addr & PAGE_MASK;
 		offs = mem_addr - base;
 		ioremap_ptr = ioremap(base, PAGE_SIZE);
+		if (!ioremap_ptr)
+			return -1;
 		mem_ptr = ioremap_ptr + offs;
 	} else {
 		ioremap_ptr = NULL;
 		mem_ptr = NULL;
 	}
 
-	/* setup I/O mapped area (if applicable) */
-	if (io_addr) {
-		if (!request_region(io_addr, io_len, "ips")) {
-			IPS_PRINTK(KERN_WARNING, pci_dev,
-				   "Couldn't allocate IO space %x len %d.\n",
-				   io_addr, io_len);
-			return -1;
-		}
-	}
-
 	/* found a controller */
 	ha = kzalloc(sizeof (ips_ha_t), GFP_KERNEL);
 	if (ha == NULL) {
@@ -7070,7 +7061,6 @@ ips_init_phase1(struct pci_dev *pci_dev, int *indexPtr)
 		return -1;
 	}
 
-
 	ips_sh[index] = NULL;
 	ips_ha[index] = ha;
 	ha->active = 1;
-- 
1.5.2.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/4] [SCSI] ips: handle scsi_add_host() failure, and other err cleanups
       [not found] <09821349085390234lkjasdflkjasflkdj24746@havoc.gtf.org>
                   ` (2 preceding siblings ...)
  2007-10-24 23:48 ` [PATCH 3/4] [SCSI] ips: PCI API cleanups Jeff Garzik
@ 2007-10-24 23:48 ` Jeff Garzik
  2007-10-25 14:39   ` Salyzyn, Mark
  3 siblings, 1 reply; 16+ messages in thread
From: Jeff Garzik @ 2007-10-24 23:48 UTC (permalink / raw)
  To: LKML, linux-scsi; +Cc: akpm

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
 drivers/scsi/ips.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index fb90b6c..b8e2f5a 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -6836,13 +6836,10 @@ ips_register_scsi(int index)
 	if (request_irq(ha->pcidev->irq, do_ipsintr, IRQF_SHARED, ips_name, ha)) {
 		IPS_PRINTK(KERN_WARNING, ha->pcidev,
 			   "Unable to install interrupt handler\n");
-		scsi_host_put(sh);
-		return -1;
+		goto err_out_sh;
 	}
 
 	kfree(oldha);
-	ips_sh[index] = sh;
-	ips_ha[index] = ha;
 
 	/* Store away needed values for later use */
 	sh->unique_id = (ha->io_addr) ? ha->io_addr : ha->mem_addr;
@@ -6858,10 +6855,21 @@ ips_register_scsi(int index)
 	sh->max_channel = ha->nbus - 1;
 	sh->can_queue = ha->max_cmds - 1;
 
-	scsi_add_host(sh, NULL);
+	if (scsi_add_host(sh, &ha->pcidev->dev))
+		goto err_out;
+
+	ips_sh[index] = sh;
+	ips_ha[index] = ha;
+
 	scsi_scan_host(sh);
 
 	return 0;
+
+err_out:
+	free_irq(ha->pcidev->irq, ha);
+err_out_sh:
+	scsi_host_put(sh);
+	return -1;
 }
 
 /*---------------------------------------------------------------------------*/
-- 
1.5.2.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
  2007-10-24 23:48 ` [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members Jeff Garzik
@ 2007-10-25  4:27   ` Andrew Morton
  2007-10-25  5:09     ` Jeff Garzik
  2007-10-25 14:32   ` Salyzyn, Mark
  1 sibling, 1 reply; 16+ messages in thread
From: Andrew Morton @ 2007-10-25  4:27 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: LKML, linux-scsi

On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik <jeff@garzik.org> wrote:

>  drivers/scsi/ips.c |  178 ++++++++++++++++++++++++----------------------------

this driver seems a bit of a basket case :(


What's going on here?

		scb->dcdb.cmd_attribute =
		    ips_command_direction[scb->scsi_cmd->cmnd[0]];

        /* Allow a WRITE BUFFER Command to Have no Data */
        /* This is Used by Tape Flash Utilites          */
        if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0)) 
            scb->dcdb.cmd_attribute = 0;                  

		if (!(scb->dcdb.cmd_attribute & 0x3))
			scb->dcdb.transfer_length = 0;

		if (scb->data_len >= IPS_MAX_XFER) {

I hope that's just busted indentation and not a missing {} block.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
  2007-10-25  4:27   ` Andrew Morton
@ 2007-10-25  5:09     ` Jeff Garzik
  2007-10-25 15:04       ` Boaz Harrosh
  0 siblings, 1 reply; 16+ messages in thread
From: Jeff Garzik @ 2007-10-25  5:09 UTC (permalink / raw)
  To: Andrew Morton; +Cc: LKML, linux-scsi

Andrew Morton wrote:
> On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik <jeff@garzik.org> wrote:
> 
>>  drivers/scsi/ips.c |  178 ++++++++++++++++++++++++----------------------------
> 
> this driver seems a bit of a basket case :(
> 
> 
> What's going on here?
> 
> 		scb->dcdb.cmd_attribute =
> 		    ips_command_direction[scb->scsi_cmd->cmnd[0]];
> 
>         /* Allow a WRITE BUFFER Command to Have no Data */
>         /* This is Used by Tape Flash Utilites          */
>         if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0)) 
>             scb->dcdb.cmd_attribute = 0;                  
> 
> 		if (!(scb->dcdb.cmd_attribute & 0x3))
> 			scb->dcdb.transfer_length = 0;
> 
> 		if (scb->data_len >= IPS_MAX_XFER) {
> 
> I hope that's just busted indentation and not a missing {} block.

The driver is one of the grotty drivers people are afraid to touch, 
therefore it bitrots even faster, in a vicious cycle.  You don't have to 
look hard at all to find checkpatch pukeage, very real bugs, and 
Pythonesque silliness.

Not having hardware I went only for changes that are provable and 
obvious, really...

	Jeff


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/4] [SCSI] ips: PCI API cleanups
  2007-10-24 23:48 ` [PATCH 3/4] [SCSI] ips: PCI API cleanups Jeff Garzik
@ 2007-10-25  6:54   ` Rolf Eike Beer
  2007-10-25 14:37   ` Salyzyn, Mark
  1 sibling, 0 replies; 16+ messages in thread
From: Rolf Eike Beer @ 2007-10-25  6:54 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: LKML, linux-scsi, akpm

[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]

Jeff Garzik wrote:
> * pass Scsi_Host to ips_remove_device() via pci_set_drvdata(),
>   allowing us to eliminate the ips_ha[] search loop and call
>   ips_release() directly.
>
> * call pci_{request,release}_regions() and eliminate individual
>   request/release_[mem_]region() calls
>
> * call pci_disable_device(), paired with pci_enable_device()
>
> * s/0/NULL/ in a few places
>
> * check ioremap() return value

> @@ -7036,32 +7042,17 @@ ips_init_phase1(struct pci_dev *pci_dev, int
> *indexPtr) uint32_t base;
>  		uint32_t offs;
>
> -		if (!request_mem_region(mem_addr, mem_len, "ips")) {
> -			IPS_PRINTK(KERN_WARNING, pci_dev,
> -				   "Couldn't allocate IO Memory space %x len %d.\n",
> -				   mem_addr, mem_len);
> -			return -1;
> -		}
> -
>  		base = mem_addr & PAGE_MASK;
>  		offs = mem_addr - base;
>  		ioremap_ptr = ioremap(base, PAGE_SIZE);

This looks odd. What are we actually doing here?

It seems that we want to map that PCI BAR. Since we're playing with PAGE_MASK 
I assume the BAR always has a length <PAGE_SIZE, else we would get page 
aligned memory anyway. If that's true something like

mem_ptr = pci_iomap(pci_dev, bar, 0)

should do the trick. Later we would do

pci_iounmap(pci_dev, mem_ptr)

This whole ioremap_ptr stuff can go away at all. Am I right? Then I'll cook up 
a patch soon.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
  2007-10-24 23:48 ` [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members Jeff Garzik
  2007-10-25  4:27   ` Andrew Morton
@ 2007-10-25 14:32   ` Salyzyn, Mark
  2007-10-25 22:42     ` Jeff Garzik
  1 sibling, 1 reply; 16+ messages in thread
From: Salyzyn, Mark @ 2007-10-25 14:32 UTC (permalink / raw)
  To: Jeff Garzik, LKML, linux-scsi; +Cc: akpm

ACK. Inspected; Mechanical, precise and no introduction of bugs.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:48 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 1/4] [SCSI] ips: remove ips_ha members that 
> duplicate struct pci_dev members
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |  178 
> ++++++++++++++++++++++++----------------------------
>  drivers/scsi/ips.h |   20 +++----
>  2 files changed, 91 insertions(+), 107 deletions(-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH 2/4] [SCSI] ips: trim trailing whitespace
  2007-10-24 23:48 ` [PATCH 2/4] [SCSI] ips: trim trailing whitespace Jeff Garzik
@ 2007-10-25 14:33   ` Salyzyn, Mark
  0 siblings, 0 replies; 16+ messages in thread
From: Salyzyn, Mark @ 2007-10-25 14:33 UTC (permalink / raw)
  To: Jeff Garzik, LKML, linux-scsi; +Cc: akpm

ACK. Inspected; trivial, clean and no sign of any code changes.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:48 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 2/4] [SCSI] ips: trim trailing whitespace
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |   44 
> ++++++++++++++++++++++----------------------
>  drivers/scsi/ips.h |   12 ++++++------
>  2 files changed, 28 insertions(+), 28 deletions(-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH 3/4] [SCSI] ips: PCI API cleanups
  2007-10-24 23:48 ` [PATCH 3/4] [SCSI] ips: PCI API cleanups Jeff Garzik
  2007-10-25  6:54   ` Rolf Eike Beer
@ 2007-10-25 14:37   ` Salyzyn, Mark
  1 sibling, 0 replies; 16+ messages in thread
From: Salyzyn, Mark @ 2007-10-25 14:37 UTC (permalink / raw)
  To: Jeff Garzik, LKML, linux-scsi; +Cc: akpm

ACK. Inspected only. Looks ok.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:49 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 3/4] [SCSI] ips: PCI API cleanups
> 
> * pass Scsi_Host to ips_remove_device() via pci_set_drvdata(),
>   allowing us to eliminate the ips_ha[] search loop and call
>   ips_release() directly.
> 
> * call pci_{request,release}_regions() and eliminate individual
>   request/release_[mem_]region() calls
> 
> * call pci_disable_device(), paired with pci_enable_device()
> 
> * s/0/NULL/ in a few places
> 
> * check ioremap() return value
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |   72 
> ++++++++++++++++++++++-----------------------------
>  1 files changed, 31 insertions(+), 41 deletions(-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH 4/4] [SCSI] ips: handle scsi_add_host() failure, and other err cleanups
  2007-10-24 23:48 ` [PATCH 4/4] [SCSI] ips: handle scsi_add_host() failure, and other err cleanups Jeff Garzik
@ 2007-10-25 14:39   ` Salyzyn, Mark
  0 siblings, 0 replies; 16+ messages in thread
From: Salyzyn, Mark @ 2007-10-25 14:39 UTC (permalink / raw)
  To: Jeff Garzik, LKML, linux-scsi; +Cc: akpm

ACK. Inspected. cleanup with zero risk.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
> Sent: Wednesday, October 24, 2007 7:49 PM
> To: LKML; linux-scsi@vger.kernel.org
> Cc: akpm@linux-foundation.org
> Subject: [PATCH 4/4] [SCSI] ips: handle scsi_add_host() 
> failure, and other err cleanups
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> ---
>  drivers/scsi/ips.c |   18 +++++++++++++-----
>  1 files changed, 13 insertions(+), 5 deletions(-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
  2007-10-25  5:09     ` Jeff Garzik
@ 2007-10-25 15:04       ` Boaz Harrosh
  2007-10-25 15:28         ` Matthew Wilcox
  2007-10-25 22:42         ` Jeff Garzik
  0 siblings, 2 replies; 16+ messages in thread
From: Boaz Harrosh @ 2007-10-25 15:04 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: Andrew Morton, linux-scsi

On Thu, Oct 25 2007 at 7:09 +0200, Jeff Garzik <jeff@garzik.org> wrote:
> Andrew Morton wrote:
>> On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik <jeff@garzik.org> wrote:
>>
>>>  drivers/scsi/ips.c |  178 ++++++++++++++++++++++++----------------------------
>> this driver seems a bit of a basket case :(
>>
>>
>> What's going on here?
>>
>> 		scb->dcdb.cmd_attribute =
>> 		    ips_command_direction[scb->scsi_cmd->cmnd[0]];
>>
>>         /* Allow a WRITE BUFFER Command to Have no Data */
>>         /* This is Used by Tape Flash Utilites          */
>>         if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0)) 
>>             scb->dcdb.cmd_attribute = 0;                  
>>
>> 		if (!(scb->dcdb.cmd_attribute & 0x3))
>> 			scb->dcdb.transfer_length = 0;
>>
>> 		if (scb->data_len >= IPS_MAX_XFER) {
>>
>> I hope that's just busted indentation and not a missing {} block.
> 
> The driver is one of the grotty drivers people are afraid to touch, 
> therefore it bitrots even faster, in a vicious cycle.  You don't have to 
> look hard at all to find checkpatch pukeage, very real bugs, and 
> Pythonesque silliness.
> 
> Not having hardware I went only for changes that are provable and 
> obvious, really...
> 
> 	Jeff
> 

>From the experience with gdth I would say that a first patch of any
serious cleanup, should be the whitespace and formating cleanup.
And now that we can all read what it says, start changing.

In the gdth case I know of 3 bugs that happen just because of that mess.
And I promise to send that patch for gdth soooon.

I found that lint, even with the command line options recommended by 
kernel, is to aggressive, and leaves lots of work to be fixed by hand.
(e.g it will touch the comments)

I found that a small util called astyle with the right settings for the 
tab==8/use-tabs will give you a clean tab indent, but will not touch the
longer than 80, broken out lines, which keeps things better formated.

Morton checkpatch is very good in whining about bad files, but did anyone
attempt a script for linux-style Indentation, formating, and whitespace cleanup,
that give you something like 95% success. As it is lint gives you 50-70%
and astyle 60-80%

Boaz

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
  2007-10-25 15:04       ` Boaz Harrosh
@ 2007-10-25 15:28         ` Matthew Wilcox
  2007-10-25 16:06           ` Boaz Harrosh
  2007-10-25 22:42         ` Jeff Garzik
  1 sibling, 1 reply; 16+ messages in thread
From: Matthew Wilcox @ 2007-10-25 15:28 UTC (permalink / raw)
  To: Boaz Harrosh; +Cc: Jeff Garzik, Andrew Morton, linux-scsi

On Thu, Oct 25, 2007 at 05:04:38PM +0200, Boaz Harrosh wrote:
> I found that lint, even with the command line options recommended by 

Do you mean Lindent / indent?

> kernel, is to aggressive, and leaves lots of work to be fixed by hand.
> (e.g it will touch the comments)

It's not perfect, but code beautification is an art, not a science ;-)

A lot of ugly code can't be made beautiful by a simple parser like
indent because what it really needs is refactoring.  But you can't
refactor until you've made it at least partially readable, so Lindent is
the first step.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
  2007-10-25 15:28         ` Matthew Wilcox
@ 2007-10-25 16:06           ` Boaz Harrosh
  0 siblings, 0 replies; 16+ messages in thread
From: Boaz Harrosh @ 2007-10-25 16:06 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Jeff Garzik, Andrew Morton, linux-scsi

Matthew Wilcox wrote:
> On Thu, Oct 25, 2007 at 05:04:38PM +0200, Boaz Harrosh wrote:
>> I found that lint, even with the command line options recommended by 
> 
> Do you mean Lindent / indent?
> 
Yes "indent". I found that there are better switches to indent than
what's in Lindent. But both are crap as for instance it does not
understand the linux style multi-line-comments and mess them up,
or it tabafy's broken-out-lines. Which is not what we usually
want. And lots of other stuff. astyle does a much better job
just by being less aggressive.

>> kernel, is to aggressive, and leaves lots of work to be fixed by hand.
>> (e.g it will touch the comments)
> 
> It's not perfect, but code beautification is an art, not a science ;-)
> 
> A lot of ugly code can't be made beautiful by a simple parser like
> indent because what it really needs is refactoring.  But you can't
> refactor until you've made it at least partially readable, so Lindent is
> the first step.
> 

I think I disagree 89% and agree 11%.
- remove trailing spaces
- Indent, 
- Convert "Indents" to tabs
- split long lines to multi lines, indent up to the last indent
  level, than fill with spaces, right align.
These can all be done by a machine, the Linux way.

Than
- Adjust broken out lines, like if and while statements to be more
  readable 
- Remove/add blank lines for readability or after end of locals.
That can be adjusted by a person.

But I think with a given code it can be made 89% acceptable by
a machine and 11% adjusted by a man. That is before I start
a single char of coding.

Boaz
 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
  2007-10-25 15:04       ` Boaz Harrosh
  2007-10-25 15:28         ` Matthew Wilcox
@ 2007-10-25 22:42         ` Jeff Garzik
  1 sibling, 0 replies; 16+ messages in thread
From: Jeff Garzik @ 2007-10-25 22:42 UTC (permalink / raw)
  To: Boaz Harrosh; +Cc: Andrew Morton, linux-scsi

Boaz Harrosh wrote:
> On Thu, Oct 25 2007 at 7:09 +0200, Jeff Garzik <jeff@garzik.org> wrote:
>> Andrew Morton wrote:
>>> On Wed, 24 Oct 2007 19:48:26 -0400 (EDT) Jeff Garzik <jeff@garzik.org> wrote:
>>>
>>>>  drivers/scsi/ips.c |  178 ++++++++++++++++++++++++----------------------------
>>> this driver seems a bit of a basket case :(
>>>
>>>
>>> What's going on here?
>>>
>>> 		scb->dcdb.cmd_attribute =
>>> 		    ips_command_direction[scb->scsi_cmd->cmnd[0]];
>>>
>>>         /* Allow a WRITE BUFFER Command to Have no Data */
>>>         /* This is Used by Tape Flash Utilites          */
>>>         if ((scb->scsi_cmd->cmnd[0] == WRITE_BUFFER) && (scb->data_len == 0)) 
>>>             scb->dcdb.cmd_attribute = 0;                  
>>>
>>> 		if (!(scb->dcdb.cmd_attribute & 0x3))
>>> 			scb->dcdb.transfer_length = 0;
>>>
>>> 		if (scb->data_len >= IPS_MAX_XFER) {
>>>
>>> I hope that's just busted indentation and not a missing {} block.
>> The driver is one of the grotty drivers people are afraid to touch, 
>> therefore it bitrots even faster, in a vicious cycle.  You don't have to 
>> look hard at all to find checkpatch pukeage, very real bugs, and 
>> Pythonesque silliness.
>>
>> Not having hardware I went only for changes that are provable and 
>> obvious, really...
>>
>> 	Jeff
>>
> 
>>From the experience with gdth I would say that a first patch of any
> serious cleanup, should be the whitespace and formating cleanup.
> And now that we can all read what it says, start changing.
> 
> In the gdth case I know of 3 bugs that happen just because of that mess.
> And I promise to send that patch for gdth soooon.
> 
> I found that lint, even with the command line options recommended by 
> kernel, is to aggressive, and leaves lots of work to be fixed by hand.
> (e.g it will touch the comments)

Yeah, I hear you, but don't mistake my kill-warnings work for embarking 
upon a new clean-this-driver project ;-)

I tend to make incremental improvements all over the tree, "rising tide 
lifts all boats" and that sort of thing.

If I remained and worked on cleaning every grotty driver I come across 
while killing warnings or fixing interrupt handlers, I would have no 
free time at all :)

	Jeff




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members
  2007-10-25 14:32   ` Salyzyn, Mark
@ 2007-10-25 22:42     ` Jeff Garzik
  0 siblings, 0 replies; 16+ messages in thread
From: Jeff Garzik @ 2007-10-25 22:42 UTC (permalink / raw)
  To: Salyzyn, Mark; +Cc: LKML, linux-scsi, akpm

thanks for reviewing these!


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2007-10-25 22:42 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <09821349085390234lkjasdflkjasflkdj24746@havoc.gtf.org>
2007-10-24 23:48 ` [PATCH 1/4] [SCSI] ips: remove ips_ha members that duplicate struct pci_dev members Jeff Garzik
2007-10-25  4:27   ` Andrew Morton
2007-10-25  5:09     ` Jeff Garzik
2007-10-25 15:04       ` Boaz Harrosh
2007-10-25 15:28         ` Matthew Wilcox
2007-10-25 16:06           ` Boaz Harrosh
2007-10-25 22:42         ` Jeff Garzik
2007-10-25 14:32   ` Salyzyn, Mark
2007-10-25 22:42     ` Jeff Garzik
2007-10-24 23:48 ` [PATCH 2/4] [SCSI] ips: trim trailing whitespace Jeff Garzik
2007-10-25 14:33   ` Salyzyn, Mark
2007-10-24 23:48 ` [PATCH 3/4] [SCSI] ips: PCI API cleanups Jeff Garzik
2007-10-25  6:54   ` Rolf Eike Beer
2007-10-25 14:37   ` Salyzyn, Mark
2007-10-24 23:48 ` [PATCH 4/4] [SCSI] ips: handle scsi_add_host() failure, and other err cleanups Jeff Garzik
2007-10-25 14:39   ` Salyzyn, Mark

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox