lkml.org 
[lkml]   [2007]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5/6] firewire: SBP-2 highlevel driver
    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


    -
    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/

    \
     
     \ /
      Last update: 2007-05-09 23:11    [W:0.039 / U:0.136 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site