[PATCH] convert 53c700 driver to use change_queue_depth API

[PATCH] convert 53c700 driver to use change_queue_depth API

[James Bottomley]

rather than using an attribute override.

James

===== drivers/scsi/53c700.c 1.58 vs edited =====
--- 1.58/drivers/scsi/53c700.c	2004-10-20 19:41:52 -05:00
+++ edited/drivers/scsi/53c700.c	2004-12-11 17:48:57 -06:00
@@ -176,6 +176,7 @@
 STATIC void NCR_700_chip_reset(struct Scsi_Host *host);
 STATIC int NCR_700_slave_configure(struct scsi_device *SDpnt);
 STATIC void NCR_700_slave_destroy(struct scsi_device *SDpnt);
+static int NCR_700_change_queue_depth(struct scsi_device *SDpnt, int depth);
 
 STATIC struct device_attribute *NCR_700_dev_attrs[];
 
@@ -338,6 +339,7 @@
 	tpnt->use_clustering = ENABLE_CLUSTERING;
 	tpnt->slave_configure = NCR_700_slave_configure;
 	tpnt->slave_destroy = NCR_700_slave_destroy;
+	tpnt->change_queue_depth = NCR_700_change_queue_depth;
 	
 	if(tpnt->name == NULL)
 		tpnt->name = "53c700";
@@ -2102,18 +2104,14 @@
 	/* to do here: deallocate memory */
 }
 
-static ssize_t
-NCR_700_store_queue_depth(struct device *dev, const char *buf, size_t count)
+static int
+NCR_700_change_queue_depth(struct scsi_device *SDp, int depth)
 {
-	int depth;
-
-	struct scsi_device *SDp = to_scsi_device(dev);
-	depth = simple_strtoul(buf, NULL, 0);
 	if(depth > NCR_700_MAX_TAGS)
-		return -EINVAL;
+		depth = NCR_700_MAX_TAGS;
 	scsi_adjust_queue_depth(SDp, MSG_ORDERED_TAG, depth);
 
-	return count;
+	return depth;
 }
 
 static ssize_t
@@ -2124,14 +2122,6 @@
 	return snprintf(buf, 20, "%d\n", NCR_700_get_depth(SDp));
 }
 
-static struct device_attribute NCR_700_queue_depth_attr = {
-	.attr = {
-		.name = 	"queue_depth",
-		.mode =		S_IWUSR,
-	},
-	.store = NCR_700_store_queue_depth,
-};
-
 static struct device_attribute NCR_700_active_tags_attr = {
 	.attr = {
 		.name =		"active_tags",
@@ -2141,7 +2131,6 @@
 };
 
 STATIC struct device_attribute *NCR_700_dev_attrs[] = {
-	&NCR_700_queue_depth_attr,
 	&NCR_700_active_tags_attr,
 	NULL,
 };

Re: [PATCH] convert 53c700 driver to use change_queue_depth API

[Guennadi Liakhovetski]

In the comment to the method declaration:

On Sat, 11 Dec 2004, James Bottomley wrote:

> 	/*
> +	 * fill in this function to allow the queue depth of this host
> +	 * to be changeable (on a per device basis).  returns either
> +	 * the current queue depth setting (may be different from what
> +	 * was passed in) or an error.  An error should only be
> +	 * returned if the requested depth is legal but the driver was
> +	 * unable to set it.  If the requested depth is illegal, the
> +	 * driver should set and return the closest legal queue depth.
> +	 *
> +	 */
> +	int (* change_queue_depth)(struct scsi_device *, int);

And then you implement it for the 53c700 as follows:

> ===== drivers/scsi/53c700.c 1.58 vs edited =====
> --- 1.58/drivers/scsi/53c700.c	2004-10-20 19:41:52 -05:00
> +++ edited/drivers/scsi/53c700.c	2004-12-11 17:48:57 -06:00

...

> @@ -2102,18 +2104,14 @@
>  	/* to do here: deallocate memory */
>  }
>  
> -static ssize_t
> -NCR_700_store_queue_depth(struct device *dev, const char *buf, size_t count)
> +static int
> +NCR_700_change_queue_depth(struct scsi_device *SDp, int depth)
>  {
> -	int depth;
> -
> -	struct scsi_device *SDp = to_scsi_device(dev);
> -	depth = simple_strtoul(buf, NULL, 0);
>  	if(depth > NCR_700_MAX_TAGS)
> -		return -EINVAL;
> +		depth = NCR_700_MAX_TAGS;
>  	scsi_adjust_queue_depth(SDp, MSG_ORDERED_TAG, depth);
>  
> -	return count;
> +	return depth;
>  }
>  
>  static ssize_t

