* 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 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 qla2xxx: Add LLD target-mode infrastructure for >= 24xx series 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-05-07 13:59 qla2xxx: Add LLD target-mode infrastructure for >= 24xx series Dan Carpenter
2012-05-07 20:51 ` Nicholas A. Bellinger
-- strict thread matches above, loose matches on Subject: below --
2012-03-13 17:20 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
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).