* Re: [patch 6/7] ps3: ROM Storage Driver [not found] ` <20070525083632.677742000@sonycom.com> @ 2007-05-29 10:49 ` Christoph Hellwig 2007-05-29 11:11 ` Geert Uytterhoeven 2007-05-29 16:21 ` Geert Uytterhoeven 0 siblings, 2 replies; 8+ messages in thread From: Christoph Hellwig @ 2007-05-29 10:49 UTC (permalink / raw) To: Geert.Uytterhoeven; +Cc: linuxppc-dev, linux-kernel, linux-scsi [Note that all scsi lldds should go to linux-scsi] > +config PS3_ROM > + tristate "PS3 ROM Storage Driver" > + depends on PPC_PS3 && BLK_DEV_SR > + select PS3_STORAGE > + default y please don't put any default y statements in. > +#define DEVICE_NAME "ps3rom" > + > +#define BOUNCE_SIZE (64*1024) > + > +#define PS3ROM_MAX_SECTORS (BOUNCE_SIZE / CD_FRAMESIZE) > + > +#define LV1_STORAGE_SEND_ATAPI_COMMAND (1) > + > + > +struct ps3rom_private { > + spinlock_t lock; > + struct task_struct *thread; > + struct Scsi_Host *host; > + struct scsi_cmnd *cmd; > + void (*scsi_done)(struct scsi_cmnd *); > +}; > +#define ps3rom_priv(dev) ((dev)->sbd.core.driver_data) > +/* > + * to position parameter > + */ > +enum { > + NOT_AVAIL = -1, > + USE_SRB_10 = -2, > + USE_SRB_6 = -3, > + USE_CDDA_FRAME_RAW = -4 > +}; none of these seem to be used at all in the driver. > + > +#ifdef DEBUG > +static const char *scsi_command(unsigned char cmd) > +{ > + switch (cmd) { > + case TEST_UNIT_READY: return "TEST_UNIT_READY/GPCMD_TEST_UNIT_READY"; > + case REZERO_UNIT: return "REZERO_UNIT"; > + case REQUEST_SENSE: return "REQUEST_SENSE/GPCMD_REQUEST_SENSE"; ... this kind of things shouldn't be in a low level driver. Either keep it in your out of tree debug patches or if you feel adventurous send a patch to linux-scsi that implements it in drivers/scsi/constant.c which has debug code for other protocol-level scsi constants. > +static int ps3rom_slave_alloc(struct scsi_device *scsi_dev) > +{ > + struct ps3_storage_device *dev; > + > + dev = (struct ps3_storage_device *)scsi_dev->host->hostdata[0]; > + > + dev_dbg(&dev->sbd.core, "%s:%u: id %u, lun %u, channel %u\n", __func__, > + __LINE__, scsi_dev->id, scsi_dev->lun, scsi_dev->channel); > + > + scsi_dev->hostdata = dev; This seems rather pointless. The scsi_device has a pointer to the host, so every access to scsi_dev->hostdata can simply be replaced by an access through the host. > +static int ps3rom_slave_configure(struct scsi_device *scsi_dev) > +{ > + struct ps3_storage_device *dev = scsi_dev->hostdata; > + > + dev_dbg(&dev->sbd.core, "%s:%u: id %u, lun %u, channel %u\n", __func__, > + __LINE__, scsi_dev->id, scsi_dev->lun, scsi_dev->channel); > + > + /* > + * ATAPI SFF8020 devices use MODE_SENSE_10, > + * so we can prohibit MODE_SENSE_6 > + */ > + scsi_dev->use_10_for_ms = 1; > + > + return 0; > +} > + > +static void ps3rom_slave_destroy(struct scsi_device *scsi_dev) > +{ > +} No need to implement an empty method here. > +static int ps3rom_queuecommand(struct scsi_cmnd *cmd, > + void (*done)(struct scsi_cmnd *)) > +{ > + struct ps3_storage_device *dev = cmd->device->hostdata; > + struct ps3rom_private *priv = ps3rom_priv(dev); > + > + dev_dbg(&dev->sbd.core, "%s:%u: command 0x%02x (%s)\n", __func__, > + __LINE__, cmd->cmnd[0], scsi_command(cmd->cmnd[0])); > + > + spin_lock_irq(&priv->lock); > + if (priv->cmd) { > + /* no more than one can be processed */ > + dev_err(&dev->sbd.core, "%s:%u: more than 1 command queued\n", > + __func__, __LINE__); > + spin_unlock_irq(&priv->lock); > + return SCSI_MLQUEUE_HOST_BUSY; > + } Just set can_queue to 1 in the host template and the midlayer will take care of this. > + > + // FIXME Prevalidate commands? > + priv->cmd = cmd; > + priv->scsi_done = done; No need to keep your own scsi_done pointer. What you should do instead in queuecommand is to set the scsi_done pointer in the scsi_cmnd here and just use it later. > + spin_unlock_irq(&priv->lock); > + wake_up_process(priv->thread); Offloading every I/O to a thread is very bad for I/O performance. Why do you need this? > + return 0; > +} > + > +/* > + * copy data from device into scatter/gather buffer > + */ > +static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf, > + int buflen) > +{ > + int k, req_len, act_len, len, active; > + void *kaddr; > + struct scatterlist *sgpnt; > + > + if (!cmd->request_bufflen) > + return 0; > + > + if (!cmd->request_buffer) > + return DID_ERROR << 16; > + > + if (cmd->sc_data_direction != DMA_BIDIRECTIONAL && > + cmd->sc_data_direction != DMA_FROM_DEVICE) > + return DID_ERROR << 16; > + > + if (!cmd->use_sg) { > + req_len = cmd->request_bufflen; > + act_len = min(req_len, buflen); > + memcpy(cmd->request_buffer, buf, act_len); > + cmd->resid = req_len - act_len; > + return 0; > + } This is never true anymore. > + > + sgpnt = cmd->request_buffer; > + active = 1; > + for (k = 0, req_len = 0, act_len = 0; k < cmd->use_sg; ++k, ++sgpnt) { > + if (active) { > + kaddr = kmap_atomic(sgpnt->page, KM_USER0); > + if (!kaddr) > + return DID_ERROR << 16; > + len = sgpnt->length; > + if ((req_len + len) > buflen) { > + active = 0; > + len = buflen - req_len; > + } > + memcpy(kaddr + sgpnt->offset, buf + req_len, len); > + kunmap_atomic(kaddr, KM_USER0); > + act_len += len; > + } > + req_len += sgpnt->length; > + } > + cmd->resid = req_len - act_len; This looks very inefficient. Just set sg_tablesize of your driver to 1 to avoid getting mutiple segments. > +static void ps3rom_request(struct ps3_storage_device *dev, > + struct scsi_cmnd *cmd) > +{ > + unsigned char opcode = cmd->cmnd[0]; > + struct ps3rom_private *priv = ps3rom_priv(dev); > + > + dev_dbg(&dev->sbd.core, "%s:%u: command 0x%02x (%s)\n", __func__, > + __LINE__, opcode, scsi_command(opcode)); > + > + switch (opcode) { > + case INQUIRY: > + ps3rom_atapi_request(dev, cmd, srb6_len(cmd), > + PIO_DATA_IN_PROTO, DIR_READ, 1); > + break; > + > + case REQUEST_SENSE: > + ps3rom_atapi_request(dev, cmd, srb6_len(cmd), > + PIO_DATA_IN_PROTO, DIR_READ, 0); > + break; > + > + case ALLOW_MEDIUM_REMOVAL: > + case START_STOP: > + case TEST_UNIT_READY: > + ps3rom_atapi_request(dev, cmd, 0, NON_DATA_PROTO, DIR_NA, 1); > + break; This switch statement looks very wrong. The data direction and length can easily be derived from the scsi_cmnd structure. > + default: > + dev_err(&dev->sbd.core, "%s:%u: illegal request 0x%02x (%s)\n", > + __func__, __LINE__, opcode, scsi_command(opcode)); > + cmd->result = DID_ERROR << 16; > + memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE); > + cmd->sense_buffer[0] = 0x70; > + cmd->sense_buffer[2] = ILLEGAL_REQUEST; Normally you should just hand down any command to the device, that's the whole point of the modular scsi protocol stack. > + struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core); > + struct ps3rom_private *priv; > + int error; > + struct Scsi_Host *host; > + struct task_struct *task; > + > + if (dev->blk_size != CD_FRAMESIZE) { > + dev_err(&dev->sbd.core, > + "%s:%u: cannot handle block size %lu\n", __func__, > + __LINE__, dev->blk_size); > + return -EINVAL; > + } > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; Normally you should allocate the private data using scsi_host_alloc, that's why it has a priv_size argument. > +static int ps3rom_remove(struct ps3_system_bus_device *_dev) > +{ > + struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core); > + struct ps3rom_private *priv = ps3rom_priv(dev); > + > + scsi_remove_host(priv->host); > + scsi_host_put(priv->host); > + kthread_stop(priv->thread); > + ps3stor_teardown(dev); > + kfree(dev->bounce_buf); > + kfree(priv); the scsi_host_put should come last. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 6/7] ps3: ROM Storage Driver 2007-05-29 10:49 ` [patch 6/7] ps3: ROM Storage Driver Christoph Hellwig @ 2007-05-29 11:11 ` Geert Uytterhoeven 2007-05-29 11:31 ` Benjamin Herrenschmidt 2007-05-30 10:13 ` Christoph Hellwig 2007-05-29 16:21 ` Geert Uytterhoeven 1 sibling, 2 replies; 8+ messages in thread From: Geert Uytterhoeven @ 2007-05-29 11:11 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linuxppc-dev, linux-kernel, linux-scsi On Tue, 29 May 2007, Christoph Hellwig wrote: > [Note that all scsi lldds should go to linux-scsi] I'll Cc linux-scsi next time. > > + sgpnt = cmd->request_buffer; > > + active = 1; > > + for (k = 0, req_len = 0, act_len = 0; k < cmd->use_sg; ++k, ++sgpnt) { > > + if (active) { > > + kaddr = kmap_atomic(sgpnt->page, KM_USER0); > > + if (!kaddr) > > + return DID_ERROR << 16; > > + len = sgpnt->length; > > + if ((req_len + len) > buflen) { > > + active = 0; > > + len = buflen - req_len; > > + } > > + memcpy(kaddr + sgpnt->offset, buf + req_len, len); > > + kunmap_atomic(kaddr, KM_USER0); > > + act_len += len; > > + } > > + req_len += sgpnt->length; > > + } > > + cmd->resid = req_len - act_len; > > This looks very inefficient. Just set sg_tablesize of your driver > to 1 to avoid getting mutiple segments. The disadvantage of setting sg_tablesize = 1 is that the driver will get small requests (PAGE_SIZE) most of the time, which is very bad for performance. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE) Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1 Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 6/7] ps3: ROM Storage Driver 2007-05-29 11:11 ` Geert Uytterhoeven @ 2007-05-29 11:31 ` Benjamin Herrenschmidt 2007-05-30 10:13 ` Christoph Hellwig 1 sibling, 0 replies; 8+ messages in thread From: Benjamin Herrenschmidt @ 2007-05-29 11:31 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Christoph Hellwig, linuxppc-dev, linux-kernel, linux-scsi On Tue, 2007-05-29 at 13:11 +0200, Geert Uytterhoeven wrote: > > This looks very inefficient. Just set sg_tablesize of your driver > > to 1 to avoid getting mutiple segments. > > The disadvantage of setting sg_tablesize = 1 is that the driver will > get small > requests (PAGE_SIZE) most of the time, which is very bad for > performance. And the joke is that not only the HW can do scatter & gather but you also have an iommu ... Ben. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 6/7] ps3: ROM Storage Driver 2007-05-29 11:11 ` Geert Uytterhoeven 2007-05-29 11:31 ` Benjamin Herrenschmidt @ 2007-05-30 10:13 ` Christoph Hellwig 2007-05-30 11:45 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2007-05-30 10:13 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Christoph Hellwig, linuxppc-dev, linux-kernel, linux-scsi On Tue, May 29, 2007 at 01:11:41PM +0200, Geert Uytterhoeven wrote: > > This looks very inefficient. Just set sg_tablesize of your driver > > to 1 to avoid getting mutiple segments. > > The disadvantage of setting sg_tablesize = 1 is that the driver will get small > requests (PAGE_SIZE) most of the time, which is very bad for performance. If you set .clustering = 1 in your host template you will frequently get larger requests. For any sane hypervisor or hardware the copy should be worth than that. Then again a sane hardware or hypervisor would support SG requests.. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 6/7] ps3: ROM Storage Driver 2007-05-30 10:13 ` Christoph Hellwig @ 2007-05-30 11:45 ` Benjamin Herrenschmidt 2007-05-30 17:18 ` Geoff Levand 0 siblings, 1 reply; 8+ messages in thread From: Benjamin Herrenschmidt @ 2007-05-30 11:45 UTC (permalink / raw) To: Christoph Hellwig Cc: Geert Uytterhoeven, linuxppc-dev, linux-scsi, linux-kernel On Wed, 2007-05-30 at 12:13 +0200, Christoph Hellwig wrote: > > For any sane hypervisor or hardware the copy should be worth > than that. Then again a sane hardware or hypervisor would support > SG requests.. Agreed... Sony should fix that, it's a bit ridiculous. Ben. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 6/7] ps3: ROM Storage Driver 2007-05-30 11:45 ` Benjamin Herrenschmidt @ 2007-05-30 17:18 ` Geoff Levand 0 siblings, 0 replies; 8+ messages in thread From: Geoff Levand @ 2007-05-30 17:18 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Christoph Hellwig, Geert Uytterhoeven, linuxppc-dev, linux-kernel, linux-scsi Benjamin Herrenschmidt wrote: > On Wed, 2007-05-30 at 12:13 +0200, Christoph Hellwig wrote: >> >> For any sane hypervisor or hardware the copy should be worth >> than that. Then again a sane hardware or hypervisor would support >> SG requests.. > > Agreed... Sony should fix that, it's a bit ridiculous. Yes, if only to put an end to seeing this kind of comment over and over again. -Geoff ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 6/7] ps3: ROM Storage Driver 2007-05-29 10:49 ` [patch 6/7] ps3: ROM Storage Driver Christoph Hellwig 2007-05-29 11:11 ` Geert Uytterhoeven @ 2007-05-29 16:21 ` Geert Uytterhoeven 2007-05-30 10:01 ` Christoph Hellwig 1 sibling, 1 reply; 8+ messages in thread From: Geert Uytterhoeven @ 2007-05-29 16:21 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linuxppc-dev, linux-kernel, linux-scsi On Tue, 29 May 2007, Christoph Hellwig wrote: > > +/* > > + * copy data from device into scatter/gather buffer > > + */ > > +static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf, > > + int buflen) > > +{ > > + int k, req_len, act_len, len, active; > > + void *kaddr; > > + struct scatterlist *sgpnt; > > + > > + if (!cmd->request_bufflen) > > + return 0; > > + > > + if (!cmd->request_buffer) > > + return DID_ERROR << 16; > > + > > + if (cmd->sc_data_direction != DMA_BIDIRECTIONAL && > > + cmd->sc_data_direction != DMA_FROM_DEVICE) > > + return DID_ERROR << 16; > > + > > + if (!cmd->use_sg) { > > + req_len = cmd->request_bufflen; > > + act_len = min(req_len, buflen); > > + memcpy(cmd->request_buffer, buf, act_len); > > + cmd->resid = req_len - act_len; > > + return 0; > > + } > > This is never true anymore. Just to be sure: all four if-cases or only the last one? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE) Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1 Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch 6/7] ps3: ROM Storage Driver 2007-05-29 16:21 ` Geert Uytterhoeven @ 2007-05-30 10:01 ` Christoph Hellwig 0 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2007-05-30 10:01 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Christoph Hellwig, linuxppc-dev, linux-kernel, linux-scsi On Tue, May 29, 2007 at 06:21:36PM +0200, Geert Uytterhoeven wrote: > On Tue, 29 May 2007, Christoph Hellwig wrote: > > > +/* > > > + * copy data from device into scatter/gather buffer > > > + */ > > > +static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf, > > > + int buflen) > > > +{ > > > + int k, req_len, act_len, len, active; > > > + void *kaddr; > > > + struct scatterlist *sgpnt; > > > + > > > + if (!cmd->request_bufflen) > > > + return 0; > > > + > > > + if (!cmd->request_buffer) > > > + return DID_ERROR << 16; > > > + > > > + if (cmd->sc_data_direction != DMA_BIDIRECTIONAL && > > > + cmd->sc_data_direction != DMA_FROM_DEVICE) > > > + return DID_ERROR << 16; > > > + > > > + if (!cmd->use_sg) { > > > + req_len = cmd->request_bufflen; > > > + act_len = min(req_len, buflen); > > > + memcpy(cmd->request_buffer, buf, act_len); > > > + cmd->resid = req_len - act_len; > > > + return 0; > > > + } > > > > This is never true anymore. > > Just to be sure: all four if-cases or only the last one? That's just in reference to the last one. The checks above could be condensed a little more aswell, but I'll comment on further in the second round of review, in the hope that the command submission path is a lot more streamline by then already. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2007-05-30 17:37 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20070525083607.784351000@sonycom.com>
[not found] ` <20070525083632.677742000@sonycom.com>
2007-05-29 10:49 ` [patch 6/7] ps3: ROM Storage Driver Christoph Hellwig
2007-05-29 11:11 ` Geert Uytterhoeven
2007-05-29 11:31 ` Benjamin Herrenschmidt
2007-05-30 10:13 ` Christoph Hellwig
2007-05-30 11:45 ` Benjamin Herrenschmidt
2007-05-30 17:18 ` Geoff Levand
2007-05-29 16:21 ` Geert Uytterhoeven
2007-05-30 10:01 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).