Shouldn't it really be something like

 	if(depth > NCR_700_MAX_TAGS)
-		return -EINVAL;
+		depth = NCR_700_MAX_TAGS;
+	if (depth < 0)
+		depth = 0;
 	scsi_adjust_queue_depth(SDp, MSG_ORDERED_TAG, depth);
 
-	return count;
+	return SDp->queue_depth == depth ? depth : -EFAIL;
 }

And, actually (nitpicking), shouldn't the comment rather be

--- include/scsi/scsi_host.h~	Tue Dec 14 22:14:07 2004
+++ include/scsi/scsi_host.h	Mon Dec 20 16:21:08 2004
@@ -219,11 +219,10 @@
 	 * fill in this function to allow the queue depth of this host
 	 * to be changeable (on a per device basis).  returns either
 	 * the current queue depth setting (may be different from what
-	 * was passed in) or an error.  An error should only be
-	 * returned if the requested depth is legal but the driver was
-	 * unable to set it.  If the requested depth is illegal, the
-	 * driver should set and return the closest legal queue depth.
-	 *
+	 * was passed in) or an error.  If the requested depth is illegal,
+	 * the driver should set and return the closest legal queue depth.
+	 * An error should only be returned if the driver was unable to
+	 * set the new depth.
 	 */
 	int (* change_queue_depth)(struct scsi_device *, int);
 
? And one more (unrelated) question (possibly, known / discussed before): 
simple_strtoul converts "-1" to 0, whereas, glibc strtoul would return 
(unsigned long)-1. It is the intended behaviour, isn't it? Perhaps, worth 
documenting in vsprintf.c...

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] convert 53c700 driver to use change_queue_depth API

[Guennadi Liakhovetski]

On Mon, 20 Dec 2004, Guennadi Liakhovetski wrote:

> Shouldn't it really be something like
> 
>  	if(depth > NCR_700_MAX_TAGS)
> -		return -EINVAL;
> +		depth = NCR_700_MAX_TAGS;
> +	if (depth < 0)
> +		depth = 0;

Ahh, yeah, sure, because of the simple_strtoul incompatibility with glibc 
this check is superfluous, but, the below doubt still remains

>  	scsi_adjust_queue_depth(SDp, MSG_ORDERED_TAG, depth);
>  
> -	return count;
> +	return SDp->queue_depth == depth ? depth : -EFAIL;
>  }
> 


Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] convert 53c700 driver to use change_queue_depth API

[James Bottomley]

On Mon, 2004-12-20 at 17:04 +0100, Guennadi Liakhovetski wrote:
> On Mon, 20 Dec 2004, Guennadi Liakhovetski wrote:
> 
> > Shouldn't it really be something like
> > 
> >  	if(depth > NCR_700_MAX_TAGS)
> > -		return -EINVAL;
> > +		depth = NCR_700_MAX_TAGS;
> > +	if (depth < 0)
> > +		depth = 0;
> 
> Ahh, yeah, sure, because of the simple_strtoul incompatibility with glibc 
> this check is superfluous, but, the below doubt still remains

Actually, depth may never be zero.  There is a check in an updated piece
of code which I'll send out shortly that checks it is never less than
one (in the main routine).  This got folded into an update I did and
then not properly separated again.

I'll send the update under a new subject.

James