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