[PATCH] kill of ->command

[PATCH] kill of ->command

[Christoph Hellwig]

It's unused now.  Also kill off the stupid = NULL initializations
in usb-storage that made this not compile the first time..


diff -Nru a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
--- a/drivers/scsi/scsi.c	Sun Jun  8 20:31:57 2003
+++ b/drivers/scsi/scsi.c	Sun Jun  8 20:31:57 2003
@@ -441,29 +441,18 @@
 		goto out;
 	}
 
-	if (host->can_queue) {
-		SCSI_LOG_MLQUEUE(3, printk("queuecommand : routine at %p\n",
-					   host->hostt->queuecommand));
+	SCSI_LOG_MLQUEUE(3, printk("queuecommand : routine at %p\n",
+				   host->hostt->queuecommand));
 
-		spin_lock_irqsave(host->host_lock, flags);
-		rtn = host->hostt->queuecommand(cmd, scsi_done);
-		spin_unlock_irqrestore(host->host_lock, flags);
-		if (rtn) {
-			scsi_queue_insert(cmd,
+	spin_lock_irqsave(host->host_lock, flags);
+	rtn = host->hostt->queuecommand(cmd, scsi_done);
+	spin_unlock_irqrestore(host->host_lock, flags);
+	if (rtn) {
+		scsi_queue_insert(cmd,
 				(rtn == SCSI_MLQUEUE_DEVICE_BUSY) ?
-					rtn : SCSI_MLQUEUE_HOST_BUSY);
-			SCSI_LOG_MLQUEUE(3,
-			    printk("queuecommand : request rejected\n"));
-		}
-	} else {
-		SCSI_LOG_MLQUEUE(3, printk("command() :  routine at %p\n",
-					host->hostt->command));
-
-		spin_lock_irqsave(host->host_lock, flags);
-		cmd->result = host->hostt->command(cmd);
-		scsi_done(cmd);
-		spin_unlock_irqrestore(host->host_lock, flags);
-		rtn = 0;
+				 rtn : SCSI_MLQUEUE_HOST_BUSY);
+		SCSI_LOG_MLQUEUE(3,
+		    printk("queuecommand : request rejected\n"));
 	}
 
  out:
diff -Nru a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
--- a/drivers/scsi/scsi_error.c	Sun Jun  8 20:31:57 2003
+++ b/drivers/scsi/scsi_error.c	Sun Jun  8 20:31:57 2003
@@ -435,6 +435,7 @@
 static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, int timeout)
 {
 	struct Scsi_Host *host = scmd->device->host;
+	DECLARE_MUTEX_LOCKED(sem);
 	unsigned long flags;
 	int rtn = SUCCESS;
 
@@ -448,71 +449,53 @@
 		scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
 			(scmd->device->lun << 5 & 0xe0);
 
-	if (host->can_queue) {
-		DECLARE_MUTEX_LOCKED(sem);
+	scsi_add_timer(scmd, timeout, scsi_eh_times_out);
 
-		scsi_add_timer(scmd, timeout, scsi_eh_times_out);
+	/*
+	 * set up the semaphore so we wait for the command to complete.
+	 */
+	scmd->device->host->eh_action = &sem;
+	scmd->request->rq_status = RQ_SCSI_BUSY;
 
-		/*
-		 * set up the semaphore so we wait for the command to complete.
-		 */
-		scmd->device->host->eh_action = &sem;
-		scmd->request->rq_status = RQ_SCSI_BUSY;
+	spin_lock_irqsave(scmd->device->host->host_lock, flags);
+	host->hostt->queuecommand(scmd, scsi_eh_done);
+	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 
-		spin_lock_irqsave(scmd->device->host->host_lock, flags);
-		host->hostt->queuecommand(scmd, scsi_eh_done);
-		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+	down(&sem);
 
-		down(&sem);
+	scmd->device->host->eh_action = NULL;
 
-		scmd->device->host->eh_action = NULL;
+	/*
+	 * see if timeout.  if so, tell the host to forget about it.
+	 * in other words, we don't want a callback any more.
+	 */
+	if (scsi_eh_eflags_chk(scmd, SCSI_EH_REC_TIMEOUT)) {
+		scsi_eh_eflags_clr(scmd,  SCSI_EH_REC_TIMEOUT);
+		scmd->owner = SCSI_OWNER_LOWLEVEL;
 
 		/*
-		 * see if timeout.  if so, tell the host to forget about it.
-		 * in other words, we don't want a callback any more.
+		 * as far as the low level driver is
+		 * concerned, this command is still active, so
+		 * we must give the low level driver a chance
+		 * to abort it. (db) 
+		 *
+		 * FIXME(eric) - we are not tracking whether we could
+		 * abort a timed out command or not.  not sure how
+		 * we should treat them differently anyways.
 		 */
-		if (scsi_eh_eflags_chk(scmd, SCSI_EH_REC_TIMEOUT)) {
-			scsi_eh_eflags_clr(scmd,  SCSI_EH_REC_TIMEOUT);
-                        scmd->owner = SCSI_OWNER_LOWLEVEL;
-
-			/*
-			 * as far as the low level driver is
-			 * concerned, this command is still active, so
-			 * we must give the low level driver a chance
-			 * to abort it. (db) 
-			 *
-			 * FIXME(eric) - we are not tracking whether we could
-			 * abort a timed out command or not.  not sure how
-			 * we should treat them differently anyways.
-			 */
-			spin_lock_irqsave(scmd->device->host->host_lock, flags);
-			if (scmd->device->host->hostt->eh_abort_handler)
-				scmd->device->host->hostt->eh_abort_handler(scmd);
-			spin_unlock_irqrestore(scmd->device->host->host_lock,
-					       flags);
+		spin_lock_irqsave(scmd->device->host->host_lock, flags);
+		if (scmd->device->host->hostt->eh_abort_handler)
+			scmd->device->host->hostt->eh_abort_handler(scmd);
+		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 			
-			scmd->request->rq_status = RQ_SCSI_DONE;
-			scmd->owner = SCSI_OWNER_ERROR_HANDLER;
+		scmd->request->rq_status = RQ_SCSI_DONE;
+		scmd->owner = SCSI_OWNER_ERROR_HANDLER;
 			
-			rtn = FAILED;
-		}
-		SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd: %p, rtn:%x\n",
-						  __FUNCTION__, scmd, rtn));
-	} else {
-		int temp;
-
-		/*
-		 * we damn well had better never use this code.  there is no
-		 * timeout protection here, since we would end up waiting in
-		 * the actual low level driver, we don't know how to wake it up.
-		 */
-		spin_lock_irqsave(host->host_lock, flags);
-		temp = host->hostt->command(scmd);
-		spin_unlock_irqrestore(host->host_lock, flags);
-
-		scmd->result = temp;
-		/* fall through to code below to examine status. */
+		rtn = FAILED;
 	}
