qemu-devel
[Top][All Lists]
Advanced

[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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]