Re: [Qemu-devel] [PATCH v3 05/33] Switch to new api in qdev/bus

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3451 bytes --]

On Wed, Jul 31, 2019 at 11:29:36AM +0200, 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.hedde@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).
> 
> 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

Ok, good.

I'm afraid the whole series still makes me pretty uncomfortable,
though, since the whole "warm reset" concept still seems way to vague
to me.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]