[openib-general] [PATCH 04/12] SRP: Changing ibsrpdm

Ishai Rabinovitz ishai at mellanox.co.il
Mon May 1 12:04:44 PDT 2006


On Mon, May 01, 2006 at 04:43:23PM +0300, Muli Ben-Yehuda wrote:
> On Mon, May 01, 2006 at 02:27:39PM +0300, Ishai Rabinovitz wrote:
> > 
> > Do not add the same target twice.
> > 
> > Signed-off-by: Ishai Rabinovitz <ishai at mellanox.co.il>
> > Index: last_stable/drivers/infiniband/ulp/srp/ib_srp.c
> > ===================================================================
> > --- last_stable.orig/drivers/infiniband/ulp/srp/ib_srp.c	2006-04-25 15:17:34.000000000 +0300
> > +++ last_stable/drivers/infiniband/ulp/srp/ib_srp.c	2006-04-25 15:19:37.000000000 +0300
> > @@ -1478,7 +1478,8 @@ static int srp_parse_options(const char 
> >  				printk(KERN_WARNING PFX "bad max sect parameter '%s'\n", p);
> >  				goto out;
> >  			}
> > -			target->scsi_host->max_sectors = token;
> > +			if (target->scsi_host != NULL)
> > +				target->scsi_host->max_sectors = token;
> >  			break;
> 
> This chunk does not look related to the rest. Is a NULL
> target->scsi_host legal here? if not, the check should be removed as
> we'd rather take an oops here than hide the problem behind the NULL
> pointer check.
> 
> > +/* srp_find_target - If the target exists return it in target,
> > +		      otherwise target is set to NULL.
> > +		      host->target_mutex should be hold */
> 
> Please use the usual kernel
> /*
>  * stuff
>  */
> style for multi line comments.

