[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. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html