Re: CDROM_SEND_PACKET oddity
From: Jens Axboe
Date: Sat Sep 27 2003 - 07:28:23 EST
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/