[PATCH] kill aac_io_done

[PATCH] kill aac_io_done

[Christoph Hellwig]

there's not need anymore to take the host lock before calling ->done


--- 1.29/drivers/scsi/aacraid/aachba.c	2004-10-01 17:10:08 +02:00
+++ edited/drivers/scsi/aacraid/aachba.c	2004-10-09 14:38:04 +02:00
@@ -335,15 +335,6 @@
 	return status;
 }
 
-static void aac_io_done(struct scsi_cmnd * scsicmd)
-{
-	unsigned long cpu_flags;
-	struct Scsi_Host *host = scsicmd->device->host;
-	spin_lock_irqsave(host->host_lock, cpu_flags);
-	scsicmd->scsi_done(scsicmd);
-	spin_unlock_irqrestore(host->host_lock, cpu_flags);
-}
-
 static void get_container_name_callback(void *context, struct fib * fibptr)
 {
 	struct aac_get_name_resp * get_name_reply;
@@ -375,7 +366,8 @@
 
 	fib_complete(fibptr);
 	fib_free(fibptr);
-	aac_io_done(scsicmd);
+
+	scsicmd->scsi_done(scsicmd);
 }
 
 /**
@@ -735,7 +727,7 @@
 	fib_complete(fibptr);
 	fib_free(fibptr);
 
-	aac_io_done(scsicmd);
+	scsicmd->scsi_done(scsicmd);
 }
 
 static void write_callback(void *context, struct fib * fibptr)
@@ -782,7 +774,7 @@
 
 	fib_complete(fibptr);
 	fib_free(fibptr);
-	aac_io_done(scsicmd);
+	scsicmd->scsi_done(scsicmd);
 }
 
 int aac_read(struct scsi_cmnd * scsicmd, int cid)
@@ -891,7 +883,7 @@
 	 *	For some reason, the Fib didn't queue, return QUEUE_FULL
 	 */
 	scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_TASK_SET_FULL;
-	aac_io_done(scsicmd);
+	scsicmd->scsi_done(scsicmd);
 	fib_complete(cmd_fibcontext);
 	fib_free(cmd_fibcontext);
 	return -1;
@@ -927,7 +919,7 @@
 	 */
 	if (!(cmd_fibcontext = fib_alloc(dev))) {
 		scsicmd->result = DID_ERROR << 16;
-		aac_io_done(scsicmd);
+		scsicmd->scsi_done(scsicmd);
 		return -1;
 	}
 	fib_init(cmd_fibcontext);
@@ -1000,7 +992,7 @@
 	 *	For some reason, the Fib didn't queue, return QUEUE_FULL
 	 */
 	scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_TASK_SET_FULL;
-	aac_io_done(scsicmd);
+	scsicmd->scsi_done(scsicmd);
 
 	fib_complete(cmd_fibcontext);
 	fib_free(cmd_fibcontext);
@@ -1568,7 +1560,7 @@
 
 	fib_complete(fibptr);
 	fib_free(fibptr);
-	aac_io_done(scsicmd);
+	scsicmd->scsi_done(scsicmd);
 }
 
 /**

Re: [PATCH] kill aac_io_done

[Mark Haverkamp]

On Sat, 2004-10-09 at 06:46, Christoph Hellwig wrote:
> there's not need anymore to take the host lock before calling ->done

I have wondered about this.  Was the host lock needed at one time in the
past?

Mark.

> 

-- 
Mark Haverkamp <markh@osdl.org>

RE: [PATCH] kill aac_io_done

[Salyzyn, Mark]

The host lock around scsi_done() was there *before* I started
maintaining this driver. I believe the reasoning could be as simple as
`we had an io_request_lock there, so we should need a host_lock for the
same reasons'. I believe this locking is a legacy of code prior to
2.1.95 though...

Before I can be comfortable removing this in Adaptec's code base
(supporting Linux variants from 2.4.2 to 2.6.8.1), I would need to know
the reasoning for this lock for all the Linux Distributions. I have to
support RH7, for instance, because we have some test groups using this
as the basis for their manufacturing testing tools. Confirmation that it
became unnecessary at some specific version without doubt would be nice.
Do we have a historian that knows this? Also, in Adaptec's code base,
because of debugging needs, the aac_io_done routine is a common point
for instrumentation. The code change places a burden on future debugging
(but I have already accepted that I am maintaining a branch forever for
purposes of debugging, investigation and a seed for the scsi_list
driver).

Sincerely -- Mark Salyzyn

-----Original Message-----
From: Mark Haverkamp [mailto:markh@osdl.org] 
Sent: Monday, October 11, 2004 11:49 AM
To: Christoph Hellwig
Cc: Salyzyn, Mark; jejb@steeleye.com; linux-scsi
Subject: Re: [PATCH] kill aac_io_done

On Sat, 2004-10-09 at 06:46, Christoph Hellwig wrote:
> there's not need anymore to take the host lock before calling ->done

I have wondered about this.  Was the host lock needed at one time in the
past?

Mark.

> 

-- 
Mark Haverkamp <markh@osdl.org>

RE: [PATCH] kill aac_io_done

[James Bottomley]

On Mon, 2004-10-11 at 11:07, Salyzyn, Mark wrote:
> The host lock around scsi_done() was there *before* I started
> maintaining this driver. I believe the reasoning could be as simple as
> `we had an io_request_lock there, so we should need a host_lock for the
> same reasons'. I believe this locking is a legacy of code prior to
> 2.1.95 though...

I think it was a 2.2 requirement; the then "new" SCSI code in 2.4 moved
to completing requests in the bottom half handler which queue was
protected by the scsi_bhqueue_lock in the scsi_done() routine and no
longer needed the io_request lock.

In 2.6 the BH handler became a softirq thanks to Mattew Wilcox.

James