Skip to contentSkip to conversation

add scsi helpers

PATCH In review Mike Christie 2 participants 5 messages Sep 14, 2005 – Sep 15, 2005 SCSI

lore
Share & tools
lore

Mike Christie

author
#0 lore raw

PATCH 2/4 add scsi helpers

add scsi helper scsi_execute_async_iov_req() and
convert some functions to user it.

Also add a __scsi_request struct that is only used internally in scsi_lib.c for our request's end_io_data. Hopefully this will be all that is left of scsi_request.

Signed-off-by Mike Christie <[email protected]>
drivers/scsi/scsi_error.c +9 -38
@@ -1366,23 +1366,6 @@ int scsi_decide_disposition(struct scsi_
 }
 
 /**
- * scsi_eh_lock_done - done function for eh door lock request
- * @scmd:	SCSI command block for the door lock request
- *
- * Notes:
- * 	We completed the asynchronous door lock request, and it has either
- * 	locked the door or failed.  We must free the command structures
- * 	associated with this request.
- **/
-static void scsi_eh_lock_done(struct scsi_cmnd *scmd)
-{
-	struct scsi_request *sreq = scmd->sc_request;
-
-	scsi_release_request(sreq);
-}
-
-
-/**
  * scsi_eh_lock_door - Prevent medium removal for the specified device
  * @sdev:	SCSI device to prevent medium removal
  *

@@ -1404,29 +1387,17 @@ static void scsi_eh_lock_done(struct scs
  **/
 static void scsi_eh_lock_door(struct scsi_device *sdev)
 {
-	struct scsi_request *sreq = scsi_allocate_request(sdev, GFP_KERNEL);
-
-	if (unlikely(!sreq)) {
-		printk(KERN_ERR "%s: request allocate failed,"
-		       "prevent media removal cmd not sent\n", __FUNCTION__);
-		return;
-	}
+	unsigned char cmnd[MAX_COMMAND_SIZE];
 
-	sreq->sr_cmnd[0] = ALLOW_MEDIUM_REMOVAL;
-	sreq->sr_cmnd[1] = 0;
-	sreq->sr_cmnd[2] = 0;
-	sreq->sr_cmnd[3] = 0;
-	sreq->sr_cmnd[4] = SCSI_REMOVAL_PREVENT;
-	sreq->sr_cmnd[5] = 0;
-	sreq->sr_data_direction = DMA_NONE;
-	sreq->sr_bufflen = 0;
-	sreq->sr_buffer = NULL;
-	sreq->sr_allowed = 5;
-	sreq->sr_done = scsi_eh_lock_done;
-	sreq->sr_timeout_per_command = 10 * HZ;
-	sreq->sr_cmd_len = COMMAND_SIZE(sreq->sr_cmnd[0]);
+	cmnd[0] = ALLOW_MEDIUM_REMOVAL;
+	cmnd[1] = 0;
+	cmnd[2] = 0;
+	cmnd[3] = 0;
+	cmnd[4] = SCSI_REMOVAL_PREVENT;
+	cmnd[5] = 0;
 
-	scsi_insert_special_req(sreq, 1);
+	scsi_execute_async_iov_req(sdev, cmnd, DMA_NONE, NULL, 0, 10 * HZ,
+				   5, NULL, NULL);
 }
drivers/scsi/scsi_lib.c +68 -34

121 lines not on this page · raw message

drivers/scsi/scsi_priv.h +0 -1
@@ -41,7 +41,6 @@ extern void scsi_exit_hosts(void);
 extern int scsi_dispatch_cmd(struct scsi_cmnd *cmd);
 extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
 extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
-extern int scsi_insert_special_req(struct scsi_request *sreq, int);
 extern void scsi_init_cmd_from_req(struct scsi_cmnd *cmd,
 		struct scsi_request *sreq);
 extern void __scsi_release_request(struct scsi_request *sreq);
include/scsi/scsi_device.h +5 -0
@@ -264,6 +264,11 @@ extern int scsi_execute(struct scsi_devi
 extern int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 			    int data_direction, void *buffer, unsigned bufflen,
 			    struct scsi_sense_hdr *, int timeout, int retries);
