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

[Muli Ben-Yehuda]

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.

> +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?

> +}
> +
>  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?

Cheers,
Muli