Re: [PATCH for-9.2 10/10] hw: Remove device_phases_reset()

[Peter Maydell]

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