+extern int scsi_execute_async_iov_req(struct scsi_device *sdev,
+				  const unsigned char *cmd, int data_direction,
+				  struct kvec *vec, int vec_count,
+				  int timeout, int retries, void *data,
+				  void (*done)(void *, char *, int, int));
 
 static inline int scsi_device_online(struct scsi_device *sdev)
 {
#1 lore raw

drivers/scsi/scsi_lib.c int scsi_execute_req(struct scsi_device · line 360

Quoted 5 lines
drivers/scsi/scsi_lib.c +5 -0
+struct __scsi_request {
+	void *data;
+	void (*done)(void *data, char *sense, int result, int resid);
+	char sense[SCSI_SENSE_BUFFERSIZE];
+};

I don't think this is a good structure name. Just something like iocontext or similar?

drivers/scsi/scsi_lib.c int scsi_execute_req(struct scsi_device · line 377

Quoted 5 lines
drivers/scsi/scsi_lib.c +5 -0
+int scsi_execute_async_iov_req(struct scsi_device *sdev,
+			       const unsigned char *cmd, int data_direction,
+			       struct kvec *vec, int vec_count, int timeout,
+			       int retries, void *privdata,
+			       void (*done)(void *, char *, int, int))

If you passed an request_queue_t instead of th scsi_device this function would not have any knowledge about scsi internals and could be moved up to the block layer. not sure that's actually a good idea.

If we stick to putting it into the scsi layer the done callback could normalize the sense data, though.

drivers/scsi/scsi_lib.c int scsi_execute_req(struct scsi_device · line 381

Quoted 3 lines
drivers/scsi/scsi_lib.c +3 -0
+	struct request *req;
+	struct __scsi_request *sreq;
+	int write = (data_direction == DMA_TO_DEVICE);

What about just passing in a write parameter directly?

drivers/scsi/scsi_lib.c int scsi_execute_req(struct scsi_device · line 388

Quoted 7 lines
drivers/scsi/scsi_lib.c +7 -0
+
+	sreq = kzalloc(sizeof(*sreq), GFP_ATOMIC);
+	if (!sreq) {
+		return DRIVER_ERROR << 24;
+	}
+
+	req = blk_get_request(sdev->request_queue, write, GFP_ATOMIC);

can you add a gfp_mask argument instead of hardcoding GFP_ATOMIC?

author
#2 lore raw
Quoted 9 lines · Christoph Hellwig

drivers/scsi/scsi_lib.c int scsi_execute_req(struct scsi_device · line 360

Quoted 5 lines
drivers/scsi/scsi_lib.c +5 -0
+struct __scsi_request {
+	void *data;
+	void (*done)(void *data, char *sense, int result, int resid);
+	char sense[SCSI_SENSE_BUFFERSIZE];
+};

I don't think this is a good structure name. Just something like iocontext or similar?

ok.

Quoted 12 lines

drivers/scsi/scsi_lib.c int scsi_execute_req(struct scsi_device · line 377

Quoted 7 lines
drivers/scsi/scsi_lib.c +5 -0
+int scsi_execute_async_iov_req(struct scsi_device *sdev,
+			       const unsigned char *cmd, int data_direction,
+			       struct kvec *vec, int vec_count, int timeout,
+			       int retries, void *privdata,
+			       void (*done)(void *, char *, int, int))

If you passed an request_queue_t instead of th scsi_device this function would not have any knowledge about scsi internals and could be moved up to the block layer. not sure that's actually a good idea.

Did you want to move scsi_execute too?

Quoted 11 lines

If we stick to putting it into the scsi layer the done callback could normalize the sense data, though.

drivers/scsi/scsi_lib.c int scsi_execute_req(struct scsi_device · line 381

Quoted 3 lines
drivers/scsi/scsi_lib.c +3 -0
+	struct request *req;
+	struct __scsi_request *sreq;
+	int write = (data_direction == DMA_TO_DEVICE);

What about just passing in a write parameter directly?

I thought most users would just have the DMA_* value so it would be nicer for them to pass that down. It also then works like scsi_execute_req/scsi_execute. I guess if we move one of those functions to the block layer it would be better to pass in write/read for the block layer versions.

Quoted 13 lines

drivers/scsi/scsi_lib.c int scsi_execute_req(struct scsi_device · line 388

Quoted 9 lines
drivers/scsi/scsi_lib.c +7 -0
+
+	sreq = kzalloc(sizeof(*sreq), GFP_ATOMIC);
+	if (!sreq) {
+		return DRIVER_ERROR << 24;
+	}
+
+	req = blk_get_request(sdev->request_queue, write, GFP_ATOMIC);

can you add a gfp_mask argument instead of hardcoding GFP_ATOMIC?

yeah will do. I think I got lazy :)

author
#3 lore raw
Quoted 10 lines · Mike Christie
Quoted 10 lines
Quoted 7 lines
drivers/scsi/scsi_lib.c +5 -0
+int scsi_execute_async_iov_req(struct scsi_device *sdev,
+                   const unsigned char *cmd, int data_direction,
+                   struct kvec *vec, int vec_count, int timeout,
+                   int retries, void *privdata,
+                   void (*done)(void *, char *, int, int))
Quoted 7 lines
Quoted 3 lines

If you passed an request_queue_t instead of th scsi_device this function would not have any knowledge about scsi internals and could be moved up to the block layer. not sure that's actually a good idea.

Did you want to move scsi_execute too?

Actually after talking with Jens, I think it is better to keep the function in scsi_lib and move my blk_rq_map_iov() function there. The problem is that sg and st have some special needs and as a result do some things like partial setup themselves. It looks like nobody wants the burdon of having these special case fucntions :) but since SCSI's ULDs are the only user and probably will be the only user does it make sense to keep them there?

#4 lore raw
Quoted 7 lines · Mike Christie

Actually after talking with Jens, I think it is better to keep the function in scsi_lib and move my blk_rq_map_iov() function there. The problem is that sg and st have some special needs and as a result do some things like partial setup themselves. It looks like nobody wants the burdon of having these special case fucntions :) but since SCSI's ULDs are the only user and probably will be the only user does it make sense to keep them there?

ok, makes sense