lkml.org 
[lkml]   [2007]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 6/7] ps3: ROM Storage Driver
    [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-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/

    \
     
     \ /
      Last update: 2007-05-29 12:53    [W:0.038 / U:0.348 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site