change scsi_cmd_ioctl to take a gendisk instead of a queue

PATCH In review Patrick Mansfield 4 participants 10 messages Feb 1, 2004 – Feb 2, 2004

Subsystems (5)

VFS SCSI Block layer SCSI tape driver +1 more subsystems

lore

Share & tools

lore

Patrick Mansfield

author

2004-02-01 20:48 UTC #0 lore raw

PATCH 1/1 change scsi_cmd_ioctl to take a gendisk instead of a queue

Jens or others, any comments?

This patch against a recent bk 2.6 changes scsi_cmd_ioctl to take a gendisk as an argument instead of a request_queue_t. This allows scsi char devices to use the scsi_cmd_ioctl interface.

In turn, change bio_map_user to also pass a request_queue_t, and add a __bio_add_page helper that takes a request_queue_t.

Tested ide cd burning with no problems.

If the scsi upper level scsi_cmd_ioctl usage were consolidated in scsi_prep_fn, we could pass a request_queue_t instead of a gendisk to scsi_cmd_ioctl.

st.c patch follows.

drivers/block/scsi_ioctl.c +14 ~~-13~~

diff -uprN -X /home/patman/dontdiff bk-2.6.2-rc3/drivers/block/scsi_ioctl.c char_sg_io-bk-2.6.2-rc3/drivers/block/scsi_ioctl.c
@@ -46,14 +46,14 @@ const unsigned char scsi_command_size[8]
 #define SCSI_SENSE_BUFFERSIZE 64
 #endif
 
