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

[Bart Van Assche]

On Mon, 2017-01-16 at 12:35 -0800, Himanshu Madhani wrote:
> +  /*
> +   * return FC_CTELS_STATUS_OK and leave the decoding of the ELS/CT
> +   * fc payload  to the caller
> +   */
> +
> +  bsg_reply->reply_data.ctels_reply.status = FC_CTELS_STATUS_OK;
> +  bsg_job->reply_len = sizeof(struct fc_bsg_reply);

smatch reports the following for the above code:

drivers/scsi/qla2xxx/qla_isr.c:1456: qla2x00_ct_entry() warn: inconsistent 
indenting

Please use tabs instead of spaces for indentation. That is sufficient to
suppress that warning.

Thanks,

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Madhani, Himanshu]

On 1/17/17, 2:27 PM, "Bart Van Assche"  wrote:

>%phC

Yes. It looks like typo in the message. 

Will update this patch with other sparse warnings fixes.

Thanks,
Himanshu
> 

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

[Bart Van Assche]

On Mon, 2017-01-16 at 12:35 -0800, Himanshu Madhani wrote:
> -   kref_get(>sess_kref);
> +   if (!kref_get_unless_zero(>sess_kref)) {
> +   ql_dbg(ql_dbg_tgt_tmr, vha, 0x,
> +   "%s: kref_get fail %8pHC \n",
> +    __func__, sess->port_name);
> +   sess = NULL;
> +   goto out_term2;
> +   }
> }

As far as I know %pH is not a valid conversion identifier. Was perhaps
%phC intended?.

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bart Van Assche]

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(>tgt.sess_lock, flags);
> - list_for_each_entry(sess, >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(>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,
> - >port_name[0], sess) < 0) {
> - 
> spin_unlock_irqrestore(>tgt.sess_lock, flags);
> - return NULL;
> - }
> - }
> -
> - kref_get(>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, >generation);
> -
> - spin_unlock_irqrestore(>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_kref)) {
> + ql_dbg(ql_dbg_disc, vha, 0x,
> + "%s: kref_get_unless_zero failed for %8phC\n",
> + __func__, sess->port_name);
> + return NULL;
>   }
> - }
> - spin_unlock_irqrestore(>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_kref);
> - INIT_LIST_HEAD(>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,
> + >port_name[0], sess) < 0) {
> + ql_dbg(ql_dbg_tgt_mgt, vha, 0x,
> + "(%d) %8phC check_initiator_node_acl failed\n",
> + vha->vp_idx, fcport->port_name);
> + return NULL;
> + } else {
> + kref_init(>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_kref)) {
> + ql_dbg(ql_dbg_disc, vha, 

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

[Bart Van Assche]

On Mon, 2017-01-16 at 12:35 -0800, Himanshu Madhani wrote:
> -static void
> +void
>  qla2x00_async_iocb_timeout(void *data)
>  {
>   srb_t *sp = (srb_t *)data;
>   fc_port_t *fcport = sp->fcport;
> + struct srb_iocb *lio = >u.iocb_cmd;
> + struct event_arg ea;
>  
>   ql_dbg(ql_dbg_disc, fcport->vha, 0x2071,
> - "Async-%s timeout - hdl=%x portid=%02x%02x%02x.\n",
> + "Async-%s timeout - hdl=%x portid=%02x%02x%02x %8phC.\n",
>   sp->name, sp->handle, fcport->d_id.b.domain, fcport->d_id.b.area,
> - fcport->d_id.b.al_pa);
> + fcport->d_id.b.al_pa, fcport->port_name);
>  
> - fcport->flags &= ~FCF_ASYNC_SENT;
> - if (sp->type == SRB_LOGIN_CMD) {
> - struct srb_iocb *lio = >u.iocb_cmd;
> - qla2x00_post_async_logout_work(fcport->vha, fcport, NULL);
> + if (fcport)
> + fcport->flags &= ~FCF_ASYNC_SENT;

This code triggers a new static checker warning because the fcport pointer
is dereferenced before it is used. Please review the code.

Thanks,

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html