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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
