On Wed, 26 October 2011 06:25:36 -0700, Roland Dreier wrote:
> On Tue, Oct 25, 2011 at 10:21 PM, Nicholas A. Bellinger
> <nab@xxxxxxxxxxxxxxx> wrote:
> > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> > index e617527..b2e8f45 100644
> > --- a/drivers/target/target_core_tmr.c
> > +++ b/drivers/target/target_core_tmr.c
> > @@ -154,7 +154,7 @@ static void core_tmr_drain_tmr_list(
> > while (!list_empty(&drain_tmr_list)) {
> > tmr = list_entry(drain_tmr_list.next, struct se_tmr_req, tmr_list);
> > list_del(&tmr->tmr_list);
> > - cmd = tmr_p->task_cmd;
> > + cmd = tmr->task_cmd;
> >
> > pr_debug("LUN_RESET: %s releasing TMR %p Function: 0x%02x,"
> > " Response: 0x%02x, t_state: %d\n",
>
> Actually it might be cleaner to set tmr_p to the first entry, rather than
> overwriting tmr (which is a parameter to the function).
Agreed.
> Also, a trivial cleanup would be to use list_first_entry(&drain_tmr_list,...).
>
> In fact is there any reason why this loop needs to be a while() instead of
> list_for_each_entry_safe()?
Not that I can see.
> Finally, earlier in the function:
>
> if (cmd->t_state == TRANSPORT_ISTATE_PROCESSING) {
> spin_unlock(&cmd->t_state_lock);
> continue;
> }
> spin_unlock(&cmd->t_state_lock);
>
> list_move_tail(&tmr->tmr_list, &drain_tmr_list);
> }
> spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
>
> looks like that tmr in list_move_tail should be tmr_p too?
Agreed. We also seem to have a backtrace to match. Nick, what do you
think about the patch below? If you dislike the cleanup bits, feel
free to remove.
Jörn
--
Public Domain - Free as in Beer
General Public - Free as in Speech
BSD License - Free as in Enterprise
Shared Source - Free as in "Work will make you..."
[PATCH] target: Fix core_tmr_drain_tmr_list
tmr should have been used only once in this function, to compare
against tmr_p. Both other uses we bugs that were hit in practice.
And after having had to look at this function twice, also remove some
superfluous brackets, a pointless NULL check and convert the
hand-rolled loop into list_for_each_entry_safe().
Signed-off-by: Joern Engel <joern@xxxxxxxxx>
---
drivers/target/target_core_tmr.c | 22 ++++++++++------------
1 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 740f312..5d067a5 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -126,11 +126,11 @@ static void core_tmr_drain_tmr_list(
/*
* Allow the received TMR to return with FUNCTION_COMPLETE.
*/
- if (tmr && (tmr_p == tmr))
+ if (tmr_p == tmr)
continue;
cmd = tmr_p->task_cmd;
- if (!(cmd)) {
+ if (!cmd) {
printk(KERN_ERR "Unable to locate struct se_cmd for TMR\n");
continue;
}
@@ -139,13 +139,12 @@ static void core_tmr_drain_tmr_list(
* parameter (eg: for PROUT PREEMPT_AND_ABORT service action
* skip non regisration key matching TMRs.
*/
- if ((preempt_and_abort_list != NULL) &&
- (core_scsi3_check_cdb_abort_and_preempt(
- preempt_and_abort_list, cmd) != 0))
+ if (preempt_and_abort_list &&
+ core_scsi3_check_cdb_abort_and_preempt(preempt_and_abort_list, cmd))
continue;
spin_lock(&cmd->t_task.t_state_lock);
- if (!(atomic_read(&cmd->t_task.t_transport_active))) {
+ if (!atomic_read(&cmd->t_task.t_transport_active)) {
spin_unlock(&cmd->t_task.t_state_lock);
continue;
}
@@ -155,19 +154,18 @@ static void core_tmr_drain_tmr_list(
}
spin_unlock(&cmd->t_task.t_state_lock);
- list_move_tail(&tmr->tmr_list, &drain_tmr_list);
+ list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
}
spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
- while (!list_empty(&drain_tmr_list)) {
- tmr = list_entry(drain_tmr_list.next, struct se_tmr_req, tmr_list);
- list_del(&tmr->tmr_list);
+ list_for_each_entry_safe(tmr_p, tmr_pp, &drain_tmr_list, tmr_list) {
+ list_del(&tmr_p->tmr_list);
cmd = tmr_p->task_cmd;
printk("LUN_RESET: %s releasing TMR %p Function: 0x%02x,"
" Response: 0x%02x, t_state: %d\n",
- (preempt_and_abort_list) ? "Preempt" : "", tmr,
- tmr->function, tmr->response, cmd->t_state);
+ (preempt_and_abort_list) ? "Preempt" : "", tmr_p,
+ tmr_p->function, tmr_p->response, cmd->t_state);
transport_cmd_finish_abort_tmr(cmd);
}
--
1.7.6.3
--
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