-static int blk_do_rq(request_queue_t *q, struct block_device *bdev, 
+static int blk_do_rq(request_queue_t *q, struct gendisk *bd_disk,
 		     struct request *rq)
 {
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	DECLARE_COMPLETION(wait);
 	int err = 0;
 
-	rq->rq_disk = bdev->bd_disk;
+	rq->rq_disk = bd_disk;
 
 	/*
 	 * we need an extra reference to the request, so we can look at

@@ -142,7 +142,7 @@ static int sg_emulated_host(request_queu
 	return put_user(1, p);
 }
 
-static int sg_io(request_queue_t *q, struct block_device *bdev,
+static int sg_io(request_queue_t *q, struct gendisk *bd_disk,
 		 struct sg_io_hdr *hdr)
 {
 	unsigned long start_time;

@@ -190,7 +190,7 @@ static int sg_io(request_queue_t *q, str
 		 * first try to map it into a bio. reading from device will
 		 * be a write to vm.
 		 */
-		bio = bio_map_user(bdev, (unsigned long) hdr->dxferp,
+		bio = bio_map_user(q, NULL, (unsigned long) hdr->dxferp,
 				   hdr->dxfer_len, reading);
 
 		/*

@@ -246,7 +246,7 @@ static int sg_io(request_queue_t *q, str
 	 * (if he doesn't check that is his problem).
 	 * N.B. a non-zero SCSI status is _not_ necessarily an error.
 	 */
-	blk_do_rq(q, bdev, rq);
+	blk_do_rq(q, bd_disk, rq);
 
 	if (bio)
 		bio_unmap_user(bio, reading);

@@ -296,7 +296,7 @@ out_buffer:
 #define READ_DEFECT_DATA_TIMEOUT	(60 * HZ )
 #define OMAX_SB_LEN 16          /* For backward compatibility */
 
-static int sg_scsi_ioctl(request_queue_t *q, struct block_device *bdev,
+static int sg_scsi_ioctl(request_queue_t *q, struct gendisk *bd_disk,
 			 Scsi_Ioctl_Command *sic)
 {
 	struct request *rq;

@@ -369,7 +369,7 @@ static int sg_scsi_ioctl(request_queue_t
 	rq->data_len = bytes;
 	rq->flags |= REQ_BLOCK_PC;
 
-	blk_do_rq(q, bdev, rq);
+	blk_do_rq(q, bd_disk, rq);
 	err = rq->errors & 0xff;	/* only 8 bit SCSI status */
 	if (err) {
 		if (rq->sense_len && rq->sense) {

@@ -389,13 +389,13 @@ error:
 	return err;
 }
 
-int scsi_cmd_ioctl(struct block_device *bdev, unsigned int cmd, unsigned long arg)
+int scsi_cmd_ioctl(struct gendisk *bd_disk, unsigned int cmd, unsigned long arg)
 {
 	request_queue_t *q;
 	struct request *rq;
 	int close = 0, err;
 
-	q = bdev_get_queue(bdev);
+	q = bd_disk->queue;
 	if (!q)
 		return -ENXIO;

@@ -446,7 +446,7 @@ int scsi_cmd_ioctl(struct block_device *
 
 			old_cdb = hdr.cmdp;
 			hdr.cmdp = cdb;
-			err = sg_io(q, bdev, &hdr);
+			err = sg_io(q, bd_disk, &hdr);
 
 			hdr.cmdp = old_cdb;
 			if (copy_to_user((struct sg_io_hdr *) arg, &hdr, sizeof(hdr)))

@@ -493,7 +493,7 @@ int scsi_cmd_ioctl(struct block_device *
 			hdr.timeout = cgc.timeout;
 			hdr.cmdp = cgc.cmd;
 			hdr.cmd_len = sizeof(cgc.cmd);
-			err = sg_io(q, bdev, &hdr);
+			err = sg_io(q, bd_disk, &hdr);
 
 			if (hdr.status)
 				err = -EIO;

@@ -514,7 +514,8 @@ int scsi_cmd_ioctl(struct block_device *
 			if (!arg)
 				break;
 
-			err = sg_scsi_ioctl(q, bdev, (Scsi_Ioctl_Command *)arg);
+			err = sg_scsi_ioctl(q, bd_disk,
+					    (Scsi_Ioctl_Command *)arg);
 			break;
 		case CDROMCLOSETRAY:
 			close = 1;

@@ -528,7 +529,7 @@ int scsi_cmd_ioctl(struct block_device *
 			rq->cmd[0] = GPCMD_START_STOP_UNIT;
 			rq->cmd[4] = 0x02 + (close != 0);
 			rq->cmd_len = 6;
-			err = blk_do_rq(q, bdev, rq);
+			err = blk_do_rq(q, bd_disk, rq);
 			blk_put_request(rq);
 			break;
 		default:

drivers/cdrom/cdrom.c +1 -1

diff -uprN -X /home/patman/dontdiff bk-2.6.2-rc3/drivers/cdrom/cdrom.c char_sg_io-bk-2.6.2-rc3/drivers/cdrom/cdrom.c
@@ -1773,7 +1773,7 @@ int cdrom_ioctl(struct cdrom_device_info
 	int ret;
 
 	/* Try the generic SCSI command ioctl's first.. */
-	ret = scsi_cmd_ioctl(ip->i_bdev, cmd, arg);
+	ret = scsi_cmd_ioctl(ip->i_bdev->bd_disk, cmd, arg);
 	if (ret != -ENOTTY)
 		return ret;

drivers/ide/ide.c +1 -1

diff -uprN -X /home/patman/dontdiff bk-2.6.2-rc3/drivers/ide/ide.c char_sg_io-bk-2.6.2-rc3/drivers/ide/ide.c
@@ -1707,7 +1707,7 @@ int generic_ide_ioctl(struct block_devic
 
 		case CDROMEJECT:
 		case CDROMCLOSETRAY:
-			return scsi_cmd_ioctl(bdev, cmd, arg);
+			return scsi_cmd_ioctl(bdev->bd_disk, cmd, arg);
 
 		case HDIO_GET_BUSSTATE:
 			if (!capable(CAP_SYS_ADMIN))

drivers/scsi/sd.c +1 -1

diff -uprN -X /home/patman/dontdiff bk-2.6.2-rc3/drivers/scsi/sd.c char_sg_io-bk-2.6.2-rc3/drivers/scsi/sd.c
@@ -554,7 +554,7 @@ static int sd_ioctl(struct inode * inode
 		case SCSI_IOCTL_GET_BUS_NUMBER:
 			return scsi_ioctl(sdp, cmd, (void *)arg);
 		default:
-			error = scsi_cmd_ioctl(bdev, cmd, arg);
+			error = scsi_cmd_ioctl(disk, cmd, arg);
 			if (error != -ENOTTY)
 				return error;
 	}

fs/bio.c +27 ~~-22~~

83 lines not on this page · raw message

include/linux/bio.h +2 -2

diff -uprN -X /home/patman/dontdiff bk-2.6.2-rc3/include/linux/bio.h char_sg_io-bk-2.6.2-rc3/include/linux/bio.h
@@ -241,8 +241,8 @@ extern inline void bio_init(struct bio *
 
 extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
 extern int bio_get_nr_vecs(struct block_device *);
-extern struct bio *bio_map_user(struct block_device *, unsigned long,
-				unsigned int, int);
+extern struct bio *bio_map_user(struct request_queue *, struct block_device *,
+				unsigned long, unsigned int, int);
 extern void bio_unmap_user(struct bio *, int);
 extern void bio_set_pages_dirty(struct bio *bio);
 extern void bio_check_pages_dirty(struct bio *bio);

include/linux/blkdev.h +1 -1

diff -uprN -X /home/patman/dontdiff bk-2.6.2-rc3/include/linux/blkdev.h char_sg_io-bk-2.6.2-rc3/include/linux/blkdev.h
@@ -508,7 +508,7 @@ extern int blk_remove_plug(request_queue
 extern void blk_recount_segments(request_queue_t *, struct bio *);
 extern inline int blk_phys_contig_segment(request_queue_t *q, struct bio *, struct bio *);
 extern inline int blk_hw_contig_segment(request_queue_t *q, struct bio *, struct bio *);
-extern int scsi_cmd_ioctl(struct block_device *, unsigned int, unsigned long);
+extern int scsi_cmd_ioctl(struct gendisk *, unsigned int, unsigned long);
 extern void blk_start_queue(request_queue_t *q);
 extern void blk_stop_queue(request_queue_t *q);
 extern void __blk_stop_queue(request_queue_t *q);

Patrick Mansfield

author

2004-02-01 20:51 UTC #1 lore raw

PATCH 1/1 add scsi_cmd_ioctl (SG_IO) support for st

Add SG_IO support for st, so we can send scsi commands directly to an st device.

Though st (still?) needs to move to move away from cdev for udev to function with it.

drivers/scsi/st.c +43 -1

diff -uprN -X /home/patman/dontdiff bk-2.6.2-rc3/drivers/scsi/st.c char_sg_io-bk-2.6.2-rc3/drivers/scsi/st.c
@@ -179,6 +179,7 @@ static int sgl_unmap_user_pages(struct s
 
 static int st_probe(struct device *);
 static int st_remove(struct device *);
+static int st_init_command(struct scsi_cmnd *);
 
 static void do_create_driverfs_files(void);
 static void do_remove_driverfs_files(void);

@@ -191,6 +192,7 @@ static struct scsi_driver st_template = 
 		.probe		= st_probe,
 		.remove		= st_remove,
 	},
+	.init_command		= st_init_command,
 };
 
 static int st_compression(Scsi_Tape *, int);

@@ -3389,7 +3391,11 @@ static int st_ioctl(struct inode *inode,
 		goto out;
 	}
 	up(&STp->lock);
-	return scsi_ioctl(STp->device, cmd_in, (void *) arg);
+	i = scsi_cmd_ioctl(STp->disk, cmd_in, arg);
+	if (i != -ENOTTY)
+		return i;
+	else
+		return scsi_ioctl(STp->device, cmd_in, (void *) arg);
 
  out:
 	up(&STp->lock);

@@ -3796,6 +3802,7 @@ static int st_probe(struct device *dev)
 	tpnt->disk = disk;
 	sprintf(disk->disk_name, "st%d", i);
 	disk->private_data = &tpnt->driver;
+	disk->queue = SDp->request_queue;
 	tpnt->driver = &st_template;
 	scsi_tapes[i] = tpnt;
 	dev_num = i;

@@ -3983,6 +3990,41 @@ static int st_remove(struct device *dev)
 
 	write_unlock(&st_dev_arr_lock);
 	return 0;
+}
+
+static void st_intr(struct scsi_cmnd *SCpnt)
+{
+	scsi_io_completion(SCpnt, (SCpnt->result ? 0: SCpnt->bufflen >> 9), 1);
+}
+
+/*
+ * st_init_command: only called via the scsi_cmd_ioctl (block SG_IO)
+ * interface for REQ_BLOCK_PC commands.
+ */
+static int st_init_command(struct scsi_cmnd *SCpnt)
+{
+	struct request *rq;
+
+	if (!(SCpnt->request->flags & REQ_BLOCK_PC))
+		return 0;
+
+	rq = SCpnt->request;
+	if (sizeof(rq->cmd) > sizeof(SCpnt->cmnd))
+		return 0;
+
+	memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
+
+	if (rq_data_dir(rq) == WRITE)
+		SCpnt->sc_data_direction = DMA_TO_DEVICE;
+	else if (rq->data_len)
+		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
+	else
+		SCpnt->sc_data_direction = DMA_NONE;
+
+	SCpnt->timeout_per_command = rq->timeout;
+	SCpnt->transfersize = rq->data_len;
+	SCpnt->done = st_intr;
+	return 1;
 }
 
 static int __init init_st(void)

Christoph Hellwig

2004-02-01 21:35 UTC #2 lore raw

Re: [PATCH] add scsi_cmd_ioctl (SG_IO) support for st

Review

Quoted 2 lines

(quoted diff) +2 -0

+static void st_intr(struct scsi_cmnd *SCpnt)
+{

What about st_done instead? I know sd and sr use the intr names, but they're really misleading. Also convention for new code is to use cmd or scmd as variable names for scsi commands.

Quoted 14 lines

drivers/scsi/st.c +14 -0

+static int st_init_command(struct scsi_cmnd *SCpnt)
+{
+	struct request *rq;
+
+	if (!(SCpnt->request->flags & REQ_BLOCK_PC))
+		return 0;
+
+	rq = SCpnt->request;
+	if (sizeof(rq->cmd) > sizeof(SCpnt->cmnd))
+		return 0;
+
+	memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
+
+	if (rq_data_dir(rq) == WRITE)

drivers/scsi/st.c static int st_remove(struct device *dev) · line 4027

Quoted 10 lines

drivers/scsi/st.c +10 -0

+		SCpnt->sc_data_direction = DMA_TO_DEVICE;
+	else if (rq->data_len)
+		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
+	else
+		SCpnt->sc_data_direction = DMA_NONE;
+
+	SCpnt->timeout_per_command = rq->timeout;
+	SCpnt->transfersize = rq->data_len;
+	SCpnt->done = st_intr;
+	return 1;

This one looks obviously good, but yeah, this would much better fit into the scsi midlayer. Not for this patch session, though.

Patrick Mansfield

author

2004-02-01 21:52 UTC #3 lore raw

Re: [PATCH] add scsi_cmd_ioctl (SG_IO) support for st

Quoted 6 lines · Christoph Hellwig

Review
Quoted 2 lines
(quoted diff) +2 -0
+static void st_intr(struct scsi_cmnd *SCpnt)
+{
What about st_done instead? I know sd and sr use the intr names, but they're really misleading. Also convention for new code is to use cmd or scmd as variable names for scsi commands.

st_done is fine, I was matching current st.c coding style for SCpnt naming, but will change it.

Quoted 27 lines

Quoted 14 lines

drivers/scsi/st.c +14 -0

+static int st_init_command(struct scsi_cmnd *SCpnt)
+{
+	struct request *rq;
+
+	if (!(SCpnt->request->flags & REQ_BLOCK_PC))
+		return 0;
+
+	rq = SCpnt->request;
+	if (sizeof(rq->cmd) > sizeof(SCpnt->cmnd))
+		return 0;
+
+	memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
+
+	if (rq_data_dir(rq) == WRITE)

drivers/scsi/st.c static int st_remove(struct device *dev) · line 4027

Quoted 10 lines

drivers/scsi/st.c +10 -0

+		SCpnt->sc_data_direction = DMA_TO_DEVICE;
+	else if (rq->data_len)
+		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
+	else
+		SCpnt->sc_data_direction = DMA_NONE;
+
+	SCpnt->timeout_per_command = rq->timeout;
+	SCpnt->transfersize = rq->data_len;
+	SCpnt->done = st_intr;
+	return 1;

This one looks obviously good, but yeah, this would much better fit into the scsi midlayer. Not for this patch session, though.

Willem Riede

2004-02-01 22:28 UTC #4 lore raw

Re: [PATCH] add scsi_cmd_ioctl (SG_IO) support for st

In reply to Christoph Hellwig

Quoted 6 lines · Christoph Hellwig

Review
Quoted 2 lines
(quoted diff) +2 -0
+static void st_intr(struct scsi_cmnd *SCpnt)
+{
What about st_done instead? I know sd and sr use the intr names, but they're really misleading. Also convention for new code is to use cmd or scmd as variable names for scsi commands.

Is there any desire to change SCpnt to scmd in existing code (such as osst)?

Thanks, Willem Riede.

Christoph Hellwig

2004-02-01 22:36 UTC #5 lore raw

Re: [PATCH] add scsi_cmd_ioctl (SG_IO) support for st

Quoted 5 lines · Willem Riede

Quoted 3 lines
What about st_done instead? I know sd and sr use the intr names, but they're really misleading. Also convention for new code is to use cmd or scmd as variable names for scsi commands.
Is there any desire to change SCpnt to scmd in existing code (such as osst)?

For the scsi midlayer (and new driver if possible) it would be nice to have a single common notation. For existing drivers it's of course left to the author.

That beeing said it would of course be nice to have driver/scsi/ as homogeneous as possible.

Patrick Mansfield

author

2004-02-01 21:50 UTC #7 lore raw

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Patrick Mansfield
Jens or others, any comments?
This patch against a recent bk 2.6 changes scsi_cmd_ioctl to take a gendisk as an argument instead of a request_queue_t. This allows scsi char devices to use the scsi_cmd_ioctl interface.
Heh, I had pretty much the same patch in 2.5. I don't rember why I dropped it but you're looks at least as nice.

Quoted 6 lines

Quoted 3 lines
If the scsi upper level scsi_cmd_ioctl usage were consolidated in scsi_prep_fn, we could pass a request_queue_t instead of a gendisk to scsi_cmd_ioctl.
I don't parse that. You mean taking REQ_PC handling from upper drivers to the scsi core?

Yes. That would also clean up the upper level driver code (for example, sd.c's sd_init_command), but I have not tried this.

Basically for the REQ_BLOCK_PC, setup our own scsi_block_pc_intr in scsi_lib.c, set up the scsi_cmnd, and set the scsi_cmnd done to scsi_block_pc_intr. And maybe make still call scsi_init_io.

Then we don't need the:

	drv = *(struct scsi_driver **)req->rq_disk->private_data

And so, in theory ;-), we don't need rq_disk to be set. My reading of current code shows none of the other scsi_cmd_ioctl users need rq_disk, so we could then pass a q to scsi_cmd_ioctl.

Then in the future we could use sg_io instead of the scsi_do_req, scsi_wait_req, etc., and get rid of REQ_CMD in favor of REQ_BLOCK_PC.

Quoted 8 lines

fs/bio.c int bio_add_page(struct bio *bio, struct · line 369
Quoted 6 lines
fs/bio.c +6 -0
+int bio_add_page(struct bio *bio, struct page *page, unsigned int len,
+		 unsigned int offset)
+{
+	return __bio_add_page(bdev_get_queue(bio->bi_bdev), bio, page,
+			      len, offset);
+}
Maybe worth inlining in a header?

Maybe, and then export __bio_add_page.

-- Patrick Mansfield

Christoph Hellwig

2004-02-01 22:14 UTC #8 lore raw

Quoted 2 lines · Patrick Mansfield

Yes. That would also clean up the upper level driver code (for example, sd.c's sd_init_command), but I have not tried this.

Heh, I just took a short look at sd_rw_intr vs rw_intr (sr) and moving those into common code defintily looks like a good thing. There's a bunch of small difference, but I think we'd better off having those two unified instead of two sets of bugs. It's probably worth investigating.

Quoted 2 lines

Then in the future we could use sg_io instead of the scsi_do_req, scsi_wait_req, etc., and get rid of REQ_CMD in favor of REQ_BLOCK_PC.

Hmmm.. Sound like an interesting path to investigate. But unlike the REQ_PC unification it sounds more like a 2.7 thing.

Jens Axboe

2004-02-02 13:27 UTC #9 lore raw

In reply to Patrick Mansfield

Quoted 6 lines · Patrick Mansfield

Jens or others, any comments?
This patch against a recent bk 2.6 changes scsi_cmd_ioctl to take a gendisk as an argument instead of a request_queue_t. This allows scsi char devices to use the scsi_cmd_ioctl interface.

Quoted 8 lines

In turn, change bio_map_user to also pass a request_queue_t, and add a __bio_add_page helper that takes a request_queue_t.
Tested ide cd burning with no problems.
If the scsi upper level scsi_cmd_ioctl usage were consolidated in scsi_prep_fn, we could pass a request_queue_t instead of a gendisk to scsi_cmd_ioctl.

Looks fine to me.

Signature · 1 line

Jens Axboe