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

[Damien Hedde]

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 <address@hidden>
>> ---
>>  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

Damien