Currently we have transitional machinery between legacy reset
and three phase reset that works in two directions:
 * if you invoke three phase reset on a device which has set
   the DeviceClass::legacy_reset method, we detect this in
   device_get_transitional_reset() and arrange that we call
   the legacy_reset method during the hold phase of reset
 * if you invoke legacy reset on a device which implements
   three phase reset, the default legacy_reset method is
   device_phases_reset(), which does a three-phase reset
   of the device

However, we have now eliminated all the places which could invoke
legacy reset on a device, which means that the function
device_phases_reset() is never called -- it serves only as the value
of DeviceClass::legacy_reset that indicates that the subclass never
overrode the legacy reset method.  So we can delete it, and instead
check for legacy_reset != NULL.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/core/qdev.c | 51 ++++++++++++--------------------------------------
 1 file changed, 12 insertions(+), 39 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 460114609b0..9af0ed3e1b7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -747,38 +747,16 @@ device_vmstate_if_get_id(VMStateIf *obj)
     return qdev_get_dev_path(dev);
 }
 
-/**
- * device_phases_reset:
- * Transition reset method for devices to allow moving
- * smoothly from legacy reset method to multi-phases
- */
-static void device_phases_reset(DeviceState *dev)
-{
-    ResettableClass *rc = RESETTABLE_GET_CLASS(dev);
-
-    if (rc->phases.enter) {
-        rc->phases.enter(OBJECT(dev), RESET_TYPE_COLD);
-    }
-    if (rc->phases.hold) {
-        rc->phases.hold(OBJECT(dev), RESET_TYPE_COLD);
-    }
-    if (rc->phases.exit) {
-        rc->phases.exit(OBJECT(dev), RESET_TYPE_COLD);
-    }
-}
-
 static void device_transitional_reset(Object *obj)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(obj);
 
     /*
-     * This will call either @device_phases_reset (for multi-phases transitioned
-     * devices) or a device's specific method for not-yet transitioned devices.
-     * In both case, it does not reset children.
+     * Device still using DeviceClass legacy_reset method. This doesn't
+     * reset children. device_get_transitional_reset() checked that
+     * this isn't NULL.
      */
-    if (dc->legacy_reset) {
-        dc->legacy_reset(DEVICE(obj));
-    }
+    dc->legacy_reset(DEVICE(obj));
 }
 
 /**
@@ -788,7 +766,7 @@ static void device_transitional_reset(Object *obj)
 static ResettableTrFunction device_get_transitional_reset(Object *obj)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(obj);
-    if (dc->legacy_reset != device_phases_reset) {
+    if (dc->legacy_reset) {
         /*
          * dc->reset has been overridden by a subclass,
          * the device is not ready for multi phase yet.
@@ -819,19 +797,14 @@ static void device_class_init(ObjectClass *class, void *data)
     rc->child_foreach = device_reset_child_foreach;
 
     /*
-     * @device_phases_reset is put as the default reset method below, allowing
-     * to do the multi-phase transition from base classes to leaf classes. It
-     * allows a legacy-reset Device class to extend a multi-phases-reset
-     * Device class for the following reason:
-     * + If a base class B has been moved to multi-phase, then it does not
-     *   override this default reset method and may have defined phase methods.
-     * + A child class C (extending class B) which uses
-     *   device_class_set_parent_reset() (or similar means) to override the
-     *   reset method will still work as expected. @device_phases_reset function
-     *   will be registered as the parent reset method and effectively call
-     *   parent reset phases.
+     * A NULL legacy_reset implies a three-phase reset device. Devices can
+     * only be reset using three-phase aware mechanisms, but we still support
+     * for transitional purposes leaf classes which set the old legacy_reset
+     * method via device_class_set_legacy_reset(). If they do so, then
+     * device_get_transitional_reset() will notice and arrange for the
+     * DeviceClass::legacy_reset() method to be called during the hold phase.
      */
