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

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

[Damien Hedde]

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.
 
 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

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

[Philippe Mathieu-Daudé]

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

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

[Peter Maydell]

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

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

[Peter Maydell]

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