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

["Kristian Høgsberg" <krh@redhat.com>]

Christoph Hellwig wrote:
>> +	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)

Yup, done.

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

As Stefan, said, dma_map_sg() breaks the limit guarantee, so we have to split 
things manually if sg entries got merged.  I've added a comment explaining this.

Isn't max_sectors the overall size limit of the request, though?  The SBP-2 
protocol imposes a maximum size of 65535 bytes per sg entry, but the total 
size of a request can be larger.  I guess, setting dma_boundary to 2^15 could 
work.

>> +	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())

Done.

>> +	orb = kzalloc(sizeof *orb, GFP_ATOMIC);
> 
> Normal kernel style is sizeof(*orb)

Oh, hmm... I though the kernel style typically was to avoid excess parens :) 
But, sure, I see the comment about preferring parens with sizeof in CodingStyle.

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

That should simplify the code a bit.  How long has that been the case?

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

Ok, I've changed that.

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

Ok, I'll patch the scsi host template definition.

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

I was trying to be clever and only allocate the host once the device had been 
discovered and initialized.  I have now changed the code to just allocate the 
host up front and use the hostdata mechanism for the sbp2_device struct, which 
also addresses the host life cycle comments below.

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

Stefan addressed this one.

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

Now fixed as described above.

Thanks for the review, will send out new patches.
Kristian