CDROM_SEND_PACKET oddity

CDROM_SEND_PACKET oddity

[Derek Foreman]

The example code from
http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c

Does not behave as expected on my 2.6.0-test5 system.  While the command 
seems to be successfully sent - 2 of my drives report it as an invalid 
opcode - for the other 2 drives, the buffer comes back all zeros.
(actually, the buffer's contents will remain in whatever state they're in 
before the ioctl is called)

Sending the same command to those 2 drives with SG_IO results in the 
expected behaviour.

Re: CDROM_SEND_PACKET oddity

[Jens Axboe]

On Fri, Sep 26 2003, Derek Foreman wrote:
> The example code from
> http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
> 
> Does not behave as expected on my 2.6.0-test5 system.  While the command 
> seems to be successfully sent - 2 of my drives report it as an invalid 
> opcode - for the other 2 drives, the buffer comes back all zeros.
> (actually, the buffer's contents will remain in whatever state they're in 
> before the ioctl is called)
> 
> Sending the same command to those 2 drives with SG_IO results in the 
> expected behaviour.

Can you try current -bk? It has some fixes for CDROM_SEND_PACKET.

However, cd_poll should be rewritten to use SG_IO. Pretty trivial
exercise.

-- 
Jens Axboe

Re: CDROM_SEND_PACKET oddity

[Jens Axboe]

On Sat, Sep 27 2003, Jens Axboe wrote:
> On Fri, Sep 26 2003, Derek Foreman wrote:
> > The example code from
> > http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
> > 
> > Does not behave as expected on my 2.6.0-test5 system.  While the command 
> > seems to be successfully sent - 2 of my drives report it as an invalid 
> > opcode - for the other 2 drives, the buffer comes back all zeros.
> > (actually, the buffer's contents will remain in whatever state they're in 
> > before the ioctl is called)
> > 
> > Sending the same command to those 2 drives with SG_IO results in the 
> > expected behaviour.
> 
> Can you try current -bk? It has some fixes for CDROM_SEND_PACKET.
> 
> However, cd_poll should be rewritten to use SG_IO. Pretty trivial
> exercise.

Actually, try this patch against current bk, it kills the
CDROM_SEND_PACKET setup and use SG_IO internally instead. Should be much
much better than what we have now. It's not tested here at all though,
I'd appreciate it if you could give it a go.

===== drivers/block/scsi_ioctl.c 1.34 vs edited =====
--- 1.34/drivers/block/scsi_ioctl.c	Fri Sep  5 12:22:31 2003
+++ edited/drivers/block/scsi_ioctl.c	Sat Sep 27 14:24:42 2003
@@ -25,6 +25,7 @@
 #include <linux/cdrom.h>
 #include <linux/slab.h>
 #include <linux/bio.h>
+#include <linux/times.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -139,41 +140,36 @@
 	return put_user(1, p);
 }
 
