On Wed, 2011-10-26 at 20:37 +0200, Jörn Engel wrote:
> 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
<SNIP>
> @@ -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);
Thanks Roland for catching this other obvious bit. I'll queue this up
in a second patch + Cc stable, and include Joern's cleanup patch as a
seperate item.
Thank you,
--nab
--
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
