lkml.org 
[lkml]   [2007]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5/6] firewire: SBP-2 highlevel driver
    > +	sg = (struct scatterlist *)orb->cmd->request_buffer;
    > + count = dma_map_sg(device->card->device, sg, orb->cmd->use_sg,
    > + orb->cmd->sc_data_direction);

    you need to handle the error case (count == 0)

    > + /* Convert the scatterlist to an sbp2 page table. If any
    > + * scatterlist entries are too big for sbp2 we split the as we go. */

    Please set the max_sectors value in your host template so that the
    block layer doesn't build sg entries too big for you.

    > + orb->page_table_bus =
    > + dma_map_single(device->card->device, orb->page_table,
    > + size, DMA_TO_DEVICE);

    This needs handling of mapping errors (dma_mapping_error())

    > + orb = kzalloc(sizeof *orb, GFP_ATOMIC);

    Normal kernel style is sizeof(*orb)

    > + if (cmd->use_sg) {
    > + sbp2_command_orb_map_scatterlist(orb);
    > + } else if (cmd->request_bufflen > SBP2_MAX_SG_ELEMENT_LENGTH) {
    > + /* FIXME: Need to split this into a sg list... but
    > + * could we get the scsi or blk layer to do that by
    > + * reporting our max supported block size? */
    > + fw_error("command > 64k\n");
    > + goto fail_bufflen;
    > + } else if (cmd->request_bufflen > 0) {
    > + sbp2_command_orb_map_buffer(orb);
    > + }

    The use_sg == 0, request_bufflen != 0 case can't happen anymore.

    > + fail_mapping:
    > + kfree(orb);
    > + fail_alloc:
    > + cmd->result = DID_ERROR << 16;
    > + done(cmd);

    Failure due to ressource shortage should not complete the command
    but return SCSI_MLQUEUE_HOST_BUSY/SCSI_MLQUEUE_DEVICE_BUSY.

    > + return 0;
    > +}

    > +static struct scsi_host_template scsi_driver_template = {
    > + .module = THIS_MODULE,
    > + .name = "SBP-2 IEEE-1394",
    > + .proc_name = (char *)sbp2_driver_name,

    Please don't use casrs here. Either fix up the definition so it
    accepts const strings or pass a non-const one.

    > +static int add_scsi_devices(struct fw_unit *unit)
    > +{
    > + struct sbp2_device *sd = unit->device.driver_data;
    > + int retval, lun;
    > +
    > + if (sd->scsi_host != NULL)
    > + return 0;
    > +
    > + sd->scsi_host = scsi_host_alloc(&scsi_driver_template,
    > + sizeof(unsigned long));
    > + if (sd->scsi_host == NULL) {
    > + fw_error("failed to register scsi host\n");
    > + return -1;
    > + }
    > +
    > + sd->scsi_host->hostdata[0] = (unsigned long)unit;

    Please take a look ar ther other scsi drivers how this is supposed
    to be used.

    > + retval = scsi_add_host(sd->scsi_host, &unit->device);
    > + if (retval < 0) {
    > + fw_error("failed to add scsi host\n");
    > + scsi_host_put(sd->scsi_host);
    > + sd->scsi_host = NULL;
    > + return retval;
    > + }
    > +
    > + /* FIXME: Loop over luns here. */
    > + lun = 0;
    > + retval = scsi_add_device(sd->scsi_host, 0, 0, lun);
    > + if (retval < 0) {
    > + fw_error("failed to add scsi device\n");
    > + scsi_remove_host(sd->scsi_host);
    > + scsi_host_put(sd->scsi_host);
    > + sd->scsi_host = NULL;
    > + return retval;
    > + }
    > +
    > + return 0;
    > +}

    Do we really need another scanning algorithm? Can't you use
    scsi_scan_target instead and let the core scsi code handle the
    scanning?

    > +
    > +static void remove_scsi_devices(struct fw_unit *unit)
    > +{
    > + struct sbp2_device *sd = unit->device.driver_data;
    > +
    > + if (sd->scsi_host != NULL) {
    > + scsi_remove_host(sd->scsi_host);
    > + scsi_host_put(sd->scsi_host);
    > + }
    > + sd->scsi_host = NULL;
    > +}

    This function seems rather oddly named. And the checking and
    setting of scsi_host looks like you have some lifetime rule
    problems.

    -
    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-02 21:47    [W:0.040 / U:0.832 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site