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 <[EMAIL PROTECTED]>
> 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
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
