Messages in this thread | | | Date | Wed, 2 May 2007 20:44:08 +0100 | From | Christoph Hellwig <> | Subject | Re: [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/
| |