diff mbox series

[v2] hw/core/resettable: make in-reset state false during exit phase call

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

Commit Message

Damien Hedde Jan. 22, 2021, 10:36 a.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Jan. 22, 2021, 2:10 p.m. UTC | #1
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);
>
Peter Maydell Jan. 22, 2021, 2:26 p.m. UTC | #2
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
diff mbox series

Patch

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);