On Wed, Jul 31, 2019 at 01:31:28PM +0200, Philippe Mathieu-Daudé wrote:
> On 7/31/19 11:29 AM, Damien Hedde wrote:
> > On 7/31/19 8:05 AM, David Gibson wrote:
> >> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote:
> >>> Deprecate old reset apis and make them use the new one while they
> >>> are still used somewhere.
> >>>
> >>> Signed-off-by: Damien Hedde <damien.he...@greensocs.com>
> >>> ---
> >>> hw/core/qdev.c | 22 +++-------------------
> >>> include/hw/qdev-core.h | 28 ++++++++++++++++++++++------
> >>> 2 files changed, 25 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >>> index 559ced070d..e9e5f2d5f9 100644
> >>> --- a/hw/core/qdev.c
> >>> +++ b/hw/core/qdev.c
> >>> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj,
> >>> void (*func)(Object *))
> >>> }
> >>> }
> >>>
> >>> -static int qdev_reset_one(DeviceState *dev, void *opaque)
> >>> -{
> >>> - device_legacy_reset(dev);
> >>> -
> >>> - return 0;
> >>> -}
> >>> -
> >>> -static int qbus_reset_one(BusState *bus, void *opaque)
> >>> -{
> >>> - BusClass *bc = BUS_GET_CLASS(bus);
> >>> - if (bc->reset) {
> >>> - bc->reset(bus);
> >>> - }
> >>> - return 0;
> >>> -}
> >>> -
> >>> void qdev_reset_all(DeviceState *dev)
> >>> {
> >>> - qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one,
> >>> NULL);
> >>> + device_reset(dev, false);
> >>> }
> >>>
> >>> void qdev_reset_all_fn(void *opaque)
> >>> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque)
> >>>
> >>> void qbus_reset_all(BusState *bus)
> >>> {
> >>> - qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one,
> >>> NULL);
> >>> + bus_reset(bus, false);
> >>> }
> >>>
> >>> void qbus_reset_all_fn(void *opaque)
> >>> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool
> >>> value, Error **errp)
> >>> }
> >>> }
> >>> if (dev->hotplugged) {
> >>> - device_legacy_reset(dev);
> >>> + device_reset(dev, true);
> >>
> >> So.. is this change in the device_reset() signature really necessary?
> >> Even if there are compelling reasons to handle warm reset in the new
> >> API, that doesn't been you need to change device_reset() itself from
> >> its established meaning of a cold (i.e. as per power cycle) reset.
> >> Warm resets are generally called in rather more specific circumstances
> >> (often under guest software direction) so it seems likely that users
> >> would want to engage with the new reset API directly. Or we could
> >> just create a device_warm_reset() wrapper. That would also avoid the
> >> bare boolean parameter, which is not great for readability (you have
> >> to look up the signature to have any idea what it means).
>
> If the boolean is not meaningful, we can use an enum...
That's certainly better, but I'm not seeing a compelling reason not to
have separate function names. It's just as clear and means less churn.
>
> > I've added device_reset_cold/warm wrapper functions to avoid having to
> > pass the boolean parameter. it seems I forgot to use them in qdev.c
> > I suppose, like you said, we could live with
> > + no function with the boolean parameter
> > + device_reset doing cold reset
> > + device_reset_warm (or device_warm_reset) for the warm version
> >
> > Damien
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