-    device_class_set_legacy_reset(dc, device_phases_reset);
+    dc->legacy_reset = NULL;
     rc->get_transitional_function = device_get_transitional_reset;
 
     object_class_property_add_bool(class, "realized",
-- 
2.34.1

On 8/14/24 02:52, Peter Maydell wrote:
> Currently we have transitional machinery between legacy reset
> and three phase reset that works in two directions:
>   * if you invoke three phase reset on a device which has set
>     theDeviceClass::legacy_reset method, we detect this in
>     device_get_transitional_reset() and arrange that we call
>     the legacy_reset method during the hold phase of reset
>   * if you invoke legacy reset on a device which implements
>     three phase reset, the default legacy_reset method is
>     device_phases_reset(), which does a three-phase reset
>     of the device
> 
> However, we have now eliminated all the places which could invoke
> legacy reset on a device, which means that the function
> device_phases_reset() is never called -- it serves only as the value
> ofDeviceClass::legacy_reset that indicates that the subclass never
> overrode the legacy reset method.  So we can delete it, and instead
> check for legacy_reset != NULL.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/core/qdev.c | 51 ++++++++++++--------------------------------------
>   1 file changed, 12 insertions(+), 39 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


Is the reason we prioritize resettable_get_tr_func over rc->phases to allow for a legacy 
device to be attached to a resettable bus?

I wonder if device_class_set_legacy_reset can simplify that, with

static void do_legacy_reset(...)
{
     dc->legacy_reset(...);
}

void device_class_set_legacy_reset(DeviceClass *dc, DeviceReset dev_reset)
{
     dc->legacy_reset = dev_reset;

     /* Parent enter/exit are not invoked with a legacy child. */
     dc->resettable.enter = NULL;
     dc->resettable.exit = NULL;
     dc->resettable.hold = do_legacy_reset;
}

Which would eliminate resettable_get_tr_func and the supporting layers completely.


r~

On Wed, 14 Aug 2024 at 01:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/14/24 02:52, Peter Maydell wrote:
> > Currently we have transitional machinery between legacy reset
> > and three phase reset that works in two directions:
> >   * if you invoke three phase reset on a device which has set
> >     theDeviceClass::legacy_reset method, we detect this in
> >     device_get_transitional_reset() and arrange that we call
> >     the legacy_reset method during the hold phase of reset
> >   * if you invoke legacy reset on a device which implements
> >     three phase reset, the default legacy_reset method is
> >     device_phases_reset(), which does a three-phase reset
> >     of the device
> >
> > However, we have now eliminated all the places which could invoke
> > legacy reset on a device, which means that the function
> > device_phases_reset() is never called -- it serves only as the value
> > ofDeviceClass::legacy_reset that indicates that the subclass never
> > overrode the legacy reset method.  So we can delete it, and instead
> > check for legacy_reset != NULL.
> >
> > Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> > ---
> >   hw/core/qdev.c | 51 ++++++++++++--------------------------------------
> >   1 file changed, 12 insertions(+), 39 deletions(-)
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
>
> Is the reason we prioritize resettable_get_tr_func over rc->phases to allow for a legacy
> device to be attached to a resettable bus?

It's to handle devices which implement only Device::legacy_reset.
Those devices must have their legacy_reset function called
(and I think it should always be the case that there are
no rc->phases methods in that case). (All buses now are always
3-phase aware, incidentally.)

> I wonder if device_class_set_legacy_reset can simplify that, with
>
> static void do_legacy_reset(...)
> {
>      dc->legacy_reset(...);
> }
>
> void device_class_set_legacy_reset(DeviceClass *dc, DeviceReset dev_reset)
> {
>      dc->legacy_reset = dev_reset;
>
>      /* Parent enter/exit are not invoked with a legacy child. */
>      dc->resettable.enter = NULL;
>      dc->resettable.exit = NULL;
>      dc->resettable.hold = do_legacy_reset;
> }
>
> Which would eliminate resettable_get_tr_func and the supporting layers completely.

I did think about something like this but wasn't sure that there
was much benefit from changing from one workaround to the other.
But this does look like it's less complication than we have now.
I'll come back to this if my idea about using coccinelle to make
a mass conversion of legacy_reset to 3-phase doesn't pan out.

thanks
-- PMM