+
+	SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd: %p, rtn:%x\n",
+					  __FUNCTION__, scmd, rtn));
 
 	/*
 	 * now examine the actual status codes to see whether the command
diff -Nru a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
--- a/drivers/usb/storage/scsiglue.c	Sun Jun  8 20:31:57 2003
+++ b/drivers/usb/storage/scsiglue.c	Sun Jun  8 20:31:57 2003
@@ -365,24 +365,15 @@
 	.name =				"usb-storage",
 	.proc_name =			"usb-storage",
 	.proc_info =			usb_storage_proc_info,
-	.proc_dir =			NULL,
 	.info =				usb_storage_info,
-	.ioctl =			NULL,
-
-	/* old-style detect and release */
-	.detect =			NULL,
-	.release =			NULL,
 
 	/* command interface -- queued only */
-	.command =			NULL,
 	.queuecommand =			usb_storage_queuecommand,
 
 	/* error and abort handlers */
 	.eh_abort_handler =		usb_storage_command_abort,
 	.eh_device_reset_handler =	usb_storage_device_reset,
 	.eh_bus_reset_handler =		usb_storage_bus_reset,
-	.eh_host_reset_handler =	NULL,
-	.eh_strategy_handler =		NULL,
 
 	/* queue commands only, only one command per LUN */
 	.can_queue =			1,
