Re: [PATCH 5/6] firewire: SBP-2 highlevel driver

Christoph Hellwig
2007-05-02T19:44:17Z

> + 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.