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

Kristian Høgsberg
2007-05-09T21:08:15Z

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