* re: qla2xxx: Add LLD target-mode infrastructure for >= 24xx series
@ 2012-03-13 17:20 Dan Carpenter
2012-03-15 23:10 ` Nicholas A. Bellinger
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2012-03-13 17:20 UTC (permalink / raw)
To: nab; +Cc: linux-driver, linux-scsi
Hello Nicholas,
The patch 962f988d031f: "qla2xxx: Add LLD target-mode infrastructure
for >= 24xx series" from Mar 8, 2012, leads to the following Smatch
warning:
drivers/scsi/qla2xxx/qla_target.c:3207 qla_tgt_handle_srr_work()
error: potential null derefence 'vha'.
drivers/scsi/qla2xxx/qla_target.c
3198 imm = NULL;
3199 list_for_each_entry_safe(i, ti, &tgt->srr_imm_list,
3200 srr_list_entry) {
3201 if (i->srr_id == sctio->srr_id) {
3202 list_del(&i->srr_list_entry);
3203 if (imm) {
3204 printk(KERN_ERR "qla_target(%d): There must "
3205 "be only one IMM SRR per CTIO SRR "
3206 "(IMM SRR %p, id %d, CTIO %p\n",
3207 vha->vp_idx, i, i->srr_id, sctio);
^^^
It looks like "vha" might not be initialized yet.
3208 qla_tgt_reject_free_srr_imm(vha, i, 0);
3209 } else
3210 imm = i;
3211 }
3212 }
3213
3214 ql_dbg(ql_dbg_tgt_mgt, tgt->vha, 0xe12f, "IMM SRR %p, CTIO SRR %p (id %d)\n",
3215 imm, sctio, sctio->srr_id);
3216
3217 if (imm == NULL) {
3218 ql_dbg(ql_dbg_tgt_mgt, tgt->vha, 0xe130, "Not found matching IMM"
3219 " for SRR CTIO (id %d)\n", sctio->srr_id);
3220 continue;
3221 } else
3222 list_del(&sctio->srr_list_entry);
3223
3224 spin_unlock_irqrestore(&tgt->srr_lock, flags);
3225
3226 cmd = sctio->cmd;
3227 vha = cmd->vha;
^^^
It gets set here.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* re: qla2xxx: Add LLD target-mode infrastructure for >= 24xx series 2012-03-13 17:20 qla2xxx: Add LLD target-mode infrastructure for >= 24xx series Dan Carpenter @ 2012-03-15 23:10 ` Nicholas A. Bellinger 2012-03-16 13:59 ` Jörn Engel 0 siblings, 1 reply; 6+ messages in thread From: Nicholas A. Bellinger @ 2012-03-15 23:10 UTC (permalink / raw) To: Dan Carpenter Cc: linux-driver, linux-scsi, target-devel, Andrew Vasquez, Giridhar Malavali On Tue, 2012-03-13 at 20:20 +0300, Dan Carpenter wrote: > Hello Nicholas, > > The patch 962f988d031f: "qla2xxx: Add LLD target-mode infrastructure > for >= 24xx series" from Mar 8, 2012, leads to the following Smatch > warning: > > drivers/scsi/qla2xxx/qla_target.c:3207 qla_tgt_handle_srr_work() > error: potential null derefence 'vha'. > > > drivers/scsi/qla2xxx/qla_target.c > 3198 imm = NULL; > 3199 list_for_each_entry_safe(i, ti, &tgt->srr_imm_list, > 3200 srr_list_entry) { > 3201 if (i->srr_id == sctio->srr_id) { > 3202 list_del(&i->srr_list_entry); > 3203 if (imm) { > 3204 printk(KERN_ERR "qla_target(%d): There must " > 3205 "be only one IMM SRR per CTIO SRR " > 3206 "(IMM SRR %p, id %d, CTIO %p\n", > 3207 vha->vp_idx, i, i->srr_id, sctio); > ^^^ > It looks like "vha" might not be initialized yet. > > 3208 qla_tgt_reject_free_srr_imm(vha, i, 0); > 3209 } else > 3210 imm = i; > 3211 } > 3212 } > 3213 > 3214 ql_dbg(ql_dbg_tgt_mgt, tgt->vha, 0xe12f, "IMM SRR %p, CTIO SRR %p (id %d)\n", > 3215 imm, sctio, sctio->srr_id); > 3216 > 3217 if (imm == NULL) { > 3218 ql_dbg(ql_dbg_tgt_mgt, tgt->vha, 0xe130, "Not found matching IMM" > 3219 " for SRR CTIO (id %d)\n", sctio->srr_id); > 3220 continue; > 3221 } else > 3222 list_del(&sctio->srr_list_entry); > 3223 > 3224 spin_unlock_irqrestore(&tgt->srr_lock, flags); > 3225 > 3226 cmd = sctio->cmd; > 3227 vha = cmd->vha; > ^^^ > It gets set here. > It looks like this should be using qla_tgt->vha everywhere. Applying the following patch. --nab iff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c index e415d1e..2af075b 100644 --- a/drivers/scsi/qla2xxx/qla_target.c +++ b/drivers/scsi/qla2xxx/qla_target.c @@ -3169,11 +3169,11 @@ static void qla_tgt_reject_free_srr_imm(struct scsi_qla_host *vha, struct qla_tg static void qla_tgt_handle_srr_work(struct work_struct *work) { struct qla_tgt *tgt = container_of(work, struct qla_tgt, srr_work); - struct scsi_qla_host *vha = NULL; + struct scsi_qla_host *vha = tgt->vha; struct qla_tgt_srr_ctio *sctio; unsigned long flags; - ql_dbg(ql_dbg_tgt_mgt, tgt->vha, 0xe12e, "Entering SRR work (tgt %p)\n", tgt); + ql_dbg(ql_dbg_tgt_mgt, vha, 0xe12e, "Entering SRR work (tgt %p)\n", tgt); restart: spin_lock_irqsave(&tgt->srr_lock, flags); @@ -3192,17 +3192,17 @@ restart: "be only one IMM SRR per CTIO SRR " "(IMM SRR %p, id %d, CTIO %p\n", vha->vp_idx, i, i->srr_id, sctio); - qla_tgt_reject_free_srr_imm(vha, i, 0); + qla_tgt_reject_free_srr_imm(tgt->vha, i, 0); } else imm = i; } } - ql_dbg(ql_dbg_tgt_mgt, tgt->vha, 0xe12f, "IMM SRR %p, CTIO SRR %p (id %d)\n", + ql_dbg(ql_dbg_tgt_mgt, vha, 0xe12f, "IMM SRR %p, CTIO SRR %p (id %d)\n", imm, sctio, sctio->srr_id); if (imm == NULL) { - ql_dbg(ql_dbg_tgt_mgt, tgt->vha, 0xe130, "Not found matching IMM" + ql_dbg(ql_dbg_tgt_mgt, vha, 0xe130, "Not found matching IMM" " for SRR CTIO (id %d)\n", sctio->srr_id); continue; } else @@ -3211,7 +3211,6 @@ restart: spin_unlock_irqrestore(&tgt->srr_lock, flags); cmd = sctio->cmd; - vha = cmd->vha; /* * Reset qla_tgt_cmd SRR values and SGL pointer+count to follow * tcm_qla2xxx_write_pending() and tcm_qla2xxx_queue_data_in() ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: qla2xxx: Add LLD target-mode infrastructure for >= 24xx series 2012-03-15 23:10 ` Nicholas A. Bellinger @ 2012-03-16 13:59 ` Jörn Engel 2012-03-17 1:22 ` Nicholas A. Bellinger 0 siblings, 1 reply; 6+ messages in thread From: Jörn Engel @ 2012-03-16 13:59 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: Dan Carpenter, linux-driver, linux-scsi, target-devel, Andrew Vasquez, Giridhar Malavali Hello Nick! On Thu, 15 March 2012 16:10:15 -0700, Nicholas A. Bellinger wrote: > > iff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c > index e415d1e..2af075b 100644 > --- a/drivers/scsi/qla2xxx/qla_target.c > +++ b/drivers/scsi/qla2xxx/qla_target.c > @@ -3169,11 +3169,11 @@ static void qla_tgt_reject_free_srr_imm(struct scsi_qla_host *vha, struct qla_tg > static void qla_tgt_handle_srr_work(struct work_struct *work) > { > struct qla_tgt *tgt = container_of(work, struct qla_tgt, srr_work); > - struct scsi_qla_host *vha = NULL; > + struct scsi_qla_host *vha = tgt->vha; > struct qla_tgt_srr_ctio *sctio; > unsigned long flags; > > - ql_dbg(ql_dbg_tgt_mgt, tgt->vha, 0xe12e, "Entering SRR work (tgt %p)\n", tgt); > + ql_dbg(ql_dbg_tgt_mgt, vha, 0xe12e, "Entering SRR work (tgt %p)\n", tgt); > > restart: > spin_lock_irqsave(&tgt->srr_lock, flags); > @@ -3192,17 +3192,17 @@ restart: > "be only one IMM SRR per CTIO SRR " > "(IMM SRR %p, id %d, CTIO %p\n", > vha->vp_idx, i, i->srr_id, sctio); > - qla_tgt_reject_free_srr_imm(vha, i, 0); > + qla_tgt_reject_free_srr_imm(tgt->vha, i, 0); Everywhere else you move from tgt->vha to vha. Is there a reason for doing the reverse here? Jörn ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: qla2xxx: Add LLD target-mode infrastructure for >= 24xx series 2012-03-16 13:59 ` Jörn Engel @ 2012-03-17 1:22 ` Nicholas A. Bellinger 0 siblings, 0 replies; 6+ messages in thread From: Nicholas A. Bellinger @ 2012-03-17 1:22 UTC (permalink / raw) To: Jörn Engel Cc: Dan Carpenter, linux-driver, linux-scsi, target-devel, Andrew Vasquez, Giridhar Malavali On Fri, 2012-03-16 at 09:59 -0400, Jörn Engel wrote: > Hello Nick! > > On Thu, 15 March 2012 16:10:15 -0700, Nicholas A. Bellinger wrote: > > > > iff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c > > index e415d1e..2af075b 100644 > > --- a/drivers/scsi/qla2xxx/qla_target.c > > +++ b/drivers/scsi/qla2xxx/qla_target.c > > @@ -3169,11 +3169,11 @@ static void qla_tgt_reject_free_srr_imm(struct scsi_qla_host *vha, struct qla_tg > > static void qla_tgt_handle_srr_work(struct work_struct *work) > > { > > struct qla_tgt *tgt = container_of(work, struct qla_tgt, srr_work); > > - struct scsi_qla_host *vha = NULL; > > + struct scsi_qla_host *vha = tgt->vha; > > struct qla_tgt_srr_ctio *sctio; > > unsigned long flags; > > > > - ql_dbg(ql_dbg_tgt_mgt, tgt->vha, 0xe12e, "Entering SRR work (tgt %p)\n", tgt); > > + ql_dbg(ql_dbg_tgt_mgt, vha, 0xe12e, "Entering SRR work (tgt %p)\n", tgt); > > > > restart: > > spin_lock_irqsave(&tgt->srr_lock, flags); > > @@ -3192,17 +3192,17 @@ restart: > > "be only one IMM SRR per CTIO SRR " > > "(IMM SRR %p, id %d, CTIO %p\n", > > vha->vp_idx, i, i->srr_id, sctio); > > - qla_tgt_reject_free_srr_imm(vha, i, 0); > > + qla_tgt_reject_free_srr_imm(tgt->vha, i, 0); > > Everywhere else you move from tgt->vha to vha. Is there a reason for > doing the reverse here? > Hi Joern, Non-critical typo here.. Fixing this up now. Thanks! --nab ^ permalink raw reply [flat|nested] 6+ messages in thread
* re: qla2xxx: Add LLD target-mode infrastructure for >= 24xx series
@ 2012-05-07 13:59 Dan Carpenter
2012-05-07 20:51 ` Nicholas A. Bellinger
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2012-05-07 13:59 UTC (permalink / raw)
To: nab; +Cc: linux-scsi
Hello Nicholas Bellinger,
This is a semi-automatic email about new static checker warnings.
The patch 2c0532cbbbb5: "qla2xxx: Add LLD target-mode infrastructure
for >= 24xx series" from May 3, 2012, leads to the following Smatch
complaint:
drivers/scsi/qla2xxx/qla_target.c:2965 qlt_abort_task()
error: we previously assumed 'sess' could be null (see line 2958)
drivers/scsi/qla2xxx/qla_target.c
2957 sess = ha->tgt.tgt_ops->find_sess_by_loop_id(vha, loop_id);
2958 if (sess == NULL) {
^^^^^^^^^^^^
New check.
2959 ql_dbg(ql_dbg_tgt_mgt, vha, 0xf025,
2960 "qla_target(%d): task abort for unexisting "
2961 "session\n", vha->vp_idx);
2962 res = qlt_sched_sess_work(ha->tgt.qla_tgt,
2963 QLA_TGT_SESS_WORK_ABORT, iocb, sizeof(*iocb));
2964 if (res != 0)
2965 sess->tgt->tm_to_unknown = 1;
^^^^^^^^^
New dereference.
2966
2967 return res;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* re: qla2xxx: Add LLD target-mode infrastructure for >= 24xx series 2012-05-07 13:59 Dan Carpenter @ 2012-05-07 20:51 ` Nicholas A. Bellinger 0 siblings, 0 replies; 6+ messages in thread From: Nicholas A. Bellinger @ 2012-05-07 20:51 UTC (permalink / raw) To: Dan Carpenter Cc: linux-scsi, target-devel, Arun Easi, Andrew Vasquez, Roland Dreier, Christoph Hellwig On Mon, 2012-05-07 at 16:59 +0300, Dan Carpenter wrote: > Hello Nicholas Bellinger, > > This is a semi-automatic email about new static checker warnings. > Hey Dan, > The patch 2c0532cbbbb5: "qla2xxx: Add LLD target-mode infrastructure > for >= 24xx series" from May 3, 2012, leads to the following Smatch > complaint: > > drivers/scsi/qla2xxx/qla_target.c:2965 qlt_abort_task() > error: we previously assumed 'sess' could be null (see line 2958) > > drivers/scsi/qla2xxx/qla_target.c > 2957 sess = ha->tgt.tgt_ops->find_sess_by_loop_id(vha, loop_id); > 2958 if (sess == NULL) { > ^^^^^^^^^^^^ > New check. > > 2959 ql_dbg(ql_dbg_tgt_mgt, vha, 0xf025, > 2960 "qla_target(%d): task abort for unexisting " > 2961 "session\n", vha->vp_idx); > 2962 res = qlt_sched_sess_work(ha->tgt.qla_tgt, > 2963 QLA_TGT_SESS_WORK_ABORT, iocb, sizeof(*iocb)); > 2964 if (res != 0) > 2965 sess->tgt->tm_to_unknown = 1; > ^^^^^^^^^ > New dereference. > So it looks like ->tm_to_unknown is actually left-over cruft before the conversion to proper workqueue usage in qla_target.c, and now it's safe to go ahead and drop this plus other unnecessary sess_works_pending bitfield.. I'm committing the following patch into lio-core.git, and will fold into for-next-merge for reference. Qlogic folks, please also fold into your linux-scsi submission, or let me know if you would prefer an updated target series. Thanks! --nab diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c index 2baef81..d1d5380 100644 --- a/drivers/scsi/qla2xxx/qla_target.c +++ b/drivers/scsi/qla2xxx/qla_target.c @@ -421,7 +421,6 @@ static int qlt_reset(struct scsi_qla_host *vha, void *iocb, int mcmd) "Using sess for qla_tgt_reset: %p\n", sess); if (!sess) { res = -ESRCH; - ha->tgt.qla_tgt->tm_to_unknown = 1; return res; } @@ -1066,10 +1065,7 @@ static int qlt_sched_sess_work(struct qla_tgt *tgt, int type, memcpy(&prm->tm_iocb, param, param_size); spin_lock_irqsave(&tgt->sess_work_lock, flags); - if (!tgt->sess_works_pending) - tgt->tm_to_unknown = 0; list_add_tail(&prm->sess_works_list_entry, &tgt->sess_works_list); - tgt->sess_works_pending = 1; spin_unlock_irqrestore(&tgt->sess_work_lock, flags); schedule_work(&tgt->sess_work); @@ -1343,7 +1339,6 @@ static void qlt_24xx_handle_abts(struct scsi_qla_host *vha, rc = qlt_sched_sess_work(ha->tgt.qla_tgt, QLA_TGT_SESS_WORK_ABORT, abts, sizeof(*abts)); if (rc != 0) { - ha->tgt.qla_tgt->tm_to_unknown = 1; qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_REJECTED, false); } @@ -2897,8 +2892,6 @@ static int qlt_handle_task_mgmt(struct scsi_qla_host *vha, void *iocb) "non-existant session\n", vha->vp_idx, fn); res = qlt_sched_sess_work(tgt, QLA_TGT_SESS_WORK_TM, iocb, sizeof(atio_from_isp_t)); - if (res != 0) - tgt->tm_to_unknown = 1; return res; } @@ -2961,8 +2954,6 @@ static int qlt_abort_task(struct scsi_qla_host *vha, imm_ntfy_from_isp_t *iocb) "session\n", vha->vp_idx); res = qlt_sched_sess_work(ha->tgt.qla_tgt, QLA_TGT_SESS_WORK_ABORT, iocb, sizeof(*iocb)); - if (res != 0) - sess->tgt->tm_to_unknown = 1; return res; } @@ -4254,7 +4245,6 @@ static void qlt_sess_work_fn(struct work_struct *work) { struct qla_tgt *tgt = container_of(work, struct qla_tgt, sess_work); struct scsi_qla_host *vha = tgt->vha; - struct qla_hw_data *ha = vha->hw; unsigned long flags; ql_dbg(ql_dbg_tgt_mgt, vha, 0xf000, "Sess work (tgt %p)", tgt); @@ -4290,15 +4280,6 @@ static void qlt_sess_work_fn(struct work_struct *work) kfree(prm); } spin_unlock_irqrestore(&tgt->sess_work_lock, flags); - - spin_lock_irqsave(&ha->hardware_lock, flags); - spin_lock(&tgt->sess_work_lock); - if (list_empty(&tgt->sess_works_list)) { - tgt->sess_works_pending = 0; - tgt->tm_to_unknown = 0; - } - spin_unlock(&tgt->sess_work_lock); - spin_unlock_irqrestore(&ha->hardware_lock, flags); } /* Must be called under tgt_host_action_mutex */ diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h index f0280ae..7d5849f 100644 --- a/drivers/scsi/qla2xxx/qla_target.h +++ b/drivers/scsi/qla2xxx/qla_target.h @@ -763,8 +763,6 @@ struct qla_tgt { /* Target's flags, serialized by pha->hardware_lock */ unsigned int tgt_enable_64bit_addr:1; /* 64-bits PCI addr enabled */ unsigned int link_reinit_iocb_pending:1; - unsigned int tm_to_unknown:1; /* TM to unknown session was sent */ - unsigned int sess_works_pending:1; /* there are sess_work entries */ /* * Protected by tgt_mutex AND hardware_lock for writing and tgt_mutex ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-07 20:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-13 17:20 qla2xxx: Add LLD target-mode infrastructure for >= 24xx series Dan Carpenter 2012-03-15 23:10 ` Nicholas A. Bellinger 2012-03-16 13:59 ` Jörn Engel 2012-03-17 1:22 ` Nicholas A. Bellinger -- strict thread matches above, loose matches on Subject: below -- 2012-05-07 13:59 Dan Carpenter 2012-05-07 20:51 ` Nicholas A. Bellinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).