-static int sg_io(request_queue_t *q, struct block_device *bdev,
-		 struct sg_io_hdr *uptr)
+int sg_io(request_queue_t *q, struct block_device *bdev, struct sg_io_hdr *hdr)
 {
 	unsigned long start_time;
 	int reading, writing;
-	struct sg_io_hdr hdr;
 	struct request *rq;
 	struct bio *bio;
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	void *buffer;
 
-	if (copy_from_user(&hdr, uptr, sizeof(*uptr)))
-		return -EFAULT;
-
-	if (hdr.interface_id != 'S')
+	if (hdr->interface_id != 'S')
 		return -EINVAL;
-	if (hdr.cmd_len > sizeof(rq->cmd))
+	if (hdr->cmd_len > sizeof(rq->cmd))
 		return -EINVAL;
 
 	/*
 	 * we'll do that later
 	 */
-	if (hdr.iovec_count)
+	if (hdr->iovec_count)
 		return -EOPNOTSUPP;
 
-	if (hdr.dxfer_len > (q->max_sectors << 9))
+	if (hdr->dxfer_len > (q->max_sectors << 9))
 		return -EIO;
 
 	reading = writing = 0;
 	buffer = NULL;
 	bio = NULL;
-	if (hdr.dxfer_len) {
-		unsigned int bytes = (hdr.dxfer_len + 511) & ~511;
+	if (hdr->dxfer_len) {
+		unsigned int bytes = (hdr->dxfer_len + 511) & ~511;
 
-		switch (hdr.dxfer_direction) {
+		switch (hdr->dxfer_direction) {
 		default:
 			return -EINVAL;
 		case SG_DXFER_TO_FROM_DEV:
@@ -191,8 +187,8 @@
 		 * 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,
-				   hdr.dxfer_len, reading);
+		bio = bio_map_user(bdev, (unsigned long) hdr->dxferp,
+				   hdr->dxfer_len, reading);
 
 		/*
 		 * if bio setup failed, fall back to slow approach
@@ -203,11 +199,11 @@
 				return -ENOMEM;
 
 			if (writing) {
-				if (copy_from_user(buffer, hdr.dxferp,
-						   hdr.dxfer_len))
+				if (copy_from_user(buffer, hdr->dxferp,
+						   hdr->dxfer_len))
 					goto out_buffer;
 			} else
-				memset(buffer, 0, hdr.dxfer_len);
+				memset(buffer, 0, hdr->dxfer_len);
 		}
 	}
 
@@ -216,11 +212,11 @@
 	/*
 	 * fill in request structure
 	 */
-	rq->cmd_len = hdr.cmd_len;
-	if (copy_from_user(rq->cmd, hdr.cmdp, hdr.cmd_len))
+	rq->cmd_len = hdr->cmd_len;
+	if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
 		goto out_request;
-	if (sizeof(rq->cmd) != hdr.cmd_len)
-		memset(rq->cmd + hdr.cmd_len, 0, sizeof(rq->cmd) - hdr.cmd_len);
+	if (sizeof(rq->cmd) != hdr->cmd_len)
+		memset(rq->cmd + hdr->cmd_len, 0, sizeof(rq->cmd) - hdr->cmd_len);
 
 	memset(sense, 0, sizeof(sense));
 	rq->sense = sense;
@@ -234,9 +230,9 @@
 		blk_rq_bio_prep(q, rq, bio);
 
 	rq->data = buffer;
-	rq->data_len = hdr.dxfer_len;
+	rq->data_len = hdr->dxfer_len;
 
-	rq->timeout = (hdr.timeout * HZ) / 1000;
+	rq->timeout = (hdr->timeout * HZ) / 1000;
 	if (!rq->timeout)
 		rq->timeout = q->sg_timeout;
 	if (!rq->timeout)
@@ -254,33 +250,30 @@
 		bio_unmap_user(bio, reading);
 
 	/* write to all output members */
-	hdr.status = rq->errors;	
-	hdr.masked_status = (hdr.status >> 1) & 0x1f;
-	hdr.msg_status = 0;
-	hdr.host_status = 0;
-	hdr.driver_status = 0;
-	hdr.info = 0;
-	if (hdr.masked_status || hdr.host_status || hdr.driver_status)
-		hdr.info |= SG_INFO_CHECK;
-	hdr.resid = rq->data_len;
-	hdr.duration = ((jiffies - start_time) * 1000) / HZ;
-	hdr.sb_len_wr = 0;
+	hdr->status = rq->errors;	
+	hdr->masked_status = (hdr->status >> 1) & 0x1f;
+	hdr->msg_status = 0;
+	hdr->host_status = 0;
+	hdr->driver_status = 0;
+	hdr->info = 0;
+	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
+		hdr->info |= SG_INFO_CHECK;
+	hdr->resid = rq->data_len;
+	hdr->duration = ((jiffies - start_time) * 1000) / HZ;
+	hdr->sb_len_wr = 0;
 
-	if (rq->sense_len && hdr.sbp) {
-		int len = min((unsigned int) hdr.mx_sb_len, rq->sense_len);
+	if (rq->sense_len && hdr->sbp) {
+		int len = min((unsigned int) hdr->mx_sb_len, rq->sense_len);
 
-		if (!copy_to_user(hdr.sbp, rq->sense, len))
-			hdr.sb_len_wr = len;
+		if (!copy_to_user(hdr->sbp, rq->sense, len))
+			hdr->sb_len_wr = len;
 	}
 
 	blk_put_request(rq);
 
-	if (copy_to_user(uptr, &hdr, sizeof(*uptr)))
-		goto out_buffer;
-
 	if (buffer) {
 		if (reading)
-			if (copy_to_user(hdr.dxferp, buffer, hdr.dxfer_len))
+			if (copy_to_user(hdr->dxferp, buffer, hdr->dxfer_len))
 				goto out_buffer;
 
 		kfree(buffer);
@@ -437,9 +430,64 @@
 		case SG_EMULATED_HOST:
 			err = sg_emulated_host(q, (int *) arg);
 			break;
-		case SG_IO:
-			err = sg_io(q, bdev, (struct sg_io_hdr *) arg);
+		case SG_IO: {
+			struct sg_io_hdr hdr;
+
+			if (copy_from_user(&hdr, (struct sg_io_hdr *) arg, sizeof(hdr))) {
+				err = -EFAULT;
+				break;
+			}
+			err = sg_io(q, bdev, &hdr);
+			if (copy_to_user((struct sg_io_hdr *) arg, &hdr, sizeof(hdr)))
+				err = -EFAULT;
+			break;
+		}
+		case CDROM_SEND_PACKET: {
+			struct cdrom_generic_command cgc;
+			struct sg_io_hdr hdr;
+
+			if (copy_from_user(&cgc, (struct cdrom_generic_command *) arg, sizeof(cgc))) {
+				err = -EFAULT;
+				break;
+			}
+			cgc.timeout = clock_t_to_jiffies(cgc.timeout);
+			memset(&hdr, 0, sizeof(hdr));
+			hdr.interface_id = 'S';
+			hdr.cmd_len = sizeof(cgc.cmd);
+			hdr.dxfer_len = cgc.buflen;
+			err = 0;
+			switch (cgc.data_direction) {
+				case CGC_DATA_UNKNOWN:
+					hdr.dxfer_direction = SG_DXFER_UNKNOWN;
+					break;
+				case CGC_DATA_WRITE:
+					hdr.dxfer_direction = SG_DXFER_TO_DEV;
+					break;
+				case CGC_DATA_READ:
+					hdr.dxfer_direction = SG_DXFER_FROM_DEV;
+					break;
+				case CGC_DATA_NONE:
+					hdr.dxfer_direction = SG_DXFER_NONE;
+					break;
+				default:
+					err = -EINVAL;
+			}
+			if (err)
+				break;
+
+			hdr.dxferp = cgc.buffer;
+			hdr.sbp = (char *) cgc.sense;
+			hdr.timeout = cgc.timeout;
+			err = sg_io(q, bdev, &hdr);
+
+			cgc.stat = err;
+			cgc.buflen = hdr.resid;
+			if (copy_to_user((struct cdrom_generic_command *) arg, &cgc, sizeof(cgc)))
+				err = -EFAULT;
+
 			break;
+		}
+
 		/*
 		 * old junk scsi send command ioctl
 		 */
@@ -475,3 +523,4 @@
 
 EXPORT_SYMBOL(scsi_cmd_ioctl);
 EXPORT_SYMBOL(scsi_command_size);
+EXPORT_SYMBOL(sg_io);
===== drivers/cdrom/cdrom.c 1.40 vs edited =====
--- 1.40/drivers/cdrom/cdrom.c	Tue Sep 23 21:18:06 2003
+++ edited/drivers/cdrom/cdrom.c	Sat Sep 27 14:24:59 2003
@@ -1856,57 +1856,6 @@
 	return cdo->generic_packet(cdi, &cgc);
 }
 
-static int cdrom_do_cmd(struct cdrom_device_info *cdi,
-			struct cdrom_generic_command *cgc)
-{
-	struct request_sense *usense, sense;
-	unsigned char *ubuf;
-	int ret;
-
-	if (cgc->data_direction == CGC_DATA_UNKNOWN)
-		return -EINVAL;
-
-	if (cgc->buflen < 0 || cgc->buflen >= 131072)
-		return -EINVAL;
-
-	usense = cgc->sense;
-	cgc->sense = &sense;
-	if (usense && !access_ok(VERIFY_WRITE, usense, sizeof(*usense))) {
-		return -EFAULT;
-	}
-
-	ubuf = cgc->buffer;
-	if (cgc->data_direction == CGC_DATA_READ ||
-	    cgc->data_direction == CGC_DATA_WRITE) {
-		cgc->buffer = kmalloc(cgc->buflen, GFP_KERNEL);
-		if (cgc->buffer == NULL)
-			return -ENOMEM;
-	}
-
-
-	if (cgc->data_direction == CGC_DATA_READ) {
-		if (!access_ok(VERIFY_READ, ubuf, cgc->buflen)) {
-			kfree(cgc->buffer);
-			return -EFAULT;
-		}
-	} else if (cgc->data_direction == CGC_DATA_WRITE) {
-		if (copy_from_user(cgc->buffer, ubuf, cgc->buflen)) {
-			kfree(cgc->buffer);
-			return -EFAULT;
-		}
-	}
-
-	ret = cdi->ops->generic_packet(cdi, cgc);
-	__copy_to_user(usense, cgc->sense, sizeof(*usense));
-	if (!ret && cgc->data_direction == CGC_DATA_READ)
-		__copy_to_user(ubuf, cgc->buffer, cgc->buflen);
-	if (cgc->data_direction == CGC_DATA_READ ||
-	    cgc->data_direction == CGC_DATA_WRITE) {
-		kfree(cgc->buffer);
-	}
-	return ret;
-}
-
 static int mmc_ioctl(struct cdrom_device_info *cdi, unsigned int cmd,
 		     unsigned long arg)
 {		
@@ -2176,14 +2125,6 @@
 		return 0;
 		}
 
-	case CDROM_SEND_PACKET: {
-		if (!CDROM_CAN(CDC_GENERIC_PACKET))
-			return -ENOSYS;
-		cdinfo(CD_DO_IOCTL, "entering CDROM_SEND_PACKET\n"); 
-		IOCTL_IN(arg, struct cdrom_generic_command, cgc);
-		cgc.timeout = clock_t_to_jiffies(cgc.timeout);
-		return cdrom_do_cmd(cdi, &cgc);
-		}
 	case CDROM_NEXT_WRITABLE: {
 		long next = 0;
 		cdinfo(CD_DO_IOCTL, "entering CDROM_NEXT_WRITABLE\n"); 

-- 
Jens Axboe

Re: CDROM_SEND_PACKET oddity

[Jens Axboe]

On Sat, Sep 27 2003, Jens Axboe wrote:
> On Sat, Sep 27 2003, Jens Axboe wrote:
> > On Fri, Sep 26 2003, Derek Foreman wrote:
> > > The example code from
> > > http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
> > > 
> > > Does not behave as expected on my 2.6.0-test5 system.  While the command 
> > > seems to be successfully sent - 2 of my drives report it as an invalid 
> > > opcode - for the other 2 drives, the buffer comes back all zeros.
> > > (actually, the buffer's contents will remain in whatever state they're in 
> > > before the ioctl is called)
> > > 
> > > Sending the same command to those 2 drives with SG_IO results in the 
> > > expected behaviour.
> > 
> > Can you try current -bk? It has some fixes for CDROM_SEND_PACKET.
> > 
> > However, cd_poll should be rewritten to use SG_IO. Pretty trivial
> > exercise.
> 
> Actually, try this patch against current bk, it kills the
> CDROM_SEND_PACKET setup and use SG_IO internally instead. Should be much
> much better than what we have now. It's not tested here at all though,
> I'd appreciate it if you could give it a go.

This has a better chance of working. Changes:

- Don't export sg_io() anyways (leftover)
- Actually set ->cmdp and ->cmd_len

still untested.

===== drivers/block/scsi_ioctl.c 1.34 vs edited =====
--- 1.34/drivers/block/scsi_ioctl.c	Fri Sep  5 12:22:31 2003
+++ edited/drivers/block/scsi_ioctl.c	Sat Sep 27 19:53:10 2003
@@ -25,6 +25,7 @@
 #include <linux/cdrom.h>
 #include <linux/slab.h>
 #include <linux/bio.h>
+#include <linux/times.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -140,40 +141,36 @@
 }
 
 static int sg_io(request_queue_t *q, struct block_device *bdev,
-		 struct sg_io_hdr *uptr)
+		 struct sg_io_hdr *hdr)
 {
 	unsigned long start_time;
 	int reading, writing;
-	struct sg_io_hdr hdr;
 	struct request *rq;
 	struct bio *bio;
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	void *buffer;
 
-	if (copy_from_user(&hdr, uptr, sizeof(*uptr)))
-		return -EFAULT;
-
-	if (hdr.interface_id != 'S')
+	if (hdr->interface_id != 'S')
 		return -EINVAL;
-	if (hdr.cmd_len > sizeof(rq->cmd))
+	if (hdr->cmd_len > sizeof(rq->cmd))
 		return -EINVAL;
 
 	/*
 	 * we'll do that later
 	 */
-	if (hdr.iovec_count)
+	if (hdr->iovec_count)
 		return -EOPNOTSUPP;
 
-	if (hdr.dxfer_len > (q->max_sectors << 9))
+	if (hdr->dxfer_len > (q->max_sectors << 9))
 		return -EIO;
 
 	reading = writing = 0;
 	buffer = NULL;
 	bio = NULL;
-	if (hdr.dxfer_len) {
-		unsigned int bytes = (hdr.dxfer_len + 511) & ~511;
+	if (hdr->dxfer_len) {
+		unsigned int bytes = (hdr->dxfer_len + 511) & ~511;
 
-		switch (hdr.dxfer_direction) {
+		switch (hdr->dxfer_direction) {
 		default:
 			return -EINVAL;
 		case SG_DXFER_TO_FROM_DEV:
@@ -191,8 +188,8 @@
 		 * 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,
-				   hdr.dxfer_len, reading);
+		bio = bio_map_user(bdev, (unsigned long) hdr->dxferp,
+				   hdr->dxfer_len, reading);
 
 		/*
 		 * if bio setup failed, fall back to slow approach
@@ -203,11 +200,11 @@
 				return -ENOMEM;
 
 			if (writing) {
-				if (copy_from_user(buffer, hdr.dxferp,
-						   hdr.dxfer_len))
+				if (copy_from_user(buffer, hdr->dxferp,
+						   hdr->dxfer_len))
 					goto out_buffer;
 			} else
-				memset(buffer, 0, hdr.dxfer_len);
+				memset(buffer, 0, hdr->dxfer_len);
 		}
 	}
 
@@ -216,11 +213,11 @@
 	/*
 	 * fill in request structure
 	 */
-	rq->cmd_len = hdr.cmd_len;
-	if (copy_from_user(rq->cmd, hdr.cmdp, hdr.cmd_len))
+	rq->cmd_len = hdr->cmd_len;
+	if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
 		goto out_request;
-	if (sizeof(rq->cmd) != hdr.cmd_len)
-		memset(rq->cmd + hdr.cmd_len, 0, sizeof(rq->cmd) - hdr.cmd_len);
+	if (sizeof(rq->cmd) != hdr->cmd_len)
+		memset(rq->cmd + hdr->cmd_len, 0, sizeof(rq->cmd) - hdr->cmd_len);
 
 	memset(sense, 0, sizeof(sense));
 	rq->sense = sense;
@@ -234,9 +231,9 @@
 		blk_rq_bio_prep(q, rq, bio);
 
 	rq->data = buffer;
-	rq->data_len = hdr.dxfer_len;
+	rq->data_len = hdr->dxfer_len;
 
-	rq->timeout = (hdr.timeout * HZ) / 1000;
+	rq->timeout = (hdr->timeout * HZ) / 1000;
 	if (!rq->timeout)
 		rq->timeout = q->sg_timeout;
 	if (!rq->timeout)
@@ -254,33 +251,30 @@
 		bio_unmap_user(bio, reading);
 
 	/* write to all output members */
-	hdr.status = rq->errors;	
-	hdr.masked_status = (hdr.status >> 1) & 0x1f;
-	hdr.msg_status = 0;
-	hdr.host_status = 0;
-	hdr.driver_status = 0;
-	hdr.info = 0;
-	if (hdr.masked_status || hdr.host_status || hdr.driver_status)
-		hdr.info |= SG_INFO_CHECK;
-	hdr.resid = rq->data_len;
-	hdr.duration = ((jiffies - start_time) * 1000) / HZ;
-	hdr.sb_len_wr = 0;
+	hdr->status = rq->errors;	
+	hdr->masked_status = (hdr->status >> 1) & 0x1f;
+	hdr->msg_status = 0;
+	hdr->host_status = 0;
+	hdr->driver_status = 0;
+	hdr->info = 0;
+	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
+		hdr->info |= SG_INFO_CHECK;
+	hdr->resid = rq->data_len;
+	hdr->duration = ((jiffies - start_time) * 1000) / HZ;
+	hdr->sb_len_wr = 0;
 
-	if (rq->sense_len && hdr.sbp) {
-		int len = min((unsigned int) hdr.mx_sb_len, rq->sense_len);
+	if (rq->sense_len && hdr->sbp) {
+		int len = min((unsigned int) hdr->mx_sb_len, rq->sense_len);
 
-		if (!copy_to_user(hdr.sbp, rq->sense, len))
-			hdr.sb_len_wr = len;
+		if (!copy_to_user(hdr->sbp, rq->sense, len))
+			hdr->sb_len_wr = len;
 	}
 
 	blk_put_request(rq);
 
-	if (copy_to_user(uptr, &hdr, sizeof(*uptr)))
-		goto out_buffer;
-
 	if (buffer) {
 		if (reading)
-			if (copy_to_user(hdr.dxferp, buffer, hdr.dxfer_len))
+			if (copy_to_user(hdr->dxferp, buffer, hdr->dxfer_len))
 				goto out_buffer;
 
 		kfree(buffer);
@@ -437,9 +431,66 @@
 		case SG_EMULATED_HOST:
 			err = sg_emulated_host(q, (int *) arg);
 			break;
-		case SG_IO:
-			err = sg_io(q, bdev, (struct sg_io_hdr *) arg);
+		case SG_IO: {
+			struct sg_io_hdr hdr;
+
+			if (copy_from_user(&hdr, (struct sg_io_hdr *) arg, sizeof(hdr))) {
+				err = -EFAULT;
+				break;
+			}
+			err = sg_io(q, bdev, &hdr);
+			if (copy_to_user((struct sg_io_hdr *) arg, &hdr, sizeof(hdr)))
+				err = -EFAULT;
+			break;
+		}
+		case CDROM_SEND_PACKET: {
+			struct cdrom_generic_command cgc;
+			struct sg_io_hdr hdr;
+
+			if (copy_from_user(&cgc, (struct cdrom_generic_command *) arg, sizeof(cgc))) {
+				err = -EFAULT;
+				break;
+			}
+			cgc.timeout = clock_t_to_jiffies(cgc.timeout);
+			memset(&hdr, 0, sizeof(hdr));
+			hdr.interface_id = 'S';
+			hdr.cmd_len = sizeof(cgc.cmd);
+			hdr.dxfer_len = cgc.buflen;
+			err = 0;
+			switch (cgc.data_direction) {
+				case CGC_DATA_UNKNOWN:
+					hdr.dxfer_direction = SG_DXFER_UNKNOWN;
+					break;
+				case CGC_DATA_WRITE:
+					hdr.dxfer_direction = SG_DXFER_TO_DEV;
+					break;
+				case CGC_DATA_READ:
+					hdr.dxfer_direction = SG_DXFER_FROM_DEV;
+					break;
+				case CGC_DATA_NONE:
+					hdr.dxfer_direction = SG_DXFER_NONE;
+					break;
+				default:
+					err = -EINVAL;
+			}
+			if (err)
+				break;
+
+			hdr.dxferp = cgc.buffer;
+			hdr.sbp = (char *) cgc.sense;
+			hdr.timeout = cgc.timeout;
+			memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));
+			hdr.cmd_len = sizeof(cgc.cmd);
+			err = sg_io(q, bdev, &hdr);
+
+			cgc.stat = err;
+			cgc.buflen = hdr.resid;
+			if (copy_to_user((struct cdrom_generic_command *) arg, &cgc, sizeof(cgc)))
+				err = -EFAULT;
+
 			break;
+		}
+
 		/*
 		 * old junk scsi send command ioctl
 		 */
===== drivers/cdrom/cdrom.c 1.40 vs edited =====
--- 1.40/drivers/cdrom/cdrom.c	Tue Sep 23 21:18:06 2003
+++ edited/drivers/cdrom/cdrom.c	Sat Sep 27 14:24:59 2003
@@ -1856,57 +1856,6 @@
 	return cdo->generic_packet(cdi, &cgc);
 }
 
-static int cdrom_do_cmd(struct cdrom_device_info *cdi,
-			struct cdrom_generic_command *cgc)
-{
-	struct request_sense *usense, sense;
-	unsigned char *ubuf;
-	int ret;
-
-	if (cgc->data_direction == CGC_DATA_UNKNOWN)
-		return -EINVAL;
-
-	if (cgc->buflen < 0 || cgc->buflen >= 131072)
-		return -EINVAL;
-
-	usense = cgc->sense;
-	cgc->sense = &sense;
-	if (usense && !access_ok(VERIFY_WRITE, usense, sizeof(*usense))) {
-		return -EFAULT;
-	}
-
-	ubuf = cgc->buffer;
-	if (cgc->data_direction == CGC_DATA_READ ||
-	    cgc->data_direction == CGC_DATA_WRITE) {
-		cgc->buffer = kmalloc(cgc->buflen, GFP_KERNEL);
-		if (cgc->buffer == NULL)
-			return -ENOMEM;
-	}
-
-
-	if (cgc->data_direction == CGC_DATA_READ) {
-		if (!access_ok(VERIFY_READ, ubuf, cgc->buflen)) {
-			kfree(cgc->buffer);
-			return -EFAULT;
-		}
-	} else if (cgc->data_direction == CGC_DATA_WRITE) {
-		if (copy_from_user(cgc->buffer, ubuf, cgc->buflen)) {
-			kfree(cgc->buffer);
-			return -EFAULT;
-		}
-	}
-
-	ret = cdi->ops->generic_packet(cdi, cgc);
-	__copy_to_user(usense, cgc->sense, sizeof(*usense));
-	if (!ret && cgc->data_direction == CGC_DATA_READ)
-		__copy_to_user(ubuf, cgc->buffer, cgc->buflen);
-	if (cgc->data_direction == CGC_DATA_READ ||
-	    cgc->data_direction == CGC_DATA_WRITE) {
-		kfree(cgc->buffer);
-	}
-	return ret;
-}
-
 static int mmc_ioctl(struct cdrom_device_info *cdi, unsigned int cmd,
 		     unsigned long arg)
 {		
@@ -2176,14 +2125,6 @@
 		return 0;
 		}
 
-	case CDROM_SEND_PACKET: {
-		if (!CDROM_CAN(CDC_GENERIC_PACKET))
-			return -ENOSYS;
-		cdinfo(CD_DO_IOCTL, "entering CDROM_SEND_PACKET\n"); 
-		IOCTL_IN(arg, struct cdrom_generic_command, cgc);
-		cgc.timeout = clock_t_to_jiffies(cgc.timeout);
-		return cdrom_do_cmd(cdi, &cgc);
-		}
 	case CDROM_NEXT_WRITABLE: {
 		long next = 0;
 		cdinfo(CD_DO_IOCTL, "entering CDROM_NEXT_WRITABLE\n"); 

-- 
Jens Axboe

Re: CDROM_SEND_PACKET oddity

[Derek Foreman]

On Sat, 27 Sep 2003, Jens Axboe wrote:

> On Sat, Sep 27 2003, Jens Axboe wrote:
> > On Sat, Sep 27 2003, Jens Axboe wrote:
> > > On Fri, Sep 26 2003, Derek Foreman wrote:
> > > > The example code from
> > > > http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
> > > > 
> > > > Does not behave as expected on my 2.6.0-test5 system.  While the command 
> > > > seems to be successfully sent - 2 of my drives report it as an invalid 
> > > > opcode - for the other 2 drives, the buffer comes back all zeros.
> > > > (actually, the buffer's contents will remain in whatever state they're in 
> > > > before the ioctl is called)
> > > > 
> > > > Sending the same command to those 2 drives with SG_IO results in the 
> > > > expected behaviour.
> > > 
> > > Can you try current -bk? It has some fixes for CDROM_SEND_PACKET.
> > > 
> > > However, cd_poll should be rewritten to use SG_IO. Pretty trivial
> > > exercise.
> > 
> > Actually, try this patch against current bk, it kills the
> > CDROM_SEND_PACKET setup and use SG_IO internally instead. Should be much
> > much better than what we have now. It's not tested here at all though,
> > I'd appreciate it if you could give it a go.
> 
> This has a better chance of working. Changes:
> 
> - Don't export sg_io() anyways (leftover)
> - Actually set ->cmdp and ->cmd_len
> 
> still untested.

The old one worked after I fixed it up to set cmdp and the sense buffer.  
I'll test this one out a little later today.

Re: CDROM_SEND_PACKET oddity

[Derek Foreman]

On Sat, 27 Sep 2003, Jens Axboe wrote:

> On Sat, Sep 27 2003, Jens Axboe wrote:
> > On Sat, Sep 27 2003, Jens Axboe wrote:
> > Actually, try this patch against current bk, it kills the
> > CDROM_SEND_PACKET setup and use SG_IO internally instead. Should be much
> > much better than what we have now. It's not tested here at all though,
> > I'd appreciate it if you could give it a go.
> 
> This has a better chance of working. Changes:
> 
> - Don't export sg_io() anyways (leftover)
> - Actually set ->cmdp and ->cmd_len
> 
> still untested.

[...]

> +			memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));

This breaks because hdr.cmdp is a pointer, not an array.

I think there has to be a hdr.mx_sb_len = sizeof(struct request_sense) in 
there too?

Also, this changes the semantics of CDROM_SEND_PACKET, currently if 
the command fails, it returns EIO, after the patch it succeeds.

how's this incremental patch?  It seems to work as I expect it to.

--- scsi_ioctl.c.orig	2003-09-27 22:45:40.708105384 -0500
+++ scsi_ioctl.c	2003-09-27 22:46:23.490917249 -0500
@@ -479,10 +479,14 @@
 			hdr.dxferp = cgc.buffer;
 			hdr.sbp = (char *) cgc.sense;
 			hdr.timeout = cgc.timeout;
-			memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));
+			hdr.cmdp = (unsigned char *)arg
+			         + offsetof(struct cdrom_generic_command, cmd);
+			hdr.mx_sb_len = sizeof(struct request_sense);
 			hdr.cmd_len = sizeof(cgc.cmd);
 			err = sg_io(q, bdev, &hdr);
 
+			if (hdr.status)
+				err = -EIO;
 			cgc.stat = err;
 			cgc.buflen = hdr.resid;
 			if (copy_to_user((struct cdrom_generic_command *) arg, &cgc, sizeof(cgc)))

Re: CDROM_SEND_PACKET oddity

[Jens Axboe]

On Sat, Sep 27 2003, Derek Foreman wrote:
> On Sat, 27 Sep 2003, Jens Axboe wrote:
> 
> > On Sat, Sep 27 2003, Jens Axboe wrote:
> > > On Sat, Sep 27 2003, Jens Axboe wrote:
> > > Actually, try this patch against current bk, it kills the
> > > CDROM_SEND_PACKET setup and use SG_IO internally instead. Should be much
> > > much better than what we have now. It's not tested here at all though,
> > > I'd appreciate it if you could give it a go.
> > 
> > This has a better chance of working. Changes:
> > 
> > - Don't export sg_io() anyways (leftover)
> > - Actually set ->cmdp and ->cmd_len
> > 
> > still untested.
> 
> [...]
> 
> > +			memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));
> 
> This breaks because hdr.cmdp is a pointer, not an array.

Duh, yes.

> I think there has to be a hdr.mx_sb_len = sizeof(struct request_sense) in 
> there too?

Yes

> Also, this changes the semantics of CDROM_SEND_PACKET, currently if 
> the command fails, it returns EIO, after the patch it succeeds.

Ok, that needs to be fixed.

> how's this incremental patch?  It seems to work as I expect it to.
> 
> --- scsi_ioctl.c.orig	2003-09-27 22:45:40.708105384 -0500
> +++ scsi_ioctl.c	2003-09-27 22:46:23.490917249 -0500
> @@ -479,10 +479,14 @@
>  			hdr.dxferp = cgc.buffer;
>  			hdr.sbp = (char *) cgc.sense;
>  			hdr.timeout = cgc.timeout;
> -			memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));
> +			hdr.cmdp = (unsigned char *)arg
> +			         + offsetof(struct cdrom_generic_command, cmd);

