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.he...@greensocs.com> > > Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> > > Reviewed-by: Peter Maydell <peter.mayd...@linaro.org> > > Reviewed-by: Richard Henderson <richard.hender...@linaro.org> > > Tested-by: Philippe Mathieu-Daudé <phi...@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? > > I see it is similar to device_cold_reset(), but TBH I'm scared > to be the first one using it. > > Regards, > > Phil. > > > */ > > void qbus_reset_all(BusState *bus); > > void qbus_reset_all_fn(void *opaque); > > > > +/** > > + * device_cold_reset: > > + * Reset device @dev and perform a recursive processing using the > > resettable > > + * interface. It triggers a RESET_TYPE_COLD. > > + */ > > +void device_cold_reset(DeviceState *dev); > > + > > +/** > > + * bus_cold_reset: > > + * > > + * Reset bus @bus and perform a recursive processing using the resettable > > + * interface. It triggers a RESET_TYPE_COLD. > > + */ > > +void bus_cold_reset(BusState *bus); > -- Eduardo