@@ -391,21 +382,9 @@
 	/* unknown initiator id */
 	.this_id =			-1,
 
-	/* no limit on commands */
-	.max_sectors =			0,
-	
-	/* pre- and post- device scan functions */
-	.slave_alloc =			NULL,
-	.slave_configure =		NULL,
-	.slave_destroy =		NULL,
-
 	/* lots of sg segments can be handled */
 	.sg_tablesize =			SG_ALL,
 
-	/* use 32-bit address space for DMA */
-	.unchecked_isa_dma =		FALSE,
-	.highmem_io =			FALSE,
-
 	/* merge commands... this seems to help performance, but
 	 * periodically someone should test to see which setting is more
 	 * optimal.
@@ -414,9 +393,6 @@
 
 	/* emulated HBA */
 	.emulated =			TRUE,
-
-	/* sorry, no BIOS to help us */
-	.bios_param =			NULL,
 
 	/* module management */
 	.module =			THIS_MODULE
diff -Nru a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h	Sun Jun  8 20:31:57 2003
+++ b/include/scsi/scsi_host.h	Sun Jun  8 20:31:57 2003
@@ -95,24 +95,10 @@
 	 * I/O pressure in the system if there are no other outstanding
 	 * commands.
 	 *
-	 * STATUS: REQUIRED	(for exceptions see below)
+	 * STATUS: REQUIRED
 	 */
 	int (* queuecommand)(struct scsi_cmnd *,
 			     void (*done)(struct scsi_cmnd *));