No that's buggy, arg is a user pointer. It needs to read:

			hdr.cmdp = cgc.cmd;

> +			hdr.mx_sb_len = sizeof(struct request_sense);
>  			hdr.cmd_len = sizeof(cgc.cmd);
>  			err = sg_io(q, bdev, &hdr);
>  
> +			if (hdr.status)
> +				err = -EIO;

Looks fine.

===== drivers/block/scsi_ioctl.c 1.34 vs edited =====
--- 1.34/drivers/block/scsi_ioctl.c	Fri Sep  5 12:22:31 2003
+++ edited/drivers/block/scsi_ioctl.c	Sun Sep 28 10:50:50 2003
@@ -25,6 +25,7 @@
 #include <linux/cdrom.h>
 #include <linux/slab.h>
 #include <linux/bio.h>
+#include <linux/times.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -140,40 +141,36 @@
 }
 
 static int sg_io(request_queue_t *q, struct block_device *bdev,
-		 struct sg_io_hdr *uptr)
+		 struct sg_io_hdr *hdr)
 {
 	unsigned long start_time;
 	int reading, writing;
-	struct sg_io_hdr hdr;
 	struct request *rq;
 	struct bio *bio;
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	void *buffer;
 
-	if (copy_from_user(&hdr, uptr, sizeof(*uptr)))
-		return -EFAULT;
-
-	if (hdr.interface_id != 'S')
+	if (hdr->interface_id != 'S')
 		return -EINVAL;
-	if (hdr.cmd_len > sizeof(rq->cmd))
+	if (hdr->cmd_len > sizeof(rq->cmd))
 		return -EINVAL;
 
 	/*
 	 * we'll do that later
 	 */
-	if (hdr.iovec_count)
+	if (hdr->iovec_count)
 		return -EOPNOTSUPP;
 
-	if (hdr.dxfer_len > (q->max_sectors << 9))
+	if (hdr->dxfer_len > (q->max_sectors << 9))
 		return -EIO;
 
 	reading = writing = 0;
 	buffer = NULL;
 	bio = NULL;
-	if (hdr.dxfer_len) {
-		unsigned int bytes = (hdr.dxfer_len + 511) & ~511;
+	if (hdr->dxfer_len) {
+		unsigned int bytes = (hdr->dxfer_len + 511) & ~511;
 
-		switch (hdr.dxfer_direction) {
+		switch (hdr->dxfer_direction) {
 		default:
 			return -EINVAL;
 		case SG_DXFER_TO_FROM_DEV:
@@ -191,8 +188,8 @@
 		 * 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,
-				   hdr.dxfer_len, reading);
+		bio = bio_map_user(bdev, (unsigned long) hdr->dxferp,
+				   hdr->dxfer_len, reading);
 
 		/*
 		 * if bio setup failed, fall back to slow approach
@@ -203,11 +200,11 @@
 				return -ENOMEM;
 
 			if (writing) {
-				if (copy_from_user(buffer, hdr.dxferp,
-						   hdr.dxfer_len))
+				if (copy_from_user(buffer, hdr->dxferp,
+						   hdr->dxfer_len))
 					goto out_buffer;
 			} else
-				memset(buffer, 0, hdr.dxfer_len);
+				memset(buffer, 0, hdr->dxfer_len);
 		}
 	}
 
@@ -216,11 +213,11 @@
 	/*
 	 * fill in request structure
 	 */
-	rq->cmd_len = hdr.cmd_len;
-	if (copy_from_user(rq->cmd, hdr.cmdp, hdr.cmd_len))
+	rq->cmd_len = hdr->cmd_len;
+	if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
 		goto out_request;
-	if (sizeof(rq->cmd) != hdr.cmd_len)
-		memset(rq->cmd + hdr.cmd_len, 0, sizeof(rq->cmd) - hdr.cmd_len);
+	if (sizeof(rq->cmd) != hdr->cmd_len)
+		memset(rq->cmd + hdr->cmd_len, 0, sizeof(rq->cmd) - hdr->cmd_len);
 
 	memset(sense, 0, sizeof(sense));
 	rq->sense = sense;
@@ -234,9 +231,9 @@
 		blk_rq_bio_prep(q, rq, bio);
 
 	rq->data = buffer;
-	rq->data_len = hdr.dxfer_len;
+	rq->data_len = hdr->dxfer_len;
 
-	rq->timeout = (hdr.timeout * HZ) / 1000;
+	rq->timeout = (hdr->timeout * HZ) / 1000;
 	if (!rq->timeout)
 		rq->timeout = q->sg_timeout;
 	if (!rq->timeout)
@@ -254,33 +251,30 @@
 		bio_unmap_user(bio, reading);
 
 	/* write to all output members */
-	hdr.status = rq->errors;	
-	hdr.masked_status = (hdr.status >> 1) & 0x1f;
-	hdr.msg_status = 0;
-	hdr.host_status = 0;
-	hdr.driver_status = 0;
-	hdr.info = 0;
-	if (hdr.masked_status || hdr.host_status || hdr.driver_status)
-		hdr.info |= SG_INFO_CHECK;
-	hdr.resid = rq->data_len;
-	hdr.duration = ((jiffies - start_time) * 1000) / HZ;
-	hdr.sb_len_wr = 0;
+	hdr->status = rq->errors;	
+	hdr->masked_status = (hdr->status >> 1) & 0x1f;
+	hdr->msg_status = 0;
+	hdr->host_status = 0;
+	hdr->driver_status = 0;
+	hdr->info = 0;
+	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
+		hdr->info |= SG_INFO_CHECK;
+	hdr->resid = rq->data_len;
+	hdr->duration = ((jiffies - start_time) * 1000) / HZ;
+	hdr->sb_len_wr = 0;
 
-	if (rq->sense_len && hdr.sbp) {
-		int len = min((unsigned int) hdr.mx_sb_len, rq->sense_len);
+	if (rq->sense_len && hdr->sbp) {
+		int len = min((unsigned int) hdr->mx_sb_len, rq->sense_len);
 
-		if (!copy_to_user(hdr.sbp, rq->sense, len))
-			hdr.sb_len_wr = len;
+		if (!copy_to_user(hdr->sbp, rq->sense, len))
+			hdr->sb_len_wr = len;
 	}
 
 	blk_put_request(rq);
 
-	if (copy_to_user(uptr, &hdr, sizeof(*uptr)))
-		goto out_buffer;
-
 	if (buffer) {
 		if (reading)
-			if (copy_to_user(hdr.dxferp, buffer, hdr.dxfer_len))
+			if (copy_to_user(hdr->dxferp, buffer, hdr->dxfer_len))
 				goto out_buffer;
 
 		kfree(buffer);
@@ -437,9 +431,71 @@
 		case SG_EMULATED_HOST:
 			err = sg_emulated_host(q, (int *) arg);
 			break;
-		case SG_IO:
-			err = sg_io(q, bdev, (struct sg_io_hdr *) arg);
+		case SG_IO: {
+			struct sg_io_hdr hdr;
+
+			if (copy_from_user(&hdr, (struct sg_io_hdr *) arg, sizeof(hdr))) {
+				err = -EFAULT;
+				break;
+			}
+			err = sg_io(q, bdev, &hdr);
+			if (copy_to_user((struct sg_io_hdr *) arg, &hdr, sizeof(hdr)))
+				err = -EFAULT;
+			break;
+		}
+		case CDROM_SEND_PACKET: {
+			struct cdrom_generic_command cgc;
+			struct sg_io_hdr hdr;
+
+			if (copy_from_user(&cgc, (struct cdrom_generic_command *) arg, sizeof(cgc))) {
+				err = -EFAULT;
+				break;
+			}
+			cgc.timeout = clock_t_to_jiffies(cgc.timeout);
+			memset(&hdr, 0, sizeof(hdr));
+			hdr.interface_id = 'S';
+			hdr.cmd_len = sizeof(cgc.cmd);
+			hdr.dxfer_len = cgc.buflen;
+			err = 0;
+			switch (cgc.data_direction) {
+				case CGC_DATA_UNKNOWN:
+					hdr.dxfer_direction = SG_DXFER_UNKNOWN;
+					break;
+				case CGC_DATA_WRITE:
+					hdr.dxfer_direction = SG_DXFER_TO_DEV;
+					break;
+				case CGC_DATA_READ:
+					hdr.dxfer_direction = SG_DXFER_FROM_DEV;
+					break;
+				case CGC_DATA_NONE:
+					hdr.dxfer_direction = SG_DXFER_NONE;
+					break;
+				default:
+					err = -EINVAL;
+			}
+			if (err)
+				break;
+
+			hdr.dxferp = cgc.buffer;
+			hdr.sbp = (char *) cgc.sense;
+			if (hdr.sbp)
+				hdr.mx_sb_len = sizeof(struct request_sense);
+			hdr.timeout = cgc.timeout;
+			hdr.cmdp = cgc.cmd;
+			hdr.cmd_len = sizeof(cgc.cmd);
+			err = sg_io(q, bdev, &hdr);
+
+			if (hdr.status)
+				err = -EIO;
+
+			cgc.stat = err;
+			cgc.buflen = hdr.resid;
+			if (copy_to_user((struct cdrom_generic_command *) arg, &cgc, sizeof(cgc)))
+				err = -EFAULT;
+
 			break;
+		}
+
 		/*
 		 * old junk scsi send command ioctl
 		 */
===== drivers/cdrom/cdrom.c 1.40 vs edited =====
--- 1.40/drivers/cdrom/cdrom.c	Tue Sep 23 21:18:06 2003
+++ edited/drivers/cdrom/cdrom.c	Sat Sep 27 14:24:59 2003
@@ -1856,57 +1856,6 @@
 	return cdo->generic_packet(cdi, &cgc);
 }
 
-static int cdrom_do_cmd(struct cdrom_device_info *cdi,
-			struct cdrom_generic_command *cgc)
-{
-	struct request_sense *usense, sense;
-	unsigned char *ubuf;
-	int ret;
-
-	if (cgc->data_direction == CGC_DATA_UNKNOWN)
-		return -EINVAL;
-
-	if (cgc->buflen < 0 || cgc->buflen >= 131072)
-		return -EINVAL;
-
-	usense = cgc->sense;
-	cgc->sense = &sense;
-	if (usense && !access_ok(VERIFY_WRITE, usense, sizeof(*usense))) {
-		return -EFAULT;
-	}
-
-	ubuf = cgc->buffer;
-	if (cgc->data_direction == CGC_DATA_READ ||
-	    cgc->data_direction == CGC_DATA_WRITE) {
-		cgc->buffer = kmalloc(cgc->buflen, GFP_KERNEL);
-		if (cgc->buffer == NULL)
-			return -ENOMEM;
-	}
-
-
-	if (cgc->data_direction == CGC_DATA_READ) {
-		if (!access_ok(VERIFY_READ, ubuf, cgc->buflen)) {
-			kfree(cgc->buffer);
-			return -EFAULT;
-		}
-	} else if (cgc->data_direction == CGC_DATA_WRITE) {
-		if (copy_from_user(cgc->buffer, ubuf, cgc->buflen)) {
-			kfree(cgc->buffer);
-			return -EFAULT;
-		}
-	}
-
-	ret = cdi->ops->generic_packet(cdi, cgc);
-	__copy_to_user(usense, cgc->sense, sizeof(*usense));
-	if (!ret && cgc->data_direction == CGC_DATA_READ)
-		__copy_to_user(ubuf, cgc->buffer, cgc->buflen);
-	if (cgc->data_direction == CGC_DATA_READ ||
-	    cgc->data_direction == CGC_DATA_WRITE) {
-		kfree(cgc->buffer);
-	}
-	return ret;
-}
-
 static int mmc_ioctl(struct cdrom_device_info *cdi, unsigned int cmd,
 		     unsigned long arg)
 {		
@@ -2176,14 +2125,6 @@
 		return 0;
 		}
 
-	case CDROM_SEND_PACKET: {
-		if (!CDROM_CAN(CDC_GENERIC_PACKET))
-			return -ENOSYS;
-		cdinfo(CD_DO_IOCTL, "entering CDROM_SEND_PACKET\n"); 
-		IOCTL_IN(arg, struct cdrom_generic_command, cgc);
-		cgc.timeout = clock_t_to_jiffies(cgc.timeout);
-		return cdrom_do_cmd(cdi, &cgc);
-		}
 	case CDROM_NEXT_WRITABLE: {
 		long next = 0;
 		cdinfo(CD_DO_IOCTL, "entering CDROM_NEXT_WRITABLE\n"); 

-- 
Jens Axboe

Re: CDROM_SEND_PACKET oddity

[Derek Foreman]

On Sun, 28 Sep 2003, Jens Axboe wrote:

> On Sat, Sep 27 2003, Derek Foreman wrote:
> > On Sat, 27 Sep 2003, Jens Axboe wrote:
> > 
> > -			memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));
> > +			hdr.cmdp = (unsigned char *)arg
> > +			         + offsetof(struct cdrom_generic_command, cmd);
> 
> No that's buggy, arg is a user pointer. It needs to read:
> 
> 			hdr.cmdp = cgc.cmd;

Actually, hdr.cmdp is expected to be a user pointer.  in sg_io we do

        rq->cmd_len = hdr->cmd_len;
        if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
                goto out_request;
        if (sizeof(rq->cmd) != hdr->cmd_len)

Re: CDROM_SEND_PACKET oddity

[Jens Axboe]

On Sun, Sep 28 2003, Derek Foreman wrote:
> On Sun, 28 Sep 2003, Jens Axboe wrote:
> 
> > On Sat, Sep 27 2003, Derek Foreman wrote:
> > > On Sat, 27 Sep 2003, Jens Axboe wrote:
> > > 
> > > -			memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));
> > > +			hdr.cmdp = (unsigned char *)arg
> > > +			         + offsetof(struct cdrom_generic_command, cmd);
> > 
> > No that's buggy, arg is a user pointer. It needs to read:
> > 
> > 			hdr.cmdp = cgc.cmd;
> 
> Actually, hdr.cmdp is expected to be a user pointer.  in sg_io we do
> 
>         rq->cmd_len = hdr->cmd_len;
>         if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
>                 goto out_request;
>         if (sizeof(rq->cmd) != hdr->cmd_len)

Ah you are right, I'd rather kill that too then like I removed user
sg_io_hdr pointer as well. It makes for less error handling in sg_io().

===== drivers/block/scsi_ioctl.c 1.34 vs edited =====
--- 1.34/drivers/block/scsi_ioctl.c	Fri Sep  5 12:22:31 2003
+++ edited/drivers/block/scsi_ioctl.c	Sun Sep 28 20:12:44 2003
@@ -25,6 +25,7 @@
 #include <linux/cdrom.h>
 #include <linux/slab.h>
 #include <linux/bio.h>
+#include <linux/times.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -140,40 +141,36 @@
 }
 
 static int sg_io(request_queue_t *q, struct block_device *bdev,
-		 struct sg_io_hdr *uptr)
+		 struct sg_io_hdr *hdr)
 {
 	unsigned long start_time;
 	int reading, writing;
-	struct sg_io_hdr hdr;
 	struct request *rq;
 	struct bio *bio;
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	void *buffer;
 
-	if (copy_from_user(&hdr, uptr, sizeof(*uptr)))
-		return -EFAULT;
-
-	if (hdr.interface_id != 'S')
+	if (hdr->interface_id != 'S')
 		return -EINVAL;
-	if (hdr.cmd_len > sizeof(rq->cmd))
+	if (hdr->cmd_len > sizeof(rq->cmd))
 		return -EINVAL;
 
 	/*
 	 * we'll do that later
 	 */
-	if (hdr.iovec_count)
+	if (hdr->iovec_count)
 		return -EOPNOTSUPP;
 
-	if (hdr.dxfer_len > (q->max_sectors << 9))
+	if (hdr->dxfer_len > (q->max_sectors << 9))
 		return -EIO;
 
 	reading = writing = 0;
 	buffer = NULL;
 	bio = NULL;
-	if (hdr.dxfer_len) {
-		unsigned int bytes = (hdr.dxfer_len + 511) & ~511;
+	if (hdr->dxfer_len) {
+		unsigned int bytes = (hdr->dxfer_len + 511) & ~511;
 
-		switch (hdr.dxfer_direction) {
+		switch (hdr->dxfer_direction) {
 		default:
 			return -EINVAL;
 		case SG_DXFER_TO_FROM_DEV:
@@ -191,8 +188,8 @@
 		 * 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,
-				   hdr.dxfer_len, reading);
+		bio = bio_map_user(bdev, (unsigned long) hdr->dxferp,
+				   hdr->dxfer_len, reading);
 
 		/*
 		 * if bio setup failed, fall back to slow approach
@@ -203,11 +200,11 @@
 				return -ENOMEM;
 
 			if (writing) {
-				if (copy_from_user(buffer, hdr.dxferp,
-						   hdr.dxfer_len))
+				if (copy_from_user(buffer, hdr->dxferp,
+						   hdr->dxfer_len))
 					goto out_buffer;
 			} else
-				memset(buffer, 0, hdr.dxfer_len);
+				memset(buffer, 0, hdr->dxfer_len);
 		}
 	}
 
@@ -216,11 +213,10 @@
 	/*
 	 * fill in request structure
 	 */
-	rq->cmd_len = hdr.cmd_len;
-	if (copy_from_user(rq->cmd, hdr.cmdp, hdr.cmd_len))
-		goto out_request;
-	if (sizeof(rq->cmd) != hdr.cmd_len)
-		memset(rq->cmd + hdr.cmd_len, 0, sizeof(rq->cmd) - hdr.cmd_len);
+	rq->cmd_len = hdr->cmd_len;
+	memcpy(rq->cmd, hdr->cmdp, hdr->cmd_len);
+	if (sizeof(rq->cmd) != hdr->cmd_len)
+		memset(rq->cmd + hdr->cmd_len, 0, sizeof(rq->cmd) - hdr->cmd_len);
 
 	memset(sense, 0, sizeof(sense));
 	rq->sense = sense;
@@ -234,9 +230,9 @@
 		blk_rq_bio_prep(q, rq, bio);
 
 	rq->data = buffer;
-	rq->data_len = hdr.dxfer_len;
+	rq->data_len = hdr->dxfer_len;
 
-	rq->timeout = (hdr.timeout * HZ) / 1000;
+	rq->timeout = (hdr->timeout * HZ) / 1000;
 	if (!rq->timeout)
 		rq->timeout = q->sg_timeout;
 	if (!rq->timeout)
@@ -254,33 +250,30 @@
 		bio_unmap_user(bio, reading);
 
 	/* write to all output members */
-	hdr.status = rq->errors;	
-	hdr.masked_status = (hdr.status >> 1) & 0x1f;
-	hdr.msg_status = 0;
-	hdr.host_status = 0;
-	hdr.driver_status = 0;
-	hdr.info = 0;
-	if (hdr.masked_status || hdr.host_status || hdr.driver_status)
-		hdr.info |= SG_INFO_CHECK;
-	hdr.resid = rq->data_len;
-	hdr.duration = ((jiffies - start_time) * 1000) / HZ;
-	hdr.sb_len_wr = 0;
+	hdr->status = rq->errors;	
+	hdr->masked_status = (hdr->status >> 1) & 0x1f;
+	hdr->msg_status = 0;
+	hdr->host_status = 0;
+	hdr->driver_status = 0;
+	hdr->info = 0;
+	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
+		hdr->info |= SG_INFO_CHECK;
+	hdr->resid = rq->data_len;
+	hdr->duration = ((jiffies - start_time) * 1000) / HZ;
+	hdr->sb_len_wr = 0;
 
-	if (rq->sense_len && hdr.sbp) {
-		int len = min((unsigned int) hdr.mx_sb_len, rq->sense_len);
+	if (rq->sense_len && hdr->sbp) {
+		int len = min((unsigned int) hdr->mx_sb_len, rq->sense_len);
 
-		if (!copy_to_user(hdr.sbp, rq->sense, len))
-			hdr.sb_len_wr = len;
+		if (!copy_to_user(hdr->sbp, rq->sense, len))
+			hdr->sb_len_wr = len;
 	}
 
 	blk_put_request(rq);
 
-	if (copy_to_user(uptr, &hdr, sizeof(*uptr)))
-		goto out_buffer;
-
 	if (buffer) {
 		if (reading)
-			if (copy_to_user(hdr.dxferp, buffer, hdr.dxfer_len))
+			if (copy_to_user(hdr->dxferp, buffer, hdr->dxfer_len))
 				goto out_buffer;
 
 		kfree(buffer);
@@ -289,8 +282,6 @@
 	/* may not have succeeded, but output values written to control
 	 * structure (struct sg_io_hdr).  */
 	return 0;
-out_request:
-	blk_put_request(rq);
 out_buffer:
 	kfree(buffer);
 	return -EFAULT;
@@ -437,9 +428,71 @@
 		case SG_EMULATED_HOST:
 			err = sg_emulated_host(q, (int *) arg);
 			break;
-		case SG_IO:
-			err = sg_io(q, bdev, (struct sg_io_hdr *) arg);
+		case SG_IO: {
+			struct sg_io_hdr hdr;
+
+			if (copy_from_user(&hdr, (struct sg_io_hdr *) arg, sizeof(hdr))) {
+				err = -EFAULT;
+				break;
+			}
+			err = sg_io(q, bdev, &hdr);
+			if (copy_to_user((struct sg_io_hdr *) arg, &hdr, sizeof(hdr)))
+				err = -EFAULT;
 			break;
+		}
+		case CDROM_SEND_PACKET: {
+			struct cdrom_generic_command cgc;
+			struct sg_io_hdr hdr;
+
+			if (copy_from_user(&cgc, (struct cdrom_generic_command *) arg, sizeof(cgc))) {
+				err = -EFAULT;
+				break;
+			}
+			cgc.timeout = clock_t_to_jiffies(cgc.timeout);
+			memset(&hdr, 0, sizeof(hdr));
+			hdr.interface_id = 'S';
+			hdr.cmd_len = sizeof(cgc.cmd);
+			hdr.dxfer_len = cgc.buflen;
+			err = 0;
+			switch (cgc.data_direction) {
+				case CGC_DATA_UNKNOWN:
+					hdr.dxfer_direction = SG_DXFER_UNKNOWN;
+					break;
+				case CGC_DATA_WRITE:
+					hdr.dxfer_direction = SG_DXFER_TO_DEV;
+					break;
+				case CGC_DATA_READ:
+					hdr.dxfer_direction = SG_DXFER_FROM_DEV;
+					break;
+				case CGC_DATA_NONE:
+					hdr.dxfer_direction = SG_DXFER_NONE;
+					break;
+				default:
+					err = -EINVAL;
+			}
+			if (err)
+				break;
+
+			hdr.dxferp = cgc.buffer;
+			hdr.sbp = (char *) cgc.sense;
+			if (hdr.sbp)
+				hdr.mx_sb_len = sizeof(struct request_sense);
+			hdr.timeout = cgc.timeout;
+			hdr.cmdp = cgc.cmd;
+			hdr.cmd_len = sizeof(cgc.cmd);
+			err = sg_io(q, bdev, &hdr);
+
+			if (hdr.status)
+				err = -EIO;
+
+			cgc.stat = err;
+			cgc.buflen = hdr.resid;
+			if (copy_to_user((struct cdrom_generic_command *) arg, &cgc, sizeof(cgc)))
+				err = -EFAULT;
+
+			break;
+		}
+
 		/*
 		 * old junk scsi send command ioctl
 		 */
===== drivers/cdrom/cdrom.c 1.40 vs edited =====
--- 1.40/drivers/cdrom/cdrom.c	Tue Sep 23 21:18:06 2003
+++ edited/drivers/cdrom/cdrom.c	Sat Sep 27 14:24:59 2003
@@ -1856,57 +1856,6 @@
 	return cdo->generic_packet(cdi, &cgc);
 }
 
-static int cdrom_do_cmd(struct cdrom_device_info *cdi,
-			struct cdrom_generic_command *cgc)
-{
-	struct request_sense *usense, sense;
-	unsigned char *ubuf;
-	int ret;
-
-	if (cgc->data_direction == CGC_DATA_UNKNOWN)
-		return -EINVAL;
-
-	if (cgc->buflen < 0 || cgc->buflen >= 131072)
-		return -EINVAL;
-
-	usense = cgc->sense;
-	cgc->sense = &sense;
-	if (usense && !access_ok(VERIFY_WRITE, usense, sizeof(*usense))) {
-		return -EFAULT;
-	}
-
-	ubuf = cgc->buffer;
-	if (cgc->data_direction == CGC_DATA_READ ||
-	    cgc->data_direction == CGC_DATA_WRITE) {
-		cgc->buffer = kmalloc(cgc->buflen, GFP_KERNEL);
-		if (cgc->buffer == NULL)
-			return -ENOMEM;
-	}
-
-
-	if (cgc->data_direction == CGC_DATA_READ) {
-		if (!access_ok(VERIFY_READ, ubuf, cgc->buflen)) {
-			kfree(cgc->buffer);
-			return -EFAULT;
-		}
-	} else if (cgc->data_direction == CGC_DATA_WRITE) {
-		if (copy_from_user(cgc->buffer, ubuf, cgc->buflen)) {
-			kfree(cgc->buffer);
-			return -EFAULT;
-		}
-	}
-
-	ret = cdi->ops->generic_packet(cdi, cgc);
-	__copy_to_user(usense, cgc->sense, sizeof(*usense));
-	if (!ret && cgc->data_direction == CGC_DATA_READ)
-		__copy_to_user(ubuf, cgc->buffer, cgc->buflen);
-	if (cgc->data_direction == CGC_DATA_READ ||
-	    cgc->data_direction == CGC_DATA_WRITE) {
-		kfree(cgc->buffer);
-	}
-	return ret;
-}
-
 static int mmc_ioctl(struct cdrom_device_info *cdi, unsigned int cmd,
 		     unsigned long arg)
 {		
@@ -2176,14 +2125,6 @@
 		return 0;
 		}
 
-	case CDROM_SEND_PACKET: {
-		if (!CDROM_CAN(CDC_GENERIC_PACKET))
-			return -ENOSYS;
-		cdinfo(CD_DO_IOCTL, "entering CDROM_SEND_PACKET\n"); 
-		IOCTL_IN(arg, struct cdrom_generic_command, cgc);
-		cgc.timeout = clock_t_to_jiffies(cgc.timeout);
-		return cdrom_do_cmd(cdi, &cgc);
-		}
 	case CDROM_NEXT_WRITABLE: {
 		long next = 0;
 		cdinfo(CD_DO_IOCTL, "entering CDROM_NEXT_WRITABLE\n"); 

-- 
Jens Axboe

Re: CDROM_SEND_PACKET oddity

[Derek Foreman]

On Sun, 28 Sep 2003, Jens Axboe wrote:

> On Sun, Sep 28 2003, Derek Foreman wrote:
> > On Sun, 28 Sep 2003, Jens Axboe wrote:
> > 
> > > On Sat, Sep 27 2003, Derek Foreman wrote:
> > > > On Sat, 27 Sep 2003, Jens Axboe wrote:
> > > > 
> > > > -			memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));
> > > > +			hdr.cmdp = (unsigned char *)arg
> > > > +			         + offsetof(struct cdrom_generic_command, cmd);
> > > 
> > > No that's buggy, arg is a user pointer. It needs to read:
> > > 
> > > 			hdr.cmdp = cgc.cmd;
> > 
> > Actually, hdr.cmdp is expected to be a user pointer.  in sg_io we do
> > 
> >         rq->cmd_len = hdr->cmd_len;
> >         if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
> >                 goto out_request;
> >         if (sizeof(rq->cmd) != hdr->cmd_len)
> 
> Ah you are right, I'd rather kill that too then like I removed user
> sg_io_hdr pointer as well. It makes for less error handling in sg_io().
> 

That one works great, thanks