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

From: Peter Maydell <peter.maydell@linaro.org>
To: Damien Hedde <damien.hedde@greensocs.com>
Cc: qemu-devel@nongnu.org,
	"Michael Peter" <michael.peter@hensoldt-cyber.com>,
	alistair@alistair23.me, edgar.iglesias@gmail.com,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>
Subject: Re: [PATCH v2] hw/core/resettable: make in-reset state false during exit phase call
Date: Thu, 20 Oct 2022 14:25:42 +0100	[thread overview]
Message-ID: <CAFEAcA-6WRUFTtSwaJgWvM-AYE8jTpstwDhZv7WdYD1OE6t36w@mail.gmail.com> (raw)
In-Reply-To: <20210122103629.5412-1-damien.hedde@greensocs.com>

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

