On Mon, 27 February 2012 12:12:54 +0000, Nicholas A. Bellinger wrote: > > @@ -885,11 +851,11 @@ void qla_tgt_fc_port_added(struct scsi_qla_host *vha, fc_port_t *fcport) > mutex_lock(&ha->tgt_mutex); > sess = qla_tgt_create_sess(vha, fcport, false); > mutex_unlock(&ha->tgt_mutex); > - > - spin_lock_irqsave(&ha->hardware_lock, flags); > /* put the extra creation ref */ > if (sess != NULL) > - __qla_tgt_sess_put(sess); > + if (ha->tgt_ops->put_sess(sess) != 0) > + return; > + spin_lock_irqsave(&ha->hardware_lock, flags); Can we get rid of the " != 0"? It does not add anything logically an to me personally it actually makes the code harder to read. Also, there is a subtle bug here. We used to have the second refcount to protect ourselves against freeing the session before we re-took ha->hardware_lock. Now we drop the second refcount, which, about half an hour of double-checking the code, is perfectly fine. In fact, there is no point in having the extra refcount at all anymore. Except that you snuck in the tiny semantic change of returning from this function in case the put_sess happens to drop the last reference. Now, if it makes sense to do so, we are racy here. But then again, if it makes sense to do so, it would have made sense with or without the conversion and your semantic change does not belong here in the first place. Jörn -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
