[openib-general] [PATCH] srp tidyups

[Christoph Hellwig]

 - you must not call the scsi_done callback in the EH routines.
   Will there be real error handling for srp one day?
 - given that you have on scsi host per srp target there's no need
   for the idr lookup, you can just use the host private data
 - no need to fill out unique_id, that's a leftover for ISA HBAs
 - and some small cleanups I couldn't resist


Index: ulp/srp/ib_srp.c
===================================================================
--- ulp/srp/ib_srp.c	(revision 3129)
+++ ulp/srp/ib_srp.c	(working copy)
@@ -34,14 +34,11 @@
 
 #include <linux/version.h>
 #include <linux/module.h>
-
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/err.h>
-#include <linux/idr.h>
 #include <linux/string.h>
 #include <linux/parser.h>
-
 #include <asm/atomic.h>
 
 #include <scsi/scsi.h>
@@ -69,11 +66,6 @@
 
 static const u8 topspin_oui[3] = { 0x00, 0x05, 0xad };
 
-static atomic_t srp_uid;
-
-static rwlock_t idr_lock;
-static DEFINE_IDR(target_idr);
-
 static void srp_add_one(struct ib_device *device)et 
 static void srp_remove_one(struct ib_device *device);
 
@@ -101,27 +93,29 @@
 
 	iu = kmalloc(sizeof *iu, gfp_mask);
 	if (!iu)
-		return NULL;
+		goto out;
 
 	iu->buf = kmalloc(size, gfp_mask);
-	if (!iu->buf) {
-		kfree(iu);
-		return NULL;
-	}
+	if (!iu->buf)
+		goto out_free_iu;
 
 	memset(iu->buf, 0, size);
 
 	iu->dma = dma_map_single(host->dev->dma_device, iu->buf, size, direction);
-	if (dma_mapping_error(iu->dma)) {
-		kfree(iu->buf);
-		kfree(iu);
-		return NULL;
-	}
+	if (dma_mapping_error(iu->dma))
+		goto out_free_buf;
 
 	iu->size      = size;
 	iu->direction = direction;
 
 	return iu;
+
+ out_free_buf:
+	kfree(iu->buf);
+ out_free_iu:
+	kfree(iu);
+ out:
+	return NULL;
 }
 
 static void srp_free_iu(struct srp_host *host, struct srp_iu *iu)
@@ -142,7 +136,7 @@
 	u8 fmt;
 
 	if (!scmnd->request_buffer || scmnd->sc_data_direction == DMA_NONE)
-		return sizeof (struct srp_cmd);
+		return sizeof(struct srp_cmd);
 
 	if (scmnd->sc_data_direction != DMA_FROM_DEVICE &&
 	    scmnd->sc_data_direction != DMA_TO_DEVICE) {
@@ -456,31 +450,11 @@
 static int srp_queuecommand(struct scsi_cmnd *scmnd,
 			    void (*done)(struct scsi_cmnd *))
 {
-	struct srp_target_port *target;
-	struct srp_iu *iu;
+	struct srp_target_port *target = host_to_target(scmnd->device->host);
+	struct srp_iu *iu = target->tx_ring[target->tx_head & SRP_SQ_SIZE];
 	struct srp_cmd *cmd;
-	unsigned long flags;
 	int len;
 
-	read_lock_irqsave(&idr_lock, flags);
-	target = idr_find(&target_idr, scmnd->device->id);
-	read_unlock_irqrestore(&idr_lock, flags);
-
-	if (!target) {
-		printk(KERN_ERR PFX "queuecommand for unknown device id %d\n",
-		       scmnd->device->id);
-		scmnd->result = DID_ERROR << 16;
-		done(scmnd);
-		return 0;
-	}
-
-	if (0) {
-		printk(KERN_ERR PFX "command for %u: ", scmnd->device->id);
-		scsi_print_command(scmnd);
-	}
-
-	iu = target->tx_ring[target->tx_head & SRP_SQ_SIZE];
-
 	dma_sync_single_for_cpu(target->srp_host->dev->dma_device, iu->dma,
 				SRP_MAX_IU_LEN, DMA_TO_DEVICE);
 
@@ -499,7 +473,7 @@
 	len = srp_map_data(scmnd, target, iu);
 	if (len < 0) {
 		printk(KERN_ERR PFX "Failed to map data\n");
-		goto err;
+		goto err_free_iu;
 	}
 
 	if (srp_post_recv(target, GFP_ATOMIC)) {
@@ -519,8 +493,7 @@
 
 err_unmap:
 	srp_unmap_data(scmnd, target, cmd);
-
-err:
+err_free_iu:
 	return SCSI_MLQUEUE_HOST_BUSY;
 }
 
@@ -528,9 +501,6 @@
 {
 	printk(KERN_ERR PFX "srp_abort called\n");
 
-        scmnd->result = DID_ABORT << 16;
-	scmnd->scsi_done(scmnd);
-
 	return SUCCESS;
 }
 
@@ -538,9 +508,6 @@
 {
 	printk(KERN_ERR PFX "srp_reset called\n");
 
-        scmnd->result = DID_ABORT << 16;
-	scmnd->scsi_done(scmnd);
-
 	return SUCCESS;
 }
 
@@ -892,10 +859,7 @@
 static void srp_release_target(struct srp_target_port *target)
 {
 	int i;
-	unsigned long flags;
 
-	/* XXX should send SRP_I_LOGOUT request */
-
 	init_completion(&target->done);
 	ib_send_cm_dreq(target->cm_id, NULL, 0);
 	wait_for_completion(&target->done);
@@ -907,10 +871,6 @@
 		srp_free_iu(target->srp_host, target->rx_ring[i]);
 	for (i = 0; i < SRP_SQ_SIZE + 1; ++i)
 		srp_free_iu(target->srp_host, target->tx_ring[i]);
-
-	write_lock_irqsave(&idr_lock, flags);
-	idr_remove(&target_idr, target->scsi_id);
-	write_unlock_irqrestore(&idr_lock, flags);
 }
 
 static struct scsi_host_template srp_template = {
@@ -933,21 +893,8 @@
 	unsigned long flags;
 	int ret;
 
-	do {
-		if (!idr_pre_get(&target_idr, GFP_KERNEL))
-			return -ENOMEM;
-
-		write_lock_irqsave(&idr_lock, flags);
-		ret = idr_get_new(&target_idr, target, &target->scsi_id);
-		write_unlock_irqrestore(&idr_lock, flags);
-	} while (ret == -EAGAIN);
-
-	if (ret)
-		goto fail;
-
 	sprintf(target->target_name, "SRP.T10:%016llX",
 		 (unsigned long long) be64_to_cpu(target->id_ext));
-	target->scsi_host->unique_id = atomic_inc_return(&srp_uid);
 
 	if (scsi_add_host(target->scsi_host, host->dev->dma_device))
 		goto fail;
@@ -963,11 +910,7 @@
 	return 0;
 
 fail:
-	write_lock_irqsave(&idr_lock, flags);
-	idr_remove(&target_idr, target->scsi_id);
-	write_unlock_irqrestore(&idr_lock, flags);
-
-	return ret;
+	return -ENODEV;
 }
 
 static void srp_release_class_dev(struct class_device *class_dev)
@@ -1366,9 +1309,6 @@
 {
 	int ret;
 
-	atomic_set(&srp_uid, 0);
-	rwlock_init(&idr_lock);
-
 	ret = class_register(&srp_class);
 	if (ret) {
 		printk(KERN_ERR PFX "couldn't register class infiniband_srp\n");