-
-	/*
-	 * This is used instead of queuecommand if can_queue == 0;
-	 *
-	 * The return value has the following meaning:
-	 * Byte What:
-	 * 0    SCSI status code
-	 * 1    SCSI 1 byte message
-	 * 2    host error return.
-	 * 3    mid level error return
-	 *
-	 * Status: OBSOLETE
-	 */
-	int (* command)(struct scsi_cmnd *);
 
 	/*
 	 * This is an error handling strategy routine.  You don't need to

Re: [PATCH] kill of ->command

[Patrick Mansfield]

On Mon, Jun 09, 2003 at 06:20:56PM +0200, Christoph Hellwig wrote:
> It's unused now.  Also kill off the stupid = NULL initializations
> in usb-storage that made this not compile the first time..

It looks like drivers/scsi/NCR53c406a.c and drivers/scsi/arm/arxescsi.c
can still set can_queue = 0. Correct?

Those two ought to be modified, and anyone setting can_queue = 0 disabled
in hosts.c (scsi_add_host?).

-- Patrick Mansfield

Re: [PATCH] kill of ->command

[Christoph Hellwig]

On Mon, Jun 09, 2003 at 09:41:30AM -0700, Patrick Mansfield wrote:
> On Mon, Jun 09, 2003 at 06:20:56PM +0200, Christoph Hellwig wrote:
> > It's unused now.  Also kill off the stupid = NULL initializations
> > in usb-storage that made this not compile the first time..
> 
> It looks like drivers/scsi/NCR53c406a.c

eek, this one would still need ->command for some devices it appears.
Looks like I should crap this patch or just disable support for that
hw until soneone steps up to fix it.

> and drivers/scsi/arm/arxescsi.c
> can still set can_queue = 0. Correct?

Russell, and idea why this one sets can_queue = 0 but implements
->queuecommand?

Re: [PATCH] kill of ->command

[Patrick Mansfield]

On Mon, Jun 09, 2003 at 06:51:57PM +0200, Christoph Hellwig wrote:

> eek, this one would still need ->command for some devices it appears.
> Looks like I should crap this patch or just disable support for that
> hw until soneone steps up to fix it.

The patch is a good idea, we ought to keep it.

We could move the ->command code (check for can_queue == 0) into the
drivers queuecommand function, and let them call their own "command"
function.

-- Patrick Mansfield

Re: [PATCH] kill of ->command

[Christoph Hellwig]

On Mon, Jun 09, 2003 at 11:04:13AM -0700, Patrick Mansfield wrote:
> The patch is a good idea, we ought to keep it.
> 
> We could move the ->command code (check for can_queue == 0) into the
> drivers queuecommand function, and let them call their own "command"
> function.

Well, the scsi midlayer handling for the two variants is a bit
different.  But I think just disabling support for the can_queue == 0
in NCR53c406a.c should be fine as it's a parameter whether to use
irqs (can_queue == 1) or not.

I'd just love to hear a comment from rmk on arxescsi.c.

Re: [PATCH] kill of ->command

[Russell King]

On Mon, Jun 09, 2003 at 06:51:57PM +0200, Christoph Hellwig wrote:
> > and drivers/scsi/arm/arxescsi.c
> > can still set can_queue = 0. Correct?
> 
> Russell, and idea why this one sets can_queue = 0 but implements
> ->queuecommand?

I would guess its because the author just did a cut and paste followed
by minimum editing to make it work.

I'd need to look at the code, but I'd imagine we could probably do
everything we need to inside ->queuecommand that's currently being
done by ->command.  My only concern would be calling the done
function from within queuecommand.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] kill of ->command

[James Bottomley]

On Mon, 2003-06-09 at 13:15, Russell King wrote:
> I'd need to look at the code, but I'd imagine we could probably do
> everything we need to inside ->queuecommand that's currently being
> done by ->command.  My only concern would be calling the done
> function from within queuecommand.

Calling done from queuecommand() is perfectly legal, as long as you
return 0 from queuecommand.

James

Re: [PATCH] kill of ->command

[Russell King]

On Mon, Jun 09, 2003 at 01:19:20PM -0500, James Bottomley wrote:
> On Mon, 2003-06-09 at 13:15, Russell King wrote:
> > I'd need to look at the code, but I'd imagine we could probably do
> > everything we need to inside ->queuecommand that's currently being
> > done by ->command.  My only concern would be calling the done
> > function from within queuecommand.
> 
> Calling done from queuecommand() is perfectly legal, as long as you
> return 0 from queuecommand.

In which case I don't see why we can't convert this driver to use just
queuecommand().

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] kill of ->command

[Matthew Dharm]

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

On Mon, Jun 09, 2003 at 06:20:56PM +0200, Christoph Hellwig wrote:
> diff -Nru a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> --- a/drivers/usb/storage/scsiglue.c	Sun Jun  8 20:31:57 2003
> +++ b/drivers/usb/storage/scsiglue.c	Sun Jun  8 20:31:57 2003
> @@ -365,24 +365,15 @@
>  	.name =				"usb-storage",
>  	.proc_name =			"usb-storage",
>  	.proc_info =			usb_storage_proc_info,
> -	.proc_dir =			NULL,
>  	.info =				usb_storage_info,
> -	.ioctl =			NULL,
> -
> -	/* old-style detect and release */
> -	.detect =			NULL,
> -	.release =			NULL,
>  
>  	/* command interface -- queued only */
> -	.command =			NULL,
>  	.queuecommand =			usb_storage_queuecommand,
>  
>  	/* error and abort handlers */
>  	.eh_abort_handler =		usb_storage_command_abort,
>  	.eh_device_reset_handler =	usb_storage_device_reset,
>  	.eh_bus_reset_handler =		usb_storage_bus_reset,
> -	.eh_host_reset_handler =	NULL,
> -	.eh_strategy_handler =		NULL,
>  
>  	/* queue commands only, only one command per LUN */
>  	.can_queue =			1,
> @@ -391,21 +382,9 @@
>  	/* unknown initiator id */
>  	.this_id =			-1,
>  
> -	/* no limit on commands */
> -	.max_sectors =			0,
> -	
> -	/* pre- and post- device scan functions */
> -	.slave_alloc =			NULL,
> -	.slave_configure =		NULL,
> -	.slave_destroy =		NULL,
> -
>  	/* lots of sg segments can be handled */
>  	.sg_tablesize =			SG_ALL,
>  
> -	/* use 32-bit address space for DMA */
> -	.unchecked_isa_dma =		FALSE,
> -	.highmem_io =			FALSE,
> -
>  	/* merge commands... this seems to help performance, but
>  	 * periodically someone should test to see which setting is more
>  	 * optimal.
> @@ -414,9 +393,6 @@
>  
>  	/* emulated HBA */
>  	.emulated =			TRUE,
> -
> -	/* sorry, no BIOS to help us */
> -	.bios_param =			NULL,
>  
>  	/* module management */
>  	.module =			THIS_MODULE

