| Message ID | 1435857603-30613-1-git-send-email-digetx@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 2 July 2015 at 18:20, Dmitry Osipenko <digetx@gmail.com> wrote: > Timer, running in periodic mode, can't be stopped or coming one-shot tick > won't be canceled because timer control code just doesn't handle timer > disabling. Fix it by deleting timer if enable bit isn't set. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > > v2: Avoid calling timer_del() if the timer was already disabled as per > Peter Maydell suggestion. > > hw/timer/arm_mptimer.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c > index 8b93b3c..51c18de 100644 > --- a/hw/timer/arm_mptimer.c > +++ b/hw/timer/arm_mptimer.c > @@ -122,11 +122,17 @@ static void timerblock_write(void *opaque, hwaddr addr, > case 8: /* Control. */ > old = tb->control; > tb->control = value; > - if (((old & 1) == 0) && (value & 1)) { > + if (((old & 1) == 0) && ((value & 1) == 0)) { > + break; > + } > + if (value & 1) { > if (tb->count == 0 && (tb->control & 2)) { > tb->count = tb->load; > } > timerblock_reload(tb, 1); > + } else { > + /* Shutdown timer. */ > + timer_del(tb->timer); This will now cause us to do the "reload the timer" logic if you write a 1 to the control bit when it was already 1, which we didn't do before. The logic I suggested in my previous review comment gets this right... -- PMM
02.07.2015 20:34, Peter Maydell пишет: > On 2 July 2015 at 18:20, Dmitry Osipenko <digetx@gmail.com> wrote: >> Timer, running in periodic mode, can't be stopped or coming one-shot tick >> won't be canceled because timer control code just doesn't handle timer >> disabling. Fix it by deleting timer if enable bit isn't set. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> >> v2: Avoid calling timer_del() if the timer was already disabled as per >> Peter Maydell suggestion. >> >> hw/timer/arm_mptimer.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c >> index 8b93b3c..51c18de 100644 >> --- a/hw/timer/arm_mptimer.c >> +++ b/hw/timer/arm_mptimer.c >> @@ -122,11 +122,17 @@ static void timerblock_write(void *opaque, hwaddr addr, >> case 8: /* Control. */ >> old = tb->control; >> tb->control = value; >> - if (((old & 1) == 0) && (value & 1)) { >> + if (((old & 1) == 0) && ((value & 1) == 0)) { >> + break; >> + } >> + if (value & 1) { >> if (tb->count == 0 && (tb->control & 2)) { >> tb->count = tb->load; >> } >> timerblock_reload(tb, 1); >> + } else { >> + /* Shutdown timer. */ >> + timer_del(tb->timer); > > > This will now cause us to do the "reload the timer" > logic if you write a 1 to the control bit when it was > already 1, which we didn't do before. > > The logic I suggested in my previous review > comment gets this right... > > -- PMM > Yep, you right. I overlooked it, let me try again.
02.07.2015 20:34, Peter Maydell пишет: > > This will now cause us to do the "reload the timer" > logic if you write a 1 to the control bit when it was > already 1, which we didn't do before. > > The logic I suggested in my previous review > comment gets this right... > > -- PMM > The problem with code you suggested is that won't start periodic count after one-shot tick was completed.
On 2 July 2015 at 18:52, Dmitry Osipenko <digetx@gmail.com> wrote: > 02.07.2015 20:34, Peter Maydell пишет: >> >> >> This will now cause us to do the "reload the timer" >> logic if you write a 1 to the control bit when it was >> already 1, which we didn't do before. >> >> The logic I suggested in my previous review >> comment gets this right... >> >> -- PMM >> > > The problem with code you suggested is that won't start periodic count after > one-shot tick was completed. Can you give more detail? This code is only for when the guest writes to the control register, so it doesn't get run when a one-shot tick completes. In any case, the code currently in master does: old value new value action 0 0 nothing 0 1 reload timer 1 0 nothing 1 1 nothing Your first patch did: old value new value action 0 0 delete timer 0 1 reload timer 1 0 delete timer 1 1 nothing Your second patch does: old value new value action 0 0 nothing 0 1 reload timer 1 0 delete timer 1 1 reload timer My suggestion was: old value new value action 0 0 nothing 0 1 reload timer 1 0 delete timer 1 1 nothing If you think that's wrong, then surely your first patch also has the same problem? thanks -- PMM
02.07.2015 21:09, Peter Maydell пишет: > On 2 July 2015 at 18:52, Dmitry Osipenko <digetx@gmail.com> wrote: >> 02.07.2015 20:34, Peter Maydell пишет: >>> >>> >>> This will now cause us to do the "reload the timer" >>> logic if you write a 1 to the control bit when it was >>> already 1, which we didn't do before. >>> >>> The logic I suggested in my previous review >>> comment gets this right... >>> >>> -- PMM >>> >> >> The problem with code you suggested is that won't start periodic count after >> one-shot tick was completed. > > Can you give more detail? This code is only for when > the guest writes to the control register, so it doesn't > get run when a one-shot tick completes. > > In any case, the code currently in master does: > old value new value action > 0 0 nothing > 0 1 reload timer > 1 0 nothing > 1 1 nothing > > Your first patch did: > > old value new value action > 0 0 delete timer > 0 1 reload timer > 1 0 delete timer > 1 1 nothing > > Your second patch does: > > old value new value action > 0 0 nothing > 0 1 reload timer > 1 0 delete timer > 1 1 reload timer > > My suggestion was: > > old value new value action > 0 0 nothing > 0 1 reload timer > 1 0 delete timer > 1 1 nothing > > If you think that's wrong, then surely your first > patch also has the same problem? > > thanks > -- PMM > Yes, my first patch has same problem. Noticed that issue couple hours ago, it's separate bug as I see now... Will make patch for it too. To clarify new issue: 1) load TIMER_LOAD with some value 2) write (TIMER_CONTROL_ENABLE | TIMER_CONTROL_ONESHOT | TIMER_CONTROL_IT_ENABLE) to TIMER_CONTROL 3) wait for one-shot complete 4) re-load TIMER_LOAD 5) write (TIMER_CONTROL_ENABLE | TIMER_CONTROL_PERIODIC | TIMER_CONTROL_IT_ENABLE) to TIMER_CONTROL <---- it won't start, bug Oh, and just noticed that timer code doesn't handle IT(interrupt enable) bit. Will fix it too.
02.07.2015 21:43, Dmitry Osipenko пишет: > 02.07.2015 21:09, Peter Maydell пишет: > TIMER_CONTROL_IT_ENABLE) to TIMER_CONTROL <---- it won't start, bug > s/it won't start/it won't start periodic/
02.07.2015 21:43, Dmitry Osipenko пишет: > 4) re-load TIMER_LOAD > And 4-th step can be omitted. Anyway, I already sent patches fixing this and IT issues.
diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c index 8b93b3c..51c18de 100644 --- a/hw/timer/arm_mptimer.c +++ b/hw/timer/arm_mptimer.c @@ -122,11 +122,17 @@ static void timerblock_write(void *opaque, hwaddr addr, case 8: /* Control. */ old = tb->control; tb->control = value; - if (((old & 1) == 0) && (value & 1)) { + if (((old & 1) == 0) && ((value & 1) == 0)) { + break; + } + if (value & 1) { if (tb->count == 0 && (tb->control & 2)) { tb->count = tb->load; } timerblock_reload(tb, 1); + } else { + /* Shutdown timer. */ + timer_del(tb->timer); } break; case 12: /* Interrupt status. */
Timer, running in periodic mode, can't be stopped or coming one-shot tick won't be canceled because timer control code just doesn't handle timer disabling. Fix it by deleting timer if enable bit isn't set. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- v2: Avoid calling timer_del() if the timer was already disabled as per Peter Maydell suggestion. hw/timer/arm_mptimer.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)