Re: [PATCH v8 08/11] hw/core: deprecate old reset functions and introduce new ones

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Apr 27, 2021 at 02:21:28PM +0200, Philippe Mathieu-Daudé wrote:
>> On 1/23/20 2:28 PM, Damien Hedde wrote:
>> > Deprecate device_legacy_reset(), qdev_reset_all() and
>> > qbus_reset_all() to be replaced by new functions
>> > device_cold_reset() and bus_cold_reset() which uses resettable API.
>> > 
>> > Also introduce resettable_cold_reset_fn() which may be used as a
>> > replacement for qdev_reset_all_fn and qbus_reset_all_fn().
>> > 
>> > Following patches will be needed to look at legacy reset call sites
>> > and switch to resettable api. The legacy functions will be removed
>> > when unused.
>> > 
>> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> > ---
>> >  include/hw/qdev-core.h  | 27 +++++++++++++++++++++++++++
>> >  include/hw/resettable.h |  9 +++++++++
>> >  hw/core/bus.c           |  5 +++++
>> >  hw/core/qdev.c          |  5 +++++
>> >  hw/core/resettable.c    |  5 +++++
>> >  5 files changed, 51 insertions(+)
>> > 
>> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> > index 1b4b420617..b84fcc32bf 100644
>> > --- a/include/hw/qdev-core.h
>> > +++ b/include/hw/qdev-core.h
>> > @@ -406,6 +406,13 @@ int qdev_walk_children(DeviceState *dev,
>> >                         qdev_walkerfn *post_devfn, qbus_walkerfn 
>> > *post_busfn,
>> >                         void *opaque);
>> >  
>> > +/**
>> > + * @qdev_reset_all:
>> > + * Reset @dev. See @qbus_reset_all() for more details.
>> > + *
>> > + * Note: This function is deprecated and will be removed when it becomes 
>> > unused.
>> > + * Please use device_cold_reset() now.
>> > + */
>> >  void qdev_reset_all(DeviceState *dev);
>> >  void qdev_reset_all_fn(void *opaque);
>> >  
>> > @@ -418,10 +425,28 @@ void qdev_reset_all_fn(void *opaque);
>> >   * hard reset means that qbus_reset_all will reset all state of the 
>> > device.
>> >   * For PCI devices, for example, this will include the base address 
>> > registers
>> >   * or configuration space.
>> > + *
>> > + * Note: This function is deprecated and will be removed when it becomes 
>> > unused.
>> > + * Please use bus_cold_reset() now.
>> 
>> Some time passed, so looking at this with some retrospective.
>> 
>> If there is an effort to introduce a new API replacing another one,
>> we should try convert all the uses of the old API to the new one,
>> instead of declaring it legacy.
>> 
>> Declare an API legacy/deprecated should be the last resort if there
>> is no way to remove it. I'd recommend to move the deprecated/legacy
>> declarations in a separate header, with the '_legacy' suffix.
>> 
>> Else:
>> 
>> 1/ we never finish API conversions,
>> 2/ the new API might not be ready for all the legacy API use cases,
>> 3/ we end up having to maintain 2 different APIs.
>> 
>> 
>> So the recommendation is to use bus_cold_reset(), but it isn't
>> used anywhere...:
>> 
>> $ git grep bus_cold_reset
>> docs/devel/reset.rst:64:- ``bus_cold_reset()``
>> hw/core/bus.c:73:void bus_cold_reset(BusState *bus)
>> include/hw/qdev-core.h:715: * Please use bus_cold_reset() now.
>> include/hw/qdev-core.h:728: * bus_cold_reset:
>> include/hw/qdev-core.h:733:void bus_cold_reset(BusState *bus);
>> 
>> IMHO we shouldn't add new public prototypes without callers.
>
> I agree.  We should make at least some effort to convert code to
> the new API, if only to serve as reference for people doing the
> conversion.  I'm surprised that a new function was added more
> than a year ago and nobody is using it.
>
> What happened here?  Was there some plan to convert existing code
> but it was abandoned?

Commit abb89dbf2 introduced bus_cold_reset() and device_cold_reset().
It was posted as part of "[PATCH v8 00/11] Multi-phase reset mechanism".
The series did not add any users.  The cover letter explains:

    The purpose of this series is to split the current reset procedure
    into multiple phases. This will help to solve some ordering
    difficulties we have during reset.

    This is a ready to merge version. I've taken the few remarks of
    Philippe about v7 in account. Thanks to him for all the tests he did.

    This series adds resettable interface and transitions base Device and
    Bus classes (sysbus subclasses are ok too). It provides new reset
    functions but does not switch anymore the old functions
    (device_reset() and qdev/qbus_reset_all()) to resettable interface.
    These functions keep the exact same behavior as before.

    The series also transition the main reset handlers registration which
    has no impact until devices and buses are transitioned.

    The series is organized as follows:
    Patch 1 prepare the reset transition. Patch 2 adds some utility trace
    events. Patches 3 to 8 adds resettable api in devices and buses. Patch
    9 adds some documentation. Patches 10 and 11 transition the call sites
    of qemu_register_reset(qdev/qbus_reset_all_fn, ...).

    After this series, the plan is then to transition devices, buses and
    legacy reset call sites. Devices and buses have to be transitioned
    from mother class to daughter classes order but until the final
    (daughter) class is transitioned, old monolitic reset behavior will
    be kept for this class.

bus_cold_reset() has never seen any use.

The only transitioning to device_cold_reset() I can find is Peter
Maydell's

    781c67ca55 cpu: Use DeviceClass reset instead of a special CPUClass reset

Then there's a QOMification series that uses device_cold_reset()
temporarily, also by Peter:

    4bebb9ad4e hw/arm/stellaris: Convert SSYS to QOM device
    14711b6f54 hw/arm/stellaris: Remove board-creation reset of STELLARIS_SYS

Finally, Bin Meng, Philippe Mathieu-Daudé, and Luc Michel added new code
using it:

    c696e1f2b3 hw/sd: Add Cadence SDHCI emulation
    65ad1da23e hw/misc/mps2-fpgaio: Use the LED device
    435db7ebf5 hw/misc/mps2-scc: Use the LED device
    1e986e25d0 hw/misc/bcm2835_cprman: add a PLL skeleton implementation
    09d56bbc9b hw/misc/bcm2835_cprman: add a PLL channel skeleton implementation
    7281362484 hw/misc/bcm2835_cprman: add a clock mux skeleton implementation
    502960ca04 hw/misc/bcm2835_cprman: add the DSI0HSCK multiplexer

>> I see it is similar to device_cold_reset(), but TBH I'm scared
>> to be the first one using it.

For what it's worth, Damien further explained the two helpers in
docs/devel/reset.rst:

    For Devices and Buses, the following helper functions exist:

    - ``device_cold_reset()``
    - ``bus_cold_reset()``

    These are simple wrappers around resettable_reset() function; they only 
cast the
    Device or Bus into an Object and pass the cold reset type. When possible
    prefer to use these functions instead of ``resettable_reset()``.

I figure what's missing is guidance on how to transition code from
legacy reset to multi-phase reset.  Ideally with a working example
people can study.  Damien, can you help us out?