Could you please not kill all these fields?  If you want to remove command,
go ahead, but leave the others.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I'm a pink gumdrop! How can anything be worse?!!
					-- Erwin
User Friendly, 10/4/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] kill of ->command

[Christoph Hellwig]

On Mon, Jun 09, 2003 at 11:32:47AM -0700, Matthew Dharm wrote:
> Could you please not kill all these fields?  If you want to remove command,
> go ahead, but leave the others.

No, this is intentional.  C99 initializers exist for a reason and
once of them is to keep people away from fields they don't care about.

Re: [PATCH] kill of ->command

[Jeff Garzik]

On Mon, Jun 09, 2003 at 11:32:47AM -0700, Matthew Dharm wrote:
> Could you please not kill all these fields?  If you want to remove command,
> go ahead, but leave the others.

What's wrong with the patch?

It's just killing explicit zero-inits...

	Jeff

Re: [PATCH] kill of ->command

[Matthew Dharm]

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

On Mon, Jun 09, 2003 at 02:37:15PM -0400, Jeff Garzik wrote:
> On Mon, Jun 09, 2003 at 11:32:47AM -0700, Matthew Dharm wrote:
> > Could you please not kill all these fields?  If you want to remove command,
> > go ahead, but leave the others.
> 
> What's wrong with the patch?
> 
> It's just killing explicit zero-inits...

I've just been bitten on the ass by lack of specific initialization before,
so I'm overly careful.

BTW, is NULL == (void*)0 everywhere?  I seem to recall places where it
isn't....

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I want my GPFs!!!
					-- Stef
User Friendly, 11/9/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] kill of ->command

[Jeff Garzik]

On Mon, Jun 09, 2003 at 11:45:15AM -0700, Matthew Dharm wrote:
> I've just been bitten on the ass by lack of specific initialization before,
> so I'm overly careful.

Linux encourages lack of specific initialization.  It's an important
feature in several areas, so that is what we prefer.


> BTW, is NULL == (void*)0 everywhere?  I seem to recall places where it
> isn't....

Yes, NULL==0.

	Jeff

RE: [PATCH] kill of ->command

[Tony Battersby]

> BTW, is NULL == (void*)0 everywhere?  I seem to recall places where it
> isn't....

>From the recesses of my memory, NULL is always equivalent to (void*)0, but
that doesn't necessarily mean that a NULL pointer has a bit pattern of all
zeros as stored in the computer's memory.  NULL is replaced with (void*)0 by
the preprocessor, and then the compiler for the target architecture
determines the appropriate bit pattern for (void*)0.  You can think of
(void*)0 as just a name for the null pointer which doesn't determine its
actual value.  On some old architectures, memset to zero does not
automatically set pointers to NULL.  I do not remember the specific
architectures which are affected, but I doubt Linux runs or will ever run on
any of them.

For modern code, it is safe to assume that memset to zero will set pointers
to NULL.

Anthony J. Battersby
Cybernetics