| Message ID | 20210122103629.5412-1-damien.hedde@greensocs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hw/core/resettable: make in-reset state false during exit phase call | expand |
Hi Damien, On 1/22/21 11:36 AM, Damien Hedde wrote: > Move the reset count decrement from "just after" to "just before" > calling the exit phase handler. The goal is to make > resettable_is_in_reset() returning false during the handler execution. > > This simplifies reset handling in resettable devices. > > Typically, a function that updates the device state will just need > to read the current reset state and not anymore treat the "in > a reset-exit transition" special case. > > As a side note, this patch also fixes the fact that the reset count was > not decremented in case of recursive reset. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > Buglink: https://bugs.launchpad.net/qemu/+bug/1905297 > Reported-by: Michael Peter <michael.peter@hensoldt-cyber.com> > -- > > Hi, > > Following our discussion: > https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg01013.html > here's my proposal to fix Michael's bug on a global scope. > > I flaged it v2 because I've taken Philippe's remarks there: > https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg00635.html > I've also changed the patch title, I think it is better this way. > > Thanks, > Damien > > Cc: f4bug@amsat.org > Cc: peter.maydell@linaro.org > Cc: alistair@alistair23.me > Cc: edgar.iglesias@gmail.com > --- > docs/devel/reset.rst | 6 +++--- > hw/core/resettable.c | 3 +-- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst > index abea1102dc..021a7277a2 100644 > --- a/docs/devel/reset.rst > +++ b/docs/devel/reset.rst > @@ -210,9 +210,9 @@ Polling the reset state > Resettable interface provides the ``resettable_is_in_reset()`` function. > This function returns true if the object parameter is currently under reset. > > -An object is under reset from the beginning of the *init* phase to the end of > -the *exit* phase. During all three phases, the function will return that the > -object is in reset. > +An object is under reset from the beginning of the *init* phase to the *exit* > +phase. During *init* and *hold* phase only, the function will return that the > +object is in reset. The state is changed just before calling the *exit* method. "An object is under reset from the beginning of the *init* phase to the beginning of the *exit* phase" ? An ASCII art would clarify all doubts :) Otherwise: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > This function may be used if the object behavior has to be adapted > while in reset state. For example if a device has an irq input, > diff --git a/hw/core/resettable.c b/hw/core/resettable.c > index 96a99ce39e..c3df75c6ba 100644 > --- a/hw/core/resettable.c > +++ b/hw/core/resettable.c > @@ -201,12 +201,11 @@ static void resettable_phase_exit(Object *obj, void *opaque, ResetType type) > resettable_child_foreach(rc, obj, resettable_phase_exit, NULL, type); > > assert(s->count > 0); > - if (s->count == 1) { > + if (--s->count == 0) { > trace_resettable_phase_exit_exec(obj, obj_typename, !!rc->phases.exit); > if (rc->phases.exit && !resettable_get_tr_func(rc, obj)) { > rc->phases.exit(obj); > } > - s->count = 0; > } > s->exit_phase_in_progress = false; > trace_resettable_phase_exit_end(obj, obj_typename, s->count); >
On Fri, 22 Jan 2021 at 10:36, Damien Hedde <damien.hedde@greensocs.com> wrote: > > Move the reset count decrement from "just after" to "just before" > calling the exit phase handler. The goal is to make > resettable_is_in_reset() returning false during the handler execution. > > This simplifies reset handling in resettable devices. > > Typically, a function that updates the device state will just need > to read the current reset state and not anymore treat the "in > a reset-exit transition" special case. > > As a side note, this patch also fixes the fact that the reset count was > not decremented in case of recursive reset. This seems like a reasonable change and looking through the sources it shouldn't break anything. > diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst > index abea1102dc..021a7277a2 100644 > --- a/docs/devel/reset.rst > +++ b/docs/devel/reset.rst > @@ -210,9 +210,9 @@ Polling the reset state > Resettable interface provides the ``resettable_is_in_reset()`` function. > This function returns true if the object parameter is currently under reset. > > -An object is under reset from the beginning of the *init* phase to the end of > -the *exit* phase. During all three phases, the function will return that the > -object is in reset. > +An object is under reset from the beginning of the *init* phase to the *exit* > +phase. During *init* and *hold* phase only, the function will return that the > +object is in reset. The state is changed just before calling the *exit* method. There is no "init" phase -- the documentation and the data structures name the phases "enter", "hold" and "exit". Agreed with Philippe about saying "beginning of the *enter* phase to the beginning of the *exit* phase" (but an ascii art diagram is probably overkill). thanks -- PMM
On Fri, 22 Jan 2021 at 10:36, Damien Hedde <damien.hedde@greensocs.com> wrote: > > Move the reset count decrement from "just after" to "just before" > calling the exit phase handler. The goal is to make > resettable_is_in_reset() returning false during the handler execution. > > This simplifies reset handling in resettable devices. > > Typically, a function that updates the device state will just need > to read the current reset state and not anymore treat the "in > a reset-exit transition" special case. > > As a side note, this patch also fixes the fact that the reset count was > not decremented in case of recursive reset. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > Buglink: https://bugs.launchpad.net/qemu/+bug/1905297 > Reported-by: Michael Peter <michael.peter@hensoldt-cyber.com> > -- > > Hi, > > Following our discussion: > https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg01013.html > here's my proposal to fix Michael's bug on a global scope. > > I flaged it v2 because I've taken Philippe's remarks there: > https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg00635.html > I've also changed the patch title, I think it is better this way. I've just discovered that we never applied this patch, which is the fix for the bug which is the underlying cause of the regression noted in https://gitlab.com/qemu-project/qemu/-/issues/1266 from my recent changes extending the use of device_cold_reset() and bus_cold_reset() to the SCSI subsystem. It looks like the only review issue with v2 was that the docs change said 'init phase' when it meant 'enter' phase, so I'm going fold in this minor docs tweak and send it out as a v3: --- a/docs/devel/reset.rst +++ b/docs/devel/reset.rst @@ -210,8 +210,8 @@ Polling the reset state Resettable interface provides the ``resettable_is_in_reset()`` function. This function returns true if the object parameter is currently under reset. -An object is under reset from the beginning of the *init* phase to the *exit* -phase. During *init* and *hold* phase only, the function will return that the +An object is under reset from the beginning of the *enter* phase to the *exit* +phase. During *enter* and *hold* phase only, the function will return that the object is in reset. The state is changed just before calling the *exit* method. This function may be used if the object behavior has to be adapted I'll probably also tweak the commit message so it foregrounds the important thing (fixing the bogus reset count and resulting assertion) a bit more clearly. thanks -- PMM > > Thanks, > Damien > > Cc: f4bug@amsat.org > Cc: peter.maydell@linaro.org > Cc: alistair@alistair23.me > Cc: edgar.iglesias@gmail.com > --- > docs/devel/reset.rst | 6 +++--- > hw/core/resettable.c | 3 +-- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst > index abea1102dc..021a7277a2 100644 > --- a/docs/devel/reset.rst > +++ b/docs/devel/reset.rst > @@ -210,9 +210,9 @@ Polling the reset state > Resettable interface provides the ``resettable_is_in_reset()`` function. > This function returns true if the object parameter is currently under reset. > > -An object is under reset from the beginning of the *init* phase to the end of > -the *exit* phase. During all three phases, the function will return that the > -object is in reset. > +An object is under reset from the beginning of the *init* phase to the *exit* > +phase. During *init* and *hold* phase only, the function will return that the > +object is in reset. The state is changed just before calling the *exit* method. > > This function may be used if the object behavior has to be adapted > while in reset state. For example if a device has an irq input, > diff --git a/hw/core/resettable.c b/hw/core/resettable.c > index 96a99ce39e..c3df75c6ba 100644 > --- a/hw/core/resettable.c > +++ b/hw/core/resettable.c > @@ -201,12 +201,11 @@ static void resettable_phase_exit(Object *obj, void *opaque, ResetType type) > resettable_child_foreach(rc, obj, resettable_phase_exit, NULL, type); > > assert(s->count > 0); > - if (s->count == 1) { > + if (--s->count == 0) { > trace_resettable_phase_exit_exec(obj, obj_typename, !!rc->phases.exit); > if (rc->phases.exit && !resettable_get_tr_func(rc, obj)) { > rc->phases.exit(obj); > } > - s->count = 0; > } > s->exit_phase_in_progress = false; > trace_resettable_phase_exit_end(obj, obj_typename, s->count); > -- > 2.29.2
diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst index abea1102dc..021a7277a2 100644 --- a/docs/devel/reset.rst +++ b/docs/devel/reset.rst @@ -210,9 +210,9 @@ Polling the reset state Resettable interface provides the ``resettable_is_in_reset()`` function. This function returns true if the object parameter is currently under reset. -An object is under reset from the beginning of the *init* phase to the end of -the *exit* phase. During all three phases, the function will return that the -object is in reset. +An object is under reset from the beginning of the *init* phase to the *exit* +phase. During *init* and *hold* phase only, the function will return that the +object is in reset. The state is changed just before calling the *exit* method. This function may be used if the object behavior has to be adapted while in reset state. For example if a device has an irq input, diff --git a/hw/core/resettable.c b/hw/core/resettable.c index 96a99ce39e..c3df75c6ba 100644 --- a/hw/core/resettable.c +++ b/hw/core/resettable.c @@ -201,12 +201,11 @@ static void resettable_phase_exit(Object *obj, void *opaque, ResetType type) resettable_child_foreach(rc, obj, resettable_phase_exit, NULL, type); assert(s->count > 0); - if (s->count == 1) { + if (--s->count == 0) { trace_resettable_phase_exit_exec(obj, obj_typename, !!rc->phases.exit); if (rc->phases.exit && !resettable_get_tr_func(rc, obj)) { rc->phases.exit(obj); } - s->count = 0; } s->exit_phase_in_progress = false; trace_resettable_phase_exit_end(obj, obj_typename, s->count);
Move the reset count decrement from "just after" to "just before" calling the exit phase handler. The goal is to make resettable_is_in_reset() returning false during the handler execution. This simplifies reset handling in resettable devices. Typically, a function that updates the device state will just need to read the current reset state and not anymore treat the "in a reset-exit transition" special case. As a side note, this patch also fixes the fact that the reset count was not decremented in case of recursive reset. Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> Buglink: https://bugs.launchpad.net/qemu/+bug/1905297 Reported-by: Michael Peter <michael.peter@hensoldt-cyber.com> -- Hi, Following our discussion: https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg01013.html here's my proposal to fix Michael's bug on a global scope. I flaged it v2 because I've taken Philippe's remarks there: https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg00635.html I've also changed the patch title, I think it is better this way. Thanks, Damien Cc: f4bug@amsat.org Cc: peter.maydell@linaro.org Cc: alistair@alistair23.me Cc: edgar.iglesias@gmail.com --- docs/devel/reset.rst | 6 +++--- hw/core/resettable.c | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-)