[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 08/11] hw/core: deprecate old reset functions and introduc
From: |
Eduardo Habkost |
Subject: |
Re: [PATCH v8 08/11] hw/core: deprecate old reset functions and introduce new ones |
Date: |
Tue, 27 Apr 2021 17:27:07 -0400 |
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?
>
> 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