Re: [PATCH v2 08/12] qla2xxx: Add framework for Async fabric discovery.

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

On Mon, 2017-01-16 at 12:35 -0800, Himanshu Madhani wrote:
>  /*
>   * Adds an extra ref to allow to drop hw lock after adding sess to the list.
>   * Caller must put it.
> @@ -839,93 +1103,65 @@ static struct fc_port *qlt_create_sess(
>  	bool local)
>  {
>  	struct qla_hw_data *ha = vha->hw;
> -	struct fc_port *sess;
> +	struct fc_port *sess = fcport;
>  	unsigned long flags;
>  
> -	/* Check to avoid double sessions */
> -	spin_lock_irqsave(&ha->tgt.sess_lock, flags);
> -	list_for_each_entry(sess, &vha->vp_fcports, list) {
> -		if (!memcmp(sess->port_name, fcport->port_name, WWN_SIZE)) {
> -			ql_dbg(ql_dbg_tgt_mgt, vha, 0xf005,
> -			    "Double sess %p found (s_id %x:%x:%x, "
> -			    "loop_id %d), updating to d_id %x:%x:%x, "
> -			    "loop_id %d", sess, sess->d_id.b.domain,
> -			    sess->d_id.b.al_pa, sess->d_id.b.area,
> -			    sess->loop_id, fcport->d_id.b.domain,
> -			    fcport->d_id.b.al_pa, fcport->d_id.b.area,
> -			    fcport->loop_id);
> -
> -			/* Cannot undelete at this point */
> -			if (sess->deleted == QLA_SESS_DELETION_IN_PROGRESS) {
> -				spin_unlock_irqrestore(&ha->tgt.sess_lock,
> -				    flags);
> -				return NULL;
> -			}
> -
> -			if (sess->deleted)
> -				qlt_undelete_sess(sess);
> -
> -			if (!sess->se_sess) {
> -				if (ha->tgt.tgt_ops->check_initiator_node_acl(vha,
> -				    &sess->port_name[0], sess) < 0) {
> -					spin_unlock_irqrestore(&ha->tgt.sess_lock, flags);
> -					return NULL;
> -				}
> -			}
> -
> -			kref_get(&sess->sess_kref);
> -			ha->tgt.tgt_ops->update_sess(sess, fcport->d_id, fcport->loop_id,
> -						(fcport->flags & FCF_CONF_COMP_SUPPORTED));
> -
> -			if (sess->local && !local)
> -				sess->local = 0;
> -
> -			qlt_do_generation_tick(vha, &sess->generation);
> -
> -			spin_unlock_irqrestore(&ha->tgt.sess_lock, flags);
> +	if (vha->vha_tgt.qla_tgt && vha->vha_tgt.qla_tgt->tgt_stop)
> +		return NULL;
>  
> -			return sess;
> +	if (fcport->se_sess) {
> +		if (!kref_get_unless_zero(&sess->sess_kref)) {
> +			ql_dbg(ql_dbg_disc, vha, 0xffff,
> +			    "%s: kref_get_unless_zero failed for %8phC\n",
> +			    __func__, sess->port_name);
> +			return NULL;
>  		}
> -	}
> -	spin_unlock_irqrestore(&ha->tgt.sess_lock, flags);
> -
> -	sess = kzalloc(sizeof(*sess), GFP_KERNEL);
> -	if (!sess) {
> -		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf04a,
> -		    "qla_target(%u): session allocation failed, all commands "
> -		    "from port %8phC will be refused", vha->vp_idx,
> -		    fcport->port_name);
> -
> -		return NULL;
> +		return fcport;
>  	}
>  	sess->tgt = vha->vha_tgt.qla_tgt;
> -	sess->vha = vha;
> -	sess->d_id = fcport->d_id;
> -	sess->loop_id = fcport->loop_id;
>  	sess->local = local;
> -	kref_init(&sess->sess_kref);
> -	INIT_LIST_HEAD(&sess->del_list_entry);
>  
> -	/* Under normal circumstances we want to logout from firmware when
> +	/*
> +	 * Under normal circumstances we want to logout from firmware when
>  	 * session eventually ends and release corresponding nport handle.
>  	 * In the exception cases (e.g. when new PLOGI is waiting) corresponding
> -	 * code will adjust these flags as necessary. */
> +	 * code will adjust these flags as necessary.
> +	 */
>  	sess->logout_on_delete = 1;
>  	sess->keep_nport_handle = 0;
> +	sess->logout_completed = 0;
>  
> -	ql_dbg(ql_dbg_tgt_mgt, vha, 0xf006,
> -	    "Adding sess %p to tgt %p via ->check_initiator_node_acl()\n",
> -	    sess, vha->vha_tgt.qla_tgt);
> +	if (ha->tgt.tgt_ops->check_initiator_node_acl(vha,
> +	    &fcport->port_name[0], sess) < 0) {
> +		ql_dbg(ql_dbg_tgt_mgt, vha, 0xffff,
> +		    "(%d) %8phC check_initiator_node_acl failed\n",
> +		    vha->vp_idx, fcport->port_name);
> +		return NULL;
> +	} else {
> +		kref_init(&fcport->sess_kref);
> +		/*
> +		 * Take an extra reference to ->sess_kref here to handle
> +		 * fc_port access across ->tgt.sess_lock reaquire.
> +		 */
> +		if (!kref_get_unless_zero(&sess->sess_kref)) {
> +			ql_dbg(ql_dbg_disc, vha, 0xffff,
> +			    "%s: kref_get_unless_zero failed for %8phC\n",
> +			    __func__, sess->port_name);
> +			return NULL;
> +		}
>  
> -	sess->conf_compl_supported = (fcport->flags & FCF_CONF_COMP_SUPPORTED);
> -	BUILD_BUG_ON(sizeof(sess->port_name) != sizeof(fcport->port_name));
> -	memcpy(sess->port_name, fcport->port_name, sizeof(sess->port_name));
> +		spin_lock_irqsave(&ha->tgt.sess_lock, flags);
> +		if (!IS_SW_RESV_ADDR(sess->d_id))
> +			vha->vha_tgt.qla_tgt->sess_count++;

These changes also introduce a new static checker warning:

drivers/scsi/qla2xxx/qla_target.c:1306: qlt_create_sess() error: we previously assumed 'vha->vha_tgt.qla_tgt' could be null (see line 1260)

Please check whether the newly introduced "if (vha->vha_tgt.qla_tgt)"
test is necessary.

Thanks,

Bart.