Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S262433AbTI0M1N (ORCPT ); Sat, 27 Sep 2003 08:27:13 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S262436AbTI0M1N (ORCPT ); Sat, 27 Sep 2003 08:27:13 -0400 Received: from ns.virtualhost.dk ([195.184.98.160]:57016 "EHLO virtualhost.dk") by vger.kernel.org with ESMTP id S262433AbTI0M05 (ORCPT ); Sat, 27 Sep 2003 08:26:57 -0400 Date: Sat, 27 Sep 2003 14:27:03 +0200 From: Jens Axboe To: Derek Foreman Cc: linux-kernel@vger.kernel.org Subject: Re: CDROM_SEND_PACKET oddity Message-ID: <20030927122703.GK3416@suse.de> References: <20030927114712.GJ3416@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20030927114712.GJ3416@suse.de> Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org 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 #include #include +#include #include #include @@ -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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/