OK, Thanks.
> 
> > +static int srp_find_target(const char *buf, struct srp_host *host,
> > +			   struct srp_target_port **target)
> > +{
> > +	struct srp_target_port *target_to_find, *curr_target;
> > +	int ret, i;
> > +
> > +	target_to_find = kzalloc(sizeof *target_to_find, GFP_KERNEL);
> > +	ret = srp_parse_options(buf, target_to_find);
> > +	if (ret)
> > +	        goto free;
> > +
> > +	list_for_each_entry(curr_target, &host->target_list, list)
> > +	        if (target_to_find->ioc_guid == curr_target->ioc_guid &&
> > +		    target_to_find->id_ext == curr_target->id_ext &&
> > +		    target_to_find->path.pkey == curr_target->path.pkey &&
> > +		    target_to_find->service_id == curr_target->service_id) {
> > +		        for (i = 0; i < 16; ++i)
> > +		                if (target_to_find->path.dgid.raw[i] != curr_target->path.dgid.raw[i])
> > +				        break;
> 
> The conditional and check here probably deserves an inline helper
> called same_target() or some such.
> 
> > +			if (i == 16) {
> > +				*target = curr_target;
> > +				goto free;
> > +			}
> > +		}
> > +
> > +	*target = NULL;
> > +
> > +free:
> > +	kfree(target_to_find);
> > +	return 0;
> 
> We always return 0 - either this should return void, or you meant to
> return ret here instead of 0?

You are right as usual, We should return ret.
> 
> > +}
> > +
> >  static ssize_t srp_create_target(struct class_device *class_dev,
> >  				 const char *buf, size_t count)
> >  {
> >  	struct srp_host *host =
> >  		container_of(class_dev, struct srp_host, class_dev);
> >  	struct Scsi_Host *target_host;
> > -	struct srp_target_port *target;
> > +	struct srp_target_port *target, *existing_target = NULL;
> >  	int ret;
> >  	int i;
> >  
> > +	/* first check if the target already exists */
> > +
> > +	mutex_lock(&host->target_mutex);
> > +	ret = srp_find_target(buf, host, &existing_target);
> > +	if (ret)
> > +		goto unlock_mutex;
> > +
> > +	if (existing_target) {
> > +		/* target already exists */
> > +		spin_lock_irq(existing_target->scsi_host->host_lock);
> 
> why _irq and not _irqsave? Are you sure this code can't ever be called
> with interrupts off via some other path?

This function is being called from userspace 
(writing to /sys/class/infiniband_srp/.../add_target) so no need for irqsave.

Do you think we should always use irqsave just to be on the safe side (Maybe
in the future someone else will call us)? 
> 
> Cheers,
> Muli

---------------  Resending the fixed patch ----------------------
-----------------------------------------------------------------

Do not add the same target twice.

Signed-off-by: Ishai Rabinovitz <ishai at mellanox.co.il>
Index: last_stable/drivers/infiniband/ulp/srp/ib_srp.c
===================================================================
--- last_stable.orig/drivers/infiniband/ulp/srp/ib_srp.c	2006-04-25 15:17:34.000000000 +0300
+++ last_stable/drivers/infiniband/ulp/srp/ib_srp.c	2006-04-25 15:19:37.000000000 +0300
@@ -1478,7 +1478,8 @@ static int srp_parse_options(const char 
 				printk(KERN_WARNING PFX "bad max sect parameter '%s'\n", p);
 				goto out;
 			}
-			target->scsi_host->max_sectors = token;
+			if (target->scsi_host != NULL)
+				target->scsi_host->max_sectors = token;
 			break;
 
 		default:
@@ -1503,20 +1504,92 @@ out:
 	return ret;
 }
 
+/* 
+ *	srp_find_target - If the target exists return it in target,
+ *			  otherwise target is set to NULL.
+ *			  host->target_mutex should be hold 
+ */
+static int srp_find_target(const char *buf, struct srp_host *host,
+			   struct srp_target_port **target)
+{
+	struct srp_target_port *target_to_find, *curr_target;
+	int ret, i;
+
+	target_to_find = kzalloc(sizeof *target_to_find, GFP_KERNEL);
+	ret = srp_parse_options(buf, target_to_find);
+	if (ret)
+	        goto free;
+
+	list_for_each_entry(curr_target, &host->target_list, list)
+	        if (target_to_find->ioc_guid == curr_target->ioc_guid &&
+		    target_to_find->id_ext == curr_target->id_ext &&
+		    target_to_find->path.pkey == curr_target->path.pkey &&
+		    target_to_find->service_id == curr_target->service_id) {
+		        for (i = 0; i < 16; ++i)
+		                if (target_to_find->path.dgid.raw[i] !=
+				    curr_target->path.dgid.raw[i])
+				        break;
+			if (i == 16) {
+				*target = curr_target;
+				goto free;
+			}
+		}
+
+	*target = NULL;
+
+free:
+	kfree(target_to_find);
+	return ret;
+}
+
 static ssize_t srp_create_target(struct class_device *class_dev,
 				 const char *buf, size_t count)
 {
 	struct srp_host *host =
 		container_of(class_dev, struct srp_host, class_dev);
 	struct Scsi_Host *target_host;
-	struct srp_target_port *target;
+	struct srp_target_port *target, *existing_target = NULL;
 	int ret;
 	int i;
 
+	/* first check if the target already exists */
+
+	mutex_lock(&host->target_mutex);
+	ret = srp_find_target(buf, host, &existing_target);
+	if (ret)
+		goto unlock_mutex;
+
+	if (existing_target) {
+		/* target already exists */
+		spin_lock_irq(existing_target->scsi_host->host_lock);
+		switch (existing_target->state) {
+		case SRP_TARGET_LIVE:
+			printk(KERN_WARNING PFX "target %s already exists\n",
+			       buf);
+			ret = -EEXIST;
+			break;
+		case SRP_TARGET_CONNECTING:
+		        /* It is in the middle of reconnecting */
+			ret = -EALREADY;
+			break;
+		case SRP_TARGET_DEAD:
+		        /* It will be removed soon - create a new one */
+		case SRP_TARGET_REMOVED:
+			/* target is dead, create a new one */
+			break;
+		}
+ 		spin_unlock_irq(existing_target->scsi_host->host_lock);
+		if (ret)
+			goto unlock_mutex;
+	}
+
+	/* really create the target */
 	target_host = scsi_host_alloc(&srp_template,
 				      sizeof (struct srp_target_port));
-	if (!target_host)
-		return -ENOMEM;
+	if (!target_host) {
+		ret = -ENOMEM;
+		goto unlock_mutex;
+	}
 
 	target_host->max_lun = SRP_MAX_LUN;
 
@@ -1533,7 +1603,7 @@ static ssize_t srp_create_target(struct 
 
 	ret = srp_parse_options(buf, target);
 	if (ret)
-		goto err;
+		goto err_put_scsi_host;
 
 	ib_get_cached_gid(host->dev, host->port, 0, &target->path.sgid);
 
@@ -1554,7 +1624,7 @@ static ssize_t srp_create_target(struct 
 
 	ret = srp_create_target_ib(target);
 	if (ret)
-		goto err;
+		goto err_put_scsi_host;
 
 	target->cm_id = ib_create_cm_id(host->dev, srp_cm_handler, target);
 	if (IS_ERR(target->cm_id)) {
@@ -1572,7 +1642,8 @@ static ssize_t srp_create_target(struct 
 	if (ret)
 		goto err_disconnect;
 
-	return count;
+	ret = count;
+	goto unlock_mutex;
 
 err_disconnect:
 	srp_disconnect_target(target);
@@ -1583,9 +1654,12 @@ err_cm_id:
 err_free:
 	srp_free_target_ib(target);
 
-err:
+err_put_scsi_host:
 	scsi_host_put(target_host);
 
+unlock_mutex:
+	mutex_unlock(&host->target_mutex);
+
 	return ret;
 }
 

-- 
Ishai Rabinovitz



More information about the general mailing list