From: Damien Hedde <damien.hedde@greensocs.com>

The code for handling the reset level count in the Resettable code
has two issues:

The reset count is only decremented for the 1->0 case.  This means
that if there's ever a nested reset that takes the count to 2 then it
will never again be decremented.  Eventually the count will exceed
the '50' limit in resettable_phase_enter() and QEMU will trip over
the assertion failure.  The repro case in issue 1266 is an example of
this that happens now the SCSI subsystem uses three-phase reset.

Secondly, the count is decremented only after the exit phase handler
is called.  Moving the reset count decrement from "just after" to
"just before" calling the exit phase handler allows
resettable_is_in_reset() to return 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" as a special case.

Note that the semantics change to the *_is_in_reset() functions
will have no effect on the current codebase, because only two
devices (hw/char/cadence_uart.c and hw/misc/zynq_sclr.c) currently
call those functions, and in neither case do they do it from the
device's exit phase methed.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1266
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>
[PMM: adjust the docs paragraph changed to get the name of the
 'enter' phase right and to clarify exactly when the count is
 adjusted; rewrite the commit message]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 docs/devel/reset.rst | 8 +++++---
 hw/core/resettable.c | 3 +--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
index abea1102dc4..7cc6a6b3140 100644
--- a/docs/devel/reset.rst
+++ b/docs/devel/reset.rst
@@ -210,9 +210,11 @@ 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 *enter* phase (before
+either its children or its own enter method is called) to the *exit*
+phase. During *enter* and *hold* phase only, the function will return that the
+object is in reset. The state is changed after the *exit* is propagated to
+its children and just before calling the object's own *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 96a99ce39ea..c3df75c6ba8 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.25.1

On 20/10/22 16:27, Peter Maydell wrote:
> From: Damien Hedde <damien.hedde@greensocs.com>
> 
> The code for handling the reset level count in the Resettable code
> has two issues:
> 
> The reset count is only decremented for the 1->0 case.  This means
> that if there's ever a nested reset that takes the count to 2 then it
> will never again be decremented.  Eventually the count will exceed
> the '50' limit in resettable_phase_enter() and QEMU will trip over
> the assertion failure.  The repro case in issue 1266 is an example of
> this that happens now the SCSI subsystem uses three-phase reset.
> 
> Secondly, the count is decremented only after the exit phase handler
> is called.  Moving the reset count decrement from "just after" to
> "just before" calling the exit phase handler allows
> resettable_is_in_reset() to return 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" as a special case.
> 
> Note that the semantics change to the *_is_in_reset() functions
> will have no effect on the current codebase, because only two
> devices (hw/char/cadence_uart.c and hw/misc/zynq_sclr.c) currently
> call those functions, and in neither case do they do it from the
> device's exit phase methed.

Typo "method".

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1266
> 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>
> [PMM: adjust the docs paragraph changed to get the name of the
>   'enter' phase right and to clarify exactly when the count is
>   adjusted; rewrite the commit message]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   docs/devel/reset.rst | 8 +++++---
>   hw/core/resettable.c | 3 +--
>   2 files changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>