Skip to contentSkip to conversation

convert 53c700 driver to use change_queue_depth API

PATCH In review James Bottomley 2 participants 4 messages Dec 12, 2004 – Dec 22, 2004

Subsystems (3)
SCSI Lasi 53c700 driver for PA-RISC 53C700 and 53C700-66 SCSI driver
lore
Share & tools
lore

James Bottomley

author
#0 lore raw

PATCH 1/1 convert 53c700 driver to use change_queue_depth API

rather than using an attribute override.

James

===== drivers/scsi/53c700.c 1.58 vs edited =====

drivers/scsi/53c700.c +6 -17
@@ -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,
 };
#1 lore raw

In the comment to the method declaration:

Review

Quoted 11 lines · James Bottomley
	/*
(quoted diff) +10 -0
+	 * 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:

Quoted 3 lines

===== drivers/scsi/53c700.c 1.58 vs edited =====

drivers/scsi/53c700.c +0 -0

...

Quoted 13 lines
drivers/scsi/53c700.c +2 -6
@@ -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);

drivers/scsi/53c700.c line 2117

Quoted 10 lines
drivers/scsi/53c700.c +2 -2
 	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

scsi/scsi_host.h renamed from scsi/scsi_host.h~ +4 -5
@@ -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

#2 lore raw

Review

Quoted 7 lines · Guennadi Liakhovetski

Shouldn't it really be something like

(quoted diff) +3 -1
 	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

Review

Quoted 6 lines
(quoted diff) +1 -1
 	scsi_adjust_queue_depth(SDp, MSG_ORDERED_TAG, depth);
 
-	return count;
+	return SDp->queue_depth == depth ? depth : -EFAIL;
 }

Thanks
Guennadi
---
Guennadi Liakhovetski

author
#3 lore raw
Quoted 4 lines · Guennadi Liakhovetski
Quoted 2 lines · Guennadi Liakhovetski

Shouldn't it really be something like

Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +3 -1
 	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