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

[Christoph Hellwig <hch@infradead.org>]

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